TryGhost/Ghost
1
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2024-11-24 06:35:49 +03:00
Code Issues Projects Releases Wiki Activity

Fixed missing defaults in model layer

Browse Source 
no issue

- reported in the forum: https://forum.ghost.org/t/publishing-with-a-single-post-request-to-posts/1648
- the defaults are defined in two places
  1. on the schema level (defaults for the database)
  2. on the ORM (model layer)
- the defaults on the db layer are set correctly when inserting a new resource
- but if we don't apply all defaults on the model layer, it will happen that model events are emitted without the correct defaults
  - see comment in code base
  - it's caused by the fact that knex only returns the inserted resource id (probably caused by the fact knex has to support x databases)
- components/modules are listening on model events and expect:
  1. a complete set of attributes
  2. a complete set of defaults
  3. sanitized values e.g. bool, date
- this commit fixes:
  1. added missing defaults for user & post model
  2. sanitize booleans (0|1 => false|true)
  3. added tests to ensure this works as expected
  4. clarfies the usage of `defaults`

Regarding https://forum.ghost.org/t/publishing-with-a-single-post-request-to-posts/1648:
  - the post event was emitted with the following values {page: undefined, featured: undefined}
  - the urlservice receives this event and won't match the resource against collection filters correctly
  - NOTE: the post data in the db were correct
This commit is contained in:
kirrg001 2018-06-26 16:00:54 +02:00
parent 61db6defde
commit 00cf043e15
6 changed files with 122 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
core/server/data/validation/index.js
View File

@ -220,6 +220,11 @@ validateSchema = function validateSchema(tableName, model, options) {
context: tableName + '.' + columnKey
}));
}
// CASE: ensure we transform 0|1 to false|true
if (!validator.empty(strVal)) {
model.set(columnKey, !!model.get(columnKey));
}
}
// TODO: check if mandatory values should be enforced

27
core/server/models/post.js
View File

@ -21,18 +21,31 @@ Post = ghostBookshelf.Model.extend({
tableName: 'posts',
/**
* ## NOTE:
* We define the defaults on the schema (db) and model level.
* When inserting resources, the defaults are available **after** calling `.save`.
* But they are available when the model hooks are triggered (e.g. onSaving).
* It might be useful to keep them in the model layer for any connected logic.
* @NOTE
*
* e.g. if `model.get('status') === draft; do something;
* We define the defaults on the schema (db) and model level.
*
* Why?
* - when you insert a resource, Knex does only return the id of the created resource
* - see https://knexjs.org/#Builder-insert
* - that means `defaultTo` is a pure database configuration (!)
* - Bookshelf just returns the model values which you have asked Bookshelf to insert
* - it can't return the `defaultTo` value from the schema/db level
* - but the db defaults defined in the schema are saved in the database correctly
* - `models.Post.add` always does to operations:
* 1. add
* 2. fetch (this ensures we fetch the whole resource from the database)
* - that means we have to apply the defaults on the model layer to ensure a complete field set
* 1. any connected logic in our model hooks e.g. beforeSave
* 2. model events e.g. "post.published" are using the inserted resource, not the fetched resource
*/
defaults: function defaults() {
return {
uuid: uuid.v4(),
status: 'draft'
status: 'draft',
featured: false,
page: false,
visibility: 'public'
};
},

4
core/server/models/user.js
View File

@ -25,7 +25,9 @@ User = ghostBookshelf.Model.extend({
defaults: function defaults() {
return {
password: security.identifier.uid(50)
password: security.identifier.uid(50),
visibility: 'public',
status: 'active'
};
},

24
core/test/unit/data/validation/index_spec.js
View File

@ -79,6 +79,30 @@ describe('Validation', function () {
{method: 'insert'}
);
});
it('transforms 0 and 1', function () {
const post = models.Post.forge(testUtils.DataGenerator.forKnex.createPost({slug: 'test', featured: 0, page: 1}));
post.get('featured').should.eql(0);
post.get('page').should.eql(1);
return validation.validateSchema('posts', post, {method: 'insert'})
.then(function () {
post.get('featured').should.eql(false);
post.get('page').should.eql(true);
});
});
it('keeps true or false', function () {
const post = models.Post.forge(testUtils.DataGenerator.forKnex.createPost({slug: 'test', featured: true, page: false}));
post.get('featured').should.eql(true);
post.get('page').should.eql(false);
return validation.validateSchema('posts', post, {method: 'insert'})
.then(function () {
post.get('featured').should.eql(true);
post.get('page').should.eql(false);
});
});
});
describe('models.edit', function () {

48
core/test/unit/models/post_spec.js
View File

@ -56,6 +56,10 @@ describe('Unit: models/post', function () {
_.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => {
should.exist(post.hasOwnProperty(key));
if (['page', 'status', 'visibility', 'featured'].indexOf(key) !== -1) {
events.post[0].data[key].should.eql(schema.tables.posts[key].defaultTo);
}
});
should.not.exist(post.authors);
@ -67,6 +71,10 @@ describe('Unit: models/post', function () {
_.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => {
should.exist(events.post[0].data.hasOwnProperty(key));
if (['page', 'status', 'visibility', 'featured'].indexOf(key) !== -1) {
events.post[0].data[key].should.eql(schema.tables.posts[key].defaultTo);
}
});
should.exist(events.post[0].data.authors);
@ -76,6 +84,29 @@ describe('Unit: models/post', function () {
});
});
it('with page:1', function () {
const events = {
post: []
};
sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) {
events.post.push({event: event, data: this.toJSON()});
});
return models.Post.add({
title: 'My beautiful title.',
page: 1
}, testUtils.context.editor)
.then((post) => {
post.get('title').should.eql('My beautiful title.');
post = post.toJSON();
// transformed 1 to true
post.page.should.eql(true);
events.post[0].data.page.should.eql(true);
});
});
it('use `withRelated=tags`', function () {
const events = {
post: []
@ -97,21 +128,12 @@ describe('Unit: models/post', function () {
post.get('title').should.eql('My beautiful title.');
post = post.toJSON();
_.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => {
should.exist(post.hasOwnProperty(key));
});
should.not.exist(post.authors);
should.not.exist(post.primary_author);
should.exist(post.tags);
should.exist(post.primary_tag);
events.post[0].event.should.eql('added');
_.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => {
should.exist(events.post[0].data.hasOwnProperty(key));
});
should.exist(events.post[0].data.authors);
should.exist(events.post[0].data.primary_author);
should.exist(events.post[0].data.tags);
@ -140,10 +162,6 @@ describe('Unit: models/post', function () {
post.get('title').should.eql('My beautiful title.');
post = post.toJSON();
_.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => {
should.exist(post.hasOwnProperty(key));
});
should.exist(post.authors);
should.exist(post.primary_author);
should.exist(post.tags);
@ -151,10 +169,6 @@ describe('Unit: models/post', function () {
events.post[0].event.should.eql('added');
_.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => {
should.exist(events.post[0].data.hasOwnProperty(key));
});
should.exist(events.post[0].data.authors);
should.exist(events.post[0].data.primary_author);
should.exist(events.post[0].data.tags);

39
core/test/unit/models/user_spec.js
View File

@ -1,5 +1,7 @@
const should = require('should'),
sinon = require('sinon'),
_ = require('lodash'),
schema = require('../../../server/data/schema'),
models = require('../../../server/models'),
validation = require('../../../server/data/validation'),
common = require('../../../server/lib/common'),
@ -334,4 +336,41 @@ describe('Unit: models/user', function () {
});
});
});
describe('Add', function () {
const events = {
user: []
};
before(function () {
models.init();
sandbox.stub(models.User.prototype, 'emitChange').callsFake(function (event) {
events.user.push({event: event, data: this.toJSON()});
});
});
after(function () {
sandbox.restore();
});
it('defaults', function () {
return models.User.add({slug: 'joe', name: 'Joe', email: 'joe@test.com'})
.then(function (user) {
user.get('name').should.eql('Joe');
user.get('email').should.eql('joe@test.com');
user.get('slug').should.eql('joe');
user.get('visibility').should.eql('public');
user.get('status').should.eql('active');
_.each(_.keys(schema.tables.users), (key) => {
should.exist(events.user[0].data.hasOwnProperty(key));
if (['status', 'visibility'].indexOf(key) !== -1) {
events.user[0].data[key].should.eql(schema.tables.users[key].defaultTo);
}
});
});
});
});
});