Hey Adam, Thanks for looking into this, it definitely seems like there are some fundamental design issues that are surfacing here.
I agree with the thrust of your proposal, but it would be good to
investigate whether it is possible to implement this lazy evaluation
without changing the syntax to have blocks all over the place.
For example, perhaps it would be possible to store the lambda-scopes on
the actual ActiveRecord::Relation object and only evaluate the lambdas
at the very last minute (i.e. when to_a is called).
I haven't thought about this particularly long and hard so there may be
other issues with this approach...
Cheers,
Jon
On Fri, 2011-03-04 at 11:37 -0800, Adam Wróbel wrote:
> To solve the default scope problem it's enough to execute the blocks
> at the `unscoped` level, but if we also want to solve the issue shown
> in example 4 we need to delay block execution until default_scope or
> named scopes are used. Then of course returning lambdas from the block
> is unnecessary and a proper code would look a bit cleaner than what I
> have shown above:
>
> # example 3
> class Post < ActiveRecord::Base
> default_scope { where( :locale => Locales.current ) }
> scope :valid { where( :valid => true ) }
> end
>
> And:
>
> # example 5
> class Product < ActiveRecord::Base
> scope :not_deleted { where("products.deleted_at is NULL") }
> scope :available do |*on|
> where("products.available_on <= ?", on.first ||
> Time.zone.now )
> end
> scope :active { not_deleted.available }
> end
>
> Adam
>
> On Mar 4, 6:15 pm, Adam Wróbel <[email protected]> wrote:
> > This is a rather broad topic covering a few related issues. Please
> > bear with me while I elaborate.
> >
> > Rails 3.0 supports lambda named scopes and there has been a request to
> > add support for lambda default_scopes in 3.1. With a help of a few
> > folks and under an eye of Aaron Patterson some work has been made
> > which is documented in this rather long ticket on
> > lighthouse:https://rails.lighthouseapp.com/projects/8994/tickets/1812-default_sc...
> >
> > Unfortunately we've stumbled upon problems which are impossible to
> > overcome with simple patches. With the rails 3 new where(), order(),
> > limit(), etc. functions ActiveRecord::Relation objects are created and
> > merged dynamically and always in the context of the current scope.
> > Consider those examples:
> >
> > # example 1
> > class Post < ActiveRecord::Base
> > default_scope lambda { where( :locale => Locales.current ) }
> > scope :valid, where( :valid => true )
> > end
> >
> > The `where` function will be called before the call to `scope` and it
> > will return a new ActiveRecord::Relation object that will be saved as
> > the named scope. Unfortunately that relation will be created within
> > the currently active scope, which for calls at the AR class level is
> > the default scope. Read: the default scope will be evaluated during
> > the call to `scope` and it's resulting conditions will be merged
> > with :valid scope conditions.
> >
> > Then whenever a user will call `Post.valid` two things will happen:
> > - first, default scope will be evaluated again and will produce a
> > Relation object with new, proper conditions
> > - second, this Relation will be merged with Relation saved in :valid
> > scope, which contains conditions from the call to `default_scope` at
> > the time of :valid scope declaration.
> >
> > As a result of this merge the current conditions will be overwritten
> > by that
> > outdated data.
> >
> > This also means that later you can't run :valid at the `unscoped`
> > level. Like `Post.unscoped.valid` - the resulting relation will
> > contain conditions taken from the `default_scope`.
> >
> > Note that this would not happen if the programmer decided to declare
> > the scope like this:
> >
> > # example 2
> > class Post < ActiveRecord::Base
> > default_scope lambda { where( :locale => Locales.current ) }
> > scope :valid, unscoped.where( :valid => true ) # notice
> > 'unscoped'
> > end
> >
> > In this case the :valid scope does not contain conditions from the
> > default scope. But this is not transparent to the coder. It's not The
> > Rails Way if you have to remember to use `unscoped` if you've used
> > lambda before.
> >
> > I had some ideas for dirty hacks that would work around this problem.
> > One of which ended up as a pull request on
> > github:https://github.com/rails/rails/pull/169
> >
> > In that patch I modified ActiveRecord::Relation to contain a mirror
> > relation without data from the default scope, I called that mirror
> > `without_default`. Each time a relation is merged with another so are
> > their `without_default` counterparts. The relation returned from
> > default scope has it's `without_default` cleared, so it's where the
> > "branch point" comes from. Then when I save a relation as new named
> > scope, I use it's `without_default` version.
> >
> > It's terrible, messy. I know. It gets the job done for this one issue,
> > but it's a bad design.
> >
> > What I have suggested to Aaron and others is changing the
> > `default_scope` and `scope` syntax. Have it always take blocks and
> > always evaluate them at the `unscoped` level. Basically do what I did
> > in example 2, but automatically.
> >
> > # example 3
> > class Post < ActiveRecord::Base
> > default_scope do
> > lambda { where( :locale => Locales.current ) }
> > end
> > scope :valid { where( :valid => true ) }
> > end
> >
> > This way `scope` and `default_scope` can run those blocks at the
> > `unscoped` level and they could also run this at the time of the named
> > scope usage.
> >
> > This has the added benefit of helping with another related issue.
> > Consider this bug I just found in Spree, a major e-commerce platform
> > for RoR:
> >
> > # example 4
> > class Product < ActiveRecord::Base
> > scope :not_deleted, where("products.deleted_at is NULL")
> > scope :available, lambda { |*on|
> > where("products.available_on <= ?", on.first ||
> > Time.zone.now )
> > }
> > scope :active, not_deleted.available
> > end
> >
> > I'd say this is typical. Not only is this is how most coders think
> > named scopes work, but it's also how they *should* work. Of course in
> > the current version of Rails `not_deleted.available` is evaluated
> > before being saved as an :active named scope and as a result the time
> > in the available_on condition is frozen and never changes in the
> > subsequent calls to `Product.active`.
> >
> > If we changed the `scope` syntax this would look like this:
> >
> > # example 5
> > class Product < ActiveRecord::Base
> > scope :not_deleted { where("products.deleted_at is NULL") }
> > scope :available do
> > lambda { |*on|
> > where("products.available_on <= ?", on.first ||
> > Time.zone.now ) }
> > end
> > scope :active { not_deleted.available }
> > end
> >
> > And the block passed to `scope :active` could be saved and run with
> > each call to `Product.active`.
> >
> > Anyway - it's is just a suggestion. Aaron has asked me to start a
> > discussion here, because we really need to make a decision about
> > default_scopes and lambdas. The code currently residing at master has
> > buggy support and even occasionally throws exceptions due to proc
> > merges.
> >
> > Please voice your opinions.
> >
> > Cheers,
> > Adam
>
--
http://jonathanleighton.com/
signature.asc
Description: This is a digitally signed message part
