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.

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

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

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

lehni
Copy link
Collaborator

@lehni lehni commented Jul 4, 2023

Closes #2233

Comment on lines +1073 to -1077
promise = chainOperationHooks(promise, builder, 'onBefore2');

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

promise = chainOperationHooks(promise, builder, 'onBefore2');
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

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

@lehni lehni force-pushed the feature/upsert-graph-before-update-mutations branch 2 times, most recently from 027868d to 0b9499a Compare July 5, 2023 07:38
@umutuzgur
Copy link

Hey @lehni 👋 Are you planning to merge and release this soon? We are looking forward to update finally to the latest objection version 😬

Copy link
Contributor

@falkenhawk falkenhawk left a comment

Choose a reason for hiding this comment

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

I've just run our test suites on a project which uses upsertGraph a lot, with the changes from this PR applied and nothing broke. I also didn't observe any performance degradation, so it looks good 👍

@lehni
Copy link
Collaborator Author

lehni commented Jul 12, 2023

Good news! I still need to write a test for this, but will merge it soon.

@lehni lehni self-assigned this Jul 12, 2023
@lehni lehni force-pushed the feature/upsert-graph-before-update-mutations branch from 0b9499a to 4df4955 Compare July 21, 2023 08:44
@lehni
Copy link
Collaborator Author

lehni commented Jul 21, 2023

I have improved the code to detect the presence of beforeUpdate hooks, and revert to the old behavior of dropping out earlier if there are none. This is to avoid any potential performance implications or other potential changes in behavior.

@lehni lehni force-pushed the feature/upsert-graph-before-update-mutations branch from 4df4955 to bf98cfb Compare July 21, 2023 08:47
@lehni lehni force-pushed the feature/upsert-graph-before-update-mutations branch from bf98cfb to 3940e7d Compare July 21, 2023 09:44
@lehni lehni merged commit 8218e96 into main Jul 21, 2023
4 checks passed
@lehni lehni deleted the feature/upsert-graph-before-update-mutations branch July 21, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't add or modify a value during $beforeUpdate using upsertGraph
3 participants