Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support $beforeUpdate() mutations in upsertGraph() #2452

Merged
merged 1 commit into from
Jul 21, 2023
Merged

Support $beforeUpdate() mutations in upsertGraph() #2452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/model/getModel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
};
2 changes: 1 addition & 1 deletion lib/queryBuilder/QueryBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Comment on lines +1073 to -1077
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit afraid of this change - could you tell more about why the execution order of hooks had to be changed to fix the issue, please?

Copy link
Collaborator Author

@lehni lehni Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need runBefore to run after onBefore2() in UpdateOperation which triggers the execution of $beforeUpdate(). I tried it and it didn't break any tests. I looked at afterExecute(), and it has the same sequence.

In the end, this is just a proof of concept more than anything for now. I do think that the behavior it produces is more according to what I would expect though.

function afterExecute(builder, result) {
  let promise = Promise.resolve(result);

  promise = chainOperationHooks(promise, builder, 'onAfter1');
  promise = chainOperationHooks(promise, builder, 'onAfter2');

  promise = chainHooks(promise, builder, builder.context().runAfter);
  promise = chainHooks(promise, builder, builder.internalContext().runAfter);

  promise = chainOperationHooks(promise, builder, 'onAfter3');

  return promise;
}

promise = chainOperationHooks(promise, builder, 'onBefore3');

return promise;
Expand Down
79 changes: 63 additions & 16 deletions lib/queryBuilder/graph/patch/GraphPatchAction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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');
Expand Down Expand Up @@ -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;
}

Expand All @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion lib/relations/manyToMany/ManyToManyRelation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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');

Expand Down
43 changes: 39 additions & 4 deletions tests/integration/upsertGraph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
});
})
Expand Down Expand Up @@ -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);
});
})
Expand Down Expand Up @@ -733,7 +733,7 @@ module.exports = (session) => {
})
.then((result) => {
expect(result.model1Relation2[0].model2Relation1[2].$beforeUpdateCalled).to.equal(
undefined
1
);

if (session.isPostgres()) {
Expand Down Expand Up @@ -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', () => {
Expand Down