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

Ask Connection to quote symbolic column names when building ORDER BY #5029

Closed
wants to merge 1 commit into from

Conversation

rdlugosz
Copy link

In some databases (e.g, mysql) column names referenced in ORDER BY clauses
should be quoted in case they are reserved words. This corrects an issue that
occurs when using the Model.order(:symbol) method if :symbol happens to be a reserved word (e.g., "key").

Most AR methods already handle this correctly, so calling Model.where(:key) does NOT fail
but Model.order(:key) does. This change impacts columns specified as symbols only.

This problem was originally reported as issue #2601. Tests pass.

Note to @tenderlove: it seems that this is something that could ultimately live within AREL, but based on what I saw the symbols are already strings and " ASC/DESC" is already attached to the column names before we get there. To be honest, after a few hours in the code I'm not completely clear as to where AR's intended responsibilities end and AREL's begin so if I'm missing something obvious pls inform.

PS - First Rails pull request so go easy on me... :)

…clauses.

In some databases (e.g, mysql) column names referenced in ORDER BY clauses
should be quoted in case they contain reserved words. This corrects an issue that
occurs when using the Model.order(:symbol) method if :symbol happens to be a reserved
word (e.g., "key").

Most AR methods handle this correctly, so calling Model.where(:key) would not fail
but Model.order(:key) would.

This problem was originally reported as issue rails#2601.
@tenderlove
Copy link
Member

I'll take a look at this. If we construct ASC / DESC nodes in the SQL AST, then quoting on a per db basis should be trivial. I need to look more closely though.

@rdlugosz
Copy link
Author

Thx for taking a look.

Definitely an edge case but it can have you scratching your head when you hit it ...Especially since a column named "key" doesn't blow up sqlite - only mysql :/

@ahawkins
Copy link

@tenderlove that is definitely the way to do it. I always wondered why it doesn't work this way atm. Have you made any progress?

@lloeki
Copy link
Contributor

lloeki commented Jun 18, 2012

How to order with quoted columns with Arel:

author_arel = Author.arel_table
email_attr = author_arel[:email]
Author.order(email_attr.asc)

@steveklabnik
Copy link
Member

This will need a rebase.

@steveklabnik
Copy link
Member

@thatRD PING! Are you interested in keeping this PR updated?

@rdlugosz
Copy link
Author

Hey Steve - I'll do a rebase and see how well it aligns to the current code. If things have changed dramatically in this area I'll probably let it go.

@steveklabnik
Copy link
Member

Allllrighty. Let me know if I can help.

@rdlugosz
Copy link
Author

Looks like this issue has been fixed by timsly@633ea6a826cb5e587165f78d15770271cb39f670. I'm closing this PR and associated issue.

@rdlugosz rdlugosz closed this Dec 17, 2012
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.

None yet

5 participants