Make AR::Relation include Enumerable #8794

Closed
wants to merge 1 commit into
from

Projects

None yet

8 participants

@bogdan
Contributor
bogdan commented Jan 7, 2013

In this way Relation will gain more Array behaviors (like as_json method that is no longer required in Relation).
Also it delegates every method to to_a without method_missing which is faster.

A few tests need a change because collect without a block doesn't load the relation anymore. This seems ok.

@robin850
Member
@luke-gru
Contributor

I'm worried about the order of the include here for Enumerable. Wouldn't this override certain methods, such as count from ActiveRecord::Calculations? It would delegate to to_a and the records would be loaded instead of just querying for the count. Not sure if there are other cases like this, just putting this out there. Otherwise I think this makes sense.

Edit: just thought of others as well. first, last, select, find

@bogdan
Contributor
bogdan commented Jan 13, 2013

@luke-gru Enumerable goes below any other module included in relation in hierarchy, so a method from any other module including Calculations and many others will be called before.

@luke-gru
Contributor

oh, right. My bad!

@guiocavalcanti
Contributor

Any news from about this?

@guiocavalcanti guiocavalcanti referenced this pull request in apotonick/roar-rails May 14, 2013
Closed

Fail to evaluate ActiveRecord::Relation #40

@apotonick
Contributor

This totally makes sense and allows us to detect collections easily.

@rafaelfranca
Member

@jonleighton @tenderlove what do you think?

@jonleighton
Member

In the past we made methods like select behave differently depending on whether or not they were passed a block. This turned out to cause confusion, so we removed that. I believe select now raises an error when passed a block. Basicalley I don't think Relation can be transparently treated as enumerable, so I don't think it should include Enumerable.

@tenderlove
Member

Agree with @jonleighton

@tenderlove tenderlove closed this Sep 24, 2013
@bogdan
Contributor
bogdan commented Sep 25, 2013

It is not raising anything when block is passed:
https://github.com/rails/rails/blob/2443e0d11c0db352ec1a7c62748f19042c2c725d/activerecord/lib/active_record/relation/finder_methods.rb#L63
https://github.com/rails/rails/blob/2443e0d11c0db352ec1a7c62748f19042c2c725d/activerecord/lib/active_record/relation/query_methods.rb#L224

As I remember "Make Relation behave like Enumerable" started from the fact that in previous versions of Rails association was magically showing itself as array:

User.has_many :projects
User.first.projects.class # => Array

Maybe that was a confusion mentioned by @jonleighton, but there is right idea behind it:
Anyway people expecting associations and relations to behave like Array even if it never was it
I think we should give people what they want instead of teaching them that they have to do instead.

My personal concern behind this patch (that supposed to be next step) was ability to embed batches into relation's each method:
Long running Relation#each can appear in places where you didn't expect it initially. Like one object appeared with 10k+ records in has_many relation while the average is below 10.
That is why random memory usage tsunamis appear.

Making Relation to include Enumerable would introduce single point of control for each method. That should make each to use batches by default an easy feature.
It also would be super-cool to forget about batches and make their usage completely automatic.

@rafaelfranca
Member

We already discussed this a lot and we will stand with this decision. Relations are not Arrays even if they have some methods in common.

@bogdan bogdan deleted the bogdan:relation-enumerable branch May 7, 2014
@bogdan bogdan referenced this pull request Jun 19, 2015
@sgrif sgrif Use `Enumerable#sum` on `ActiveRecord::Relation` when a block is given
This matches our behavior in other cases where useful enumerable methods
might have a different definition in `Relation`. Wanting to actually
enumerate over the records in this case is completely reasonable, and
wanting `.sum` is reasonable for the same reason it is on `Enumerable`
in the first place.
7d14bd3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment