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

Ignore empty values and expressions to prevent syntax error #190

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

huandu
Copy link
Owner

@huandu huandu commented Jan 26, 2025

This PR is to address the issue that builders will generate bad SQL statements if empty values and/or expressions are provided.

For instance, if we build SQL like following, we will get a SQL statement with syntax error.

// Intentionally let values be nil.
var values []int

sb := Select("c").From("t")
sb.Where(sb.In("c", ...Flatten(values)))

print(sb) // SELECT c FROM t WHERE c IN ()

In this PR, if there is no values or expressions provided in Cond.IN/Cond.NotIn/Cond.Some/Cond.All/Cond.Any/Cond.And/Cond.Or, the condition will be ignored completely. Thus, the case above will print SELECT c FROM t with this PR.

It's a better solution to address the same issue mentioned in PR #188. cc @ErikBooijMB

@coveralls
Copy link

Coverage Status

coverage: 96.732% (-0.1%) from 96.879%
when pulling fc56bc8 on feature/prevent-syntax-error-with-zero-values
into b0b0b02 on master.

Copy link

@ErikBooijMB ErikBooijMB left a comment

Choose a reason for hiding this comment

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

Sorry, totally missed this PR in in my notification. Really appreciate your effort to jump on this and fix it in a way that's cleaner than my approach was.

Not 100% sure that silently ignoring these conditions with no values is right approach. For NOT IN I think that's fine, and yields the expected result, but for IN/ANY/SOME/ALL, this may do the opposite of what a user expected.

@@ -186,7 +186,7 @@ func (c *Cond) LTE(field string, value interface{}) string {

// In is used to construct the expression "field IN (value...)".
func (c *Cond) In(field string, values ...interface{}) string {
if len(field) == 0 {
if len(field) == 0 || len(values) == 0 {

Choose a reason for hiding this comment

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

I'm wondering if this is what users will intuitively expect. I wouldn't. If I have an "allowlist" of values that I'm passing here, and it's empty for some reason, that query should yield no results, not silently ignore the filter and thus actually widen the result set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants