Hi, I''ve been using AR::Base.with_scope lately, and it''s great. I''m finding it especially useful with the new around_filters that let you specify a block and yield to the filter chain. But, when you pass with_scope a :create hash, there is some strange behavior: Base.create respects the scoping, but Base.save does not, even if you are saving a new record. This feels inconsistent. A before_create filter in a model will fire in both cases; shouldn''t scoping behave the same? I''d be happy to provide tests and a patch, but wanted to check before I put those together. Is anybody working on this? Do people feel that this is the right behavior for with_scope to have? Thanks, phil -- Phillip Kast (909)630-9562 phil@unimedia.org --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
D''oh! It''s already been submitted, long ago: http://dev.rubyonrails.org/ticket/3734 Should have dug in trac more before posting -- sorry folks. Any chance this will make it into core? It has tests and cleans up a surprise lurking in with_scope. Phillip Kast wrote:> But, when you pass with_scope a :create hash, there is some strange > behavior: Base.create respects the scoping, but Base.save does not, even > if you are saving a new record. > > This feels inconsistent. A before_create filter in a model will fire in > both cases; shouldn''t scoping behave the same?--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1"
http-equiv="Content-Type">
<title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
<tt>Phil,<br>
<br>
I would love to see this patch applied. The current behavior has been
irritating me <br>
for a long time now.<br>
<br>
Thanks,<br>
Keith</tt><br>
<br>
Phillip Kast wrote:
<blockquote cite="mid4531623C.4050408@unimedia.org"
type="cite">
<pre wrap="">D''oh! It''s already been
submitted, long ago:
<a class="moz-txt-link-freetext"
href="http://dev.rubyonrails.org/ticket/3734">http://dev.rubyonrails.org/ticket/3734</a>
Should have dug in trac more before posting -- sorry folks.
Any chance this will make it into core? It has tests and cleans up a
surprise lurking in with_scope.
Phillip Kast wrote:
</pre>
<blockquote type="cite">
<pre wrap="">But, when you pass with_scope a :create hash,
there is some strange
behavior: Base.create respects the scoping, but Base.save does not, even
if you are saving a new record.
This feels inconsistent. A before_create filter in a model will fire in
both cases; shouldn''t scoping behave the same?
</pre>
</blockquote>
<pre wrap=""><!---->
</pre>
</blockquote>
<br>
--~--~---------~--~----~------------~-------~--~----~<br>
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group. <br> To post to this group,
send email to rubyonrails-core@googlegroups.com <br> To unsubscribe from
this group, send email to rubyonrails-core-unsubscribe@googlegroups.com
<br> For more options, visit this group at
http://groups.google.com/group/rubyonrails-core <br>
-~----------~----~----~----~------~----~------~--~---<br>
</body>
</html>
<br>
> D''oh! It''s already been submitted, long ago: > http://dev.rubyonrails.org/ticket/3734 > Should have dug in trac more before posting -- sorry folks. > > Any chance this will make it into core? It has tests and cleans up a > surprise lurking in with_scope.Could you give us an example of what you''re doing with with_scope that needs this. The example in the trac ticket is definitely not something that needs with_scope. login = Login.new({:name=>"anonymous"}.merge(params[:login])) login.save -- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
No prob - the ticket example does look pretty contrived.
I''m using with_scope in an around_filter, similar to the way the
scoped_access plugin does.
The idea is to cleanly ensure that a number of models are always
associated with a user''s account.
But, we have some places where stuff needs to be done to a new model
object before it''s ready to save.
Something along the lines of:
class ApplicationController < AC::Base
def scope_story_by_account
Story.with_scope(:find => {:conditions => {:account_id =>
current_account.id}, :create => {:account_id => current_account.id}) {
yield }
end
around_filter :scope_by_account
end
class StoryController < ApplicationController
def create
@story = Story.new(params[:story])
# do some stuff to @story here...
@story.save #@story.account_id won''t be set, without the
patch
we''re discussing.
end
end
...But with a bunch of models, controllers, and actions thrown into the mix.
Philosophically, it also seems to me that with_scope and ActiveRecord
callbacks should have the same behavior: if new_record is true, the
create callbacks/scoping should happen, regardless of whether you called
create or save.
phil
Michael Koziarski wrote:>> http://dev.rubyonrails.org/ticket/3734
>>
> Could you give us an example of what you''re doing with with_scope
that
> needs this. The example in the trac ticket is definitely not
> something that needs with_scope.
>
> login =
Login.new({:name=>"anonymous"}.merge(params[:login]))
> login.save
--
Phillip Kast
(909)630-9562
phil@unimedia.org
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core
-~----------~----~----~----~------~----~------~--~---
> class StoryController < ApplicationController > def create > @story = Story.new(params[:story]) > # do some stuff to @story here... > > @story.save #@story.account_id won''t be set, without the patch > we''re discussing. > end > endThe ''right'' way to do this would be class StoryController < ApplicationController def create @story = current_account.stories.build params[:story] # ... @story.save # account_id set as required. end end with_scope was originally just an implementation method for the association proxies, and any time you find yourself using it, you''re probably missing out on a much much simpler option. As it stands, I think I''d prefer the philosophical inconsistencies, if it points people at the right way to do it. -- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
> > class StoryController < ApplicationController > > def create > > @story = Story.new(params[:story]) > > # do some stuff to @story here... > > > > @story.save #@story.account_id won''t be set, without the patch > > we''re discussing. > > end > > endThis is the same example I see for with_scope over and over again, which troubles me greatly. I''ve only ever seen one example of with_scope that seemed reasonable. Mike Clark had something in his model class to merge multiple conditions into one. Even though that seemed sorta legit, I really don''t like features that point people in the wrong direction 95% of the time. Having with_scope publicly available seems to be exactly that. I''m seriously considering that we should deprecate with_scope as a public method and make it private by 2.0. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
> This is the same example I see for with_scope over and over again, > which troubles me greatly. I''ve only ever seen one example of > with_scope that seemed reasonable. Mike Clark had something in his > model class to merge multiple conditions into one. Even though that > seemed sorta legit, I really don''t like features that point people in > the wrong direction 95% of the time. Having with_scope publicly > available seems to be exactly that. > > I''m seriously considering that we should deprecate with_scope as a > public method and make it private by 2.0.Deprecation is a step too far, there are legitimate uses of with_scope, particularly around encapsulated finds for permissions models. I just think that there needs to be some serious warnings in the documentation: "You almost certainly can find a nicer way to do this" The documentation should be updated to reflected it''s intended use as well. That being said, encapsulated finds with permissions models don''t need create or new or any of that stuff. As it stands, this is a wontfix for me. -- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
I agree with Michael, Maybe just putting some warnings in the documentation would be nice. I used the feature in some limited case. I found that sometimes it helps keeping a block of the code readable and easy to understand. That been said with_scope has its limitations. Diego On Oct 15, 7:29 pm, "Michael Koziarski" <mich...@koziarski.com> wrote:> > This is the same example I see for with_scope over and over again, > > which troubles me greatly. I''ve only ever seen one example of > > with_scope that seemed reasonable. Mike Clark had something in his > > model class to merge multiple conditions into one. Even though that > > seemed sorta legit, I really don''t like features that point people in > > the wrong direction 95% of the time. Having with_scope publicly > > available seems to be exactly that. > > > I''m seriously considering that we should deprecate with_scope as a > > public method and make it private by 2.0.Deprecation is a step too far, there are legitimate uses of > with_scope, particularly around encapsulated finds for permissions > models. I just think that there needs to be some serious warnings in > the documentation: > > "You almost certainly can find a nicer way to do this" > > The documentation should be updated to reflected it''s intended use as well. > > That being said, encapsulated finds with permissions models don''t > need create or new or any of that stuff. As it stands, this is a > wontfix for me. > > -- > Cheers > > Koz--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
Thanks for the feedback. I think what I''m ultimately trying to do is legit though, so I want to try and make the case one more time. So what I want is to be able to say something in a controller like: scoped_by_current_account :story with the result that all find calls on a Story model from that controller will only return stories that belong to the current account, and any new stories will always be associated with the current account. I''ve done this. It''s easy to whip up using with_scope and around filters, and it seems like a clean and useful abstraction to me. Among other things, it reduces the risk that a hole in test coverage and/or a missing find condition could result in, say, returning stories for the wrong account. You can nest scoping and do this for several models at a time too, no problem. This works great for finds. The trouble is that you can''t do the same thing for creates, because the association won''t be set if you call save instead of create, which is too fragile. But, the abstraction makes the most sense if it works in both directions. It seems like the legit uses of with_scope that folks have mentioned all involve passing :find, and the bad uses I''ve given all use :create. Documentation would help, but the bigger issue is consistency. I think either with_scope/:create should treat new records the same way the rest of ActiveRecord does, or it should go away. Perhaps the way to make it go away would be to leave with_scope public, but extract the part that handles the :create hash to a private method. Thanks, and I hope I''m not ranting too much here -- I''m just interested in seeing this little corner of Rails be less surprising. phil DHH wrote:> This is the same example I see for with_scope over and over again, > which troubles me greatly. I''ve only ever seen one example of > with_scope that seemed reasonable. Mike Clark had something in his > model class to merge multiple conditions into one. Even though that > seemed sorta legit, I really don''t like features that point people in > the wrong direction 95% of the time. Having with_scope publicly > available seems to be exactly that. > > I''m seriously considering that we should deprecate with_scope as a > public method and make it private by 2.0. >--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
Hi, 2006/10/15, Phillip Kast <phil@unimedia.org>:> So what I want is to be able to say something in a controller like: > scoped_by_current_account :story > with the result that all find calls on a Story model from that > controller will only return stories that belong to the current account, > and any new stories will always be associated with the current account.Isn't this clearer / more understandable ? current_user.stories.find(params[:id]) It is obvious from this that you're fetching a user story, not a random story. If I don't see your filters (which I almost never bother to parse unless that's what I'm working on currently), then I won't catch your filter usage. Same goes for creating too: current_user.stories.create!(params[:story]) # works the same with build Bye ! -- François Beausoleil http://blog.teksol.info/ http://piston.rubyforge.org/ --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
The problem with with_scope, regarding showing short and simple examples of when it is particularly useful, is that the uses where with_scope shines aren''t particularly short nor simple in nature. I used with_scope once when I made a whole lot of complex finds and I wanted to limit all of them to finding objects which had is_published = 1, but only in _this_ particular case. It did, in fact, make the code easier to read, but the biggest reason I used it was to make sure I didn''t mistakenly allow one of those many finds to select non-published objects, in one fell swoop I eliminated the possibility of that happening by mistake. Improved documentation seems like a better solution than to limit the useage. This is Ruby we''re talking about, there are lots of features available to misuse already, that doesn''t take away the legitimate uses. Best regards, Tomas Jogin On 10/16/06, Francois Beausoleil <francois.beausoleil@gmail.com> wrote:> Hi, > > 2006/10/15, Phillip Kast <phil@unimedia.org>: > > So what I want is to be able to say something in a controller like: > > scoped_by_current_account :story > > with the result that all find calls on a Story model from that > > controller will only return stories that belong to the current account, > > and any new stories will always be associated with the current account. > > Isn''t this clearer / more understandable ? > > current_user.stories.find(params[:id]) > > It is obvious from this that you''re fetching a user story, not a > random story. If I don''t see your filters (which I almost never > bother to parse unless that''s what I''m working on currently), then I > won''t catch your filter usage. > > Same goes for creating too: > > current_user.stories.create!(params[:story]) > # works the same with build > > Bye ! > -- > François Beausoleil > http://blog.teksol.info/ > http://piston.rubyforge.org/ > > > >--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
> Deprecation is a step too far, there are legitimate uses of > with_scope, particularly around encapsulated finds for permissions > models. I just think that there needs to be some serious warnings in > the documentation:The decision has been rendered as follows: with_scope will go protected from Rails 2.0. That means you''ll be able to use with_scope in your own private finders (the only legitimate use I''ve seen), but not in, say, filters. Now we just have to find a way to get those deprecation messages in there when its being called in the public space. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
I think it''s a poor choice to limit something that people find useful just because it doesn''t fit your idea of useful. Why not let the coder make these decisions? I wrote that filter patch specifically for using with_scope in controllers (besides that it was a much needed feature in general). I think it''s incredibly useful. I have an application where 5-6 of the controllers are all scoped under an Order. I use simply restful to make a path like orders/1/controller/ so that all of those methods can be scoped by that order. I add one around_filter and it''s taken care of. I also access some of those controllers separately and scoped under a person. Because I have with_scope, I don''t have to write all these weird exceptions in the controller code. Just clean and simple CRUD actions. I feel very passionately that this is an excellent use of with_scope. There were plugins before the new around_filter that did this, and as soon as it''s introduced, people used it for that. Denying this usefulness is just going to irritate people. I myself will just write a public method that calls the protected with_scope if the public with_scope is deprecated. If this is viewed as illegitimate, so be it. I urge you to reconsider your decision to deprecate public with_scope and instead give a *real argument* against it in the documentation. Respectfully, Martin Emde On 10/17/06, DHH <david.heinemeier@gmail.com> wrote:> > > The decision has been rendered as follows: with_scope will go protected > from Rails 2.0. That means you''ll be able to use with_scope in your own > private finders (the only legitimate use I''ve seen), but not in, say, > filters. >--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
> I think it''s a poor choice to limit something that people find useful just > because it doesn''t fit your idea of useful. Why not let the coder make these > decisions?You must be new here :). Rails is opinionated software. We make things that we consider good style easy to do and bad style hard. Or rather, we make good style beautiful and bad style ugly. So even though with_scope is going protected, you can still use it in filters if you really, really want to. Just use send(:with_scope) -- that''ll side-step the access control. Yes, that''ll be ugly and it''s intended to be. Using around_filters in the manor you describe makes it really hard to figure out what''s going on from looking at the action in isolation. It looks like you''re doing a straight, in-secure find to the naked eye. And there''s no bread crumbs for others to follow that''ll leave them to your around_filter. It''s a recipe for hard-to-read code. And it shields the reader from understand your model hierarchy. In summary, Rails will shake its head at you for using with_scope in filters, but will ultimately leave it at your discretion to choose the right path. We believe in encouragement and discouragement, not allowing or forbidding. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---
On 10/18/06, DHH <david.heinemeier@gmail.com> wrote:> > > In summary, Rails will shake its head at you for using with_scope in > filters, but will ultimately leave it at your discretion to choose the > right path. We believe in encouragement and discouragement, not > allowing or forbidding. >Ok, I''ll accept that. I''ve always known that Rails is "oppinionated" but I''ve never run into a problem with it until now. My opinions are usually right in line with those of Rails core. I agree it can make the controller a bit harder to scan with those around_filters so I see your point. -Martin --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core -~----------~----~----~----~------~----~------~--~---