Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• 

  Santiago Pastorino August 21st, 2010 @ 11:18 PM

  • Importance changed from “” to “Low”

  Hey Jérémy, can you make a patch or at least make the failing cases you are mentioning?.
  Thanks!

• You flagged this item as spam.
  

  Jérémy Lecour August 22nd, 2010 @ 12:01 AM

  Hi,

  I'd like to help by writing some failing tests, but I don't really know what I have to test.
  If I knew what is the expected behavior, it'd be easier.

  I've (quickly) scanned the test suite of ActiveRecord and I've seen that there is indeed a "reorder" method to override a previous order clause, so the 2n part of my issue is irrelevant.

  For the 1st part, I need to know if the default_scope is expected to have a higher priority when merging the 2 order clauses, or the "named" scope has the highest priority. This way I could write a test that verifies that the 2 syntaxes are right

  I'll read the test suite with more attention to see if I can figure this out by myself, but if someone has a straight answer, that'll save me some time.

  Thanks.

• You flagged this item as spam.
  

  Jérémy Lecour August 22nd, 2010 @ 12:24 AM

  I've found where I have to make the tests.

  Basically, I've duplicated the DeveloperOrderedBySalary model and changed the syntax for the order clauses (in the default_scope and the named scope). Then I duplicated the test named "test_named_scope_overwrites_default" in relation_scoping_test.rb.

  I'll send you a patch for those changes as soon as possible.

• You flagged this item as spam.
  

  Jérémy Lecour August 23rd, 2010 @ 08:42 AM

  Here is my patch. I contains only a failing case. I'll try later to make it pass, but I'm not sure I'll have the time in the next few days.

  • add-a-failing-test-case-for-5424.patch 2 KB
• 

  Neeraj Singh August 23rd, 2010 @ 12:09 PM

  • Milestone cleared.
  • State changed from “new” to “open”
  • Importance changed from “Low” to “High”

  I do not see an easy fix for this issue without breaking with_scope.

  The fix would require structural change in the way Relation handles 'order'.

  May be someone else can find an elegant fix that I am not able to see.

• 

  José Valim August 23rd, 2010 @ 04:23 PM

  Neeraj, if we change the order "order" arguments are added, couldn't we solve this issue?

  http://github.com/rails/rails/blob/master/activerecord/lib/active_r...

• 

  Neeraj Singh August 23rd, 2010 @ 05:01 PM

  • Tag set to “rails 3, activerecord”

  @ José If you change the order there then User.order('name desc').order('id desc') will start producing order by 'id desc, name desc'. That is not what was agreed upon.

  The main issue is the way apply_finder_options work. This is the current code.

      # Give precedence to newly-applied orders and groups to play nicely with with_scope
        [:group, :order].each do |finder|
          relation.send("#{finder}_values=", Array.wrap(options[finder]) + relation.send("#{finder}_values")) if options.has_key?(finder)
        end
  

  As the comment says author did this way to fix with_scope but in the process it introduced problem like this ticket.

  Fixing the problem mentioned in this ticket is easy. just change the order mentioned above. That will fix this ticket but tests for with_scope will start failing.

  Developer.send(:with_scope, :find => { :order => 'id DESC' }) do
    Developer.find(:all, :order => 'salary ASC')
  end
  

  In the above case with the outer circle and inner circle call apply_finder_options. The outer one calls apply_finder_options and we have a relation. Now that we changed the order when the inner one call apply_finder_options then the inner one gets existing relation.order(inner_order_by) . This means the external order_by takes precedence.

  Looks like a catch 22 situation. The only way to fix this would be if something Relation internally holds the information that next order will take precedence over me.

  One more thing. In pure Arel gem if multiple orders are applied then the last one has wins. In AR it was decided that order will be maintained in the order in which it was declared. I personally think that it would have been better if ActiveRelation behaves as close to Arel as possible.

  I couldn't sleep and got up early. looked at this ticket at 4 AM without my coffee. So don't take my words :-)

• You flagged this item as spam.
  

  Jérémy Lecour August 23rd, 2010 @ 05:24 PM

  I may be way out of my league here, but isn't it just a matter of when the default_scope's "order" is applied ?

• 

  Neeraj Singh August 23rd, 2010 @ 05:27 PM

  As I mentioned default_scope issue can easily be fixed by changing the order in apply_finder_options. But that will break with_scope and that's the issue.

• 

  Jeremy Kemper August 30th, 2010 @ 04:10 AM

  • Milestone cleared.

  [bulk edit]

• 

  Santiago Pastorino September 1st, 2010 @ 02:15 AM

  Jérémy can you take a look with the latest 3-0-stable code?

• 

  Santiago Pastorino September 7th, 2010 @ 12:13 AM

  The point 1) you mentioned should be fixed by Neeraj now.
  On the point 2 now default_scope is always concatenated to the order clauses and the other clauses too. If you want to "unscope" the order you have the following ways to do that.
  unscoped method which unscope all the clauses and only and except methods which can unscope partially applying unscope over some clauses only.

  Please Jérémy, Neeraj confirm this so we can close the issue.

• 

  Neeraj Singh September 7th, 2010 @ 09:29 AM

  • State changed from “open” to “resolved”

  #5519 should fix this.

• You flagged this item as spam.
  

  Jérémy Lecour September 7th, 2010 @ 03:37 PM

  The 1st point is OK for me with the latest 3-0-stable branch.

  But the 2nd point is not entirely OK.

  If I want to reset the order, I can use "reorder" if I specify the new order clause, but I can't simply ask to reset the order and then give another scope.
  A similar situation is when I unscope a model, then add a scope, the original default_scope is here again.

  For example, with this simple class :

  class Article < ActiveRecord::Base
    default_scope :order => 'title'
    scope :by_position, :order => 'position desc'
  end
  

  In the Rails console, I get these results :

  $ Article.scoped.to_sql
   => "SELECT     \"articles\".* FROM       \"articles\"  ORDER BY  title" 
  $ Article.unscoped.to_sql
   => "SELECT     \"articles\".* FROM       \"articles\"" 
  $ Article.unscoped.by_position.to_sql
   => "SELECT     \"articles\".* FROM       \"articles\"  ORDER BY  title, position desc" 
  $ Article.reorder("created_at desc").to_sql
   => "SELECT     \"articles\".* FROM       \"articles\"  ORDER BY  created_at desc" 
  $ Article.reorder.to_sql
   => "SELECT     \"articles\".* FROM       \"articles\"  ORDER BY  title" 
  $ Article.unscoped.reorder.by_position.to_sql
   => "SELECT     \"articles\".* FROM       \"articles\"  ORDER BY  title, position desc"
  

  • Line 1 is expected, I get the default_scope
  • Line 2 is expected, I get the unscoped model
  • Line 3 is not expected the default_scope shouldn't be here
  • Line 4 is expected, the "manual" reorder method reset the ordering
  • Line 5 is not expected, the order clause from the default_scope is not reset (or it is reapplied)
  • Line 6 is not expected but it makes sense regarding the result of the line 5
• 

  José Valim September 7th, 2010 @ 04:12 PM

  This is tricky because any scope is built on top of the default scope.
  So when you apply a scope, the default scope cones together. You can
  easily notice it working by moving the default scope declaration after
  the scope one.

  Not sure if there is an easy fix, but this surely should be documented.

  On Sep 7, 2010, at 16:38, "Lighthouse" <no-reply@lighthouseapp.com>
  wrote:

• 

  Neeraj Singh September 7th, 2010 @ 04:23 PM

  This is a bug. There is no easy way to put it.

   Article.unscoped.by_position.to_sql
   => "SELECT     \"articles\".* FROM       \"articles\"  ORDER BY  title, position desc"
  

  This issue is already in LH as ticket #5386.

  Jeremy: I have already updated the doc suggesting people to use block form of unscoped. Above code could be fixed by writing

   Article.unscoped { User.by_position.to_sql }
  

• You flagged this item as spam.
  

  Jérémy Lecour September 7th, 2010 @ 04:58 PM

  Neeraj: OK, I understand the block syntax.

  In the current version of the file (http://github.com/rails/rails/blob/master/activerecord/lib/active_r...) the documentation suggest to do this

  Post.unscoped {
    limit(10) # Fires "SELECT * FROM posts LIMIT 10"
  }
  

  But I get a "NoMethodError: undefined method limit' for #<Object:0x1001c8288>".<br/> If I use a scope instead of a limit method, I get a "NameError: undefined local variable or methodby_position' for #<Object:0x1001c8288>" (and I've obviously verified that the scope 'by_position' exists and is working).

  If I change the call for this, it is working :

  Post.unscoped {
    Post.limit(10)
  }
  

  It's also working if I use a scope.

  It might just be a matter of documentation though, not a Rails bug.

• You flagged this item as spam.
  

  Jérémy Lecour September 7th, 2010 @ 05:00 PM

  Sorry for the bad formatting, I didn't use the preview to make sure it was OK.
  It seems I can't edit my own comments, strange.

• 

  Neeraj Singh September 7th, 2010 @ 05:24 PM

  @Jeremy Feel free to update the doc at http://github.com/lifo/docrails . Anyone can update it. Sorry about that omission.

• 

  José Valim September 7th, 2010 @ 05:25 PM

  The example in the documentation works if you are inside the model
  class level. Still it would be nice to change the docs!

  On Sep 7, 2010, at 18:00, "Lighthouse" <no-reply@lighthouseapp.com>
  wrote:

• 

  Neeraj Singh September 7th, 2010 @ 05:26 PM

  I have a feeling that a lot of people would run into this issue. Wish I could make unscoped code only support block usage. :-)

• 

  Ryan Bigg October 9th, 2010 @ 09:45 PM

  • Tag cleared.

  Automatic cleanup of spam.

• 

  Jeremy Kemper October 15th, 2010 @ 11:02 PM

  • Milestone set to “3.0.2”

  [bulk edit]