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

Conds can produce syntax errors #175

Merged
merged 2 commits into from
Oct 28, 2024
Merged

Conversation

rodionovv
Copy link

fixes #172

Everything discussed in #172 is added.

But i mentioned that solution discussed in #172 is not working for all cases. Checking the size of bytes to be written is working only when all input strings are empty. But if some of them are empty and some are not syntax errors will still be produced. So case like this:

sb.And("", "1 = 1", "")

will produce this sql string:

( AND 1 = 1 AND )

Since total length of input is not zero everything will be inserted. The best way to fix this issue i found is to edit WriteStrings function. It checks for the size of strings its writing to buffer and if it's empty it skips the step also ignoring separator.

Also i added validation of strings in all Conds where string argument is present. All of them might produce syntax errors if string is empty. For example EQ will produce string like: = 1 if field is empty.

@coveralls
Copy link

coveralls commented Oct 27, 2024

Coverage Status

coverage: 96.84% (+0.3%) from 96.582%
when pulling 961c268 on rodionovv:string-validation
into c329655 on huandu:master.

@@ -92,6 +92,12 @@ func (wc *WhereClause) AddWhereExpr(args *Args, andExpr ...string) *WhereClause
return wc
}

andExprsBytesLen := estimateStringsBytes(andExpr)

if andExprsBytesLen == 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

Need some cases to cover this branch.

Copy link
Author

@rodionovv rodionovv Oct 27, 2024

Choose a reason for hiding this comment

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

I've added test covering those lines. But found another problem.

Now building with WHERE clause can produce an extra blank space. In case WHERE was added through AddWhereExpr function. Since it can be only applied to an existing and non nil WhereClause it will be replaced with empty space during Build stage.

This problem can be reproduced for both empty array and array of empty strings. I've added two test cases TestEmptyStringsWhereAddWhereExpr and TestEmptyAddWhereExpr to demonstrate how it can be achieved. They have empty spaces at the end of expected results strings so they will pass.

I felt that fixing this is not a part of current pr. I think its a bit another problem rather the one i was trying to fix here. I can still try to fix it in a current or maybe in a new pr. What do you think about it?

@huandu
Copy link
Owner

huandu commented Oct 27, 2024

Thanks for your contribution!

Your PR looks good to me in general. Just one comment: You can add some cases to test against the empty WHERE clause case, e.g. Select("*").From("a").Where("", "") should build SELECT * FROM a.

Copy link
Owner

@huandu huandu left a comment

Choose a reason for hiding this comment

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

LGTM

@huandu huandu merged commit be6fb8b into huandu:master Oct 28, 2024
2 checks passed
@huandu
Copy link
Owner

huandu commented Oct 28, 2024

Released in tag v1.31.0.

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.

"WHERE" and "AND" clauses can produce syntax errors
3 participants