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

Database Engine: escaping table field names #15

Open
gibbonweb opened this issue May 16, 2012 · 8 comments · Fixed by #17
Open

Database Engine: escaping table field names #15

gibbonweb opened this issue May 16, 2012 · 8 comments · Fixed by #17

Comments

@gibbonweb
Copy link
Contributor

I just ran into an issue where my queries on one table would always fail, although they were managed through an SQLBean.
Turns out I had a field in my table called 'as' which produced SQL whenever mentioned.
This could be fixed by using backticks around the field names like so:

SELECT `id`, `as` FROM mytable WHERE `id` = 2;

Instead of:

SELECT id, as FROM mytable WHERE id = 2;

...which would produce an error due to incorrect use of the keyword AS.

While you could argue that naming a field 'AS' is bad practice due to exactly this kind of possible confusion, 'AS' in this case stood for a german word with 15 letters in a table with 10 similar fields, which would be horrible to write out or display as a whole table...
What do you think? Would this have to be implemented in the MysqlPDOEngine, or even in the PDOEngine? I guess it could be solved by just wrapping field names with backticks anywhere they appear, but I'm not sure wether all supported database engines handle field name escaping the same way?

@TomFrost
Copy link
Owner

Excellent find! This addition should go in StandardSQLFormatter.php -- very very simple fix. I'll check it out later today.

@gibbonweb
Copy link
Contributor Author

Ok, now that I know WHERE to put it, I'll look into it in the next few days - if you haven't already fixed it by then ;-)

@gibbonweb
Copy link
Contributor Author

I have fixed this problem locally and would like to post a pull request; since somehow our fork histories have diverged a bit, this is kinda ugly... All my random rebasing attempts have failed - how do I create a branch which is based on YOUR HEAD, into which I could write the fix and send it back to you? Currently, any pull request I could post would contain a bunch of my idiotic commits I'd like to disappear ;)
cheers, Git Noob.

@gibbonweb
Copy link
Contributor Author

OK as you can see, I somehow managed to do it myself ;)

@hamador
Copy link
Contributor

hamador commented Oct 9, 2012

This solution actually ends up breaking some of the SQLBeans functionality (i.e. mapping) and limiting what can be done with query. Might I suggest removing the escapes from the Query class, as anyone that is using query directly should be capable escaping their own possibly offending fields, and rethink a solution to be added to SQLBeans.

@gibbonweb
Copy link
Contributor Author

Wouldn't it be better to just add proper escaping support to SQLBeans?

@hamador
Copy link
Contributor

hamador commented Oct 9, 2012

At the moment all fields sent to Query are escaped as a whole meaning that table.field syntax would not work. The only way to join/map tables with ambiguous field names is to use that syntax.
Now there are a few options that come to mind:

Tell Query not to escape a field, either through an argument or separate function
Tell Query when you are using table.field syntax so it can escape accordingly
Always use table.field syntax in Query, removing that burden from SQLBeans, and thus allowing you to code the escape accordingly

There are other things to take into account, before the escaping I could use Query to execute this without any special considerations :

SELECT COUNT(DISTINCT source_id) FROM charges WHERE CONVERT(VARCHAR,create_timestamp,105) = CONVERT(VARCHAR,getdate(),105)

When you escape fields at the Query level it becomes impossible to execute that through the Query class and would require a standard PDO Statement. Now we could build in COUNT,CONVERT, ect, functions into Query and the formatters but that brings to light a whole new problem of Nesting these functions to run queries like :

SELECT COALESCE(CONVERT(VARCHAR,modify_timestamp,105),CONVERT(VARCHAR,create_timestamp,105))

In my opinion leaving the freedom in Query to do more complex statements would be more ideal. Anyone with an offending field name in their tables could simply escape when calling $query->field() or in the fields array of the SQLBean (ie $fields = array( 'as','distinct'); ) if a solution outside of Query cannot be found, like say adding the escaping ot all of the members of the $fields array in the loop inside the select function of SQLBeans

@hamador
Copy link
Contributor

hamador commented Oct 10, 2012

Removed the escape from SQLFormater, and added it to SQLBeans here: https://github.com/hamador/Hydrogen/tree/DatabaseChanges
If you care to take a look at how that would work, I feel that may be a more acceptable solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants