Support association strict_loading option

[rhannequin]

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

[matsales28]

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]

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

[rhannequin]

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]

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.

[laicuRoot]

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]

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]

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!

[rhannequin]

@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.