From 40f23a57602c604f5c144c22a2815d38d7957927 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ju=CC=88rg=20Lehni?= Date: Tue, 13 Feb 2024 10:29:43 +0100 Subject: [PATCH] Fix upsertGraph() $beforeUpdate() calls on empty relates (#2605) Closes #2605 --- .../graph/patch/GraphPatchAction.js | 17 +++++++-- testUtils/TestSession.js | 6 +-- tests/integration/upsertGraph.js | 38 ++++++++++++++----- 3 files changed, 42 insertions(+), 19 deletions(-) diff --git a/lib/queryBuilder/graph/patch/GraphPatchAction.js b/lib/queryBuilder/graph/patch/GraphPatchAction.js index 34a8a9298..33f3df339 100644 --- a/lib/queryBuilder/graph/patch/GraphPatchAction.js +++ b/lib/queryBuilder/graph/patch/GraphPatchAction.js @@ -3,7 +3,7 @@ const { getModel } = require('../../../model/getModel'); const { GraphAction } = require('../GraphAction'); const { isInternalProp } = require('../../../utils/internalPropUtils'); -const { union, difference, isObject, jsonEquals } = require('../../../utils/objectUtils'); +const { difference, isObject, jsonEquals } = require('../../../utils/objectUtils'); const promiseUtils = require('../../../utils/promiseUtils'); class GraphPatchAction extends GraphAction { @@ -33,7 +33,7 @@ class GraphPatchAction extends GraphAction { const handleUpdate = () => { const { changedProps, unchangedProps } = this._findChanges(node); - const allProps = union(changedProps, unchangedProps); + const allProps = [...changedProps, ...unchangedProps]; const propsToUpdate = difference( shouldPatch || shouldUpdate @@ -59,12 +59,16 @@ class GraphPatchAction extends GraphAction { // 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. + // called before calling handleUpdate, but only if the node contains changes + // that aren't id properties (relates). See issues #2233, #2605. const hasBeforeUpdate = node.obj.constructor.beforeUpdate !== Model.beforeUpdate || node.obj.$beforeUpdate !== Model.prototype.$beforeUpdate; - if (!hasBeforeUpdate && !handleUpdate()) { + if ( + (hasBeforeUpdate && !this._hasNonIdPropertyChanges(node)) || + (!hasBeforeUpdate && !handleUpdate()) + ) { return null; } @@ -207,6 +211,11 @@ class GraphPatchAction extends GraphAction { }; } + _hasNonIdPropertyChanges(node) { + const idProps = node.modelClass.getIdPropertyArray(); + return this._findChanges(node).changedProps.some((prop) => !idProps.includes(prop)); + } + _createBuilder(node) { if (node.parentEdge && !node.parentEdge.relation.isObjectionHasManyRelation) { return this._createRelatedBuilder(node); diff --git a/testUtils/TestSession.js b/testUtils/TestSession.js index 6ff5c50c0..f1466e495 100644 --- a/testUtils/TestSession.js +++ b/testUtils/TestSession.js @@ -429,11 +429,7 @@ function createHook(name, delay, extraAction) { } function inc(obj, key) { - if (!_.has(obj, key)) { - obj[key] = 1; - } else { - obj[key]++; - } + obj[key] = (obj[key] || 0) + 1; } function registerUnhandledRejectionHandler() { diff --git a/tests/integration/upsertGraph.js b/tests/integration/upsertGraph.js index cbde48cf9..5ac663a38 100644 --- a/tests/integration/upsertGraph.js +++ b/tests/integration/upsertGraph.js @@ -261,7 +261,7 @@ module.exports = (session) => { } } - expect(result.$beforeUpdateCalled).to.equal(1); + expect(result.$beforeUpdateCalled).to.equal(undefined); 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(1); + expect(result.model1Relation1.$beforeUpdateCalled).to.equal(undefined); 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(1); + expect(result.model1Relation1.$beforeUpdateCalled).to.equal(undefined); 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( - 1, + undefined, ); if (session.isPostgres()) { @@ -4116,9 +4116,9 @@ module.exports = (session) => { model1Prop2: 101, }; - return transaction(session.knex, (trx) => - Model1.query(trx).upsertGraph(upsert, { fetchStrategy }), - ) + return transaction(session.knex, (trx) => { + return Model1.query(trx).upsertGraph(upsert, { fetchStrategy }); + }) .then((res) => { expect(res.model1Prop1).to.equal('updated in before update'); expect(res.model1Prop2).to.equal(101); @@ -4152,13 +4152,31 @@ module.exports = (session) => { }); }; - await transaction(session.knex, (trx) => - Model1.query(trx).upsertGraph(upsert, { fetchStrategy }), - ); + await transaction(session.knex, (trx) => { + return Model1.query(trx).upsertGraph(upsert, { fetchStrategy }); + }); expect(error).to.equal(null); }); }); + describe('should not call $beforeUpdate() on empty relates (#2605)', () => { + it('should not call $beforeUpdate() on empty relates', async () => { + const upsert = { + id: 2, + model1Relation1: { id: 3 }, + }; + + await transaction(session.knex, (trx) => { + return Model1.query(trx) + .upsertGraph(upsert, { relate: true, fetchStrategy }) + .then((result) => { + expect(result.$beforeUpdateCalled).to.equal(undefined); + expect(result.model1Relation1.$beforeUpdateCalled).to.equal(undefined); + }); + }); + }); + }); + if (session.isPostgres()) { describe('returning', () => { it('should propagate returning(*) to all update an insert operations', () => {