Fixes #75

[luan-cestari]

This PR aims to fix #75

[coveralls]

Coverage remained the same at 68.78% when pulling 2287293 on luan-cestari:issue75 into 0807f6d on lightblue-platform:master.

[bserdar]

In general, I don't think this is a good idea. All this class does is giving the ability to specify names in metadata instead of argument indexes, and in my opinion, we can live without it. Parsing the SQL query is not a good idea, especially if the parsed query is doing string manipulation. That said, there are some problems with the parser:

• The parser cannot deal with escaped quotes,
• The parser cannot deal if : or ' is used as an escape in LIKE
• parser fails if : is the last character
• parser assumes anything that starts with : is a variable

Also, pls rebase from master

[luan-cestari]

I think I can fix the issues and this feature is nice to have, specially to simplify the user wor, but I agree that SQL manipulation can be dangerous (SQL-Injection)

[coveralls]

Coverage increased (+0.31%) to 73.81% when pulling cf37e42 on luan-cestari:issue75 into 43fad2d on lightblue-platform:master.

[coveralls]

Coverage decreased (-0.08%) to 73.42% when pulling 78c93a2 on luan-cestari:issue75 into 43fad2d on lightblue-platform:master.

[luan-cestari]

Created some tests to increase the coverage and fold some cases that it would be wrong (they are a bit complex but they are valid).

@bserdar From the possible problems you mentioned, only the "parser fails if : is the last character" I didn`t get it . A SQL statement that ends with a : would be malformed. Could you give me an example the syntax is correct but would lead to an error please? Also, I updated the PR.

[bserdar]

If the last character is a :, the parser fails with an "array index out of
bounds" exception, instead of a malformed sql statement exception.

On Wed, Feb 4, 2015 at 5:55 AM, luan-cestari notifications@github.com
wrote:

Created some tests to increase the coverage and fold some cases that it
would be wrong (they are a bit complex but they are valid).

@bserdar https://github.com/bserdar From the possible problems you
mentioned, only the "parser fails if : is the last character" I didn`t get
it . A SQL statement that ends with a : would be malformed. Could you give
me an example the syntax is correct but would lead to an error please?
Also, I updated the PR.

—
Reply to this email directly or view it on GitHub
#107 (comment)
.

[luan-cestari]

You are right @bserdar . Pushed a fix for that. Let me know if you want me to handle that in a different way

[luan-cestari]

I also think that after we finish we could push a PR for spacewalk which have the same flaws as we had

[coveralls]

Coverage decreased (-0.08%) to 73.42% when pulling f9cc5bc on luan-cestari:issue75 into 43fad2d on lightblue-platform:master.

[coveralls]

Coverage decreased (-0.16%) to 73.35% when pulling f9cc5bc on luan-cestari:issue75 into 43fad2d on lightblue-platform:master.

[jewzaam]

@luan-cestari open an issue if nothing else..

[luan-cestari]

@jewzaam done, the issue is colloquium/spacewalk#1

[bserdar]

Your quote logic is wrong. You can't do what you're trying to do without a stack. It won't parse this:

' 123 "123"123 '

Why are you even working with double quotes?

Also, there is no way to deal with this:

select * from table where column like 'abc:%def%' escape ':'

These kind of edge cases are the reason why I am against parsing sql statements.

[coveralls]

Coverage decreased (-0.08%) to 73.42% when pulling 896024f on luan-cestari:issue75 into 43fad2d on lightblue-platform:master.

[luan-cestari]

I just pushed new test scenarios following what you mentioned but I didn't find any issue. So you can check them out and see if it is fine that way or not.

I think we can also make the edge cass more clear, so we could define each edge case and make a specific test for it.

[bserdar]

How about this:

select * from table where column like ' 123 "123"1 :xyz 23 '

I believe it will parse :xyz as a field.

[luan-cestari]

@bserdar I just pushed a new test and it worked fine. Any other suggestions?

[coveralls]

Coverage decreased (-0.08%) to 73.42% when pulling 697088f on luan-cestari:issue75 into 43fad2d on lightblue-platform:master.

[coveralls]

Coverage decreased (-0.08%) to 73.42% when pulling a8a2535 on luan-cestari:issue75 into 43fad2d on lightblue-platform:master.

[coveralls]

Coverage decreased (-0.08%) to 73.42% when pulling a8a2535 on luan-cestari:issue75 into 43fad2d on lightblue-platform:master.

[coveralls]

Coverage decreased (-0.1%) to 73.4% when pulling d45f361 on luan-cestari:issue75 into 43fad2d on lightblue-platform:master.

[coveralls]

Coverage decreased (-0.1%) to 73.4% when pulling d45f361 on luan-cestari:issue75 into 43fad2d on lightblue-platform:master.