Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `ActiveRecord::Relation#extract_associated` for extracting associated record #35784

Merged
merged 4 commits into from Mar 29, 2019

Conversation

Projects
None yet
5 participants
@dhh
Copy link
Member

commented Mar 28, 2019

It's sometimes more convenient to access a certain set of records by going through a scoped relation and its associations. It's easy enough to do this today using #preload and #collect, but it's repetitive and not descriptive of the intent.

This is where #extract_associated comes in. Like so:

account.memberships.extract_associated(:user)

Which is short-hand for and describing the intent of:

account.memberships.preload(:user).collect(&:user)

@rails-bot rails-bot bot added the activerecord label Mar 28, 2019

@@ -602,6 +602,11 @@ def test_preload_applies_to_all_chained_preloaded_scopes
end
end

def test_extracted_association
authors = Post.all.extract_associated(:author)
assert_equal Post.all.collect(&:author), authors

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 28, 2019

Member

Perhaps we should wrap the extract in an assert_queries 1, such that we test the preload query optimization.

This comment has been minimized.

Copy link
@dhh

dhh Mar 28, 2019

Author Member

Good idea!

@simi

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

🤔 what about load_associated?

@dhh

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

@simi

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

I'm just trying to find out something similar in current method names since extract is not common term in ActiveRecord terminology. There's already load method and that was origin of my initial idea.

I understand this as a pluck of association on relation. My second idea was pluck_associated, but it sounds a little strange me. 🤔

@dhh dhh merged commit 4e076b0 into master Mar 29, 2019

3 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
buildkite/rails Build #59939 passed (18 minutes, 14 seconds)
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dhh dhh deleted the extract-associated branch Mar 29, 2019

@rafbm

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

As it is, I feel extract_associated brings little gain, especially as it only “supports” belongs_to and has_one associations. It would be a much more meaningful addition if it also supported has_many, ie. changing collect to flat_map under the hood.

I actually think the current behavior can be quite confusing. As the doc states, extract_associated returns a “collection of X records”:

account.memberships.extract_associated(:user)
# => Returns collection of User records

You could easily expect the following to return a “collection of Comment records”:

category.posts.extract_associated(:comments)

…but you’d be wrong, as this will instead return a “collection of Comment collections”.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.