Re: [Rails-core] [ActiveRecord] scope for collection associations

[Luís Ferreira]

Love the idea!

It clearly separates the concerns of the two models.

One thing though, would it be interesting for the scope to take and array of scopes?

On May 21, 2013, at 11:41 PM, Caleb Thompson <cjay...@gmail.com> wrote:

I'm considering implementing a feature by which a collection

association might limit the results.

By default, a collection association returns all values where the

foreign key on the `belongs_to` or `has_and_belongs_to` model matches

the parent object's primary key. These results can be filtered using the

`conditions` option, but that requires that other models have knowledge

of the parent model's table structure.

The feature I'm proposing is to add a `scope` option to collection

associations which takes a symbol representing a scope (or class mehtod)

defined on the associated model class.

A basic example would look like this:

    class Comment

      belongs_to :post

      def self.visible

        where(deleted_at: nil)

      end

    end

    class Post

      has_many :posts, scope: :visible

    end

In the above example, `post.comments` would return `Comment` instances

whose `post_id = post.id` and whose `deleted_at = NULL`, providing basic

soft-deletion functionality.

While the same effect could be achieved with a declaration such as

`has_many :posts, conditions: ['deleted_at = ?', nil]`, that betrays

knowledge of the implementation of deletion on `Post` and would break if

the implementation were changed to a boolean value for deletion rather

than a timestamp field.

If I were to work on adding this functionality into ActiveRecord, is it

something that core might entertain merging?

Thank you,

Caleb Thompson

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.
To post to this group, send email to rubyonra...@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

Cumprimentos,

Luís Ferreira

[Caleb Thompson]

I don't think that multiple scopes would add any real value. Scopes can themselves be chains of scopes:

    def recent

      visible.where('created_at >= ?' Time.now)

    end

[Luís Ferreira]

Yeah, you're right. Makes perfect sense.

I really like the approach.

[Nicolás Sanguinetti]

IIRC with Rails 4 associations introduce their methods via a module, which means you can `super` to them, so why not:

class Post

  has_many :comments

  def comments(*)

    super.visible

  end

end

I'm not too keen on having moar DSL when defining a simple method suffices. It also gives you more flexibility to call whatever on the association.

For example, your suggestion to avoid an Array of scopes is to define an extra scope / singleton method on Comment for the sole purpose of having something you can call here. But then you're defining a method on Comment that you only use from Post, which is weird (and potentially confusing—later one someone might change the scope passed to the association, but might not know that the scope definition on Comment is no longer used, so you end up with dead code).

With this approach you can simply call whatever you want on the method overriding the association, instead of having to do extra work just for the sake of having a DSL.

Cheers,

-foca

[Dheeraj Kumar]

foca, in your example, I'd use Post.find(1).comments to get only the visible comments. How would I be able to get ALL the comments, including the hidden ones? I'm thinking of passing a parameter to the comments method.

class Post

  has_many :comments

  def comments(show_hidden = false)

    show_hidden ? super : super.visible

  end

end

Is this the right way?

-- 

Dheeraj Kumar

[Nicolás Sanguinetti]

The problem with that is that you're overloading the meaning of association(flag). When passing a boolean to an association, you are telling it to force-reload the association (if true). If I read in the code "post.comments(true)" I will always think you're trying to force-reload the association, which would be weird :)

If you want to have both versions of the association (with and without the scope), I would just define a different method (`visible_comments`).

There's also a subtle bug with your code: The version of super without parentheses will invoke the superclass method *with the same arguments than the current method*, which would pass the boolean flag to the association per-se, which would have weird effects (since you'd force an extra query every time you try to load the visible comments).

You would need to call super() like so:

  def comments(show_hidden = false)

    show_hidden ? super() : super().visible

  end

Cheers,

-foca

[Dheeraj Kumar]

Ah-ha! Didn't think of the super-without-paranthesis bug! Very subtle, good catch!

-- 

Dheeraj Kumar

[Wesley Gamble]

+1

Sent from my iPhone

[Jon Leighton]

In Rails 4 you should be able to do:

class Post
has_many :comments, -> { visible }
end

[Dheeraj Kumar]

@Jon Leighton

Can that scope be removed later? Can I retrieve all comments, including hidden ones?

-- 

Dheeraj Kumar

[Gary Weaver]

@Dheeraj if you need two associations to the same model with different scopes, you could just create another association for each one that is needed, e.g.

class Post
  has_many :comments, -> { visible }

  has_many :unscoped_comments, class_name: 'Comment'
end

However, I wouldn't rely on having multiple associations with different scopes, etc. too much. Going beyond scoping, lets consider the association's "include" option. Let's say that you use include in the association to help avoid n+1 queries that you are seeing. You do this because the show method is loading associations. But when you add that, you forget that index didn't need those associations. This could significantly increase time to perform an index action. So, in the "lets just add a new association" mentality, you try to have one association that uses include and one that doesn't- except that won't work, because either the association is eagerly included or it isn't. So, instead of using include on the association in the model, you end up with Comment.all for the index method and Comment.where(...).includes(...).first in the show method. Why not leave as much as you can in the controller that relates to those queries in the individual action methods from the beginning?

[Gary Weaver]

> except that won't work, because either the association is eagerly included or it isn't.

self-correction: You could have an association that defined include and another that didn't to the same model- it isn't eager loading just because you defined another association that uses the include option. But the end result is the same- you likely end up defining the include in the controller in one action method and not the other.

[Dheeraj Kumar]

Hi Gary,

You're right. I understand that they should be left in the controller as much as possible.

Thank you for the detailed scenario and the explanations! They were very helpful :)

-- 

Dheeraj Kumar