Fixes #75

[luan-cestari]

This PR aims to fix #75

[coveralls]

Coverage remained the same at 68.78% when pulling 22872936682095b5a91a6301b3a7b552104b5919 on luan-cestari:issue75 into 0807f6da8ee6f7d08f5e5f5afa7f2ea8d041791a 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 cf37e421296ba4f5814f1828898fa4d832b1c181 on luan-cestari:issue75 into 43fad2d3b2515b160a1d09bcca7ab5d596fdf7fc on lightblue-platform:master.

[coveralls]

Coverage decreased (-0.08%) to 73.42% when pulling 78c93a29f98283522ec38bb1c786784f4d0a7bef on luan-cestari:issue75 into 43fad2d3b2515b160a1d09bcca7ab5d596fdf7fc 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 https://github.com/lightblue-platform/lightblue-rdbms/pull/107#issuecomment-72849627 .

[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 f9cc5bc03c6e661af7dfde5f9633830cf0d3627d on luan-cestari:issue75 into 43fad2d3b2515b160a1d09bcca7ab5d596fdf7fc on lightblue-platform:master.

[coveralls]

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

[jewzaam]

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

[luan-cestari]

@jewzaam done, the issue is https://github.com/colloquium/spacewalk/issues/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 896024f7b7b5bcdff6b72ab0c3f4d93097731fa3 on luan-cestari:issue75 into 43fad2d3b2515b160a1d09bcca7ab5d596fdf7fc 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 697088f9dc8aa2704258f98c905c27712286a6ce on luan-cestari:issue75 into 43fad2d3b2515b160a1d09bcca7ab5d596fdf7fc on lightblue-platform:master.

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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