Rails core - Mar 2006 - comment on new :select support for with_scope

Hi there,

I noticed that :select has been added as a supported keyword for with_scope.

I created a patch at http://dev.rubyonrails.org/ticket/4132 for adding
:include support to with_scope because of issues I was having with
using just :joins and the lack of :select support.

The reason I created this patch is explained in the ticket and that I
figured the :include route was a bit cleaner from a users perspective
than a combo of :select and :joins.

So my question is... were my assumptions wrong (ie. my reasonings for
implementing :include) and will :include just not be supported and
should I just use the :select, :join combonation from this point?

Thanks for any insight,
Andrew

> So my question is... were my assumptions wrong (ie. my reasonings for
> implementing :include) and will :include just not be supported and
> should I just use the :select, :join combonation from this point?
I understand the mechanics of the patch, but not the benefit. What''s
the use case where you want to scope-in :includes?
--
David Heinemeier Hansson
http://www.loudthinking.com -- Broadcasting Brain
http://www.basecamphq.com   -- Online project management
http://www.backpackit.com   -- Personal information manager
http://www.rubyonrails.com  -- Web-application framework

> > So my question is... were my assumptions wrong (ie. my reasonings for
> > implementing :include) and will :include just not be supported and
> > should I just use the :select, :join combonation from this point?
>
> I understand the mechanics of the patch, but not the benefit.
What''s
> the use case where you want to scope-in :includes?
Ok, this is basically where the need for this came from (a bit long,
but I hope it''s the detail you''re looking for :).

I implemented a scoping filter that is similar to the one found at
this page (see the sections closer to the bottom titled
"ScopedAccess::ClassScoping" and "More ideas to be
implemented") ...

http://habtm.com/articles/2006/02/22/nested-with_scope

My web app supports many seperate "websites" (sub-sites) and each site
should not be able to view data from others, only their own.  So my
initial option is to add a condition to all model find (Model.find)
calls specifying the current site that''s being viewed (based on
domain) to make sure that a user from site X can''t view data from site
Y from a given domain.

This can get messy and error prone due to adding the same condition
line after line and the developer may forget to add the condition to
new code.

So I decided to implement a scoping filter (again, similar to the
above implementation) which would automatically add the given
condition to the models that required them.

My models look similar to this...

- SiteRoot: contains data (id, domain) about each sub-site of the app
- SiteInfo: contains data that can change on a more frequent basis and
has a foreign key to SiteRoot
- And then SiteSpecificModels which contain a foreign key to SiteInfo,
NOT SiteRoot.

So based on this layout, my scope filter would contain something like this...
:find => {:include => [:site_root], :conditions =>
''site_root.site_id = X''}

The :include is used to connect my site specific models to the root
information and this would be used to automatically filter my models
to the given sub-site.

So now I have at least one :include in use now (from the scope filter)
in the context of a controllers method.  Now within the controller
method I may include and additional find on the model which may add
another :include option to it''s query.  With this scenario, the
include''s now have to be merged because of the outer scoping.

ex.
class WidgetController < ApplicationController
  around_filter ScopedFilter.new(SiteWidget, {:find => {:include =>
[:site_root], :condtions => ''site_root.site_id = X''}})

  def show_widgets
    @widgets = Widget.find :all, :include => [:some_other_relation]
  end
end

In this scenario the Widget.find will have to merge its :include with
the :include in the scoped filter to get the desired results.

Now back to the :select, :joins combo point I brought up in my eariler
message.  I could have used :joins in the scope filter instead of
:include, but when I did that, inner finds (a find done in a
controllers method) would be using a "select *" by default and this
would screw up the id fields (only one field of the same name is
returned) from multiple tables therefore returning incorrect results. 
A custom :select could be added to the inner find, but this would just
add the same kind repetition as adding conditions to all the model
finds, which the scope filter is trying to remove.

So in ending, I added support for :include to get the results I needed
in the cleanest way I could come up with. :)  Does this clear things
up?  Again, I could be thinking about this the wrong way, so please
tell me if this is the case.

Thanks again,
Andrew

On 3/27/06, David Heinemeier Hansson <david.heinemeier@gmail.com>
wrote:
> > So my question is... were my assumptions wrong (ie. my reasonings for
> > implementing :include) and will :include just not be supported and
> > should I just use the :select, :join combonation from this point?
>
> I understand the mechanics of the patch, but not the benefit.
What''s
> the use case where you want to scope-in :includes?
Another case is where you''re granting or revoking access to an item on
a per user basis.   You could implement the security with item
has_many :authorisations.

You can then use with_scope and :include to take care of scoping the finders.


--
Cheers

Koz

> class WidgetController < ApplicationController
>   around_filter ScopedFilter.new(SiteWidget, {:find => {:include =>
> [:site_root], :condtions => ''site_root.site_id = X''}})
>
>   def show_widgets
>     @widgets = Widget.find :all, :include => [:some_other_relation]
>   end
> end
Get your site object in a before filter form the domain, then use its
has_many relationships to all your models.
>   def show_widgets
>     @widgets = Widget.find :all, :include => [:some_other_relation]
>   end
becomes

   def show_widgets
     @widgets = @site.widgets.find :all, :include => [:some_other_relation]
   end

if you need the variable scope you can alternatively do this:

   def show_widgets
     @widgets = scope.find :all, :include => [:some_other_relation]
   end

   def scope
      params[:global] ? Widget : @site.widgets
   end

Yes, you have to get the site object by a dedicated select and can''t
eager load it in but by no stretch of imagination would that have any
considerable negative impact on your performance.




--
Tobi
http://shopify.com       - modern e-commerce software
http://typo.leetsoft.com - Open source weblog engine
http://blog.leetsoft.com - Technical weblog

> The reason I created this patch is explained in the ticket and that I
> figured the :include route was a bit cleaner from a users perspective
> than a combo of :select and :joins.
The patch no longer applies cleanly, and seems to cause quite a few
failures (unless I''ve screwed up the merge).   Could you post an
updated version?  I''m happy to apply this if we can get it merged and
tested in time.

--
Cheers

Koz

On 3/26/06, Michael Koziarski <michael@koziarski.com>
wrote:
> > The reason I created this patch is explained in the ticket and that I
> > figured the :include route was a bit cleaner from a users perspective
> > than a combo of :select and :joins.
>
> The patch no longer applies cleanly, and seems to cause quite a few
> failures (unless I''ve screwed up the merge).   Could you post an
> updated version?  I''m happy to apply this if we can get it merged
and
> tested in time.
>
> --
> Cheers
>
> Koz
I''ll take another look right away.  I know I received a conflict with
it when I did an update today.

I''ll report back shortly.

On 3/26/06, Andrew Kaspick <akaspick@gmail.com>
wrote:
> On 3/26/06, Michael Koziarski <michael@koziarski.com> wrote:
> > > The reason I created this patch is explained in the ticket and
that I
> > > figured the :include route was a bit cleaner from a users
perspective
> > > than a combo of :select and :joins.
> >
> > The patch no longer applies cleanly, and seems to cause quite a few
> > failures (unless I''ve screwed up the merge).   Could you post
an
> > updated version?  I''m happy to apply this if we can get it
merged and
> > tested in time.
> >
> > --
> > Cheers
> >
> > Koz
>
> I''ll take another look right away.  I know I received a conflict
with
> it when I did an update today.
>
> I''ll report back shortly.
>
I get the following without the patch...

test_create_table_with_binary_column(MigrationTest)
[./test/migration_test.rb:470]:
<""> expected but was
<nil>.

With the patch I seem to be getting 5 extra errors and the above
failure.  I''ll look into the errors right now.

On 3/26/06, Andrew Kaspick <akaspick@gmail.com>
wrote:
> On 3/26/06, Andrew Kaspick <akaspick@gmail.com> wrote:
> > On 3/26/06, Michael Koziarski <michael@koziarski.com> wrote:
> > > > The reason I created this patch is explained in the ticket
and that I
> > > > figured the :include route was a bit cleaner from a users
perspective
> > > > than a combo of :select and :joins.
> > >
> > > The patch no longer applies cleanly, and seems to cause quite a
few
> > > failures (unless I''ve screwed up the merge).   Could you
post an
> > > updated version?  I''m happy to apply this if we can get
it merged and
> > > tested in time.
> > >
> > > --
> > > Cheers
> > >
> > > Koz
> >
> > I''ll take another look right away.  I know I received a
conflict with
> > it when I did an update today.
> >
> > I''ll report back shortly.
> >
>
> I get the following without the patch...
>
> test_create_table_with_binary_column(MigrationTest)
> [./test/migration_test.rb:470]:
> <""> expected but was
> <nil>.
>
> With the patch I seem to be getting 5 extra errors and the above
> failure.  I''ll look into the errors right now.
>
I have submitted a new patch file for
http://dev.rubyonrails.org/ticket/4132 that no longer produces any
conflicts or errors during the unit tests.

I''m not sure if my implemention of "merge_includes" (in 
activerecord/lib/active_record/base.rb) is ideal though...

# Merge includes so that the result is a valid +include+
def merge_includes(first, second)
  array_wrap = lambda { |val| val.is_a?(Array) ? val : ([val] unless val.nil?) }
  Array(array_wrap.call(first)) + Array(array_wrap.call(second))
end

This is supposed to combine :include''s in a compatible fashion since
they can either be an Array, Hash or a Symbol.

So if you have a better, alternate method of doing this then please
feel free to modify it or I can merge any suggestions.

Thanks again,
Andrew

> I have submitted a new patch file for
> http://dev.rubyonrails.org/ticket/4132 that no longer produces any
> conflicts or errors during the unit tests.
Applied
> This is supposed to combine :include''s in a compatible fashion
since
> they can either be an Array, Hash or a Symbol.
Shame that Object#to_a is deprecated,  I just added a local method for
it,  more code,  but easier to follow.

Thanks!
--
Cheers

Koz