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
dialect/sql/builder: make sql.In() with empty args fallback to False() #2735
Conversation
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.
Looks good @Liooo! Thanks for working on it.
return p | ||
return p.False() |
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.
Small comment, can you please copy the comment from here:
https://github.com/ent/ent/blob/master/entc/gen/template/dialect/sql/predicate.tmpl#L21-L22
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.
We can also remove this if-statement from 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.
We can also remove this if-statement from the codegen.
Really agreed, fixed in 52c502b 🙏
return p | ||
return p.False() |
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.
return p | |
return p.False() | |
// If no arguments were provided, append the FALSE constants, | |
// since we can't apply "IN ()". This will make this predicate falsy. | |
return p.False() |
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.
thanks added in f1fcc1c 👍
dialect/sql/builder.go
Outdated
@@ -1535,8 +1535,10 @@ func In(col string, args ...interface{}) *Predicate { | |||
|
|||
// In appends the `IN` predicate. | |||
func (p *Predicate) In(col string, args ...interface{}) *Predicate { | |||
// if not arguments were provided, append the FALSE constants, |
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 not arguments were provided, append the FALSE constants, | |
// If no arguments were provided, append the FALSE constants, |
if len({{ $arg }}) == 0 { | ||
s.Where(sql.False()) | ||
return | ||
} |
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.
Run go generate ./...
from the root of the project.
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.
🚀
Thanks for the contribution @Liooo
thanks for the review 🙌 |
…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
Encountered a SQL syntax error when executing
sql.In("col", []interface{})
. The new test case compiles into"SELECT * FROM "users" WHERE "a" = $1 OR"
without this PR change.entc generated code at
ent/<modelname>/where.go
has the sameFALSE
fallback logic butsql.In
didn't, and I'm assuming this is not the expected behavior.