This project is archived and is in readonly mode.

#5094 ✓wontfix
Matt Moriarity

ActiveRelation doesn't quote columns that are SQL keywords

Reported by Matt Moriarity | July 12th, 2010 @ 03:19 PM

I created a table using migrations called lists, which has a column named order for maintaining the order of the lists. This worked fine, but then I tried to call List.order(:order), which generated this SQL:

SELECT "lists".* FROM "lists" ORDER BY order

This is a syntax error since order is a SQL keyword. It should be quoted, similar to how "lists" is quoted in the same query.

Comments and changes to this ticket

  • Neeraj Singh

    Neeraj Singh July 13th, 2010 @ 12:57 AM

    • Importance changed from “” to “Low”

    As far as I know order by should never be quoted.

    SELECT cars.* FROM "cars" ORDER BY id 
    SELECT cars.* FROM "cars" ORDER BY 'id'
    

    Above two sql statements are not same. The second one is meaningless. It will sort on literal string 'id'.

  • Matt Moriarity

    Matt Moriarity July 13th, 2010 @ 01:25 AM

    I mean double-quoted, which will treat it as a column name and not a keyword:

    SELECT cars.* FROM "cars" ORDER BY "id"

    would be the same as the original, but if "id" was "order", this one would work while the original wouldn't.

  • Neeraj Singh

    Neeraj Singh July 13th, 2010 @ 02:09 AM

    I did not know that in the case you mentioned double quote would work but not single quote.

    Anyway here is snippet from mysql adapter.

          def quote(value, column = nil)
            if value.kind_of?(String) && column && column.type == :binary && column.class.respond_to?(:string_to_binary)
              s = column.class.string_to_binary(value).unpack("H*")[0]
              "x'#{s}'"
            elsif value.kind_of?(BigDecimal)
              value.to_s("F")
            else
              super
            end
          end
    

    As you can see even if code is put into quote the data then in mysql it would be quoted using single quote unless code is changed to handle this special case.

  • Miles Egan

    Miles Egan July 13th, 2010 @ 03:18 AM

    I think this is actually an Arel thing. If you look at the code for the ordering clause in arel. Specifically, if you look at
    lib/arel/engines/sql/formatters.rb on line 48:

    class OrderClause < PassThrough
      def ordering(ordering)
        "#{quote_table_name(name_for(ordering.attribute.original_relation))}.#{quote_column_name(ordering.attribute.name)} #{ordering.\
    direction_sql}"
      end
    end
    

    You can see that the OrderClause class doesn't override the value() method of its parent and just passes the order value through unchanged. If you add a value clause like this:

    class OrderClause < PassThrough
      def value(value)
        quote_column_name(value)
      end
    
      def ordering(ordering)
      "#{quote_table_name(name_for(ordering.attribute.original_relation))}.#{quote_column_name(ordering.attribute.name)} #{ordering.\
    direction_sql}"
      end
    end
    

    Then queries like Lists.order(:order) start to work but this breaks queries like Lists.order("order asc"). The only way I can see to make this work is to make the OrderClause class smart enough to parse the various expressions you can put in an order clause, but maybe this is easy in Arel. I don't understand the internals of Arel well enough to say.

  • Neeraj Singh

    Neeraj Singh July 13th, 2010 @ 03:28 AM

    In query_methods.rb in ActiveRecord you can do following

    def order(*args)
      clone.tap { |r| r.order_values += args if args.present? }
      #clone.tap { |r| r.order_values += args.collect {|e| connection.quote(e)} if args.present? }
    end
    

    commented out code is the change I did.

    I ran this test with mysql and lots of tests failed. That is because mysql put single quotes around values. I had no idea that single quote vs double quotes matter. I guess quote can be patched to accept parameter then this might work.

    Question:

    Do all databases(sqlite3, mysql, postgresql, oracle) support having double quotes around order by values?

  • Miles Egan

    Miles Egan July 13th, 2010 @ 10:18 PM

    I just noticed that this page:
    http://wiki.rubyonrails.org/rails/pages/reservedwords

    Which advises against using any sql reserved words for column names. So, I wouldn't say that people should expect this to work. I guess it's worth an easy fix if there is one but maybe not worth tearing up much of the ActiveRelation code if that's what it takes.

  • Neeraj Singh

    Neeraj Singh August 5th, 2010 @ 04:07 AM

    • State changed from “new” to “wontfix”

    It will take a lot of work to fix it and even then it would be flaky. It is best to not to use reserved words.

  • Matt Moriarity

    Matt Moriarity August 5th, 2010 @ 04:32 AM

    I guess I'm just surprised this is such a difficult fix, since the double quotes are already used earlier in the SQL statement in the SELECT block. There's no reason not to apply them, even if the word is not reserved.

    But then again, I'm not intimately familiar with how ActiveRelation and ActiveRecord work internally, so I'll have to take you at your word that this is not as easy as it seems to me. I changed my column name anyway, so I'm no longer hindered by this.

    Thanks for the response.

  • Miles Egan

    Miles Egan August 5th, 2010 @ 04:45 AM

    The thing that makes it difficult is that the order by parameter can be any sql expression, so you'd have to try to parse it, not just throw quotes around it.

  • Matt Moriarity

    Matt Moriarity August 5th, 2010 @ 08:47 PM

    Perhaps it could be quoted automatically only if the user provided a symbol as the parameter.

    For instance, List.order(:order) would generate '... ORDER BY "order"' but List.order('order') would generate '... ORDER BY order'

  • Miles Egan

    Miles Egan August 5th, 2010 @ 08:53 PM

    I'm inclined to agree with Neeraj here. For the sake of simplicity and maintainability it's better to ask people to avoid used reserved SQL words as field names.

  • Matt Moriarity

    Matt Moriarity August 5th, 2010 @ 08:54 PM

    Alright, it was worth a shot. Still loving Rails 3 anyway!

  • Neeraj Singh

    Neeraj Singh August 5th, 2010 @ 08:56 PM

    If rails start supporting usage of reserved words then hundreds of test cases need to be added to ensure that rails through and through works with reserved words. Just not worth the effort in my opinion.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages