From 60e16298f245dc62d885728a667516e5c430559d Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Thu, 18 Oct 2018 10:32:16 -0700 Subject: [PATCH] Report errors for missing id props for include https://github.com/strongloop/loopback/issues/4028 --- lib/include.js | 18 +++++++++++++++--- lib/include_utils.js | 18 ++++++++++++++++-- test/include.test.js | 28 ++++++++++++++++++++++++++++ test/include_util.test.js | 22 ++++++++++++++++++++++ 4 files changed, 81 insertions(+), 5 deletions(-) diff --git a/lib/include.js b/lib/include.js index 04282738..195c5e3a 100644 --- a/lib/include.js +++ b/lib/include.js @@ -14,6 +14,7 @@ var isPlainObject = utils.isPlainObject; var defineCachedRelations = utils.defineCachedRelations; var uniq = utils.uniq; var idName = utils.idName; +var debug = require('debug')('loopback:include'); /*! * Normalize the include to be an array @@ -172,6 +173,7 @@ Inclusion.include = function(objects, include, options, cb) { } include = normalizeInclude(include); + debug('include: %j', include); // Find the limit of items for `inq` var inqLimit = 256; @@ -181,8 +183,14 @@ Inclusion.include = function(objects, include, options, cb) { } async.each(include, function(item, callback) { - processIncludeItem(objects, item, options, callback); + try { + processIncludeItem(objects, item, options, callback); + } catch (err) { + // async does not catch the error and report to the outer callback + callback(err); + } }, function(err) { + debug(err, objects); cb && cb(err, objects); }); @@ -293,6 +301,7 @@ Inclusion.include = function(objects, include, options, cb) { cb(new Error(g.f('Relation "%s" is not defined for %s model', relationName, self.modelName))); return; } + debug('Relation: %j', relation); var polymorphic = relation.polymorphic; // if (polymorphic && !polymorphic.discriminator) { // cb(new Error('Relation "' + relationName + '" is polymorphic but ' + @@ -309,6 +318,7 @@ Inclusion.include = function(objects, include, options, cb) { // Just skip if inclusion is disabled if (relation.options.disableInclude) { + debug('Relation is disabled from include', relation); return cb(); } // prepare filter and fields for making DB Call @@ -870,10 +880,10 @@ Inclusion.include = function(objects, include, options, cb) { for (var i = 0; i < objs.length; i++) { var obj = objs[i]; if (relation.type === 'belongsTo') { - if (obj[relation.keyFrom] === null || - obj[relation.keyFrom] === undefined) { + if (obj[relation.keyFrom] == null) { defineCachedRelations(obj); obj.__cachedRelations[relationName] = null; + debug('ID property "%s" is missing in item %j', relation.keyFrom, obj); continue; } } @@ -883,6 +893,8 @@ Inclusion.include = function(objects, include, options, cb) { var targetIdStr = targetId.toString(); objTargetIdMap[targetIdStr] = objTargetIdMap[targetIdStr] || []; objTargetIdMap[targetIdStr].push(obj); + } else { + debug('ID property "%s" is missing in item %j', relation.keyFrom, obj); } defineCachedRelations(obj); obj.__cachedRelations[relationName] = null; diff --git a/lib/include_utils.js b/lib/include_utils.js index 536e6258..70d70597 100644 --- a/lib/include_utils.js +++ b/lib/include_utils.js @@ -10,6 +10,20 @@ module.exports.buildOneToManyIdentityMapWithOrigKeys = buildOneToManyIdentityMap module.exports.join = join; module.exports.KVMap = KVMap; +const util = require('util'); + +function getId(obj, idName) { + var id = obj && obj[idName]; + if (id == null) { + const msg = util.format('ID property "%s" is missing for included item: %j. ' + + 'Please make sure `fields` include "%s" if it\'s present in the `filter`', + idName, obj, idName); + const err = new Error(msg); + err.statusCode = 400; + throw err; + } + return id; +} /** * Effectively builds associative map on id -> object relation and stores original keys. * Map returned in form of object with ids in keys and object as values. @@ -22,7 +36,7 @@ function buildOneToOneIdentityMapWithOrigKeys(objs, idName) { var kvMap = new KVMap(); for (var i = 0; i < objs.length; i++) { var obj = objs[i]; - var id = obj[idName]; + var id = getId(obj, idName); kvMap.set(id, obj); } return kvMap; @@ -32,7 +46,7 @@ function buildOneToManyIdentityMapWithOrigKeys(objs, idName) { var kvMap = new KVMap(); for (var i = 0; i < objs.length; i++) { var obj = objs[i]; - var id = obj[idName]; + var id = getId(obj, idName); var value = kvMap.get(id) || []; value.push(obj); kvMap.set(id, value); diff --git a/test/include.test.js b/test/include.test.js index 6092bf27..e41e6f2c 100644 --- a/test/include.test.js +++ b/test/include.test.js @@ -46,6 +46,15 @@ describe('include', function() { }); }); + it('does not return included item if FK is excluded', function(done) { + Passport.find({include: 'owner', fields: 'number'}, function(err, passports) { + if (err) return done(err); + var owner = passports[0].toJSON().owner; + should.not.exist(owner); + done(); + }); + }); + it('should fetch hasMany relation', function(done) { User.find({include: 'posts'}, function(err, users) { should.not.exist(err); @@ -67,6 +76,14 @@ describe('include', function() { }); }); + it('should report errors if the PK is excluded', function(done) { + User.find({include: 'posts', fields: 'posts'}, function(err) { + should.exist(err); + err.message.should.match(/ID property "id" is missing/); + done(); + }); + }); + it('should not have changed the __strict flag of the model', function(done) { const originalStrict = User.definition.settings.strict; User.definition.settings.strict = true; // Change to test regression for issue #1252 @@ -579,6 +596,16 @@ describe('include', function() { }); }); + it('does not return included item if hasOne is missing the id property', function(done) { + User.findOne({include: {relation: 'profile'}, fields: 'name'}, function(err, user) { + if (err) return done(err); + should.exist(user); + // Convert to JSON as the user instance has `profile` as a relational method + should.not.exist(user.toJSON().profile); + done(); + }); + }); + it('works when hasMany is called', function(done) { User.findOne({include: {relation: 'posts'}}, function(err, user) { if (err) return done(); @@ -1487,6 +1514,7 @@ function setup(done) { }); Passport = db.define('Passport', { number: String, + expirationDate: Date, }); Post = db.define('Post', { title: String, diff --git a/test/include_util.test.js b/test/include_util.test.js index cca11720..545af718 100644 --- a/test/include_util.test.js +++ b/test/include_util.test.js @@ -21,6 +21,17 @@ describe('include_util', function() { result.get(22).should.be.ok; }); + it('should report errors if id is missing', function() { + var objs = [ + {letter: 'A'}, + {id: 22, letter: 'B'}, + ]; + function build() { + includeUtils.buildOneToOneIdentityMapWithOrigKeys(objs, 'id'); + } + build.should.throw(/ID property "id" is missing/); + }); + it('should overwrite keys in case of collision', function() { var objs = [ {id: 11, letter: 'A'}, @@ -58,6 +69,17 @@ describe('include_util', function() { result.exist(22).should.be.true; }); + it('should report errors if id is missing', function() { + var objs = [ + {letter: 'A'}, + {id: 22, letter: 'B'}, + ]; + function build() { + includeUtils.buildOneToManyIdentityMapWithOrigKeys(objs, 'id'); + } + build.should.throw(/ID property "id" is missing/); + }); + it('should collect keys in case of collision', function() { var objs = [ {'fk_id': 11, letter: 'A'},