-
Notifications
You must be signed in to change notification settings - Fork 121
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
Conversation
@@ -92,6 +92,12 @@ func (wc *WhereClause) AddWhereExpr(args *Args, andExpr ...string) *WhereClause | |||
return wc | |||
} | |||
|
|||
andExprsBytesLen := estimateStringsBytes(andExpr) | |||
|
|||
if andExprsBytesLen == 0 { |
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.
Need some cases to cover this branch.
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.
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?
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. |
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.
LGTM
Released in tag v1.31.0. |
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
Cond
s where string argument is present. All of them might produce syntax errors if string is empty. For exampleEQ
will produce string like:= 1
iffield
is empty.