Closed rhannequin closed 6 months ago
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.
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.
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.
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.
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 theModelReflection
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 Thanks a lot for helping us with a suggested change. We just applied it and also nested all specs inside a single describe
block.
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