From 3940e7d954aa884ee77b40a100c651395fc1263d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ju=CC=88rg=20Lehni?= Date: Fri, 21 Jul 2023 10:44:16 +0200 Subject: [PATCH] Support $beforeUpdate() mutations in upsertGraph() Closes #2233 --- lib/model/getModel.js | 9 +++ lib/queryBuilder/QueryBuilder.js | 2 +- .../graph/patch/GraphPatchAction.js | 79 +++++++++++++++---- .../manyToMany/ManyToManyRelation.js | 2 +- tests/integration/upsertGraph.js | 43 +++++++++- 5 files changed, 113 insertions(+), 22 deletions(-) create mode 100644 lib/model/getModel.js 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..39c8891ef 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,20 +31,40 @@ 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 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; + } - const propsToUpdate = difference( - shouldPatch || shouldUpdate - ? changedProps - : [...changedPropsBecauseOfBelongsToOneInsert, ...changePropsBecauseOfBelongsToOneDelete], + return update; + }; - // Remove id properties from the props to update. With upsertGraph - // it never makes sense to change the id. - node.modelClass.getIdPropertyArray() - ); + 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 (propsToUpdate.length === 0) { + if (!hasBeforeUpdate && !handleUpdate()) { return null; } @@ -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..4e8f3f721 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()) { @@ -4096,6 +4096,41 @@ module.exports = (session) => { }); }); + describe('modifying properties in $beforeUpdate (#2233)', () => { + let $beforeUpdate; + + before(() => { + $beforeUpdate = Model1.prototype.$beforeUpdate; + Model1.prototype.$beforeUpdate = function () { + this.model1Prop1 = 'updated in before update'; + }; + }); + + after(() => { + Model1.prototype.$beforeUpdate = $beforeUpdate; + }); + + it('should include modified properties in update', () => { + const upsert = { + id: 1, + model1Prop2: 101, + }; + + return transaction(session.knex, (trx) => + Model1.query(trx).upsertGraph(upsert, { fetchStrategy }) + ) + .then((res) => { + expect(res.model1Prop1).to.equal('updated in before update'); + expect(res.model1Prop2).to.equal(101); + return Model1.query(session.knex).findById(1); + }) + .then((model) => { + expect(model.model1Prop1).to.equal('updated in before update'); + expect(model.model1Prop2).to.equal(101); + }); + }); + }); + if (session.isPostgres()) { describe('returning', () => { it('should propagate returning(*) to all update an insert operations', () => {