Skip to content

Commit

Permalink
Removed toJSON serialization in findPage method (#9899)
Browse files Browse the repository at this point in the history
refs #9866

- Removed `toJSON` call in `findPage`
- Added JSON serialization on API layer
- Reason: model and api layer were coupled - all other model actions just returned the raw data and no specific format
- Corrected test suites to serialize fetched models to JSON
- Removed `absolute_urls` attribute from validOptions findPage methods as it's no longer needed in the data layer
- Changed 'include' test as this option is now tolerated and returns data
  • Loading branch information
naz authored and kirrg001 committed Sep 26, 2018
1 parent 7b950f6 commit 4c5bff0
Show file tree
Hide file tree
Showing 14 changed files with 163 additions and 115 deletions.
12 changes: 9 additions & 3 deletions core/server/api/invites.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,13 @@ const invites = {
let tasks;

function modelQuery(options) {
return models.Invite.findPage(options);
return models.Invite.findPage(options)
.then(({data, meta}) => {
return {
invites: data.map(model => model.toJSON(options)),
meta: meta
};
});
}

tasks = [
Expand Down Expand Up @@ -163,7 +169,7 @@ const invites = {
return invite.destroy(options);
})
.then(() => {
return options;
return options;
});
}

Expand Down Expand Up @@ -205,7 +211,7 @@ const invites = {
}));
}
}).then(() => {
return options;
return options;
});
}

Expand Down
8 changes: 7 additions & 1 deletion core/server/api/posts.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,13 @@ posts = {
* @returns {Object} options
*/
function modelQuery(options) {
return models.Post.findPage(options);
return models.Post.findPage(options)
.then(({data, meta}) => {
return {
posts: data.map(model => model.toJSON(options)),
meta: meta
};
});
}

// Push all of our tasks into a `tasks` array in the correct order
Expand Down
8 changes: 7 additions & 1 deletion core/server/api/subscribers.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,13 @@ subscribers = {
* @returns {Object} options
*/
function doQuery(options) {
return models.Subscriber.findPage(options);
return models.Subscriber.findPage(options)
.then(({data, meta}) => {
return {
subscribers: data.map(model => model.toJSON(options)),
meta: meta
};
});
}

// Push all of our tasks into a `tasks` array in the correct order
Expand Down
8 changes: 7 additions & 1 deletion core/server/api/tags.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,13 @@ tags = {
* @returns {Object} options
*/
function doQuery(options) {
return models.Tag.findPage(options);
return models.Tag.findPage(options)
.then(({data, meta}) => {
return {
tags: data.map(model => model.toJSON(options)),
meta: meta
};
});
}

// Push all of our tasks into a `tasks` array in the correct order
Expand Down
8 changes: 7 additions & 1 deletion core/server/api/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ users = {
* @returns {Object} options
*/
function doQuery(options) {
return models.User.findPage(options);
return models.User.findPage(options)
.then(({data, meta}) => {
return {
users: data.map(post => post.toJSON(options)),
meta: meta
};
});
}

// Push all of our tasks into a `tasks` array in the correct order
Expand Down
48 changes: 24 additions & 24 deletions core/server/models/base/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -665,29 +665,27 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
* information about the request (page, limit), along with the
* info needed for pagination (pages, total).
*
* @TODO:
* - this model function does return JSON O_O
* - if you refactor that out, you should double check the allowed filter options
* - because `toJSON` is called in here and is using the filtered options for the `findPage` function
*
* **response:**
*
* {
* posts: [
* {...}, ...
* ],
* page: __,
* limit: __,
* pages: __,
* total: __
* data: [
* {...}, ...
* ],
* meta: {
* pagination: {
* page: __,
* limit: __,
* pages: __,
* total: __
* }
* }
* }
*
* @param {Object} unfilteredOptions
*/
findPage: function findPage(unfilteredOptions) {
var options = this.filterOptions(unfilteredOptions, 'findPage'),
itemCollection = this.forge(),
tableName = _.result(this.prototype, 'tableName'),
requestedColumns = options.columns;

// Set this to true or pass ?debug=true as an API option to get output
Expand Down Expand Up @@ -717,20 +715,22 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
}

return itemCollection.fetchPage(options).then(function formatResponse(response) {
var data = {},
models;

options.columns = requestedColumns;
models = response.collection.toJSON(options);
// Attributes are being filtered here, so they are not leaked into calling layer
// where models are serialized to json and do not do more filtering.
// Re-add and pick any computed properties that were stripped before fetchPage call.
const data = response.collection.models.map((model) => {
if (requestedColumns) {
model.attributes = _.pick(model.attributes, requestedColumns);
model._previousAttributes = _.pick(model._previousAttributes, requestedColumns);
}

// re-add any computed properties that were stripped out before the call to fetchPage
// pick only requested before returning JSON
data[tableName] = _.map(models, function transform(model) {
return options.columns ? _.pick(model, options.columns) : model;
return model;
});

data.meta = {pagination: response.pagination};
return data;
return {
data: data,
meta: {pagination: response.pagination}
};
});
},

Expand Down
2 changes: 1 addition & 1 deletion core/server/models/post.js
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ Post = ghostBookshelf.Model.extend({
// these are the only options that can be passed to Bookshelf / Knex.
validOptions = {
findOne: ['columns', 'importing', 'withRelated', 'require'],
findPage: ['page', 'limit', 'columns', 'filter', 'order', 'status', 'staticPages', 'absolute_urls'],
findPage: ['page', 'limit', 'columns', 'filter', 'order', 'status', 'staticPages'],
findAll: ['columns', 'filter'],
destroy: ['destroyAll']
};
Expand Down
2 changes: 1 addition & 1 deletion core/server/models/tag.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ Tag = ghostBookshelf.Model.extend({
// whitelists for the `options` hash argument on methods, by method name.
// these are the only options that can be passed to Bookshelf / Knex.
validOptions = {
findPage: ['page', 'limit', 'columns', 'filter', 'order', 'absolute_urls'],
findPage: ['page', 'limit', 'columns', 'filter', 'order'],
findAll: ['columns'],
findOne: ['visibility'],
destroy: ['destroyAll']
Expand Down
2 changes: 1 addition & 1 deletion core/server/models/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ User = ghostBookshelf.Model.extend({
setup: ['id'],
edit: ['withRelated', 'importPersistUser'],
add: ['importPersistUser'],
findPage: ['page', 'limit', 'columns', 'filter', 'order', 'status', 'absolute_urls'],
findPage: ['page', 'limit', 'columns', 'filter', 'order', 'status'],
findAll: ['filter']
};

Expand Down
5 changes: 2 additions & 3 deletions core/test/functional/routes/api/posts_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ describe('Post API', function () {
});
});

it('fields and formats and include ignores include', function (done) {
it('fields and formats and include', function (done) {
request.get(testUtils.API.getApiQuery('posts/?formats=mobiledoc,html&fields=id,title&include=authors'))
.set('Authorization', 'Bearer ' + ownerAccessToken)
.expect('Content-Type', /json/)
Expand All @@ -200,15 +200,14 @@ describe('Post API', function () {
should.not.exist(res.headers['x-cache-invalidate']);
var jsonResponse = res.body;
should.exist(jsonResponse.posts);
should.not.exist(jsonResponse.posts[0].authors);
testUtils.API.checkResponse(jsonResponse, 'posts');
jsonResponse.posts.should.have.length(11);
testUtils.API.checkResponse(
jsonResponse.posts[0],
'post',
null,
null,
['mobiledoc', 'id', 'title', 'html']
['mobiledoc', 'id', 'title', 'html', 'authors']
);

testUtils.API.checkResponse(jsonResponse.meta.pagination, 'pagination');
Expand Down
Loading

0 comments on commit 4c5bff0

Please sign in to comment.