Skip to content

Ignore empty values and expressions to prevent syntax error #190

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

Merged
merged 2 commits into from
Feb 11, 2025

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

coveralls commented Jan 26, 2025

Coverage Status

coverage: 96.743% (-0.1%) from 96.879%
when pulling 06070c6 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.

@huandu huandu merged commit 2253d89 into master Feb 11, 2025
1 check passed
@huandu huandu deleted the feature/prevent-syntax-error-with-zero-values branch February 13, 2025 06:47
@Sora233
Copy link

Sora233 commented Feb 18, 2025

sqlbuilder.List好像没有同步修改

@huandu
Copy link
Owner Author

huandu commented Feb 18, 2025

@Sora233 嗯,下个版本补一下

@rodionovv
Copy link

Hello.

I want to share my thoughts on suggestion made by @ErikBooijMB earlier. I fully agree that silent drop of IN query is wrong and always FALSE statement must remain in the query. But i think the same logic must apply to NOT IN operator. It is an always TRUE statement and can be only sometimes dropped without affecting result of query. In many cases dropping it will also affect the result. For example in an OR query if all other statements are FALSE the always TRUE statement will make full query TRUE but now its just dropped and query remains FALSE. So i think its better not to drop it but replace with 0 = 0. This way in many cases it wont affect anything but in some cases query will remain correct.

Also its probably more predictable for user that almost same operators behave in the almost same way. if something is always FALSE its replaced with a FALSE statement and the same with TRUE. c.NotIn("$a") and c.Not(c.In("$a")) statements produce different SQL query which very misleading from my view.

What do you think about it?

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.

5 participants