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?
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.
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.
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
The '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.
> 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
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.
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.
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.
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
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
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.
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.
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.
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.
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.
