8000 dialect/sql/builder: make sql.NotIn() with empty args fallback to NOT FALSE by adayNU · Pull Request #2757 · ent/ent · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

dialect/sql/builder: make sql.NotIn() with empty args fallback to NOT FALSE #2757

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 19, 2022
Merged

dialect/sql/builder: make sql.NotIn() with empty args fallback to NOT FALSE #2757

merged 3 commits into from
Jul 19, 2022

Conversation

adayNU
Copy link
Contributor
@adayNU adayNU commented Jul 14, 2022

This is basically the identical change to #2735, but for NotIn(). Also added tests for both cases.

This bug currently prevents anyone using NotIn() from upgrading from v0.10.x to v0.11.x.

adayNU and others added 2 commits July 14, 2022 16:50
@adayNU
dialect/sql/builder: make sql.NotIn() with empty args fallback to Fal…
692874c 
…se()

This is basically the identical change to ent#2735, but for NotIn().

This bug currently prevents anyone using NotIn() from upgrading from v0.10.x to v0.11.x
@adayNU
Update go.sum
49cc88e 
untidy
@a8m
Copy link
Member
a8m commented Jul 15, 2022

Hey @adayNU 👋

A small comment on this behavior, the x NOT IN (...) operator is equivalent to NOT (x IN (...)). Hence, the predicate should be true and not false in the case of an empty set. Thoughts?

The SQL standard requires at least one value, but SQLite allows this:

sqlite> select 1 in ();
0
sqlite> select 1 not in ();
1

This bug currently prevents anyone using NotIn() from upgrading from v0.10.x to v0.11.x.

Can you elaborate on why?

@adayNU
Copy link
Contributor Author
adayNU commented Jul 18, 2022

@a8m sorry for the delay here. Updated based on your feedback.

So for the first question - this is obviously correct lol. I (naively) compared the output of client.Debug from v0.10.x and v.0.11.x and saw the difference was that v0.10.x had the FALSE keyword. So fwiw, it looks like this was also a bug in v0.10.x, just a different one :) (in v0.11.x you end up with an invalid query, in v0.10.x you end up with a silent bug).

For the latter question, given the statement above, it doesn't really feel like a major blocker given it was incorrect in both versions and you can workaround it in v0.11.x by simply ensuring you don't call NotIn() w/o arguments.

@adayNU adayNU changed the title dialect/sql/builder: make sql.NotIn() with empty args fallback to False() dialect/sql/builder: make sql.NotIn() with empty args fallback to NOT FALSE Jul 18, 2022
a8m
a8m approved these changes Jul 19, 2022
View reviewed changes
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.

💯

@a8m a8m merged commit 8beaef8 into ent:master Jul 19, 2022
@a8m
Copy link
Member
a8m commented Jul 19, 2022

Thanks for the contribution @adayNU 🙏🏻

@timoha timoha mentioned this pull request Oct 7, 2022
Closed
@a8m a8m mentioned this pull request Oct 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a8m a8m a8m approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adayNU @a8m
0