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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix propagating post-save mutations #2524

Conversation

booleangate
Copy link
Contributor

@booleangate booleangate commented May 5, 2022

This addresses the bug I reported in #2523. I've broken commits up so you can review just the first two in isolation (a failing test, and the fix) to cut out all the noise from the regeneration.

馃摀 I had to branch my changes off of the fix/bump-atlas branch in order to be able to run go generate ./.... I'm just waiting for that to merge, then I'll rebase.

@masseelch masseelch requested review from a8m and masseelch May 5, 2022 04:23
Copy link
Member

@a8m a8m left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @booleangate! The changes look good.

Please, fix the minor suggestions and I'll merge this PR. Also note, that the changes need to be applied on the other hook types as well (update, update-one, etc). Would you like to contribute them in different PRs?

entc/gen/template/builder/create.tmpl Outdated Show resolved Hide resolved
}, ent.OpCreate))

u := client.User.Create().SetName("a8m").SetVersion(1).SaveX(ctx)
require.Equal(t, u.Version, 2, "version mutation in hook should have propagated back to call site")
Copy link
Member

Choose a reason for hiding this comment

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

馃挴

entc/integration/hooks/hooks_test.go Outdated Show resolved Hide resolved
entc/integration/hooks/hooks_test.go Outdated Show resolved Hide resolved
Co-authored-by: Ariel Mashraki <7413593+a8m@users.noreply.github.com>
@booleangate
Copy link
Contributor Author

booleangate commented May 5, 2022

Also note, that the changes need to be applied on the other hook types as well (update, update-one, etc). Would you like to contribute them in different PRs?

@a8m I'm afraid I don't follow but I'm happy to learn. The signature on the update and delete clients is Save(...) (int, error) and Exec(...) (int, error) respectively. Because they return affected row count and not the model itself, I don't see how we could make a similar change to those clients.

@a8m
Copy link
Member

a8m commented May 5, 2022

Also note, that the changes need to be applied on the other hook types as well (update, update-one, etc). Would you like to contribute them in different PRs?

@a8m I'm afraid I don't follow but I'm happy to learn. The signature on the update and delete clients is Save(...) (int, error) and Exec(...) (int, error) respectively. Because they return affected row count and not the model itself, I don't see how we could make a similar change to those clients.

Let's start with update-one - https://github.com/ent/ent/blob/master/entc/gen/template/builder/update.tmpl#L200-L202

@a8m
Copy link
Member

a8m commented May 5, 2022

CI is red. Please, run go generate ./... from the root of the project. Thanks 馃檹

@masseelch masseelch deleted the branch ent:fix/bump-atlas May 5, 2022 06:30
@masseelch masseelch closed this May 5, 2022
@a8m
Copy link
Member

a8m commented May 5, 2022

@booleangate can you rebase your branch on master and create a new PR?

@booleangate
Copy link
Contributor Author

@a8m yup, working on a new PR now with the UpdateOne changes.

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.

None yet

3 participants