8000 Prepend fix for #4159 by gebes · Pull Request #4169 · ent/ent · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Prepend fix for #4159 #4169

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Prepend fix for #4159 #4169

wants to merge 4 commits into from

Conversation

gebes
Copy link
@gebes gebes commented Aug 10, 2024

This PR fixes an issue #4159 with the M2M load function where additional select expressions added in the With{Node} query functions were being lost. The problem occurred because the selected columns were copied, the join column was selected, and then the selected columns were added back—but the expressions aren't included in SelectedColumns

By switching to prepending instead of appending + copying, we keep the expressions, so everything works as expected. This change has been tested in several production projects, and all tests are passing.

I have also added comments to explain the new prepend functions.

Thanks for taking a look!

@gebes gebes requested a review from a8m August 10, 2024 12:57
const key = "computed_value"
users := client.User.Query().WithGroups(func(query *ent.GroupQuery) {
query.Where(func(selector *sql.Selector) {
selector.AppendSelectExprAs(sql.Expr("min(3, 5)"), key)
Copy link
Author

Choose a reason for hiding this comment

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

The main issue was that expressions (which contained computed fields, function calls, etc) where discarded and hence not selected @a8m

Comment on lines +2887 to +2890
query.Where(func(selector *sql.Selector) {
selector.AppendSelectExprAs(sql.Expr("min(3, 5)"), key)
})
}).AllX(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

If you need to modify the query selection, use the query modifiers. e.g.,

Suggested change
query.Where(func(selector *sql.Selector) {
selector.AppendSelectExprAs(sql.Expr("min(3, 5)"), key)
})
}).AllX(ctx)
query.Modify(func(s *sql.Selector) {
s.AppendSelectExprAs(sql.Expr("min(3, 5)"), key)
})
}).AllX(ctx)

This should solve your issue. Please, try update the test and see if this works without changing the codegen.

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.

See my suggested change and let me know if this solves your issue.

@gebes
Copy link
Author
gebes commented Aug 10, 2024

@a8m It resolved the issue, thanks so much!

I would still suggest merging this PR, though, as it makes the behavior consistent. O2O and O2M don't have this issue, but M2M does, so this change would be a nice way to align everything and as a bonus it reduces unnecessary copying

@a8m
Copy link
Member
a8m commented Aug 10, 2024

@a8m It resolved the issue, thanks so much!

I would still suggest merging this PR, though, as it makes the behavior consistent. O2O and O2M don't have this issue, but M2M does, so this change would be a nice way to align everything and as a bonus it reduces unnecessary copying

Thanks for the suggestion, but I prefer to avoid incorrect usage of the API. If we "officially" support it, that means we need to support it for all methods, not just for custom selections. Instead, users can use the right API.

@gebes
Copy link
Author
gebes commented Aug 10, 2024

@a8m It resolved the issue, thanks so much!
I would still suggest merging this PR, though, as it makes the behavior consistent. O2O and O2M don't have this issue, but M2M does, so this change would be a nice way to align everything and as a bonus it reduces unnecessary copying

Thanks for the suggestion, but I prefer to avoid incorrect usage of the API. If we "officially" support it, that means we need to support it for all methods, not just for custom selections. Instead, users can use the right API.

I understand your point about the correct API usage, and I didn't mean officially support this. I am aware that internal APIs can change frequently. My suggestion was just focused on achieving the same result with cleaner code and reduced allocations. So, I would see this PR as a minor code improvement rather than an official bug fix if my mistake was incorrect API usage.

I would be happy if you could merge this PR to appreciate the effort

I'm also curious about the project's guidelines on clean code and optimizing performance by reducing allocations/reducing code. There are some code optimizations that could be made to the generated code, and I'm wondering about the process for making changes. If it's challenging to make such a small adjustment, how do you see the project progressing towards the 1.0 release? Of course, ent shouldn't move fast and break things, but steady quality improvements are still important and better than none imo, as this doesn't break anything.

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.

2 participants
0