Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Order method should quote column names #2601

Closed
rdlugosz opened this Issue · 34 comments

10 participants

@rdlugosz

ActiveRecord should be quoting column names used in ORDER BY clauses to avoid issues where DB reserved words can cause issues. Seems reasonable to just always quote the names rather than to only do it when there's a collision with a reserved word.

The WHERE methods appear to handle this properly already.

Variable.order(:key)  #FAILS with ActiveRecord::StatementInvalid ... sql syntax error near 'key'
Variable.order("'key'")  #works; note ugly double quoting
Variable.order(:value)  #works; not a reserved word

Variable.where(:key => 'foo')  #works; the where method must handle quoting properly
Variable.where("key like ?", "foo%") #fails as expected (once you realize 'key' is a reserved word)

The above is what happens on MySQL using the mysql2 gem in Rails 3.0.9. Doesn't happen on sqlite. Not sure if that's because it's more forgiving or if it's just not a reserved word.

As an aside, is there a reason why there is no chainable "like" method in AREL such that you could write queries similar to what you can do with the "where" method, e.g., Variable.like(:key => 'foo%') ? If that's a reasonable enhancement request I can open a separate issue for tracking.

@rdlugosz

ugh... related to my aside comment, this is even uglier when you do it in a WHERE LIKE clause... for some reason if you quote the column name with regular quotes you get no values returned. You must quote it with backticks:

Variable.where("key like ?", "foo%")  #fails because of reserved word
Variable.where("'key' like ?", "foo%") #does not fail but returns empty array!
Variable.where("`key` like ?", "foo%") #works as expected

Maybe all column quotes are supposed to be backticks, but single quotes appear to work at least some of the time... In any event, I mention this since this is the kind of minutia that Rails is supposed to hide from the developer (or maybe more accurately "protect" the developer from (himself)).

@13k

@thatRD, you can dive a little into Arel's features using Model.arel_table. This is a Arel::Table object, from where you can grab table attributes; and with attributes you can generate those predicates you are needing, for example:

author_arel = Author.arel_table
email_attr = author_arel['email']
Author.where(email_attr.matches("foo%"))
# SELECT `authors`.* FROM `authors` WHERE (`authors`.`email` LIKE 'foo%')

It's a little extra work, but ActiveRecord, as far as I know, currently doesn't expose this in an easy way. You can check out all the available predicates on Arel's API documentation or, more specifically, Arel::Attributes::Attribute.

And if this bothers you too much, there is Squeel, that does this and much more.

@rdlugosz

@13k - thanks, that's helpful & I'll have a look at that API. A quick test confirms that it handles the quoting issue properly. But, while that's a potential workaround for LIKE queries it seems Rails could do better by simply wrapping that logic up into a like method, no?

Of course, my original issue still remains - the order method should be quoting column names in the queries it generates.

@mperrando

Tried to approach the problem.
See mperrando/arel@7139502

@13k

@thatRD - I can only guess the reason these methods won't quote anything passed to them is that you can have custom SQL messing it all up, something like:

Author.select("id as identifier").order("identifier desc").first

So how is AR/Arel supposed to know identifier came from authors table? You could have some solutions here, but this is a stupid example. You can imagine that when JOINs come to the party, it ruins the fun.

@rdlugosz

@13k - this would only apply in cases similar to the one shown in my initial post where you are referencing the column names as symbols. In your example where you are essentially providing your own strings to be inserted into the SQL query I agree that the developer is responsible for his own quoting.

The key point to my issue is that there is an inconsistency to the way the where method and the order method handle quoting column names.

@mperrando - thanks for posting; I haven't had a chance to take a look at this yet but hope to soon.

@mperrando

Then autoquoting should happen only whem a symbol is provided, especially if this already happens fir the "where" clause. When using strings, the current conversion to SqlLiteral should happen, leaving the responsability of quoting to the user.

I will update my branch in this way and see what happens.

@rdlugosz rdlugosz referenced this issue from a commit in rdlugosz/rails
@rdlugosz rdlugosz Ask Connection to quote symbolic column names when building ORDER BY …
…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 #2601.
c586e0c
@isaacsanders

Shouldn't this be a PR to Arel? https://github.com/rails/arel

@rdlugosz

Possibly, and that was the path I was heading toward when I originally opened the issue. However, when I got into the code in I did not see a way to solve it in AREL.

My fix makes the change in the Rails codebase in query_methods.rb. This also happens to be where the query munging logic that inverts ORDER BY (ASC to DESC & vice-versa) lives.

I've updated the title of this issue to drop the "AREL" mention, since clearly that's misleading; it's just where my head was at when I opened it.

@lloeki

How to order with quoted columns with Arel:

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

@lloeki shouldn't this just work , instead of the workaround you just mentioned ?

@rdlugosz

@rishav yep, at least, IMHO. That's why I opened an issue. :)

The way WHERE and ORDER BY work in this case should be consistent.

@rymohr

Ran into a similar issue today with pluck(:key) -- worked fine for me in development on sqlite but busted in production on mysql.

@rdlugosz

Looks like this issue has been fixed by timsly@633ea6a, at least in the case of Order By. I'm closing this issue and associated PR.

@rdlugosz rdlugosz closed this
@lassebunk

In Rails 4 using MySQL:

User.order(:name)
=> SELECT `users`.* FROM `users` ORDER BY `users`.name ASC

The documentation says:

User.order(:name)
=> SELECT "users".* FROM "users" ORDER BY "users"."name" ASC

Does this mean this issue is still relevant?

@rdlugosz

That certainly looks a little suspect. Could you try an example against MySQL similar to my original post on this issue? Try adding an attribute called "key" to the model and attempt to order by that as a symbol.

FWIW, 11 months ago when I closed this issue the patch I referenced above did fix it.

@lassebunk

Yes, the :key symbol causes trouble because it's unquoted and reserved. That was my original issue. The user :name was to align the example with the documentation.

@rdlugosz rdlugosz reopened this
@rdlugosz

Interesting. It would appear that we've had a regression. Hey @timsly, any thoughts? Your patch (ref'd above) had previously fixed this problem.

@timsly

@rdlugosz Main purpose of my pull request was to "proxy" symbol and hash attributes to Arel::Nodes::Ordering.
Main advantage of using Arel::Nodes::Ordering is that arel will handle correct table name and i think quoting too.
So if there are some reserved words validations i believe you should search them in arel repo.
But AFAIK only type is reserved. Here are some tests where key is using as column name

@rdlugosz

Thanks. As I recall one of the issues was that the column names were being properly quoted for most operations (e.g., where) but not order by. Unfortunately I'm not able to dig into this much more now (esp since I don't have any projects using mysql any longer) but will leave this issue open in case @lassebunk is able to do some more troubleshooting.

@lassebunk

Thanks @timsly and @rdlugosz. where(key: 'xx') is working fine. The problem is order(:key).

@timsly

@lassebunk try order(key: :asc)
Strange, but for some reasons order(:key) and order(key: :asc) are not returning same results(but in my init implementation they supposed to be the same)

@lassebunk

@timsly,

  • On Rails 3.2, order(key: :asc) doesn't work – it orders by some yaml.
  • On Rails 4, order(key: :asc) works as expected.
@timsly

It shouldn't work on 3.2, because hash order attribute was introduced in Rails 4

@lassebunk

Ok, thought so :)

@timsly

I think i found why order :key and order key: :asc are not returning the same sql.
There is some order args preprocessing

@lassebunk

Looks like it. Could this be fixed by doing this:

arg.is_a?(Symbol) ? { arg => :asc } : arg

?

@timsly

It's already there
But since we are preprocessing symbol args we never rich that part.

@lassebunk

Thanks for working on this, @timsly :+1:

@rdlugosz

Awesome. Thx @timsly. I'll close this issue once I see the PR merged.

@gmcculloug gmcculloug referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@senny
Owner

@rdlugosz is this still an issue on the latest master? I tried to reproduce but had no luck:

  create_table :minivans, force: true, id: false do |t|
    t.string :minivan_id
    t.string :name
    t.string :speedometer_id
    t.string :color
    t.string :key
  end

# test code
puts Minivan.order(:key).to_sql
# => SELECT `minivans`.* FROM `minivans`   ORDER BY `minivans`.`key` ASC

If my reproduction is flawed and this is still an issue, can you please write an executable test-case to reproduce? You can use this script as a starting point.

@senny
Owner

I'm closing this one as it should be fixed on master. If you can reproduce this problem agains master please report back so we can reopen.

@senny senny closed this
@rdlugosz

Thanks @senny. Sorry I missed your ping about retesting!

@lassebunk

Thanks! Looking forward to trying it.

@mperrando mperrando referenced this issue in rails/arel
Closed

Columns quoting proposal. #85

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.