From 8a48534c839b4636198e4aac14f3e2b7fa2cc7d6 Mon Sep 17 00:00:00 2001 From: Jeremy Trufier Date: Tue, 21 Aug 2018 14:14:01 +0200 Subject: [PATCH 1/4] test(fix): included object order is not guaranteed --- test/hasManyRelationships.test.js | 124 +++++++++++++++++------------- test/scopeInclude.test.js | 22 ++++-- 2 files changed, 86 insertions(+), 60 deletions(-) diff --git a/test/hasManyRelationships.test.js b/test/hasManyRelationships.test.js index e04f5d2..cf0d370 100644 --- a/test/hasManyRelationships.test.js +++ b/test/hasManyRelationships.test.js @@ -100,62 +100,76 @@ describe('loopback json api hasMany relationships', function () { }) expect(res.body.included).to.be.an('array') expect(res.body.included.length).to.equal(3) - const relatedPostLink = id => { - return res.body.included[0].relationships.posts.links.related - } - expect(res.body.included[0]).to.deep.equal({ - id: '1', - type: 'authors', - attributes: { - firstName: 'Joe', - lastName: 'Shmoe' - }, - relationships: { - posts: { - links: { - related: relatedPostLink(0) - } - } - } - }) - expect(res.body.included[1]).to.deep.equal({ - id: '1', - type: 'comments', - attributes: { - title: 'My comment', - comment: 'My comment text' - }, - relationships: { - post: { - data: { - id: 1, - type: 'posts' - }, - links: { - related: res.body.included[1].relationships.post.links.related - } - } - } - }) - expect(res.body.included[2]).to.deep.equal({ - id: '2', - type: 'comments', - attributes: { - title: 'My second comment', - comment: 'My second comment text' - }, - relationships: { - post: { - data: { - id: 1, - type: 'posts' - }, - links: { - related: res.body.included[2].relationships.post.links.related - } - } + + const validateData = (slug, included) => { + switch (slug) { + case 'authors1': + return expect(included).to.deep.equal({ + id: '1', + type: 'authors', + attributes: { + firstName: 'Joe', + lastName: 'Shmoe' + }, + relationships: { + posts: { + links: { + related: included.relationships.posts.links.related + } + } + } + }) + case 'comments1': + return expect(included).to.deep.equal({ + id: '1', + type: 'comments', + attributes: { + title: 'My comment', + comment: 'My comment text' + }, + relationships: { + post: { + data: { + id: 1, + type: 'posts' + }, + links: { + related: included.relationships.post.links.related + } + } + } + }) + case 'comments2': + return expect(included).to.deep.equal({ + id: '2', + type: 'comments', + attributes: { + title: 'My second comment', + comment: 'My second comment text' + }, + relationships: { + post: { + data: { + id: 1, + type: 'posts' + }, + links: { + related: included.relationships.post.links.related + } + } + } + }) } - }) + } + + const verifiedIncluded = [] + for (const included of res.body.included) { + const slug = included.type + included.id + expect(verifiedIncluded).to.not.include(slug) + validateData(slug, included) + verifiedIncluded.push(slug) + } + done() }) }) diff --git a/test/scopeInclude.test.js b/test/scopeInclude.test.js index 03b86cf..5b3b32f 100644 --- a/test/scopeInclude.test.js +++ b/test/scopeInclude.test.js @@ -118,14 +118,17 @@ describe('include option', function () { }) }) - it('with include paramter should have both models', function (done) { + it('with include parameter should have both models', function (done) { request(app) .get('/posts/1?filter[include]=author') .end(function (err, res) { expect(err).to.equal(null) expect(res.body.included.length).equal(3) - expect(res.body.included[0].type).equal('authors') - expect(res.body.included[1].type).equal('comments') + + var types = res.body.included.map(r => r.type) + types.sort() + expect(types).to.deep.equal(['authors', 'comments', 'comments']) + done() }) }) @@ -150,8 +153,17 @@ describe('include option', function () { .end(function (err, res) { expect(err).to.equal(null) expect(res.body.included.length).equal(3) - expect(res.body.included[0].type).equal('categories') - expect(res.body.included[0].attributes).to.include({}) + + var hasCategory = false + res.body.included.forEach(r => { + if (r.type === 'categories') { + hasCategory = true + expect(r.attributes).to.include({}) + } + }) + + expect(hasCategory).to.equal(true, 'categories to be present') + done() }) }) From 201e095baf03bc5ea45644d34b9dc4dbebdd3964 Mon Sep 17 00:00:00 2001 From: Jeremy Trufier Date: Tue, 21 Aug 2018 14:38:14 +0200 Subject: [PATCH 2/4] feat(serializer): support multi-level includes BREAKING CHANGE: remove "requestedIncludes" option This option should not handle the way this adapter serialize the resources, if there is embedded resources, even if they do not appear in this option, recommended behavior is to have them in the include property as well. Security consideration: nested includes were previously not handled, it is now the case, like in the loopback documentation, you need to ensure it is secured your side. --- README.md | 6 -- lib/serialize.js | 31 +--------- lib/serializer.js | 106 +++++++++++++++++++------------- lib/utils.js | 21 ------- test/nestedInclude.test.js | 121 +++++++++++++++++++++++++++++++++++++ 5 files changed, 186 insertions(+), 99 deletions(-) create mode 100644 test/nestedInclude.test.js diff --git a/README.md b/README.md index 0d744cc..405162b 100644 --- a/README.md +++ b/README.md @@ -566,12 +566,6 @@ context. The name of the property that is the primary key for the model. This is usually just `id` unless defined differently in a model.json file. -###### `options.requestedIncludes` -The relationships that the user has requested be side loaded with the request. -For example, for the request `GET /api/posts?include=comments` options.requestedIncludes -would be `'comments'`. -- Type: `string` or `array` -- eg: `'comments'` or `['posts', 'comments']` ###### `options.host` The host part of the url including any port information. diff --git a/lib/serialize.js b/lib/serialize.js index d8cb19b..adbeba3 100644 --- a/lib/serialize.js +++ b/lib/serialize.js @@ -29,8 +29,7 @@ module.exports = function (app, defaults) { relatedModelPlural, relatedModelPath, relations, - model, - requestedIncludes + model if (utils.shouldNotApplyJsonApi(ctx, defaults)) { return next() @@ -103,33 +102,6 @@ module.exports = function (app, defaults) { } } - // If we're sideloading, we need to add the includes - if (ctx.req.isSideloadingRelationships) { - requestedIncludes = utils.setRequestedIncludes( - ctx.req.remotingContext.args.filter.include - ) - } - - if (model.definition.settings.scope) { - // bring requestedIncludes in array form - if (typeof requestedIncludes === 'undefined') { - requestedIncludes = [] - } else if (typeof requestedIncludes === 'string') { - requestedIncludes = [requestedIncludes] - } - - // add include from model - var include = model.definition.settings.scope.include - - if (typeof include === 'string') { - requestedIncludes.push(include) - } else if (_.isArray(include)) { - requestedIncludes = requestedIncludes.concat( - utils.setRequestedIncludes(include) - ) - } - } - options = { app: app, model: model, @@ -137,7 +109,6 @@ module.exports = function (app, defaults) { method: ctx.method.name, meta: ctx.meta ? utils.clone(ctx.meta) : null, primaryKeyField: primaryKeyField, - requestedIncludes: requestedIncludes, host: defaults.host || utils.hostFromContext(ctx), dataLinks: { self: function (item) { diff --git a/lib/serializer.js b/lib/serializer.js index 0d355c1..bfb7a48 100644 --- a/lib/serializer.js +++ b/lib/serializer.js @@ -35,20 +35,10 @@ function defaultSerialize (options, cb) { resultData.data = result if (options.meta) resultData.meta = options.meta - /** - * If we're requesting to sideload relationships... - */ - if (options.requestedIncludes) { - try { - handleIncludes( - resultData, - options.requestedIncludes, - options.relationships, - options - ) - } catch (err) { - cb(err) - } + try { + handleIncludes(resultData, options.relationships, options) + } catch (err) { + cb(err) } options.results = resultData @@ -339,34 +329,21 @@ function makeLinks (links, item) { } /** - * Handles serializing the requested includes to a seperate included property - * per JSON API spec. + * From resources, it returns an array of related resources, and turn the + * embedded resource into relationships with id/type couple. * @private * @memberOf {Serializer} - * @param {Object} resp - * @param {Array|String} + * @param {Array} resources + * @param {Object} relations + * @param {Object} options * @throws {Error} - * @return {undefined} + * @return {Array} */ -function handleIncludes (resp, includes, relations, options) { +function getEmbeddedAndSubstituteEmbeddedForIds (resources, relations, options) { var app = options.app - relations = utils.setIncludedRelations(relations, app) - - var resources = _.isArray(resp.data) ? resp.data : [resp.data] - - if (typeof includes === 'string') { - includes = [includes] - } - - if (!_.isArray(includes)) { - throw RelUtils.getInvalidIncludesError( - 'JSON API unable to detect valid includes' - ) - } - - var embedded = resources.map(function subsituteEmbeddedForIds (resource) { - return includes.map(function (include) { + return resources.map(function subsituteEmbeddedForIds (resource) { + return Object.keys(relations).map(function (include) { var relation = relations[include] var includedRelations = relations[include].relations var propertyKey = relation.keyFrom @@ -378,6 +355,7 @@ function handleIncludes (resp, includes, relations, options) { } else { plural = utils.pluralForModel(relation.modelTo) } + var embeds = [] // If relation is belongsTo then pk and fk are the other way around @@ -391,9 +369,11 @@ function handleIncludes (resp, includes, relations, options) { ) } - resource.relationships[include] = resource.relationships[include] || {} - - if (resource.relationships[include] && resource.attributes[include]) { + if ( + resource.relationships && + resource.relationships[include] && + resource.attributes[include] + ) { if (_.isArray(resource.attributes[include])) { embeds = resource.attributes[include].map(function (rel) { rel = utils.clone(rel) @@ -406,7 +386,14 @@ function handleIncludes (resp, includes, relations, options) { options ) }) - embeds = _.compact(embeds) + + var subEmbed = getEmbeddedAndSubstituteEmbeddedForIds( + embeds, + utils.setIncludedRelations(relations[include].relations || {}, app), + options + ) + embeds = embeds.concat(subEmbed) + const included = resource.attributes[include] resource.relationships[include].data = included.map(relData => { return { @@ -429,15 +416,47 @@ function handleIncludes (resp, includes, relations, options) { id: String(resource.attributes[include][propertyKey]), type: plural } + embeds.push(compoundIncludes) + + var subEmbeds = getEmbeddedAndSubstituteEmbeddedForIds( + embeds, + utils.setIncludedRelations(relations[include].relations || {}, app), + options + ) + embeds = embeds.concat(subEmbeds) } delete resource.attributes[relation.keyFrom] delete resource.attributes[relation.keyTo] delete resource.attributes[include] } - return embeds + return _.compact(embeds) }) }) +} + +/** + * Handles serializing the requested includes to a seperate included property + * per JSON API spec. + * @private + * @memberOf {Serializer} + * @param {Object} resp + * @param {Array|String} + * @throws {Error} + * @return {undefined} + */ +function handleIncludes (resp, relations, options) { + var app = options.app + + relations = utils.setIncludedRelations(relations, app) + + var resources = _.isArray(resp.data) ? resp.data : [resp.data] + + var embedded = getEmbeddedAndSubstituteEmbeddedForIds( + resources, + relations, + options + ) if (embedded.length) { // This array may contain duplicate models if the same item is referenced multiple times in 'data' @@ -458,7 +477,10 @@ function handleIncludes (resp, includes, relations, options) { unique.push(d) } }) - resp.included = unique + + if (unique.length) { + resp.included = unique + } } } diff --git a/lib/utils.js b/lib/utils.js index 3efac19..9f1655a 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -22,7 +22,6 @@ module.exports = { shouldNotApplyJsonApi: shouldNotApplyJsonApi, shouldApplyJsonApi: shouldApplyJsonApi, relationFkOnModelFrom: relationFkOnModelFrom, - setRequestedIncludes: setRequestedIncludes, setIncludedRelations: setIncludedRelations } @@ -278,23 +277,3 @@ function setIncludedRelations (relations, app) { } return relations } - -function setRequestedIncludes (include) { - if (!include) return undefined - - if (typeof include === 'string') { - return include - } - if (include instanceof Array) { - return include.map(function (inc) { - if (typeof inc === 'string') { - return inc - } - - if (inc instanceof Object) { - return inc.relation - } - }) - } - return include -} diff --git a/test/nestedInclude.test.js b/test/nestedInclude.test.js new file mode 100644 index 0000000..968f966 --- /dev/null +++ b/test/nestedInclude.test.js @@ -0,0 +1,121 @@ +'use strict' + +var request = require('supertest') +var loopback = require('loopback') +var expect = require('chai').expect +var JSONAPIComponent = require('../') +var ds, app, Post, Author, Comment + +describe('nested include option', function () { + beforeEach(function () { + app = loopback() + app.set('legacyExplorer', false) + ds = loopback.createDataSource('memory') + Post = ds.createModel('post', { + id: { type: Number, id: true }, + title: String, + content: String + }) + app.model(Post) + + Comment = ds.createModel('comment', { + id: { type: Number, id: true }, + postId: Number, + authorId: Number, + title: String, + comment: String + }) + + app.model(Comment) + + Author = ds.createModel('author', { + id: { type: Number, id: true }, + name: String + }) + + app.model(Author) + + Post.hasMany(Comment, { as: 'comments', foreignKey: 'postId' }) + Comment.belongsTo(Author, { as: 'author', foreignKey: 'authorId' }) + Comment.settings.plural = 'comments' + + app.use(loopback.rest()) + JSONAPIComponent(app, { restApiRoot: '/' }) + }) + + describe('include defined at model level', function () { + beforeEach(function (done) { + Post.create( + { + title: 'my post', + content: 'my post content' + }, + function (err, post) { + expect(err).to.equal(null) + Author.create( + { + name: 'Joe' + }, + function (err, author) { + post.comments.create( + { + title: 'My comment', + comment: 'My comment text', + authorId: author.getId() + }, + function () { + post.comments.create( + { + title: 'My second comment', + comment: 'My second comment text', + authorId: author.getId() + }, + done + ) + } + ) + } + ) + } + ) + }) + + describe('response', function () { + it('should include author of comments', function (done) { + request(app) + .get('/posts/1?filter[include][comments]=author') + .end(function (err, res) { + expect(err).to.equal(null) + expect(res.body.included).to.be.an('array').of.length(3) + res.body.included.sort((a, b) => a.type > b.type) + expect(res.body.included[0].type).to.equal('authors') + expect(res.body.included[1].type).to.equal('comments') + expect(res.body.included[2].type).to.equal('comments') + + expect(res.body.included[1].relationships.author.data.id).to.equal( + '1' + ) + expect(res.body.included[2].relationships.author.data.id).to.equal( + '1' + ) + done() + }) + }) + + it('attributes of comments should not have relationship key', function ( + done + ) { + request(app) + .get('/posts/1?filter[include][comments]=author') + .end(function (err, res) { + expect(err).to.equal(null) + res.body.included.sort((a, b) => a.type > b.type) + expect(res.body.included[1].attributes).to.not.include.key( + 'author' + ) + done() + }) + }) + }) + }) +}) From 0b3c89c0f2ae867a9acb5a1ce66ae969480d17e7 Mon Sep 17 00:00:00 2001 From: Jeremy Trufier Date: Tue, 21 Aug 2018 14:52:35 +0200 Subject: [PATCH 3/4] test(nested-include): fix travis issue --- test/nestedInclude.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/nestedInclude.test.js b/test/nestedInclude.test.js index 968f966..c334fd9 100644 --- a/test/nestedInclude.test.js +++ b/test/nestedInclude.test.js @@ -57,13 +57,15 @@ describe('nested include option', function () { name: 'Joe' }, function (err, author) { + expect(err).to.equal(null) post.comments.create( { title: 'My comment', comment: 'My comment text', authorId: author.getId() }, - function () { + function (err) { + expect(err).to.equal(null) post.comments.create( { title: 'My second comment', From dc36ee0fe6723499376d9ec38ebc0ae1061d5c06 Mon Sep 17 00:00:00 2001 From: Jeremy Trufier Date: Tue, 21 Aug 2018 15:03:15 +0200 Subject: [PATCH 4/4] ci(travis): node@4 is in End-Of-Life, test long term supported versions --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 194fcc7..27c2913 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,9 +6,9 @@ cache: notifications: email: false node_js: + - '10' - '8' - '6' - - '4' before_script: - npm prune - 'npm i -g jsinspect'