learn ActiveReccord::Querying#order work with hash arguments #7765

Merged
merged 1 commit into from Oct 12, 2012

3 participants

@timsly

Added posibility work with hashes and symbols as agruments for order
When symbol or hash passed we convert it to Arel::Node, and it make requests more flexible

For example

string_order = User.where(:name => 1).order('id ASC')
string_order.to_sql # SELECT "users".* FROM "users" WHERE "users"."name" = 1 ORDER BY 'id ASC'

symbol_order = User.where(:name => 1).order(:id) # or order(id: :asc) 
symbol_order.to_sql # SELECT "users".* FROM "users" WHERE "users"."name" = 1 ORDER BY "users"."id" ASC

User.arel_table.table_alias = 'u'

string_order.to_sql # SELECT "u".* FROM "users" "u" WHERE "u"."name" = 1 ORDER BY 'id ASC'

symbol_order.to_sql # SELECT "u".* FROM "users" "u" WHERE "u"."name" = 1 ORDER BY "u"."id" ASC
@timsly

In the previous verion of order implementation, when you pass string and symbol to order it create Arel::Nodes::SqlLiteral object(see here). This is like a raw sql, withour any metadata like table, etc

I have added hash support and rewrite symbol parsing, so right now it create Arel::Nodes::Ordering object, which contanins information about current arel table

@timsly

@rafaelfranca @steveklabnik
Guys, can you please provide some feedback for this feature?

@blatyo blatyo and 1 other commented on an outdated diff Oct 1, 2012
activerecord/lib/active_record/relation/query_methods.rb
o.to_s.split(',').collect do |s|
s.strip!
s.gsub!(/\sasc\Z/i, ' DESC') || s.gsub!(/\sdesc\Z/i, ' ASC') || s.concat(' DESC')
end
+ when Symbol
+ { o => :desc }
+ when Hash
+ o.inject({}) do |memo, (field, dir)|
+ memo[field] = (dir == :asc ? :desc : :asc )
@blatyo
blatyo added a line comment Oct 1, 2012

Should be either:

memo[field] = (dir == :desc ? :desc : :asc )

or

memo[field] = (dir == :asc ? :asc : :desc )
@timsly
timsly added a line comment Oct 1, 2012

Purpose of reverse_sql_order is to reverse current order directions
So if we have :asc we need to change to :desc, etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Oct 2, 2012
activerecord/lib/active_record/relation/query_methods.rb
o.to_s.split(',').collect do |s|
s.strip!
s.gsub!(/\sasc\Z/i, ' DESC') || s.gsub!(/\sdesc\Z/i, ' ASC') || s.concat(' DESC')
end
+ when Symbol
+ { o => :desc }
+ when Hash
+ o.inject({}) do |memo, (field, dir)|
@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Oct 2, 2012

I think is better to use each_with_object here

@timsly
timsly added a line comment Oct 2, 2012

You mean like this

o.each_with_object({}) do |(field, dir), memo| 
  memo[field] = (dir == :asc ? :desc : :asc )
end

This will be a little bit shorter, but i thought using native functions will be faster

@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Oct 3, 2012

I remember that we are changing some inject by alternative solutions because of performance issues. I don't know if it is still valid.

@timsly
timsly added a line comment Oct 3, 2012

So i will use each_with_object then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Oct 2, 2012
activerecord/lib/active_record/relation/query_methods.rb
@@ -800,6 +805,30 @@ def reverse_sql_order(order_query)
def array_of_strings?(o)
o.is_a?(Array) && o.all?{|obj| obj.is_a?(String)}
end
+
+ def build_order(arel)
+ orders = order_values
+ orders = reverse_sql_order(orders) if reverse_order_value
+
+ orders = orders.uniq.reject{|o| o.blank?}.map do |order|
+ case order
+ when Symbol
+ create_order_node(order)
+ when Hash
+ order.map { |field, dir| create_order_node(field, dir) }
+ else
+ order
+ end
+ end.flatten
@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Oct 2, 2012

Do we need to call flatten?

@timsly
timsly added a line comment Oct 2, 2012

We need flatten for cases such order(:id, [:date, :name])
But i agree, overall we do not need it

@timsly
timsly added a line comment Oct 2, 2012

@rafaelfranca, i remembered why i use flatten, because i found same method inside reverse_sql_order method.
So i thought this two methods should be symmetrical

@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Oct 3, 2012

Seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca
Ruby on Rails member

Sorry for the delay. I like the idea. 👍

@tenderlove @jonleighton what you guys think?

@timsly

@rafaelfranca, i have removed inject and replace it with each_with_object also added few docs line for order method

@jonleighton jonleighton commented on the diff Oct 5, 2012
activerecord/lib/active_record/relation/query_methods.rb
o.to_s.split(',').collect do |s|
s.strip!
s.gsub!(/\sasc\Z/i, ' DESC') || s.gsub!(/\sdesc\Z/i, ' ASC') || s.concat(' DESC')
end
+ when Symbol
+ { o => :desc }
+ when Hash
+ o.each_with_object({}) do |(field, dir), memo|
@jonleighton
Ruby on Rails member
jonleighton added a line comment Oct 5, 2012

I don't really care that much, but I hate stuff like this just for the sake of saving one object allocation. To me:

Hash[o.map { |field, dir| [field, dir.downcase == :asc ? :desc : :asc] }]

is way cleaner and easier to read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton jonleighton commented on an outdated diff Oct 5, 2012
activerecord/lib/active_record/relation/query_methods.rb
@@ -800,6 +813,30 @@ def reverse_sql_order(order_query)
def array_of_strings?(o)
o.is_a?(Array) && o.all?{|obj| obj.is_a?(String)}
end
+
+ def build_order(arel)
+ orders = order_values
+ orders = reverse_sql_order(orders) if reverse_order_value
+
+ orders = orders.uniq.reject{|o| o.blank?}.map do |order|
@jonleighton
Ruby on Rails member
jonleighton added a line comment Oct 5, 2012

reject(&:blank?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton jonleighton and 1 other commented on an outdated diff Oct 5, 2012
activerecord/test/cases/relations_test.rb
@@ -158,6 +158,18 @@ def test_finding_with_arel_order
assert_equal 4, topics.to_a.size
assert_equal topics(:first).title, topics.first.title
end
+
+ def test_finding_with_assoc_order
+ topics = Topic.order(:id => :DESC)
@jonleighton
Ruby on Rails member
jonleighton added a line comment Oct 5, 2012

I don't really think we should support :DESC and :desc and :DeSc. :desc is fine, we don't need to mimic sql by allowing all-caps.

@jonleighton
Ruby on Rails member
jonleighton added a line comment Oct 5, 2012

You could make id: :DESC and id: :foo etc raise an ArgumentError though.

@timsly
timsly added a line comment Oct 5, 2012

Right now, i am checking if dir in [:desc, :asc], and if not - set to :asc
Should i raise ArgumentError instead?

@jonleighton
Ruby on Rails member
jonleighton added a line comment Oct 5, 2012

Yeah, but you should raise it when #order is called, not in #build_order. So Post.order(id: :DESC) should raise an error.

@timsly
timsly added a line comment Oct 5, 2012

Hm, but this mean that i need to add extra logic into order function and parse order function arguments just for indicate if hash was passed correctly(all values are in [:asc, :desc]). Also i need to modify reorder function.

So maybe better use :asc as default direction and set it even if value not in [:asc, :desc]

@timsly
timsly added a line comment Oct 5, 2012

I just reviewed all methods like order, where, join, and all of them just update relation object. So all magic happens inside build_arel(agruments parsing, etc).

So the easiest way is to raise ArgumentError inside build_order or even create_order_node method(where i build Arel::Nodes::Ordering object)

@jonleighton
Ruby on Rails member
jonleighton added a line comment Oct 5, 2012

It definitely doesn't make sense to raise ArgumentError in build_order. Otherwise you'll have a situation like this:

posts = Post.order(id: :DESC) # ArgumentError should be raised now, as the argument to `order` is invalid
# ... some other code ...
posts.to_a # but instead it is raised now

We could just default to :asc but this seems more surprising from a user's perspective if they make a mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton
Ruby on Rails member

I am 👍 on this in general. I added a few code comments.

@timsly

@jonleighton i have made few updates according to your suggestions.
I left support only for downcase directions and raised exception on invalid order directions

@jonleighton jonleighton commented on an outdated diff Oct 12, 2012
activerecord/lib/active_record/relation/query_methods.rb
+ when Hash
+ order.map { |field, dir| create_order_node(field, dir) }
+ else
+ order
+ end
+ end.flatten
+
+ arel.order(*orders) unless orders.empty?
+ end
+
+ def create_order_node(field, dir = :asc)
+ dir = :asc unless [:asc, :desc].include?(dir)
+ table[field].send(dir)
+ end
+
+ def detect_invalid_params!(args)
@jonleighton
Ruby on Rails member
jonleighton added a line comment Oct 12, 2012

The method name is too generic. I'd rename it to something like validate_order_args. I don't think the bang is necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton jonleighton commented on an outdated diff Oct 12, 2012
activerecord/lib/active_record/relation/query_methods.rb
@@ -800,6 +817,37 @@ def reverse_sql_order(order_query)
def array_of_strings?(o)
o.is_a?(Array) && o.all?{|obj| obj.is_a?(String)}
end
+
+ def build_order(arel)
+ orders = order_values
+ orders = reverse_sql_order(orders) if reverse_order_value
+
+ orders = orders.uniq.reject(&:blank?).map do |order|
+ case order
+ when Symbol
+ create_order_node(order)
+ when Hash
+ order.map { |field, dir| create_order_node(field, dir) }
@jonleighton
Ruby on Rails member
jonleighton added a line comment Oct 12, 2012

I would get rid of create_order_node and change this to:

        when Symbol
          table[order].asc
        when Hash
          order.map { |field, dir| table[field].send(dir) }

We don't need to validate the dir here as we're doing that earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timsly

@jonleighton thanks for feadback. I have updated code

@jonleighton
Ruby on Rails member

ok, looks good. please squash the commit and I will merge.

@timsly

@jonleighton Shell i add release notes?

@jonleighton
Ruby on Rails member

yep, good point, please do

@timsly

I have added changelog and squash commits

@jonleighton jonleighton commented on the diff Oct 12, 2012
activerecord/CHANGELOG.md
@@ -1,5 +1,12 @@
## Rails 4.0.0 (unreleased) ##
+* Learn ActiveRecord::QueryMethods#order work with hash arguments
+
+ When symbol or hash passed we convert it to Arel::Nodes::Ordering.
+ If we pass invalid direction(like name: :DeSc) ActiveRecord::QueryMethods#order will raise an exception
@jonleighton
Ruby on Rails member
jonleighton added a line comment Oct 12, 2012

Please can you add a code example? It's a bit unclear what the functionality does at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton
Ruby on Rails member

Please rebase against master as well, as this doesn't merge cleanly at the moment.

@jonleighton jonleighton merged commit 1cb7cb0 into rails:master Oct 12, 2012
@rafaelfranca
Ruby on Rails member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment