Merge pull request #1642 from strongloop/fix-4028

Report errors for missing id properties for include

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;

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);

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,

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'},