This project is archived and is in readonly mode.

#5519 ✓committed
Neeraj Singh

order is reversed

Reported by Neeraj Singh | September 1st, 2010 @ 04:10 AM | in 3.x

Not sure if this format is supported

Car.order('name').find(:all, :order => 'id')

#=> SELECT "cars".* FROM "cars" ORDER BY id, name

Notice that order is reversed. Usually order is concatenated.

Tested with rails master after this commit http://github.com/rails/rails/commit/c07f0ae52ecf9a45116e1b6ab422e0...

If it is a bug then I can create a failing test and possibly look into patch.

Comments and changes to this ticket

  • Pratik

    Pratik September 1st, 2010 @ 01:32 PM

    Yeah, looks like a bug. Patch please!

  • Neeraj Singh

    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

    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

    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

    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

    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

    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.

  • Neeraj Singh

    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

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>

Attachments

Referenced by

Pages