From 7c98ecbb772d967e46ac306e6ea14a72322b1a41 Mon Sep 17 00:00:00 2001 From: regevbr Date: Sat, 30 Nov 2019 15:19:37 +0200 Subject: [PATCH] Fix bug #1795 Count issues related models --- lib/scope.js | 290 ++++++++++++++++++++++------ test/relations.test.js | 418 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 654 insertions(+), 54 deletions(-) diff --git a/lib/scope.js b/lib/scope.js index decefde3c..8d79f3ae2 100644 --- a/lib/scope.js +++ b/lib/scope.js @@ -91,7 +91,7 @@ ScopeDefinition.prototype.related = function(receiver, scopeParams, condOrRefres actualRefresh; if (refreshIsNeeded) { // It either doesn't hit the cache or refresh is required - const params = mergeQuery(actualCond, scopeParams, {nestedInclude: true}); + let params = mergeQuery(actualCond, scopeParams, {nestedInclude: true}); const targetModel = this.targetModel(receiver); // If there is a through model @@ -100,64 +100,39 @@ ScopeDefinition.prototype.related = function(receiver, scopeParams, condOrRefres const scopeOnRelatedModel = params.collect && params.include.scope !== null && typeof params.include.scope === 'object'; - let filter, queryRelated; + let filter, queryRelated, keyFrom, relatedModel, IdKey; if (scopeOnRelatedModel) { filter = params.include; // The filter applied on relatedModel queryRelated = filter.scope; - delete params.include.scope; + delete params.include; + relatedModel = targetModel.relations[filter.relation].modelTo; + IdKey = idName(relatedModel); + keyFrom = targetModel.relations[filter.relation].keyFrom || IdKey; + params.fields = [keyFrom]; + if (queryRelated && queryRelated.where && queryRelated.where[IdKey]) { + params = mergeQuery(params, { + where: { + [keyFrom]: queryRelated.where [IdKey], + }, + }); + } } - targetModel.find(params, options, function(err, data) { + targetModel.find(params, options, function(err, findData) { if (!err && saveOnCache) { defineCachedRelations(self); - self.__cachedRelations[name] = data; + self.__cachedRelations[name] = findData; } if (scopeOnRelatedModel === true) { - const relatedModel = targetModel.relations[filter.relation].modelTo; - const IdKey = idName(relatedModel); - - // return {inq: [1,2,3]}} - const smartMerge = function(idCollection, qWhere) { - if (!qWhere[IdKey]) return idCollection; - let merged = {}; - - const idsA = idCollection.inq; - const idsB = qWhere[IdKey].inq ? qWhere[IdKey].inq : [qWhere[IdKey]]; - - const intersect = _.intersectionWith(idsA, idsB, _.isEqual); - if (intersect.length === 1) merged = intersect[0]; - if (intersect.length > 1) merged = {inq: intersect}; - - return merged; - }; - - if (queryRelated.where !== undefined) { - // Merge queryRelated filter and targetId filter - const IdKeyCondition = {}; - IdKeyCondition[IdKey] = smartMerge(collectTargetIds(data, IdKey), - queryRelated.where); - - // if the id in filter doesn't exist after the merge, - // return empty result - if (_.isObject(IdKeyCondition[IdKey]) && _.isEmpty(IdKeyCondition[IdKey])) return cb(null, []); - - const mergedWhere = { - and: [ - IdKeyCondition, - _.omit(queryRelated.where, IdKey), - ], - }; - queryRelated.where = mergedWhere; - } else { - queryRelated.where = {}; - queryRelated.where[IdKey] = collectTargetIds(data, IdKey); + const smartMergeSuccessful = smartMergeRelatedModelQuery(findData, queryRelated, keyFrom, IdKey); + if (!smartMergeSuccessful) { + return cb(null, []); } - relatedModel.find(queryRelated, options, cb); } else { - cb(err, data); + cb(err, findData); } }); } else { @@ -298,6 +273,16 @@ function defineScope(cls, targetClass, name, params, methods, options) { options = {}; } options = options || {}; + // Check if there is a through model + // see https://github.com/strongloop/loopback/issues/1076 + if (f._scope.collect && + condOrRefresh !== null && typeof condOrRefresh === 'object') { + f._scope.include = { + relation: f._scope.collect, + scope: condOrRefresh, + }; + condOrRefresh = {}; + } return definition.related(self, f._scope, condOrRefresh, options, cb); }; @@ -431,11 +416,49 @@ function defineScope(cls, targetClass, name, params, methods, options) { options = {}; } options = options || {}; - + cb = cb || utils.createPromiseCallback(); const targetModel = definition.targetModel(this._receiver); + // If there is a through model + // run another query to apply filter on relatedModel(targetModel) + // see github.com/strongloop/loopback-datasource-juggler/issues/1795 + let scopeOnRelatedModel = false; + let queryRelated, keyFrom, relatedModel, IdKey, fieldsRelated; + if (this._scope && this._scope.collect && + where !== null && typeof where === 'object') { + queryRelated = { + relation: this._scope.collect, + scope: { + where: where, + }, + }; + scopeOnRelatedModel = true; + relatedModel = targetModel.relations[queryRelated.relation].modelTo; + IdKey = idName(relatedModel); + keyFrom = targetModel.relations[queryRelated.relation].keyFrom || IdKey; + fieldsRelated = [keyFrom]; + if (where[IdKey]) { + where = { + [keyFrom]: where[IdKey], + }; + } else { + where = {}; + } + } const scoped = (this._scope && this._scope.where) || {}; - const filter = mergeQuery({where: scoped}, {where: where || {}}); - return targetModel.destroyAll(filter.where, options, cb); + const filter = mergeQuery({where: scoped}, {where: where || {}, fields: fieldsRelated}); + if (!scopeOnRelatedModel) { + targetModel.destroyAll(filter.where, options, cb); + } else { + targetModel.find(filter, options, function(err, findData) { + const smartMergeSuccessful = + smartMergeRelatedModelQuery(findData, queryRelated.scope, keyFrom, IdKey); + if (!smartMergeSuccessful) { + return cb(null, {count: 0}); + } + return relatedModel.destroyAll(queryRelated.scope.where, options, cb); + }); + } + return cb.promise; } function updateAll(where, data, options, cb) { @@ -452,10 +475,49 @@ function defineScope(cls, targetClass, name, params, methods, options) { options = {}; } options = options || {}; + cb = cb || utils.createPromiseCallback(); const targetModel = definition.targetModel(this._receiver); + // If there is a through model + // run another query to apply filter on relatedModel(targetModel) + // see github.com/strongloop/loopback-datasource-juggler/issues/1795 + let scopeOnRelatedModel = false; + let queryRelated, keyFrom, relatedModel, IdKey, fieldsRelated; + if (this._scope && this._scope.collect && + where !== null && typeof where === 'object') { + queryRelated = { + relation: this._scope.collect, + scope: { + where: where, + }, + }; + scopeOnRelatedModel = true; + relatedModel = targetModel.relations[queryRelated.relation].modelTo; + IdKey = idName(relatedModel); + keyFrom = targetModel.relations[queryRelated.relation].keyFrom || IdKey; + fieldsRelated = [keyFrom]; + if (where[IdKey]) { + where = { + [keyFrom]: where[IdKey], + }; + } else { + where = {}; + } + } const scoped = (this._scope && this._scope.where) || {}; - const filter = mergeQuery({where: scoped}, {where: where || {}}); - return targetModel.updateAll(filter.where, data, options, cb); + const filter = mergeQuery({where: scoped}, {where: where || {}, fields: fieldsRelated}); + if (!scopeOnRelatedModel) { + targetModel.updateAll(filter.where, data, options, cb); + } else { + targetModel.find(filter, options, function(err, findData) { + const smartMergeSuccessful = + smartMergeRelatedModelQuery(findData, queryRelated.scope, keyFrom, IdKey); + if (!smartMergeSuccessful) { + return cb(null, {count: 0}); + } + return relatedModel.updateAll(queryRelated.scope.where, data, options, cb); + }); + } + return cb.promise; } function findById(id, filter, options, cb) { @@ -500,10 +562,51 @@ function defineScope(cls, targetClass, name, params, methods, options) { options = {}; } options = options || {}; + cb = cb || utils.createPromiseCallback(); const targetModel = definition.targetModel(this._receiver); + // If there is a through model + // run another query to apply filter on relatedModel(targetModel) + // see github.com/strongloop/loopback-datasource-juggler/issues/1795 + let scopeOnRelatedModel = false; + let queryRelated, keyFrom, relatedModel, IdKey; + if (this._scope && this._scope.collect && + filter && filter.where !== null && typeof filter.where === 'object') { + queryRelated = { + relation: this._scope.collect, + scope: filter, + }; + scopeOnRelatedModel = true; + relatedModel = targetModel.relations[queryRelated.relation].modelTo; + IdKey = idName(relatedModel); + keyFrom = targetModel.relations[queryRelated.relation].keyFrom || IdKey; + if (filter.where[IdKey]) { + filter = { + fields: [keyFrom], + where: { + [keyFrom]: filter.where [IdKey], + }, + }; + } else { + filter = { + fields: [keyFrom], + }; + } + } const scoped = (this._scope && this._scope.where) || {}; filter = mergeQuery({where: scoped}, filter || {}); - return targetModel.findOne(filter, options, cb); + if (!scopeOnRelatedModel) { + targetModel.findOne(filter, options, cb); + } else { + targetModel.find(filter, options, function(err, findData) { + const smartMergeSuccessful = + smartMergeRelatedModelQuery(findData, queryRelated.scope, keyFrom, IdKey); + if (!smartMergeSuccessful) { + return cb(null, null); + } + return relatedModel.findOne(queryRelated.scope, options, cb); + }); + } + return cb.promise; } function count(where, options, cb) { @@ -517,12 +620,91 @@ function defineScope(cls, targetClass, name, params, methods, options) { options = {}; } options = options || {}; - + cb = cb || utils.createPromiseCallback(); const targetModel = definition.targetModel(this._receiver); + // If there is a through model + // run another query to apply filter on relatedModel(targetModel) + // see github.com/strongloop/loopback-datasource-juggler/issues/1795 + let scopeOnRelatedModel = false; + let queryRelated, keyFrom, relatedModel, IdKey, fieldsRelated; + if (this._scope && this._scope.collect && + where !== null && typeof where === 'object') { + queryRelated = { + relation: this._scope.collect, + scope: { + where: where, + }, + }; + scopeOnRelatedModel = true; + relatedModel = targetModel.relations[queryRelated.relation].modelTo; + IdKey = idName(relatedModel); + keyFrom = targetModel.relations[queryRelated.relation].keyFrom || IdKey; + fieldsRelated = [keyFrom]; + if (where[IdKey]) { + where = { + [keyFrom]: where[IdKey], + }; + } else { + where = {}; + } + } const scoped = (this._scope && this._scope.where) || {}; - const filter = mergeQuery({where: scoped}, {where: where || {}}); - return targetModel.count(filter.where, options, cb); + const filter = mergeQuery({where: scoped}, {where: where || {}, fields: fieldsRelated}); + if (!scopeOnRelatedModel) { + targetModel.count(filter.where, options, cb); + } else { + targetModel.find(filter, options, function(err, findData) { + const smartMergeSuccessful = + smartMergeRelatedModelQuery(findData, queryRelated.scope, keyFrom, IdKey); + if (!smartMergeSuccessful) { + return cb(null, 0); + } + return relatedModel.count(queryRelated.scope.where, options, cb); + }); + } + return cb.promise; } return definition; } + +function smartMergeRelatedModelQuery(findData, queryRelated, keyFrom, IdKey) { + const smartMerge = function(idCollection, qWhere) { + if (!qWhere[IdKey]) return idCollection; + let merged = {}; + + const idsA = idCollection.inq; + const idsB = qWhere[IdKey].inq ? qWhere[IdKey].inq : [qWhere[IdKey]]; + + const intersect = _.intersectionWith(idsA, idsB, _.isEqual); + if (intersect.length === 1) merged = intersect[0]; + if (intersect.length > 1) merged = {inq: intersect}; + + return merged; + }; + + if (queryRelated.where !== undefined) { + // Merge queryRelated filter and targetId filter + const IdKeyCondition = {}; + IdKeyCondition[IdKey] = smartMerge(collectTargetIds(findData, keyFrom), + queryRelated.where); + + // if the id in filter doesn't exist after the merge, + // return empty result + if (_.isObject(IdKeyCondition[IdKey]) && _.isEmpty(IdKeyCondition[IdKey])) { + return false; + } + + const mergedWhere = { + and: [ + IdKeyCondition, + _.omit(queryRelated.where, IdKey), + ], + }; + queryRelated.where = mergedWhere; + } else { + queryRelated.where = {}; + queryRelated.where[IdKey] = collectTargetIds(findData, keyFrom); + } + return true; +} diff --git a/test/relations.test.js b/test/relations.test.js index 4a7eac2c7..e2b0aae45 100644 --- a/test/relations.test.js +++ b/test/relations.test.js @@ -605,6 +605,424 @@ describe('relations', function() { db.automigrate(['Physician', 'Patient', 'Appointment', 'Address'], done); }); + describe('scoped queries', function() { + describe('find', function() { + it('should allow to use fields on related model', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a', age: 5}); + const address = await Address.create({name: 'z'}); + patient.address(address); + await patient.save(); + const ch = await physician.patients({fields: 'age'}); + should.exist(ch); + ch.should.have.lengthOf(1); + ch[0].age.should.eql(5); + should.not.exist(ch[0].name); + }); + + it('should find related model using model id', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a', age: 5}); + await physician.patients.create({name: 'b', age: 5}); + await physician.patients.create({name: 'c', age: 5}); + const ch = await physician.patients({where: {id: patient.id}}); + should.exist(ch); + ch.should.have.lengthOf(1); + ch[0].age.should.eql(5); + }); + + it('should find related model using model id list', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a', age: 5}); + await physician.patients.create({name: 'b', age: 5}); + await physician.patients.create({name: 'c', age: 5}); + const ch = await physician.patients({where: {id: {inq: [patient.id]}}}); + should.exist(ch); + ch.should.have.lengthOf(1); + ch[0].age.should.eql(5); + }); + + it('should find scoped record with promises based on related model properties', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patients = await physician.patients({ + where: { + name: 'a', + }, + }); + should.exist(patients); + patients.should.have.lengthOf(1); + patients[0].name.should.equal('a'); + }); + + it('should find scoped record with promises based on related model' + + ' properties with empty results', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patients = await physician.patients({ + where: { + id: 'bar', + }, + }); + should.exist(patients); + patients.should.have.lengthOf(0); + }); + + it('should find scoped record with promises based on related model id', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patients = await physician.patients({ + where: { + id: patient.id, + }, + }); + should.exist(patients); + patients.should.have.lengthOf(1); + patients[0].name.should.equal('a'); + }); + + it('should find scoped record with promises based on related model id list', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patients = await physician.patients({ + where: { + id: { + inq: [patient.id], + }, + }, + }); + should.exist(patients); + patients.should.have.lengthOf(1); + patients[0].name.should.equal('a'); + }); + }); + + describe('find explicit', function() { + it('should find (explicit) scoped record with promises based on related' + + ' model properties', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patients = await physician.patients({ + where: { + name: 'a', + }, + }); + should.exist(patients); + patients.should.have.lengthOf(1); + patients[0].name.should.equal('a'); + }); + + it('should find (explicit) scoped record with promises based on related model' + + ' properties with empty results', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patients = await physician.patients({ + where: { + id: 'bar', + }, + }); + should.exist(patients); + patients.should.have.lengthOf(0); + }); + + it('should find (explicit) scoped record with promises based on related model id', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patients = await physician.patients({ + where: { + id: patient.id, + }, + }); + should.exist(patients); + patients.should.have.lengthOf(1); + patients[0].name.should.equal('a'); + }); + + it('should find (explicit) scoped record with promises based on ' + + 'related model id list', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patients = await physician.patients({ + where: { + id: { + inq: [patient.id], + }, + }, + }); + should.exist(patients); + patients.should.have.lengthOf(1); + patients[0].name.should.equal('a'); + }); + }); + + describe('count', function() { + it('should count scoped record with promises based on related model properties', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const count = await physician.patients.count({ + name: 'a', + }); + should.exist(count); + count.should.equal(1); + }); + + it('should count scoped record with promises based on related model ' + + 'properties with no results', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const count = await physician.patients.count({ + id: 'bar', + }); + should.exist(count); + count.should.equal(0); + }); + + it('should count scoped record with promises based on related model id', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const count = await physician.patients.count({ + id: patient.id, + }); + should.exist(count); + count.should.equal(1); + }); + + it('should count scoped record with promises based on related model id list', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const count = await physician.patients.count({ + id: { + inq: [patient.id], + }, + }); + should.exist(count); + count.should.equal(1); + }); + }); + + describe('find one', function() { + it('should find one scoped record with promises based on related model properties', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patient = await physician.patients.findOne({ + where: { + name: 'a', + }, + }); + should.exist(patient); + patient.name.should.equal('a'); + }); + + it('should find one scoped record with promises based on related model' + + ' properties with empty results', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patient = await physician.patients.findOne({ + where: { + id: 'bar', + }, + }); + should.not.exist(patient); + }); + + it('should find one scoped record with promises based on related model id', async function() { + const physician = await Physician.create(); + const createdPatient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patient = await physician.patients.findOne({ + where: { + id: createdPatient.id, + }, + }); + should.exist(patient); + patient.name.should.equal('a'); + }); + + it('should find one scoped record with promises based on related model id list', async function() { + const physician = await Physician.create(); + const createdPatient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const patient = await physician.patients.findOne({ + where: { + id: { + inq: [createdPatient.id], + }, + }, + }); + should.exist(patient); + patient.name.should.equal('a'); + }); + }); + + describe('update all', function() { + it('should update all scoped record with promises based on related model properties', + async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const result = await physician.patients.updateAll({ + name: 'a', + }, { + age: 5, + }); + should.exist(result); + result.count.should.equal(1); + const patient = await physician.patients.findOne({ + where: { + name: 'a', + }, + }); + should.exist(patient); + patient.age.should.equal(5); + }); + + it('should update all scoped record with promises based on related ' + + 'model properties no results', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const result = await physician.patients.updateAll({ + id: 'bar', + }, { + age: 5, + }); + should.exist(result); + result.count.should.equal(0); + }); + + it('should update all scoped record with promises based on related model id', async function() { + const physician = await Physician.create(); + const createdPatient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const result = await physician.patients.updateAll({ + id: createdPatient.id, + }, { + age: 5, + }); + should.exist(result); + result.count.should.equal(1); + const patient = await physician.patients.findOne({ + where: { + name: 'a', + }, + }); + should.exist(patient); + patient.age.should.equal(5); + }); + + it('should update all scoped record with promises based on related model id list', async function() { + const physician = await Physician.create(); + const createdPatient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const result = await physician.patients.updateAll({ + id: { + inq: [createdPatient.id], + }, + }, { + age: 5, + }); + should.exist(result); + result.count.should.equal(1); + const patient = await physician.patients.findOne({ + where: { + name: 'a', + }, + }); + should.exist(patient); + patient.age.should.equal(5); + }); + }); + + describe('destroy all', function() { + it('should destroyAll all scoped record with promises based on ' + + 'related model properties', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const result = await physician.patients.destroyAll({ + name: 'a', + }); + should.exist(result); + result.count.should.equal(1); + }); + + it('should destroyAll all scoped record with promises based on related ' + + 'model properties no results', async function() { + const physician = await Physician.create(); + await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const result = await physician.patients.destroyAll({ + id: 'foo', + }); + should.exist(result); + result.count.should.equal(0); + }); + + it('should destroyAll all scoped record with promises based on related model id', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const result = await physician.patients.destroyAll({ + id: patient.id, + }); + should.exist(result); + result.count.should.equal(1); + }); + + it('should destroyAll all scoped record with promises based on ' + + 'related model id list', async function() { + const physician = await Physician.create(); + const patient = await physician.patients.create({name: 'a'}); + await physician.patients.create({name: 'z'}); + await physician.patients.create({name: 'c'}); + const result = await physician.patients.destroyAll({ + id: { + inq: [patient.id], + }, + }); + should.exist(result); + result.count.should.equal(1); + }); + }); + }); + it('should build record on scope', function(done) { Physician.create(function(err, physician) { const patient = physician.patients.build();