Support association strict_loading option by rhannequin · Pull Request #1607 · thoughtbot/shoulda-matchers

rhannequin · 2024-01-26T14:04:21Z

strict_loading was added to Rails 6.1 to prevent lazy loading of associations.

As adding it to an association declaration can have a massive impact on the way the record and its association is treated, it can be useful to ensure in a test suite the presence of this option.

This adds support for adding the strict_loading option to an association declaration.

Co-authored-by: Jose Blanco @laicuRoot jose.blanco@thoughtbot.com

@laicuRoot

`strict_loading` was added to [Rails 6.1] to prevent lazy loading of associations. As adding it to an association declaration can have a massive impact on the way the record and its association is treated, it can be useful to ensure in a test suite the presence of this option. This adds support for adding the `strict_loading` option to an association declaration. [Rails 6.1]: rails/rails#37400 Co-authored-by: Jose Blanco @laicuRoot <jose.blanco@thoughtbot.com>

matsales28 · 2024-01-26T17:01:37Z

That's a great contribution! Thanks for working on this.

The strict_loading option can also be defined at the model and application levels.

# In the model level
class Example < ApplicationRecord
  self.strict_loading_by_default = true
end

# Globally
config.active_record.strict_loading_by_default = true

We should consider that when making the check in the AssociationMatcher. We should also create test cases for the possible combinations of those assignment levels. Enabling it in the application, and disabling it in the model, or the association, for example. There's an example of setting up the configuration on the specs using the ApplicationConfigurationHelpers#with_belongs_to_required_by_default method.

matsales28 · 2024-01-26T17:02:57Z

lib/shoulda/matchers/active_record/association_matcher.rb

+      #     # RSpec
+      #     RSpec.describe Organization, type: :model do
+      #       it { should have_many(:people).strict_loading(true) }
+      #     end
+      #
+      #     # Minitest (Shoulda)
+      #     class OrganizationTest < ActiveSupport::TestCase
+      #       should have_many(:people).strict_loading(true)
+      #     end
+      #


We could add to the documentation that we can use strict_loading without specifying the argument and it'll default to true.

rhannequin · 2024-01-26T17:53:40Z

Thank you for your review, @matsales28

It got into our mind to deal with more possible situations with this option, as you mentioned.
We encountered an issue with strict_loading: false explained in this issue: #1608
With that in mind, we started with one simple use-case. I agree that this would be better to support every single combination with strict loading.

We might be missing something, so please forgive us if the issue is written as "bug report" if it's not a bug.

I'm happy to discuss the situation, and make the necessary changes to this PR based on the outcome of our discussion.

rhannequin · 2024-02-02T14:56:43Z

We have started working on covering all possible combinations but it's going to take a while, so I'm moving the PR to draft again.

This PR adds the tests and logic for support at model level for strict_loading.

Co-authored-by: Rémy Hannequin <remy@thoughtbot.com>

laicuRoot · 2024-02-16T15:29:26Z

I'm reopening the PR as we have a working solution for all the cases.

We have added the logic in the associated_class method of the ModelReflection class, as this was the most straightforward working approach. @matsales28, please let us know if you feel this logic should live elsewhere. We are open to suggestions.

matsales28 · 2024-02-23T14:00:24Z

I'm reopening the PR as we have a working solution for all the cases.

Thanks for adding the logic to handle all the cases. It's very much appreciated!

We have added the logic in the associated_class method of the ModelReflection class, as this was the most straightforward working approach. @matsales28, please let us know if you feel this logic should live elsewhere. We are open to suggestions.

I think we could split this logic into new methods on ModelReflection and OptionVerifier, and also refactor a bit. I tested this change on a new commit, here's the diff of the commit.

diff --git a/lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb b/lib/shoulda/matchers/active_record
/association_matchers/model_reflection.rb
index 92e1f394..a415d84c 100644
--- a/lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb
+++ b/lib/shoulda/matchers/active_record/association_matchers/model_reflection.rb
@@ -13,13 +13,7 @@ module Shoulda
           end

           def associated_class
-            associated_class = reflection.klass
-
-            if subject.strict_loading_by_default && !(reflection.options[:strict_loading] == false)
-              reflection.options[:strict_loading] = true
-            end
-
-            associated_class
+            reflection.klass
           end

           def polymorphic?
@@ -76,6 +70,10 @@ module Shoulda
             reflection.options[:through]
           end

+          def strict_loading?
+            reflection.options.fetch(:strict_loading, subject.strict_loading_by_default)
+          end
+
           protected

           attr_reader :reflection, :subject
diff --git a/lib/shoulda/matchers/active_record/association_matchers/option_verifier.rb b/lib/shoulda/matchers/active_record/
association_matchers/option_verifier.rb
index ecf7818b..15cf0032 100644
--- a/lib/shoulda/matchers/active_record/association_matchers/option_verifier.rb
+++ b/lib/shoulda/matchers/active_record/association_matchers/option_verifier.rb
@@ -122,6 +122,10 @@ module Shoulda
             reflector.associated_class
           end

+          def actual_value_for_strict_loading
+            reflection.strict_loading?
+          end
+

What I'm doing is basically creating a method specific for checking the value for strict loading, and that method basically uses a new method on the ModelReflection class using the same but refactored logic we had previously. What do you think about this change?

matsales28 · 2024-02-23T14:03:19Z

spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb

      expect(Parent.new).to have_many(:children)
    end

+    context 'when the application is configured with strict_loading disabled by default' do


What do you think about nesting all those specs under the same context? When trying this branch, I wrapped everything in a context 'strict_loading' to run the specs in isolation, it also helps to identify between all the other features presents in the association_matcher_spec

matsales28

I left two comments after adjusting those comments, it's ready for merge to me!

- Refactor verifier logic - Nest all specs in one `describe` block Co-authored-by: Jose Blanco <jose.blanco@thoughtbot.com>

rhannequin · 2024-02-23T15:34:27Z

@matsales28 Thanks a lot for helping us with a suggested change. We just applied it and also nested all specs inside a single describe block.

laicuRoot marked this pull request as ready for review January 26, 2024 15:21

laicuRoot requested review from matsales28 and vsppedro as code owners January 26, 2024 15:21

matsales28 reviewed Jan 26, 2024

View reviewed changes

rhannequin mentioned this pull request Jan 26, 2024

Should implicit default values have valid expectations? #1608

Open

rhannequin added 2 commits February 2, 2024 14:46

Document when no argument is passed to strict_loading

bf00caa

Start implementing multiple configuration combinations

56df66c

rhannequin marked this pull request as draft February 2, 2024 14:56

laicuRoot and others added 2 commits February 16, 2024 14:21

Support strict_loading in model and application

5bdbb23

This PR adds the tests and logic for support at model level for strict_loading.

Refactor implementation and add application level tests

dedadd9

Co-authored-by: Rémy Hannequin <remy@thoughtbot.com>

laicuRoot marked this pull request as ready for review February 16, 2024 15:29

rhannequin requested a review from matsales28 February 16, 2024 20:55

matsales28 reviewed Feb 23, 2024

View reviewed changes

matsales28 approved these changes Feb 23, 2024

View reviewed changes

Apply reviewer's comments

75b2c5c

- Refactor verifier logic - Nest all specs in one `describe` block Co-authored-by: Jose Blanco <jose.blanco@thoughtbot.com>

rhannequin force-pushed the support-for-strict-loading branch from f3c02f3 to 75b2c5c Compare February 23, 2024 15:11

vsppedro approved these changes Feb 26, 2024

View reviewed changes

laicuRoot merged commit 30407f2 into main Feb 27, 2024

laicuRoot deleted the support-for-strict-loading branch February 27, 2024 09:25

matsales28 mentioned this pull request Mar 15, 2024

chore: Bump version to 6.2.0 #1618

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support association strict_loading option#1607

Support association strict_loading option#1607
laicuRoot merged 6 commits intomainfrom
support-for-strict-loading

rhannequin commented Jan 26, 2024

Uh oh!

matsales28 commented Jan 26, 2024

Uh oh!

matsales28 Jan 26, 2024

Uh oh!

rhannequin commented Jan 26, 2024

Uh oh!

rhannequin commented Feb 2, 2024

Uh oh!

laicuRoot commented Feb 16, 2024

Uh oh!

matsales28 commented Feb 23, 2024

Uh oh!

matsales28 Feb 23, 2024

Uh oh!

matsales28 left a comment

Uh oh!

rhannequin commented Feb 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

rhannequin commented Jan 26, 2024

Uh oh!

matsales28 commented Jan 26, 2024

Uh oh!

matsales28 Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

rhannequin commented Jan 26, 2024

Uh oh!

rhannequin commented Feb 2, 2024

Uh oh!

laicuRoot commented Feb 16, 2024

Uh oh!

matsales28 commented Feb 23, 2024

Uh oh!

matsales28 Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

matsales28 left a comment

Choose a reason for hiding this comment

Uh oh!

rhannequin commented Feb 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants