Support $beforeUpdate() mutations in upsertGraph()

Closes Vincit#2233

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,
+};

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;

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;

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');
 

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', () => {