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

Fix eq_any's nullability #4354

Merged
merged 4 commits into from
Nov 22, 2024
Merged

Conversation

Ten0
Copy link
Member

@Ten0 Ten0 commented Nov 20, 2024

PG returns null for IN expressions if LHS is null or if result depends on a NULL inner element of the array.
The return value of the IN & co expressions was incorrectly represented as non-nullable in this case.

While this is a breaking change, it should probably also be considered a bugfix, so we should be able to release it.

Copy link

@kw217 kw217 left a comment

Choose a reason for hiding this comment

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

Code looks good, and the test is very clear - thank you. I agree this is technically a bugfix, but I fear it has the potential to cause quite a bit of breakage so I'm nervous about releasing it. Do we have any way of checking impact (e.g., anything like crater?). Anyone who has a nullable column that actually never has null in it, and uses eq_any, will have been getting away with this and will be surprised by the change.

@Ten0
Copy link
Member Author

Ten0 commented Nov 21, 2024

I imagine most uses of eq_any are actually to filter, and because filter accepts both they will keep compiling.
We have 500 uses of eq_any in our codebase and all of those are .filter.

Copy link

@kw217 kw217 left a comment

Choose a reason for hiding this comment

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

Fair enough. In the first 30 or so repo hits in a quick GitHub search all but one instance was inside a filter; the exception was inside an order: https://github.com/johnmai-dev/newbee-mall-api-rs/blob/f46091f75fb5be7962e4c9676bb6d9e871534535/src/models/goods.rs#L180 . That too will accept either, so I agree there probably shoudn't be breakage.

@weiznich
Copy link
Member

I personally would expect that this won't cause much breakage for using a.eq_any(b) outside of filter. I see more potential for breakage by having some functions that return Box<dyn BoxableExpression<SqlType = Bool>> which contains such an IN expression and suddenly turn to Nullable<Bool> due to this change.

That written: I still would see that as bugfix and therefore allowed breakage, but we should have an explicit changelog entry for this, so that it does not get lost before the next update.

@weiznich weiznich added the maybe backport Maybe backport this pr to the latest diesel release label Nov 22, 2024
@weiznich weiznich added this pull request to the merge queue Nov 22, 2024
Merged via the queue into diesel-rs:master with commit 75c5846 Nov 22, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maybe backport Maybe backport this pr to the latest diesel release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants