Rails Ruby on Rails → 3.x

Comments and changes to this ticket

• 

  Pratik September 1st, 2010 @ 01:32 PM

  Yeah, looks like a bug. Patch please!

• 

  Neeraj Singh September 1st, 2010 @ 02:46 PM

  • State changed from “new” to “open”

  This code is not working because of the reason mentioned here https://rails.lighthouseapp.com/projects/8994/tickets/5424-merging-... .

  However I have a larger question about the usage of with_scope and the order.

      Car.send(:with_scope, :find => { :order => 'id DESC' }) do
        Car.find(:all, :order => 'name asc')
      end
  
      # existing code is producing
      #=> SELECT "cars".* FROM "cars" ORDER BY name asc, id DESC
  

  Since order is always concatenated, I would expect the final query to have order 'id desc, name asc'. However the current code produces the sql which has order 'name asc, id desc'.

  It means that if I have deeply nested with_scope then current code will give precedence to the innermost order. However I would expect order from the outermost with_scope to have precedence.

  Any reason why the innermost code will have priority over outermost when it comes to building order in with_scope?

• 

  José Valim September 1st, 2010 @ 03:04 PM

  with_scope is definitely wrong. However, note that with_scope will be a deprecated API in 3.1, so it is also interesting to add tests to ensure that the scoping API recently added (http://blog.plataformatec.com.br/2010/07/new-active-record-scoping-...) also works!

  Thanks for you work Neeraj!

• 

  Neeraj Singh September 1st, 2010 @ 03:14 PM

  It is broken even with scoping. Check this out

      Car.order('id DESC').scoping do
        Car.find(:all, :order => 'name asc')
      end
      
      #=>  SELECT "cars".* FROM "cars" ORDER BY name asc, id DESC
  

  That is because no matter what the relation holds when finder is applied then the order of find is taking precedence.

  If I reverse the order in apply_finder_methods then that will fix all of this. However it breaks tons of test. So before I fix all those tests want to be sure that this new behavior is the desired behavior in both with_scope and scoping.

• 

  Santiago Pastorino September 1st, 2010 @ 03:22 PM

  Neeraj, nice work it could be awesome if you can provide test cases at least.
  Of course if you provide tests and a patch is better :)

• 

  Neeraj Singh September 1st, 2010 @ 03:51 PM

  class DeveloperOrderedBySalary < ActiveRecord::Base
    default_scope :order => 'salary DESC'
  end
  
  DeveloperOrderedBySalary.find(:all, :order => 'salary').
  

  current code is producing sql with order "salary"

  After my patch the order would bd "salary desc"

  If there is an objection then please let me know.

  Since the order is always concatenated the order mentioned in default_scope should prevail just like it prevails with with_scope and scoping.

• 

  Neeraj Singh September 1st, 2010 @ 05:09 PM

  Attached is patch with test.

  I am attaching the patch for the sake of completeness. This patch makes order behave consistent in rails 3.

  While discussing with José Valim , he mentioned that we should not break backward compatibility. So now I am going to take a look at rails 2.3.8 to see what kind of behavior it has. I will report my findings here.

  • 5519_fix_for_edge.diff 5.8 KB
• 

  Neeraj Singh September 1st, 2010 @ 05:54 PM

  • Tag set to “activerecord”

  # Following code has been tested with rails 2.3.8
  #
  # In rails 2.3.8 this is what's happening
  # 
  # * find has the highest priority. order defined in find wins all the time
  # * named_scope comes second. order defined in named_scope wins over order defined in default_scope
  # * default_scope has lowest priority.
  #
  
  class User < ActiveRecord::Base
  
    named_scope :order_by_name, :order => 'name desc'
  
    def self.lab
      User.order_by_name.find(:all, :order => 'id desc')  
      #=> SELECT * FROM "users" ORDER BY id desc, name desc
    end
  end
  
  
  class BigUser < User
  
    default_scope :order => 'id desc'
  
    def self.lab
  
      BigUser.order_by_name.find(:all, :order => 'name')
      #=> SELECT * FROM "users" ORDER BY name, name desc
      
      BigUser.order_by_name
      #=> SELECT * FROM "users" ORDER BY name desc
    end
  
  end
  

  Current code in rails 3 master is backward compatible.

  However I feel the rules are too vague in 2.3.x. I like the rules defined in rails3 where order is always concatenated. I fully understand that fixing ticket like this one will break backward compatibility.

• 

  Santiago Pastorino September 5th, 2010 @ 12:16 PM

  • State changed from “open” to “committed”

  master: http://github.com/rails/rails/commit/91fec0d24d50d3b3e90c48b2501ca9...
  3-0-stable: http://github.com/rails/rails/commit/1c7a7545eb108b297252cf8d683501...