diff --git a/lib/model/getModel.js b/lib/model/getModel.js new file mode 100644 index 000000000..e0a7f116c --- /dev/null +++ b/lib/model/getModel.js @@ -0,0 +1,9 @@ +'use strict'; + +// A small helper method for cached lazy-importing of the Model class. +let Model; +const getModel = () => Model || (Model = require('./Model').Model); + +module.exports = { + getModel, +}; diff --git a/lib/queryBuilder/QueryBuilder.js b/lib/queryBuilder/QueryBuilder.js index 2efba7cac..1dceb8702 100644 --- a/lib/queryBuilder/QueryBuilder.js +++ b/lib/queryBuilder/QueryBuilder.js @@ -1070,11 +1070,11 @@ function beforeExecute(builder) { // Resolve all before hooks before building and executing the query // and the rest of the hooks. promise = chainOperationHooks(promise, builder, 'onBefore1'); + promise = chainOperationHooks(promise, builder, 'onBefore2'); promise = chainHooks(promise, builder, builder.context().runBefore); promise = chainHooks(promise, builder, builder.internalContext().runBefore); - promise = chainOperationHooks(promise, builder, 'onBefore2'); promise = chainOperationHooks(promise, builder, 'onBefore3'); return promise; diff --git a/lib/queryBuilder/graph/patch/GraphPatchAction.js b/lib/queryBuilder/graph/patch/GraphPatchAction.js index a0891f1a8..b41fe3a1d 100644 --- a/lib/queryBuilder/graph/patch/GraphPatchAction.js +++ b/lib/queryBuilder/graph/patch/GraphPatchAction.js @@ -1,5 +1,6 @@ 'use strict'; +const { getModel } = require('../../../model/getModel'); const { GraphAction } = require('../GraphAction'); const { isInternalProp } = require('../../../utils/internalPropUtils'); const { union, difference, isObject, jsonEquals } = require('../../../utils/objectUtils'); @@ -30,21 +31,41 @@ class GraphPatchAction extends GraphAction { // properties. That's why we handle them here. const changePropsBecauseOfBelongsToOneDelete = this._handleBelongsToOneDeletes(node); - const { changedProps, unchangedProps } = this._findChanges(node); - const allProps = union(changedProps, unchangedProps); - - const propsToUpdate = difference( - shouldPatch || shouldUpdate - ? changedProps - : [...changedPropsBecauseOfBelongsToOneInsert, ...changePropsBecauseOfBelongsToOneDelete], + const handleUpdate = () => { + const { changedProps, unchangedProps } = this._findChanges(node); + const allProps = union(changedProps, unchangedProps); + + const propsToUpdate = difference( + shouldPatch || shouldUpdate + ? changedProps + : [...changedPropsBecauseOfBelongsToOneInsert, ...changePropsBecauseOfBelongsToOneDelete], + + // Remove id properties from the props to update. With upsertGraph + // it never makes sense to change the id. + node.modelClass.getIdPropertyArray() + ); + + const update = propsToUpdate.length > 0; + if (update) { + // Don't update the fields that we know not to change. + node.obj.$omitFromDatabaseJson(difference(allProps, propsToUpdate)); + node.userData.updated = true; + } - // Remove id properties from the props to update. With upsertGraph - // it never makes sense to change the id. - node.modelClass.getIdPropertyArray() - ); + return update; + }; - if (propsToUpdate.length === 0) { - return null; + const Model = getModel(); + // See if the model defines a beforeUpdate or $beforeUpdate hook. If it does + // not, we can check for updated properties now and drop out immediately if + // there is nothing to update. Otherwise, we need to wait for the hook to be + // called before calling handleUpdate. See issue #2233. + const hasBeforeUpdate = + node.obj.constructor.beforeUpdate !== Model.beforeUpdate || + node.obj.$beforeUpdate !== Model.prototype.$beforeUpdate; + + if (!hasBeforeUpdate && !handleUpdate()) { + return false; } delete node.obj[node.modelClass.uidProp]; @@ -56,14 +77,31 @@ class GraphPatchAction extends GraphAction { patch: shouldPatch || (!shouldPatch && !shouldUpdate), }); - // Don't update the fields that we know not to change. - node.obj.$omitFromDatabaseJson(difference(allProps, propsToUpdate)); - node.userData.updated = true; - const updateBuilder = this._createBuilder(node) - .childQueryOf(builder) + .childQueryOf(builder, childQueryOptions()) .copyFrom(builder, GraphAction.ReturningAllSelector); + if (hasBeforeUpdate) { + updateBuilder + .context({ + runBefore: () => { + // Call handleUpdate in the runBefore hook which runs after the + // $beforeUpdate hook, allowing it to modify the object before the + // updated properties are determined. See issue #2233. + if (hasBeforeUpdate && !handleUpdate()) { + throw new ReturnNullException(); + } + }, + }) + .onError((error) => { + if (error instanceof ReturnNullException) { + return null; + } else { + return Promise.reject(error); + } + }); + } + if (shouldPatch) { updateBuilder.patch(node.obj); } else { @@ -197,6 +235,15 @@ class GraphPatchAction extends GraphAction { } } +class ReturnNullException {} + +function childQueryOptions() { + return { + fork: true, + isInternalQuery: true, + }; +} + function nonStrictEquals(val1, val2) { if (val1 == val2) { return true; diff --git a/lib/relations/manyToMany/ManyToManyRelation.js b/lib/relations/manyToMany/ManyToManyRelation.js index 188f548e8..34864a33d 100644 --- a/lib/relations/manyToMany/ManyToManyRelation.js +++ b/lib/relations/manyToMany/ManyToManyRelation.js @@ -1,6 +1,6 @@ 'use strict'; -const getModel = () => require('../../model/Model').Model; +const { getModel } = require('../../model/getModel'); const { Relation } = require('../Relation'); const { RelationProperty } = require('../RelationProperty'); diff --git a/tests/integration/upsertGraph.js b/tests/integration/upsertGraph.js index 3453ae145..fbf388e20 100644 --- a/tests/integration/upsertGraph.js +++ b/tests/integration/upsertGraph.js @@ -261,7 +261,7 @@ module.exports = (session) => { } } - expect(result.$beforeUpdateCalled).to.equal(undefined); + expect(result.$beforeUpdateCalled).to.equal(1); expect(result.$afterUpdateCalled).to.equal(undefined); expect(result.model1Relation1.$beforeUpdateCalled).to.equal(1); @@ -501,7 +501,7 @@ module.exports = (session) => { expect(result.$beforeUpdateCalled).to.equal(1); expect(result.$afterUpdateCalled).to.equal(1); - expect(result.model1Relation1.$beforeUpdateCalled).to.equal(undefined); + expect(result.model1Relation1.$beforeUpdateCalled).to.equal(1); expect(result.model1Relation1.$afterUpdateCalled).to.equal(undefined); }); }) @@ -577,7 +577,7 @@ module.exports = (session) => { expect(result.$beforeUpdateCalled).to.equal(1); expect(result.$afterUpdateCalled).to.equal(1); - expect(result.model1Relation1.$beforeUpdateCalled).to.equal(undefined); + expect(result.model1Relation1.$beforeUpdateCalled).to.equal(1); expect(result.model1Relation1.$afterUpdateCalled).to.equal(undefined); }); }) @@ -733,7 +733,7 @@ module.exports = (session) => { }) .then((result) => { expect(result.model1Relation2[0].model2Relation1[2].$beforeUpdateCalled).to.equal( - undefined + 1 ); if (session.isPostgres()) {