Comments and changes to this ticket

• 

  Repository March 6th, 2009 @ 05:01 PM

  • State changed from “new” to “resolved”

  (from [6a13376525f34a00e013fc3a6022838329dfe856]) Methods invoked within named scope Procs should respect the scope stack. [#1267 state:resolved]

  Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...

• You flagged this item as spam.
  

  Joseph Palermo July 27th, 2009 @ 07:46 PM

  • Assigned user set to “Pratik”

  This leads to named_scopes that are order dependent, and queries inside of lambdas do not behave as you would expect.

  Given a User class with a 'friends' association (pointing at other Users) with the following named_scopes:

  
  named_scope :named_bob, {
    :conditions => {:name => 'bob'}
  }
  
  named_scope :second_degree_friends, lambda{|user|
    user_friends = user.friends
    second_degree_friend_ids = user_friends.collect{|u| u.friend_ids}
    {
      :conditions => {:id => second_degree_friend_ids.flatten}
    }
  }
  
  User.named_bob.second_degree_friends(user_sam) 
  User.second_degree_friends(user_sam).named_bob
  

  So one of these queries will only pull user_sam's friends named 'bob' and then see if any of them have friends named 'bob' which is not what I want. The other one will work as expected where it finds all of user_sam's second degree friends named bob.

• 

  Michael Koziarski July 27th, 2009 @ 10:47 PM

  I'm not seeing this as a huge problem, if you don't want the scope applied to the stack then you could use with_exclusive_scope or something in the named_scope declaration?

• You flagged this item as spam.
  

  Joseph Palermo July 28th, 2009 @ 11:13 AM

  To me, named_scopes should be composable. If I can't rely on the internals of a named_scope to create the same conditions every time I pass it the same parameters, then it's not reliable enough to use in a composable manner.

  We often have search objects where a chain of named_scopes will get passed around, and you don't care what has already been applied, you just know you need to add an additional condition, so you tack on an additional named_scope knowing that it should add the condition regardless of what has already been added to the scope.

  But now, the scopes no longer work in isolation. You need to know what scopes have been applied, and how those scopes will affect the implementation details of the scope you are about to apply.

  You could, as you mention, construct all of your named_scopes using with_exclusive_scope inside the lambda, but that seems like too much work for what should be the default behavior (and possibly prone to error if there are default_scopes or larger with_scopes that you actually want for some reason).

  It's not clear from the tests what the original use case was for this patch, but if it is necessary behavior, it feels like it should not be the default behavior.

• You flagged this item as spam.
  

  Adam Milligan July 28th, 2009 @ 05:29 PM

  I have to agree with Joseph. This change significantly effects the behavior of existing named scopes, and it's not clear what the original motivation is. Changing the internal behavior of a named_scope based on the scope in which it's applied at runtime strikes me as a violation of Least Surprise. Named scopes have always been somewhat order dependent (with regard to SELECT, ORDER BY, etc. clauses), as others have pointed out, but this effect on CONDITIONS takes order dependence to a new extreme.

  Perhaps someone could explain the scenario that led to the change, and explain why it's important enough to break existing applications? Unless there's a truly compelling need I fully support rolling this change back.

• You flagged this item as spam.
  

  jvoorhis July 28th, 2009 @ 05:32 PM

  At this point, I should explain our motivation for this patch. On a couple occasions we have created some named scopes for complex queries that execute their own, auxiliary queries to limit the complexity of any one query for performance reasons. Since the outcome of the named scope chain's final query is dependent on the results of an auxiliary query, we'd like the same conditions to be applied to it – otherwise a named scope with an inner query is not composable.

  Admittedly, this is not the most common case, but regardless of the api I would like it to be convenient. I think the with_exclusive_scope is a decent "escape hatch" mechanism, but I would be open to a change in the opposite direction – perhaps using with_chained_scopes to apply the accumulated query options instead of having them available by default. Regardless of the outcome, I'm glad we're discussing the semantics of named scopes now.

• You flagged this item as spam.
  

  Joseph Palermo July 28th, 2009 @ 08:51 PM

  Here is a test that shows the original functionality, before the patch.

  I could get behind a :with_chained_scopes option to named_scope.

  • finds_inside_lambdas_should_not_use_chained_scopes.patch 1.8 KB

• You flagged this item as spam.
  

  Joseph Palermo July 28th, 2009 @ 09:06 PM

  3rd try, not sure why the patch file is not making it to S3

  • finds_inside_lambdas_should_not_use_chained_scopes.diff 1.8 KB
• 

  Michael Koziarski July 29th, 2009 @ 03:01 AM

  • Assigned user changed from “Pratik” to “Michael Koziarski”
  • State changed from “resolved” to “open”
  • Milestone changed from “2.x” to “2.3.4”

  I'm still not sold the the abstract idea of composibility is worth making this change. Scopes aren't purely composable because there are certain options where composing makes no sense. Order and limit clauses for example.

  I don't see why the current behaviour is any more confusing / surprising than what you're proposing. The proc / lamda is evaluated in a given context, and that context either does or doesn't have scope applied. If you want to opt out of scoping then you use with_exclusive_scope. You'd get the same behaviour with class methods and association method_missing.

  All the same, opening the ticket and marking for resolution by 2.3.4. However a resolution I'm leaning towards is a documentation fix.

• You flagged this item as spam.
  

  Joseph Palermo July 29th, 2009 @ 07:59 AM

  Named scopes can be composable. Conditions and Joins are all merged together. Selects, Order, and Limit are not things that are usually used with named scopes, at least not ones that are intended to be chained with others.

  The current behavior is confusing because a named_scope is not a scoping block like a with_scope. In a with_scope I expect that scope to be applied to the block and only the block. A named_scope is something even more targeted, a named_scope is applied directly to a finder, a single target. I would expect the scope to apply only to that target, and the lambda is not the target.

  with_exclusive_scope is not something I want to sprinkle around inside any lambda that accesses an association or does a find. Especially with the arrival of default scopes. That will almost certainly lead to bugs.

  I think that the usefulness of this original change is very small. If you are actually utilizing the accumulated scopes in your lambda to further restrict the result set size, it should not even change your final result set since that was already going to be constrained by the accumulated scopes. I think this is somewhat evident by the test that was committed with this change, it doesn't actually check the result set, because there is no change in the result set. From what I can tell, this is only a performance optimization for a very specific case.

  What it does do however, is cause any associations that you access from inside your lambda, that happen to be the same class that your named_scope operations on, to be restricted by the accumulated scopes. The vast majority of time, this will be non-obvious, non-desirable, and lead to strange bugs.

• 

  Jeremy Kemper July 30th, 2009 @ 12:42 AM

  • State changed from “open” to “incomplete”

  Let's keep the original behavior and introduce a new option instead. Jeremy, are you interested in revisiting this patch and updating for 2.3.4 release?

• 

  Repository July 30th, 2009 @ 12:55 AM

  • State changed from “incomplete” to “resolved”

  (from [d83b1828577c268de56e1b3942e16002c9efdd57]) Revert "Methods invoked within named scope Procs should respect the scope stack. [#1267 state:resolved]"

  This reverts commit 6a13376525f34a00e013fc3a6022838329dfe856.

  Conflicts:

  activerecord/test/cases/named_scope_test.rb
  

  http://github.com/rails/rails/commit/d83b1828577c268de56e1b3942e160...

• 

  Repository July 30th, 2009 @ 12:55 AM

  (from [17f336e2f00f419a41eb7effb817bd7ad3e84f0d]) Revert "Methods invoked within named scope Procs should respect the scope stack. [#1267 state:resolved]"

  This reverts commit 6a13376525f34a00e013fc3a6022838329dfe856.

  Conflicts:

  activerecord/test/cases/named_scope_test.rb
  

  http://github.com/rails/rails/commit/17f336e2f00f419a41eb7effb817bd...

• 

  Jeremy Kemper July 30th, 2009 @ 12:55 AM

  • State changed from “resolved” to “incomplete”

• You flagged this item as spam.
  

  Joseph Palermo July 30th, 2009 @ 08:11 AM

  Here is the patch for the new functionality. Since named scopes have either taken a Proc OR a options hash, there is now the ability to pass the Proc as a :proc option.

  I'm not terribly happy with how I ended up parsing the options inside named_scope, if somebody has something cleaner it would be great.

  I included the tests for both old and new functionality.

  I did a rough change to the documentation to explain the new functionality, but I didn't provide an example. Partially because I'm still not totally clear on a use case for this, and partially because I think it would probably take up a lot of space to make an understandable example.

  • with_chained_scopes.diff 4.1 KB

• You flagged this item as spam.
  

  Joseph Palermo July 30th, 2009 @ 08:12 AM

  Why does it keep eating my uploads?

• 

  Joseph Palermo July 30th, 2009 @ 08:12 AM

  • no changes were found...

  • with_chained_scopes.diff 4.1 KB
• 

  Jeremy Kemper September 11th, 2009 @ 11:04 PM

  • Milestone changed from “2.3.4” to “2.3.6”

  [milestone:id#50064 bulk edit command]

• You flagged this item as spam.
  

  Paweł Kondzior September 22nd, 2009 @ 08:36 PM

  Why just not let named_scope to be chained and be able to manipulate with_scope merge_conditions connector ? It would give named_scope new dimension of manipulating scopes, and save a lot of work that now is need to be done to allow match any criteria in ActiveRecord finders, i think right now using simple array condition builders are the best way for such thing, and IMHO for AR this is really shame, when you compare how mature this library should be after so many years of development.

  For example it would allow to do something like this:

  Ticket.any { pending.in_progress.resolved }.all(:conditions => { :created_at < Time.now })
  

  And it would result with where conditions: "status_id = 1 OR status_id = 2 OR status_id = 3 AND created_at = '1970-01-01 00:00:00'"

  This could also work:

  Ticket.any(:conditions => { :created_at < Time.now, :status_id => [1,2,3] })
  

  And where conditions should look like "created_at = '1970-01-01 00:00:00' OR status_id IN(1,2,3)"
  And of course more deep chains should be also welcome.

  Ticket.all { any { }.all { } }
  

  all and #any methods without blocks should act as #find(:all) with AND/OR conditions connector, and with blocks, as part of whole named_scope stack including chained named_scope used inside #any/#all blocks

• 

  Rizwan Reza May 16th, 2010 @ 02:41 AM

  • Tag changed from “activerecord, named_scope, patch” to “activerecord, bugmash, named_scope, patch”

  [bulk edit]

• 

  Jeremy Kemper May 23rd, 2010 @ 05:54 PM

  • Milestone changed from “2.3.6” to “2.3.7”

  [bulk edit]

• 

  Jeremy Kemper May 24th, 2010 @ 09:40 AM

  • Milestone changed from “2.3.7” to “2.3.8”

  [bulk edit]

• 

  Jeremy Kemper May 25th, 2010 @ 11:45 PM

  • Milestone changed from “2.3.8” to “2.3.9”

  [bulk edit]

• 

  Jeremy Kemper August 30th, 2010 @ 02:28 AM

  • Milestone changed from “2.3.9” to “2.3.10”
  • Importance changed from “” to “Medium”

  [bulk edit]

• 

  Santiago Pastorino February 2nd, 2011 @ 04:33 PM

  • State changed from “incomplete” to “open”

  This issue has been automatically marked as stale because it has not been commented on for at least three months.

  The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

  Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

• 

  Santiago Pastorino February 2nd, 2011 @ 04:33 PM

  • State changed from “open” to “stale”