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

Relations: #decorate queries DB every time #944

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cgunther
Copy link
Contributor

Similar to #932 for associations, calling #decorate on a relation loads the records every time, regardless of whether the underlying relation has already been loaded.

This is because the default of passing all as the collection to decorate ends up being a new relation, so even if it were previously loaded, the new relation including all needs to be loaded.

ActiveRecord::Associations::CollectionProxy actually inherits from ActiveRecord::Relation, so by pushing this up to the relation, we can cover both scenarios.
https://github.com/rails/rails/blob/57c24948eb5cc9e5f9a4cecb6f2060f53e2246e1/activerecord/lib/active_record/associations/collection_proxy.rb#L31

The documentation included in #932 (company.products.popular.decorate) wasn't correct as you're decorating a relation there (popular), not just the association (products).

This now covers the association itself (company.products.decorate), as well as a relation from that association (company.products.popular.decorate), as well as a relation right from the class without associations (Product.popular.decorate).

Decorating the class itself (Product.decorate) still calls all since the model class is not a relation itself.

I had to tweak one association test as previously load_target would eagerly load the association's records when decorating, whereas now the decorated association is still lazy so the records wont actually be loaded until you iterate over the decorated collection.

@cgunther cgunther force-pushed the decorate-relation-reuse-loaded-records branch from fbd0580 to bb295d1 Compare February 12, 2025 01:08
@cgunther
Copy link
Contributor Author

CI seems to be failing due to an issue with codeclimate.

@Alexander-Senko
Copy link
Collaborator

It looks like the test is failing:

rspec ./spec/models/post_spec.rb:33 # Post relations when decorated returns a decorated collection

@cgunther
Copy link
Contributor Author

cgunther commented Mar 6, 2025

I think that failing test would be fixed by #943. Without that, as each test creates posts, they remain in the DB, potentially influencing future tests. In the test that's failing, we're creating 0-2 posts, then asserting the decorated relation (all posts) matches the created posts, but the decorated relation is including some posts from prior tests, not just the posts created for this example.

@Alexander-Senko
Copy link
Collaborator

IMO, it would be perfect if tests do not depend on the DB state or other tests.

@cgunther
Copy link
Contributor Author

cgunther commented Mar 6, 2025

I think it'd help to get clarity on #943 first. If that's rejected because it's by design that the DB is not reset between each example and data from one example should leak into the next example (and leak from one run to the next run locally), then I'll rework the test here to be less affected by existing data in the DB.

I agree with the sentiment of one test not being dependent on another test, and #943 truly enforces that, as any data created for one test is wiped out before the next test.

Without #943, I think it becomes slightly trickier (though doable) to write tests as it needs to pass with whatever data is lingering in my local test DB, whatever data is lingering in your local test DB, and whatever data might linger in the CI DB from previous tests. With #943, I find the test runs to be more repeatable and deterministic as each test is exactly in control of what's in the DB for that test.

If it helps, rspec-rails by-default enables transactional fixtures (#943) in their generated rails helper:
https://github.com/rspec/rspec-rails/blob/94c4be4cafd81d155300265692cf7378cbea124a/lib/generators/rspec/install/templates/spec/rails_helper.rb#L55

Similar to drapergem#932 for associations, calling `#decorate` on a relation
loads the records every time, regardless of whether the underlying
relation has already been loaded.

This is because the default of passing `all` as the collection to
decorate ends up being a new relation, so even if it were previously
loaded, the new relation including `all` needs to be loaded.

`ActiveRecord::Associations::CollectionProxy` actually inherits from
`ActiveRecord::Relation`, so by pushing this up to the relation, we can
cover both scenarios.
https://github.com/rails/rails/blob/57c24948eb5cc9e5f9a4cecb6f2060f53e2246e1/activerecord/lib/active_record/associations/collection_proxy.rb#L31

The documentation included in drapergem#932 (`company.products.popular.decorate`)
wasn't correct as you're decorating a relation there (`popular`), not
just the association (`products`).

This now covers the association itself (`company.products.decorate`), as
well as a relation from that association (`company.products.popular.decorate`),
as well as a relation right from the class without associations
(`Product.popular.decorate`).

Decorating the class itself (`Product.decorate`) still calls `all` since
the model class is not a relation itself.
@Alexander-Senko Alexander-Senko force-pushed the decorate-relation-reuse-loaded-records branch from bb295d1 to 6c0ea6c Compare March 6, 2025 14:36
@Alexander-Senko
Copy link
Collaborator

It still fails with Ruby 2.4.

Unfortunately, those outdated actions are required and I can’t change that. Me and @y-yagi aren’t granted all the rights here.

@seanlinsley
Copy link
Member

According to https://www.ruby-lang.org/en/downloads/branches/ Ruby 3.0 and lower are no longer receiving updates, so we can probably stop supporting them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants