Fix upsertGraph() $beforeUpdate() calls on empty relates (#2605)

Closes #2605
Vincit · Feb 13, 2024 · c5f6dc4 · c5f6dc4
1 parent 1ecdc39
commit c5f6dc4
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 19 deletions.
diff --git 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
@@ -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
@@ -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', () => {