fix(lucid:relations): keep relation output consistent

eager loading relations has inconsistent output, whereas it needs to be consistent Closes #45
adonisjs · Aug 12, 2016 · c74e081 · c74e081
1 parent 4d72794
commit c74e081
Show file tree

Hide file tree

Showing 7 changed files with 268 additions and 6 deletions.
diff --git a/src/Lucid/Model/Mixins/Serializer.js b/src/Lucid/Model/Mixins/Serializer.js
@@ -44,7 +44,7 @@ Serializer.toJSON = function () {
  */
 Serializer.serializeRelations = function () {
   return _.transform(this.relations, function (result, value, index) {
-    result[index] = value && value.toJSON() ? value.toJSON() : value
+    result[index] = _.size(value) && value.toJSON() ? value.toJSON() : value
   })
 }
 

diff --git a/src/Lucid/Relations/BelongsTo.js b/src/Lucid/Relations/BelongsTo.js
@@ -22,6 +22,19 @@ class BelongsTo extends Relation {
     this.fromKey = foreignKey || this.related.foreignKey
   }
 
+  /**
+   * empty placeholder to be used when unable to eagerload
+   * relations. It needs to be an array of many to many
+   * relationships.
+   *
+   * @method eagerLoadFallbackValue
+   *
+   * @return {Null}
+   */
+  get eagerLoadFallbackValue () {
+    return null
+  }
+
   /**
    * returns result of this.first
    *

diff --git a/src/Lucid/Relations/EagerLoad.js b/src/Lucid/Relations/EagerLoad.js
@@ -155,9 +155,10 @@ class EagerLoad {
    *
    * @private
    */
-  _getJoinKeyValues (mappedKeys, mappedKeysValues, relation, relationInstance, values) {
+  _getJoinKeyValues (mappedKeys, mappedKeysValues, fallbackValues, relation, relationInstance, values) {
     if (!mappedKeys[relation]) {
       mappedKeys[relation] = relationInstance.fromKey
+      fallbackValues[relation] = relationInstance.eagerLoadFallbackValue
       if (_.isArray(values)) {
         mappedKeysValues[relation] = values.map((value) => value[relationInstance.fromKey])
       } else {
@@ -221,6 +222,7 @@ class EagerLoad {
   load (result, parent, isSingle) {
     const self = this
     const mappedKeys = {}
+    const fallbackValues = {}
     const mappedKeysValues = {}
 
     /**
@@ -230,7 +232,7 @@ class EagerLoad {
     const response = cf.map(function * (relation) {
       const relationScope = self.relationsScope[relation]
       const relationInstance = isSingle ? self._getRelationInstance(relation, parent) : self._getRelationProtoInstance(relation, parent)
-      const values = self._getJoinKeyValues(mappedKeys, mappedKeysValues, relation, relationInstance, result)
+      const values = self._getJoinKeyValues(mappedKeys, mappedKeysValues, fallbackValues, relation, relationInstance, result)
 
       /**
        * here we pass nested relations and scopes to the relationInstance
@@ -245,7 +247,7 @@ class EagerLoad {
       return yield relationInstance.eagerLoad(values, relationScope)
     }, this.withRelations)
 
-    return {values: response, keys: mappedKeys}
+    return {values: response, keys: mappedKeys, fallbackValues}
   }
 
   /**
@@ -262,12 +264,13 @@ class EagerLoad {
     if (_.size(eagerLoadResult)) {
       const keys = eagerLoadResult.keys
       const values = eagerLoadResult.values
+      const fallbackValues = eagerLoadResult.fallbackValues
       const relationsArray = Object.keys(keys)
 
       _.each(values, (value, index) => {
         const relationKey = relationsArray[index]
         const fromKey = keys[relationKey]
-        parent.relations[relationKey] = value[row[fromKey]] || null
+        parent.relations[relationKey] = value[row[fromKey]] || fallbackValues[relationKey]
       })
     }
   }

diff --git a/src/Lucid/Relations/HasOne.js b/src/Lucid/Relations/HasOne.js
@@ -20,6 +20,19 @@ class HasOne extends Relation {
     this.toKey = foreignKey || this.parent.constructor.foreignKey
   }
 
+  /**
+   * empty placeholder to be used when unable to eagerload
+   * relations. It needs to be an array of many to many
+   * relationships.
+   *
+   * @method eagerLoadFallbackValue
+   *
+   * @return {Null}
+   */
+  get eagerLoadFallbackValue () {
+    return null
+  }
+
   /**
    * returns result of this.first
    *

diff --git a/src/Lucid/Relations/Relation.js b/src/Lucid/Relations/Relation.js
@@ -26,6 +26,19 @@ class Relation {
     return new Proxy(this, proxyHandler)
   }
 
+  /**
+   * empty placeholder to be used when unable to eagerload
+   * relations. It needs to be an array of many to many
+   * relationships.
+   *
+   * @method eagerLoadFallbackValue
+   *
+   * @return {Array}
+   */
+  get eagerLoadFallbackValue () {
+    return []
+  }
+
   /**
    * returns the model from IoC container or returns
    * the actual binding if model representation is

diff --git a/test/unit/fixtures/relations.js b/test/unit/fixtures/relations.js
@@ -88,6 +88,13 @@ module.exports = {
         table.timestamps()
         table.timestamp('deleted_at').nullable()
       }),
+      knex.schema.createTable('subjects', function (table) {
+        table.increments()
+        table.string('title')
+        table.integer('course_id')
+        table.timestamps()
+        table.timestamp('deleted_at').nullable()
+      }),
       knex.schema.createTable('course_student', function (table) {
         table.integer('student_id')
         table.integer('course_id')
@@ -132,6 +139,7 @@ module.exports = {
       knex.schema.dropTable('comments'),
       knex.schema.dropTable('courses'),
       knex.schema.dropTable('students'),
+      knex.schema.dropTable('subjects'),
       knex.schema.dropTable('course_student'),
       knex.schema.dropTable('authors'),
       knex.schema.dropTable('publications'),

diff --git a/test/unit/lucid.relations.spec.js b/test/unit/lucid.relations.spec.js
@@ -293,7 +293,7 @@ describe('Relations', function () {
       yield relationFixtures.truncate(Database, 'accounts')
     })
 
-    it('should return null when unable to fetch related results', function * () {
+    it('should return null when unable to fetch related results via eager loading', function * () {
       yield relationFixtures.createRecords(Database, 'suppliers', {name: 'redtape'})
       class Account extends Model {
       }
@@ -2831,4 +2831,216 @@ describe('Relations', function () {
       }
     })
   })
+
+  context('Regression:HasOne', function () {
+    it('should return null when unable to fetch related results via eager loading', function * () {
+      yield relationFixtures.createRecords(Database, 'suppliers', {name: 'redtape'})
+      class Account extends Model {
+      }
+      class Supplier extends Model {
+        account () {
+          return this.hasOne(Account)
+        }
+      }
+      const supplier = yield Supplier.query().with('account').first()
+      expect(supplier.toJSON().account).to.equal(null)
+      yield relationFixtures.truncate(Database, 'suppliers')
+    })
+
+    it('should return null when unable to fetch related results of the model instance', function * () {
+      yield relationFixtures.createRecords(Database, 'suppliers', {name: 'redtape'})
+      class Account extends Model {
+      }
+      class Supplier extends Model {
+        account () {
+          return this.hasOne(Account)
+        }
+      }
+      const supplier = yield Supplier.find(1)
+      const account = yield supplier.account().first()
+      expect(account).to.equal(null)
+      yield relationFixtures.truncate(Database, 'suppliers')
+    })
+
+    it('should return null when unable to fetch related results via lazy eager loading', function * () {
+      yield relationFixtures.createRecords(Database, 'suppliers', {name: 'redtape'})
+      class Account extends Model {
+      }
+      class Supplier extends Model {
+        account () {
+          return this.hasOne(Account)
+        }
+      }
+      const supplier = yield Supplier.find(1)
+      yield supplier.related('account').load()
+      expect(supplier.toJSON().account).to.equal(null)
+      yield relationFixtures.truncate(Database, 'suppliers')
+    })
+  })
+
+  context('Regression:BelongsTo', function () {
+    it('should return null when unable to fetch related results via eager loading', function * () {
+      yield relationFixtures.createRecords(Database, 'comments', {body: 'Nice article', post_id: 1})
+      class Post extends Model {
+      }
+      class Comment extends Model {
+        post () {
+          return this.belongsTo(Post)
+        }
+      }
+      const comment = yield Comment.query().with('post').first()
+      expect(comment.toJSON().post).to.equal(null)
+      yield relationFixtures.truncate(Database, 'comments')
+    })
+
+    it('should return null when unable to fetch related results of model instance', function * () {
+      yield relationFixtures.createRecords(Database, 'comments', {body: 'Nice article', post_id: 1})
+      class Post extends Model {
+      }
+      class Comment extends Model {
+        post () {
+          return this.belongsTo(Post)
+        }
+      }
+      const comment = yield Comment.query().first()
+      const post = yield comment.post().first()
+      expect(post).to.equal(null)
+      yield relationFixtures.truncate(Database, 'comments')
+    })
+
+    it('should return null when unable to fetch related results via lazy eager loading', function * () {
+      yield relationFixtures.createRecords(Database, 'comments', {body: 'Nice article', post_id: 1})
+      class Post extends Model {
+      }
+      class Comment extends Model {
+        post () {
+          return this.belongsTo(Post)
+        }
+      }
+      const comment = yield Comment.query().first()
+      yield comment.related('post').load()
+      expect(comment.toJSON().post).to.equal(null)
+      yield relationFixtures.truncate(Database, 'comments')
+    })
+  })
+
+  context('Regression:HasMany', function () {
+    it('should return an empty array when unable to fetch related results via eager loading', function * () {
+      yield relationFixtures.createRecords(Database, 'posts', {title: 'Adonis 101', body: 'Let\'s learn Adonis'})
+      class Comment extends Model {
+      }
+      class Post extends Model {
+        comments () {
+          return this.hasMany(Comment)
+        }
+      }
+      const post = yield Post.query().with('comments').first()
+      expect(post.toJSON().comments).deep.equal([])
+      yield relationFixtures.truncate(Database, 'posts')
+    })
+
+    it('should return an empty array when unable to fetch related results of model instance', function * () {
+      yield relationFixtures.createRecords(Database, 'posts', {title: 'Adonis 101', body: 'Let\'s learn Adonis'})
+      class Comment extends Model {
+      }
+      class Post extends Model {
+        comments () {
+          return this.hasMany(Comment)
+        }
+      }
+      const post = yield Post.query().first()
+      const comments = yield post.comments().fetch()
+      expect(comments.toJSON()).deep.equal([])
+      yield relationFixtures.truncate(Database, 'posts')
+    })
+
+    it('should return an empty array when unable to fetch related results via lazy eager loading', function * () {
+      yield relationFixtures.createRecords(Database, 'posts', {title: 'Adonis 101', body: 'Let\'s learn Adonis'})
+      class Comment extends Model {
+      }
+      class Post extends Model {
+        comments () {
+          return this.hasMany(Comment)
+        }
+      }
+      const post = yield Post.query().first()
+      yield post.related('comments').load()
+      expect(post.toJSON().comments).deep.equal([])
+      yield relationFixtures.truncate(Database, 'posts')
+    })
+  })
+
+  context('Regression:BelongsToMany', function () {
+    it('should return an empty array when unable to fetch related results via eager loading', function * () {
+      yield relationFixtures.createRecords(Database, 'students', {name: 'ricky', id: 29})
+      class Course extends Model {
+      }
+      class Student extends Model {
+        courses () {
+          return this.belongsToMany(Course)
+        }
+      }
+      const student = yield Student.query().with('courses').first()
+      expect(student.toJSON().courses).deep.equal([])
+      yield relationFixtures.truncate(Database, 'students')
+    })
+
+    it('should return an empty array when unable to fetch related results of model instance', function * () {
+      yield relationFixtures.createRecords(Database, 'students', {name: 'ricky', id: 29})
+      class Course extends Model {
+      }
+      class Student extends Model {
+        courses () {
+          return this.belongsToMany(Course)
+        }
+      }
+      const student = yield Student.query().first()
+      const courses = yield student.courses().fetch()
+      expect(courses.toJSON()).deep.equal([])
+      yield relationFixtures.truncate(Database, 'students')
+    })
+
+    it('should return an empty array when unable to fetch related results via lazy eager loading', function * () {
+      yield relationFixtures.createRecords(Database, 'students', {name: 'ricky', id: 29})
+      class Course extends Model {
+      }
+      class Student extends Model {
+        courses () {
+          return this.belongsToMany(Course)
+        }
+      }
+      const student = yield Student.query().first()
+      yield student.related('courses').load()
+      expect(student.toJSON().courses).deep.equal([])
+      yield relationFixtures.truncate(Database, 'students')
+    })
+
+    it('should return expected output with nested relations', function * () {
+      const savedStudent = yield relationFixtures.createRecords(Database, 'students', {name: 'ricky', id: 29})
+      const savedCourse = yield relationFixtures.createRecords(Database, 'courses', {title: 'geometry'})
+      yield relationFixtures.createRecords(Database, 'course_student', {student_id: savedStudent[0], course_id: savedCourse[0]})
+
+      class Subject extends Model {
+      }
+
+      class Course extends Model {
+        subject () {
+          return this.hasOne(Subject) // situational for testing only
+        }
+      }
+
+      class Student extends Model {
+        courses () {
+          return this.belongsToMany(Course)
+        }
+      }
+
+      const student = yield Student.query().with('courses.subject').first()
+      expect(student.toJSON().courses).to.be.an('array')
+      expect(student.toJSON().courses[0].subject).to.equal(null)
+      yield relationFixtures.truncate(Database, 'students')
+      yield relationFixtures.truncate(Database, 'courses')
+      yield relationFixtures.truncate(Database, 'course_student')
+    })
+  })
 })