Skip to content

Commit

Permalink
fix(eagerloading): eagerloading with .first should behave same as .fetch
Browse files Browse the repository at this point in the history
The `.first` took a shortcut and instead lazily eagerloaded the relationships, causing some runtime
constraints to fail. Now the behavior is same for both `.first` and `.fetch`
  • Loading branch information
thetutlage committed Dec 7, 2017
1 parent 6abaa6a commit 9bcb09d
Show file tree
Hide file tree
Showing 6 changed files with 23 additions and 15 deletions.
2 changes: 1 addition & 1 deletion src/Lucid/EagerLoad/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class EagerLoad {
const relationsKeys = _.keys(this._relations)

/**
* Since all rows will become to the same user, we just need
* Since all rows will belongs to the same user, we just need
* any one instance for reading some properties of the
* instance.
*/
Expand Down
8 changes: 1 addition & 7 deletions src/Lucid/QueryBuilder/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -327,13 +327,7 @@ class QueryBuilder {
}

const modelInstance = this._mapRowToInstance(row)

/**
* Eagerload relations when defined on query
*/
if (_.size(this._eagerLoads)) {
await modelInstance.loadMany(this._eagerLoads)
}
await this._eagerLoad([modelInstance])

if (this.Model.$hooks) {
await this.Model.$hooks.after.exec('find', modelInstance)
Expand Down
6 changes: 5 additions & 1 deletion src/Lucid/Relations/BaseRelation.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,11 @@ class BaseRelation {
* @return {Object}
*/
async eagerLoad (rows) {
this._eagerLoadFn(this.relatedQuery, this.foreignKey, this.mapValues(rows))
const mappedRows = this.mapValues(rows)
if (!mappedRows || !mappedRows.length) {
return this.group([])
}
this._eagerLoadFn(this.relatedQuery, this.foreignKey, mappedRows)
const relatedInstances = await this.relatedQuery.fetch()
return this.group(relatedInstances.rows)
}
Expand Down
7 changes: 6 additions & 1 deletion src/Lucid/Relations/BelongsToMany.js
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,11 @@ class BelongsToMany extends BaseRelation {
* @return {Object}
*/
async eagerLoad (rows) {
this._eagerLoadFn(this.relatedQuery, this.foreignKey, this.mapValues(rows))
const mappedRows = this.mapValues(rows)
if (!mappedRows || !mappedRows.length) {
return this.group([])
}
this._eagerLoadFn(this.relatedQuery, this.foreignKey, mappedRows)
const relatedInstances = await this.relatedQuery.fetch()
return this.group(relatedInstances.rows)
}
Expand Down Expand Up @@ -616,6 +620,7 @@ class BelongsToMany extends BaseRelation {
const transformedValues = _.transform(relatedInstances, (result, relatedInstance) => {
const foreignKeyValue = relatedInstance.$sideLoaded[`pivot_${this.foreignKey}`]
const existingRelation = _.find(result, (row) => row.identity === foreignKeyValue)
this._addPivotValuesAsRelation(relatedInstance)

/**
* If there is an existing relation, add row to
Expand Down
7 changes: 6 additions & 1 deletion src/Lucid/Relations/HasManyThrough.js
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,12 @@ class HasManyThrough extends BaseRelation {
* @return {Object}
*/
async eagerLoad (rows) {
this._eagerLoadFn(this.relatedQuery, this.foreignKey, this.mapValues(rows), {
const mappedRows = this.mapValues(rows)
if (!mappedRows || !mappedRows.length) {
return this.group([])
}

this._eagerLoadFn(this.relatedQuery, this.foreignKey, mappedRows, {
foreignTable: this.$foreignTable,
foreignKey: this.foreignKey
})
Expand Down
8 changes: 4 additions & 4 deletions test/unit/lucid-has-many.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ test.group('Relations | Has Many', (group) => {
assert.instanceOf(user.getRelated('cars'), VanillaSerializer)
assert.equal(user.getRelated('cars').size(), 2)
assert.deepEqual(user.getRelated('cars').rows.map((car) => car.$parent), ['User', 'User'])
assert.equal(carQuery.sql, helpers.formatQuery('select * from "cars" where "user_id" = ?'))
assert.equal(carQuery.sql, helpers.formatQuery('select * from "cars" where "user_id" in (?)'))
assert.deepEqual(carQuery.bindings, helpers.formatBindings([1]))
})

Expand Down Expand Up @@ -318,7 +318,7 @@ test.group('Relations | Has Many', (group) => {
assert.equal(user.getRelated('cars').size(), 2)
assert.equal(user.getRelated('cars').first().getRelated('parts').size(), 2)
assert.equal(user.getRelated('cars').last().getRelated('parts').size(), 2)
assert.equal(carQuery.sql, helpers.formatQuery('select * from "cars" where "user_id" = ?'))
assert.equal(carQuery.sql, helpers.formatQuery('select * from "cars" where "user_id" in (?)'))
assert.equal(partQuery.sql, helpers.formatQuery('select * from "parts" where "car_id" in (?, ?)'))
})

Expand Down Expand Up @@ -363,7 +363,7 @@ test.group('Relations | Has Many', (group) => {
assert.equal(user.getRelated('cars').size(), 2)
assert.equal(user.getRelated('cars').first().getRelated('parts').size(), 1)
assert.equal(user.getRelated('cars').last().getRelated('parts').size(), 1)
assert.equal(carQuery.sql, helpers.formatQuery('select * from "cars" where "user_id" = ?'))
assert.equal(carQuery.sql, helpers.formatQuery('select * from "cars" where "user_id" in (?)'))
assert.equal(partQuery.sql, helpers.formatQuery('select * from "parts" where "part_name" = ? and "car_id" in (?, ?)'))
})

Expand Down Expand Up @@ -410,7 +410,7 @@ test.group('Relations | Has Many', (group) => {

assert.equal(user.getRelated('cars').size(), 1)
assert.equal(user.getRelated('cars').first().getRelated('parts').size(), 1)
assert.equal(carQuery.sql, helpers.formatQuery('select * from "cars" where "name" = ? and "user_id" = ?'))
assert.equal(carQuery.sql, helpers.formatQuery('select * from "cars" where "name" = ? and "user_id" in (?)'))
assert.equal(partQuery.sql, helpers.formatQuery('select * from "parts" where "part_name" = ? and "car_id" in (?)'))
})

Expand Down

0 comments on commit 9bcb09d

Please sign in to comment.