Support $beforeUpdate() mutations in upsertGraph()

Closes #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,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;

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()) {