-
8000
Notifications
You must be signed in to change notification settings - Fork 963
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
base: master
Are you sure you want to change the base?
Prepend fix for #4159 #4169
Conversation
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) |
There was a problem hiding this comment.
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
query.Where(func(selector *sql.Selector) { | ||
selector.AppendSelectExprAs(sql.Expr("min(3, 5)"), key) | ||
}) | ||
}).AllX(ctx) |
There was a problem hiding this comment.
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.,
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.
There was a problem hiding this 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.
@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. |
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 inSelectedColumns
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!