Problem with conditional validations

[sergiopatricio]

I have the following model:

class Document < ApplicationRecord
  has_one_attached :asset

  validates :asset, attached: true

  validates :asset, size: { less_than: 20.megabytes, message: 'must be smaller than 20MB' }, if: :condition1?
  validates :asset, size: { less_than: 10.megabytes, message: 'must be smaller than 10MB' }, if: :condition2?
end

When I run this test:

context 'when condition2' do
  subject(:document) { Document.new(...set condition2...) }

  it { is_expected.to validate_size_of(:asset).less_than(10.megabytes) }
end

the test fails.

I noticed that if I remove message from the validator options the test passes.

This problem started happening when updating to >= 1.1.0.

---

I did some debugging and the problem seems to be in https://github.com/igorkasyanchuk/active_storage_validations/blob/master/lib/active_storage_validations/matchers/concerns/validatable.rb, with this code:

@subject.class.validators_on(@attribute_name).find do |validator|
  validator.class == validator_class
end

This will use the first SizeValidator that it finds, but it may not be the correct one.

 @subject.class.validators_on(@attribute_name)
=> [#<ActiveStorageValidations::AttachedValidator:0x000000010efb5388 @attributes=[:asset], @options={}>,
 #<ActiveStorageValidations::SizeValidator:0x000000010ef7f4b8 @attributes=[:asset], @options={:if=>:condition1?, :less_than=>20971520, :message=>"must be smaller than 20MB"}>,
 #<ActiveStorageValidations::SizeValidator:0x000000010ef7eba8 @attributes=[:asset], @options={:if=>:condition2?, :less_than=>10485760, :message=>"must be smaller than 10MB"}>]

It finds the SizeValidator for 20MB, but I'm testing the validator for "10MB".

[Mth0158]

Hi @sergiopatricio,
Thanks for raising the issue and the debugging, we are apparently missing an extra step to identify the validation that raises the error.

A good fix would be to add another condition in the Validatable concern method that you mentioned, would you mind creating a PR with the fix + its associated tests? I would encourage the use of a shared_example for the Validatable concern that we could use on all validators during the test phase.

Or I can dedicate some time to solve it in the coming days, as you prefer

[sergiopatricio]

Thanks @Mth0158, I can try although I need to investigate a bit more how I can create a test for this.

Regarding the solution, I do not see how I could select the exact validator, I suppose it may not be possible and that's why we also check all errors in validator_class::ERROR_TYPES.
So maybe we can do something similar with the custom messages:

diff --git a/lib/active_storage_validations/matchers/concerns/validatable.rb b/lib/active_storage_validations/matchers/concerns/validatable.rb
index b3b0fa2..ac38c6d 100644
--- a/lib/active_storage_validations/matchers/concerns/validatable.rb
+++ b/lib/active_storage_validations/matchers/concerns/validatable.rb
@@ -26,7 +26,7 @@ module ActiveStorageValidations
       def available_errors
         [
           *validator_class::ERROR_TYPES,
-          *error_from_custom_message
+          *errors_from_custom_messages
         ].compact
       end

@@ -34,14 +34,14 @@ module ActiveStorageValidations
         self.class.name.gsub(/::Matchers|Matcher/, '').constantize
       end

-      def attribute_validator
-        @subject.class.validators_on(@attribute_name).find do |validator|
+      def attribute_validators
+        @subject.class.validators_on(@attribute_name).select do |validator|
           validator.class == validator_class
         end
       end

-      def error_from_custom_message
-        attribute_validator.options[:message]
+      def errors_from_custom_messages
+        attribute_validators.map { |validation| validation.options[:message] }
       end
     end
   end

But not sure if I'm breaking anything related to attribute_validator.

[Mth0158]

At first glance I think you solved the problem.
The issue was that attribute_validator was only returning the first size/dimension/etc (depending on context) validator it finds for the matcher attribute in the ActiveModel::Errors object. Therefore in your case it does not work properly because you have 2 size errors for the same attribute.
Your code will now get all the size/dimension/etc (depending on context) validators in error for the matcher attribute, and then compare it with all possible values (base case (ie gem custom message), all the custom messages added on the different size/dimension/etc (depending on context) validators).

However attribute_validator is also used in the Contextable concern to test the Rails :on validation option, so you would have to update this part too, it's a bit more work :s

[Mth0158]

@sergiopatricio forgot to tag you to get you notified :)

[sergiopatricio]

@Mth0158 my doubt is if the attribute_validator I renamed is used in https://github.com/igorkasyanchuk/active_storage_validations/blob/master/lib/active_storage_validations/matchers/concerns/contextable.rb which would brake it. (And also means that Contextable may suffer from the same bug I reported for Validatable)

[Mth0158]

Yes @sergiopatricio you are absolutely right, its a test case that is not handled properly for by these 2 concerns 😕
How do you want to operate to solve this issue?

[sergiopatricio]

I do not see how we can select the exact validator.

I could fix my case with something like this:

diff --git a/lib/active_storage_validations/matchers/concerns/validatable.rb b/lib/active_storage_validations/matchers/concerns/validatable.rb
index b3b0fa2..842c600 100644
--- a/lib/active_storage_validations/matchers/concerns/validatable.rb
+++ b/lib/active_storage_validations/matchers/concerns/validatable.rb
@@ -26,7 +26,7 @@ module ActiveStorageValidations
       def available_errors
         [
           *validator_class::ERROR_TYPES,
-          *error_from_custom_message
+          *errors_from_custom_messages
         ].compact
       end

@@ -34,14 +34,18 @@ module ActiveStorageValidations
         self.class.name.gsub(/::Matchers|Matcher/, '').constantize
       end

+      # TODO: this may return the wrong validator when we have multiple validations based on conditions (if, on, etc.)
       def attribute_validator
         @subject.class.validators_on(@attribute_name).find do |validator|
           validator.class == validator_class
         end
       end

-      def error_from_custom_message
-        attribute_validator.options[:message]
+      def errors_from_custom_messages
+        validators = @subject.class.validators_on(@attribute_name).select do |validator|
+          validator.class == validator_class
+        end
+        validators.map { |validation| validation.options[:message] }
       end
     end
   end

But Contextable would still have the bug.

I need to dig a bit more into the gem to see if I can get some ideas.

[Mth0158]

@sergiopatricio If think a little of change in the Contextable concern could do it. If we change the is_context_valid? method internals to compare to attribute_validators.map{ |attribute_validator| attribute_validator.options[:on]}.uniq with the idea that @context should be strictly included in it.
In the previous code, uniq should be replaced by something that checks uniqueness regardless of type to handle :custom and 'custom' being interpreted as 2 different contexts.

[Mth0158]

@sergiopatricio any news on this issue? Tell me if you would like some help on it, I'll gladly help!

[sergiopatricio]

@Mth0158 sorry, I will not have time to look at this for the next few days.

[Mth0158]

Hi @sergiopatricio,
The fix has been merged, it'll be available in the next release ;)

[sergiopatricio]

@Mth0158 thank you!