Implement ActiveRecord::Relation#eql? and ActiveRecord::Relation#hash

[eric-hemasystems]

Per the Ruby documentation regarding eql?:

For objects of class Object, eql? is synonymous with ==. Subclasses
normally continue this tradition by aliasing eql? to their
overridden == method....

My reading is that unless there is another good reason not to alias
eql? to == it should be.

In addition, per the Ruby documentation regarding the hash method:

must have the property that a.eql?(b) implies a.hash == b.hash

For this reason I have also implemented hash so that if two relation
objects contain the same records they return the same hash value.

My specific need is that I am doing a group_by where the key is a
ActiveRecord::Relation. Internally group_by is using a Hash which
a Hash uses eql? and hash to index.

The only relevant pre-existing issue I found was:

https://github.com/rails/rails/issues/11947#issuecomment-22967919

In that issue it was offered that eql? could alias == if that was
needed by rspec, but since it wasn't the issue went stale.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

[schneems]

is there anywhere else in Rails that we’ve aliased “eql?” like this? If not then this is a bigger change and we will need to consider adding it everywhere that the pattern is to subclass a class.

For the hash change I think you can use “as_json” to generate a hash.

Looks like Code climate is failing.

[matthewd]

This seems rather unlike the == method above.

I'm also not sure we want eql? to be quite so generous; Relation#==(Array) is convenient, but I think eql? should only be true for something more clearly Relationy.

[eric-hemasystems]

I think you are correct. I did an alias since that seemed to be the common behavior in other parts of Rails but looking again, I don't think the == method is quite the right implementation. We probably only want it to return true if they are both relation objects.

Also we want it looser than == in some ways. The == methods only consider them == if their SQL is the same if they are both relation objects. But I think we want to allow them to possibly have different SQL that arrives at the same records and still consider them eql?. This would make them behave more like Array#eql?.

If it seems reasonable to you guys I would like to just move this entirely to the activerecord/relation/delegation.rb file and just add both eql? and hash to the list of methods delegated.

This will give it the exact same semantics as Array which is what I think we want.

[matthewd]

While I can see where you're coming from, I really don't like the idea of eql? potentially causing a bunch of database activity -- both because that seems fundamentally surprising, and because it could interfere with someone trying to cache relations [in a hash] specifically to avoid extra queries.

Thus I think if a caller definitely wants a list-of-objects based interpretation, an explicit to_a is the way to go.

[eric-hemasystems]

What if we moved it down a level to an AssociationRelation? This is really my use case as I am grouping records by their associated objects. I.E.

class Widget < ApplicationRecord
  has_many :properties
end

class Property < ApplicationRecord
  belong_to :widget
end

Widget.all.group_by &:properties

I.E. I want to group all records that have the same properties.

The semantics between Relation#== and AssociationRelation#== are different. In Relation#== it doesn't materialize the records while in AssociationRelation#== it does. I think it is surprising that they have different semantics (and it leads to the complicated == implementation in Relation as it has to deal with comparing a Relation and a AssociationRelation). But that is besides the point.

Since AssociationRelation#== does load the records it would also seem reasonable that AssociationRelation#eql? would load the records. And that would at least allow that type of relation to be grouped (my use case and probably the most common use case). This way we limit the scope of the PR but also hopefully don't surprise someone else down the road when they try to group on associated records like the existing implementation surprised me.

[eric-hemasystems]

There are a number of places where eql? and == are aliased. Most of them are in Arel but the most relevant is probably on the base Active Record object itself. This allows an ActiveRecord object to be a hash key.

It seems to me that if we allow a ActiveRecord object to be a hash key then we should allow a group of ActiveRecord objects to be one. Especially since ActiveRecord::Relation logically behaves like an array often delegating methods to the materialized array.

If I were to implement a work-around I would likely call to_a instead of to_json. This is really the reason for my patch. It surprised me that if I was using a real array it would work but using the array-like ActiveRecord::Relation object it doesn't work.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.