feat: Implement comparison of matcher #1552

matsales28 · 2023-04-14T21:09:00Z

Closes: #1514

Why:

In Rails, 7 was added a new option to perform validations: the ComparisonValidator with it you make a comparison between any object.

Before Rails 7

class Event < ApplicationRecord
  validates :start_date, presence: true
  validates :end_date, presence: true

  validate :end_date_is_after_start_date

  private

  def end_date_is_after_start_date
    if end_date < start_date
      errors.add(:end_date, 'cannot be before the start date')
    end
  end
end

After Rails 7

class Event < ApplicationRecord
  validates :start_date, presence: true
  validates :end_date, presence: true
  
  validates :end_date, comparison: { greater_than: :start_date }
  # OR
  validates_comparison_of :end_date, greater_than: :start_date
end

Those examples were extracted from this blogpost.

How

On shoulda, we used to have a class called lib/shoulda/matchers/active_model/comparison_matcher.rb. That class was used to implement logic to make comparisons for the numericality matcher. In this PR we are making that class generic more generic to be used on both validate_numericality_of_matcher.rb and validate_comparison_of_matcher.rb files.

Compatibility changes

The first two commits of this PR are making adjustments to that and other classes to make it easy to use on the new ValidateComparisoOfMatcher class.

Compatibility changes to Submatchers and ComparisonMatcher

We needed to make sure the ComparisonMatcher would be working for both
ValidateComparisonOfMatcher and ValidateNumericalityOfMatcher, some
changes were required to make it work properly.

We also changed the order of the submatchers, where the DissalowMatcher
of the ValidateNumericalityOfMatcher will always be evaluated first,
because of that change, it was required some minor changes in the specs.

fix: Get failure_message_when_negated from DisalllowMatcher

This commit intends to, instead of getting the first submatcher
failure_message_when_negated message to get from the DisallowMatcher
first, because the messages are easier to read and debug.

Implementation of the new Matcher and specs

feat: Implement ValidateComparisonOfMatcher class

This commit implements the base of the validate_comparion_of_matcher.
Creating the ValidateComparisonOfMatcher class and specs for the
submatchers is_greater_than, is_greater_than_or_equal_to and
is_less_than. In the next commits we'll implement the specs for the
other submatchers and for combinations of submatchers.

We are also missing the specs and implementation for comparing with
other values than just pure integers. We need to implement an way to
compare with Procs and also symbols.

feat: Implement support for comparing between string values

In this commit we added the support for making validations using the
validate_comparison_of matcher for string values. Ruby compares
strings using their ASCII/Unicode value, so for example:

'aaa' > 'bbb' # false
'bbb' > 'aaa' # true

In this example 'aaa' gets converted to an array with the unicode
values for the letters

'aaa' #=> [97, 97, 97]

Ruby compares two strings character by character based on their Unicode
code point values. When comparing two strings, it compares the
corresponding characters at each position until it finds a difference.

If the characters at the current position are the same, it moves on to
the next position and continues the comparison. If the characters at
the current position are different, it compares their Unicode code
point values to determine which character is greater. The string with
the greater character at the first differing position is considered
greater than the other string.

feat: Implement support for comparing between date values

In this commit we added the support for making validations using the
validate_comparison_of matcher for date values.

feat: Implement allow_nil, strict and with_message qualifiers

This commit inteds to implement the allow_nil, strict and
with_message qualifiers on top of ComparisonMatcher.

These qualifiers were copy/pasted from the NumericalityMatcher on
previous commits and some of them required some minor adjustments to
work properly.

Disclaimer

I'm sorry for the super long PR but I wanted to try to cover all the cases with specs.

The comparison matcher will be used now on both ValidateNumericalityOfMatcher and on the new ValidateComparisonOfMatcher, that's why we removed it from the NumericalityMatcher module.

We needed to make sure the ComparisonMatcher will be working for both ValidateComparisonOfMatcher and ValidateNumericalityOfMatcher, some changes were required to make it work properly. We also changed the order of the submatchers, where the `DissalowMatcher` of the `ValidateNumericalityOfMatcher` will always be evaluated first, because of that change it was required some minor changes in the specs.

This commit intends to instead of getting the first submatcher failure_message_when_negated message, to get from the DisallowMatcher first because the messages are more easy to read and to debug.

This commit implements the base of the validate_comparion_of_matcher. Creating the `ValidateComparisonOfMatcher` class and specs for the submatchers `is_greater_than`, `is_greater_than_or_equal_to` and `is_less_than`. In the next commits we'll implement the specs for the other submatchers and for combinations of submatchers. We are also missing the specs and implementation for comparing with other values than just pure integers. We need to implement an way to compare with Procs and also symbols.

In this commit we added the support for making validations using the `validate_comparison_of` matcher for string values. Ruby compares strings using their ASCII/Unicode value, so for example: ```ruby 'aaa' > 'bbb' # false 'bbb' > 'aaa' # true ``` In this example `'aaa'` gets converted to an array with the unicode values for the letters ```ruby 'aaa' #=> [97, 97, 97] ``` Ruby compares two strings character by character based on their Unicode code point values. When comparing two strings, it compares the corresponding characters at each position until it finds a difference. If the characters at the current position are the same, it moves on to the next position and continues the comparison. If the characters at the current position are different, it compares their Unicode code point values to determine which character is greater. The string with the greater character at the first differing position is considered greater than the other string.

In this commit we added the support for making validations using the `validate_comparison_of` matcher for date values.

This commit inteds to implement the `allow_nil`, `strict` and `with_message` qualifiers on top of ComparisonMatcher. These qualifiers were copy/pasted from the NumericalityMatcher on previous commits and some of them required some minor adjustments to work properly.

lib/shoulda/matchers/active_model/comparison_matcher.rb

matsales28 · 2023-04-16T21:08:56Z

lib/shoulda/matchers/active_model/numericality_matchers/submatchers.rb

+          def not_failing_submatcher
+            not_failing_submatchers.detect { |submatcher|
+              submatcher.instance_of?(DisallowValueMatcher)
+            } || not_failing_submatchers.first
+          end


That was something that bothered me for a while when creating the first specs for the validate_comparison_of matcher, sometimes the failure message was very hard to understand because it was using AllowValueMatcher failure messages, the DisallowValueMatcher ones are easy to understand when dealing with matcher that didn't fail but was supposed to.

Hmm... I'm wondering if perhaps we should leave it to the developer to add matchers in the order that would lead to a better user experience instead of trying to "fix" it for them. I can see this behavior being somewhat confusing as well. Is there another approach we could take here or what do you think?

This is tricky because the developer can't define the order of the submatchers. After all, in this specific case, when the user defines one comparison matcher, we define three submatchers for that matcher, and that order is defined by the shoulda-matchers code.

Generates

validates_comparison_of :participant_count, greater_than_or_equal_to: 2 # Generates in shoulda-matchers # ComparisonMatcher with the following submatchers # DisallowValueMatcher with value 1 # AllowValueMatcher with value 2 # AllowValue matcher with value 2

I totally get what you're saying about this behavior being confusing. Honestly, I'm fresh out of ideas here. I'm also fine reverting that.

I reverted that change on 5906112

Ah sorry for not replying, I'm slammed with work currently 😵‍💫 Thank you! If this indeed continues to be confusing, I'm all for revisiting this. It'll also be easier to see the consequences of this change separately.

matsales28 · 2023-04-16T21:12:46Z

lib/shoulda/matchers/active_model/validate_numericality_of_matcher.rb

@@ -377,6 +377,8 @@ def initialize(attribute)
          @allowed_type_name = 'number'
          @context = nil
          @expected_message = nil
+
+          add_disallow_non_numeric_value_matcher


I changed that behavior to the initializer because by doing this, we can ensure that the first matcher to be checked is always the one to disallow non-numerical values. With that, we can display more intuitive failure messages.

spec/unit/shoulda/matchers/active_model/validate_comparison_of_matcher_spec.rb

matsales28 · 2023-04-28T12:56:49Z

@mcmire @vsppedro Just wanted to check in on this PR - it's been sitting there for a little while now. I understand that you folks may be busy with other things, so there's no rush on my end. However, if you could take a look and let me know your thoughts when you get the chance, I'd really appreciate it. I'd love to hear if there's anything I can improve.

Thanks for all your hard work on this project!

vsppedro

LGTM! Sorry for taking so long.

tabfugnic · 2023-05-23T02:10:18Z

Nicely done! Would love to see this get merged in.

matsales28 · 2023-05-23T16:51:39Z

@vsppedro do you think we can merge, or should we wait for another review? I would love another review, especially from @mcmire, as this PR contains new features and some refactoring.

vsppedro · 2023-05-24T18:16:15Z

I think both options are good, but it's always helpful to hear what @mcmire thinks because he really knows this project inside out and has way more experience.

mcmire · 2023-05-25T05:51:22Z

I'll try to prioritize this tomorrow!

mcmire

Hi @matsales28, I'm so sorry it took me a while to review this. Overall this looks like great work — I just had some comments.

lib/shoulda/matchers/active_model/comparison_matcher.rb

spec/support/unit/change_value.rb

lib/shoulda/matchers/active_model/validate_comparison_of_matcher.rb

lib/shoulda/matchers/active_model/numericality_matchers/submatchers.rb

mcmire · 2023-05-30T20:24:06Z

lib/shoulda/matchers/active_model/numericality_matchers/submatchers.rb

+          def not_failing_submatcher
+            not_failing_submatchers.detect { |submatcher|
+              submatcher.instance_of?(DisallowValueMatcher)
+            } || not_failing_submatchers.first
+          end


Hmm... I'm wondering if perhaps we should leave it to the developer to add matchers in the order that would lead to a better user experience instead of trying to "fix" it for them. I can see this behavior being somewhat confusing as well. Is there another approach we could take here or what do you think?

mcmire

This looks good to me. Again sorry it took me a while to get to this! Thank you for your hard work on this.

michaelgpearce · 2023-06-22T19:24:05Z

I tried this out with a database time column, and it looks to not be working. I am getting this message:

     Failure/Error: it { is_expected.to validate_comparison_of(:schedule_start_time).is_less_than(:schedule_end_time) }
     
       Expected Routing::ScheduleTemplate to validate that :schedule_start_time
       looks like a value less than schedule_end_time, but this could not be
       proved.
         After setting :schedule_start_time to ‹"2000-01-02 01:00:01 UTC"› --
         which was read back as ‹Sat, 01 Jan 2000 01:00:01.000000000 UTC
         +00:00› -- the matcher expected the Routing::ScheduleTemplate to be
         invalid, but it was valid instead.
     
         As indicated in the message above, :schedule_start_time seems to be
         changing certain values as they are set, and this could have something
         to do with why this test is failing. If you've overridden the writer
         method for this attribute, then you may need to change it to make this
         test pass, or do something else entirely.

It looks like 1 day is being added, but for a time column, Rails always has the day as 2000-01-01 (so there should never be a value of 2000-01-02).

matsales28 · 2023-07-07T12:31:18Z

I tried this out with a database time column, and it looks to not be working. I am getting this message:

     Failure/Error: it { is_expected.to validate_comparison_of(:schedule_start_time).is_less_than(:schedule_end_time) }
     
       Expected Routing::ScheduleTemplate to validate that :schedule_start_time
       looks like a value less than schedule_end_time, but this could not be
       proved.
         After setting :schedule_start_time to ‹"2000-01-02 01:00:01 UTC"› --
         which was read back as ‹Sat, 01 Jan 2000 01:00:01.000000000 UTC
         +00:00› -- the matcher expected the Routing::ScheduleTemplate to be
         invalid, but it was valid instead.
     
         As indicated in the message above, :schedule_start_time seems to be
         changing certain values as they are set, and this could have something
         to do with why this test is failing. If you've overridden the writer
         method for this attribute, then you may need to change it to make this
         test pass, or do something else entirely.

It looks like 1 day is being added, but for a time column, Rails always has the day as 2000-01-01 (so there should never be a value of 2000-01-02).

Hi @michaelgpearce, thanks for reporting that! I just got back from PTO, I'll take a look at that as soon as possible!

matsales28 · 2023-07-14T17:48:54Z

Hey @michaelgpearce, I've thoroughly investigated the issue and discovered that the problem occurs not only with past dates but all dates when using a time column in Rails. The behaviour you're observing is caused by the way Rails' typecast of dates to time handles comparisons.

Rails convert any date assigned to a time column to the (default timezone)[https://guides.rubyonrails.org/configuring.html#config-active-record-default-timezone] and sets the date as 2000-01-01. This means that any validation comparison expecting a time value other than 2000-01-01 will fail because Rails compares the entire Date object, not only the hours, minutes and seconds.

For example:

class Example
  validates :start_time, comparison: { other_than: Time.zone.local(2021, 1, 1) }
end

example = Example.new(start_time: Time.zone.local(2000, 1, 1)) # We are not specifying the time, so the time should be the same as declared in the comparison
example.start_time #=> "2000-01-01 00:00:00.000000000 +0000"
example.valid? #=> true

# Rails will make the following comparison
example.start_time != Time.zone.local(2000, 1, 1) #=> true

This example demonstrates that Rails performs the comparison using the Time object, which, in this case, will be different due to the year and not the time. It's important to note that this behaviour is not limited to past dates but affects all dates when using a time column.

Another issue arises when using shoulda-matchers to validate time boundaries, such as the start and end of a day. The gem adds one second to the comparison value, which can cause unexpected behaviour.

For example:

class Example
  validates :start_time, comparison: { less_than: Time.zone.local(2020, 1, 1).end_of_day }
end

# We cannot ensure that this validation will work using `shoulda-matchers` because we cannot test that a value greater than the end of the day will be invalid.
# Adding one second to the comparison value causes it to fall into the next day (2020-01-02). However, when we assign that value to `start_time`, Rails typecasts it to the beginning of the day (2000-01-01).

Due to these behaviours, validating time boundaries precisely when using a time column and shoulda-matchers can be challenging when working with borderline time values. Alternative approaches or custom validations might be necessary to test such scenarios accurately.

Please let me know if you have further questions or need additional assistance!

@mcmire do you think that makes sense?

Some references:
Implementation of Time in Rails and how it's typecasted: https://github.dev/rails/rails/rails/activemodel/lib/active_model/type/time.rb
How the comparison matcher compares values (I was expecting both the value and the value to be compared to be typecasted, but that's not true):
https://github.dev/rails/rails/rails/activemodel/lib/active_model/validations/comparison.rb
https://stackoverflow.com/questions/34978905/rails-activerecord-postgres-time-format

jeffdill2 · 2023-12-15T21:56:26Z

@matsales28 @vsppedro Hi! Awesome work on this PR. 👏

Do you know when this will be released? Not sure if you're waiting for some other things to be wrapped up before releasing a 5.4.0.

matsales28 · 2023-12-15T23:54:05Z

@matsales28 @vsppedro Hi! Awesome work on this PR. 👏

Do you know when this will be released? Not sure if you're waiting for some other things to be wrapped up before releasing a 5.4.0.

Hey @jeffdill2, we're planning to cut a release soon. We'd like to include support for Ruby 3.3 on that release, so I'd say until the end of the year, we'll launch a new release. In the meantime, I've created a branch that includes this feature, you can use it if you want to.

gem 'shoulda-matchers', github: 'thoughtbot/shoulda-matchers', branch: 'with-comparison-matcher'

jeffdill2 · 2023-12-16T02:04:02Z

@matsales28 @vsppedro Hi! Awesome work on this PR. 👏
Do you know when this will be released? Not sure if you're waiting for some other things to be wrapped up before releasing a 5.4.0.

Hey @jeffdill2, we're planning to cut a release soon. We'd like to include support for Ruby 3.3 on that release, so I'd say until the end of the year, we'll launch a new release. In the meantime, I've created a branch that includes this feature, you can use it if you want to.
gem 'shoulda-matchers', github: 'thoughtbot/shoulda-matchers', branch: 'with-comparison-matcher'

Awesome, thanks @matsales28! 😄

Extract comparison matcher from the NumericalityMatcher module

a385eb4

The comparison matcher will be used now on both ValidateNumericalityOfMatcher and on the new ValidateComparisonOfMatcher, that's why we removed it from the NumericalityMatcher module.

matsales28 self-assigned this Apr 14, 2023

matsales28 added 6 commits April 16, 2023 18:10

fix: Get failure_message_when_negated from DisalllowMatcher

1df8859

This commit intends to instead of getting the first submatcher failure_message_when_negated message, to get from the DisallowMatcher first because the messages are more easy to read and to debug.

feat: Implement support for comparing between date values

9eb9130

In this commit we added the support for making validations using the `validate_comparison_of` matcher for date values.

matsales28 force-pushed the feat/implement-comparison-of-matcher branch 2 times, most recently from 09d8beb to 266ed7b Compare April 16, 2023 21:24

matsales28 commented Apr 16, 2023

View reviewed changes

matsales28 force-pushed the feat/implement-comparison-of-matcher branch 2 times, most recently from 12c311d to 23c76b3 Compare April 16, 2023 21:56

matsales28 marked this pull request as ready for review April 16, 2023 21:56

doc: Add documentation to validate_comparison_of matcher

9428461

matsales28 force-pushed the feat/implement-comparison-of-matcher branch from 23c76b3 to 9428461 Compare April 16, 2023 22:37

matsales28 changed the title ~~[WIP] feat: Implement comparison of matcher~~ feat: Implement comparison of matcher Apr 16, 2023

matsales28 requested review from mcmire and vsppedro April 28, 2023 12:52

vsppedro approved these changes May 17, 2023

View reviewed changes

mcmire reviewed May 30, 2023

View reviewed changes

refactor: Adjust build_option_value method to option_value

ed3d371

matsales28 requested a review from guialbuk as a code owner June 2, 2023 21:09

fix: Minor syntax adjustments

0737f1c

matsales28 force-pushed the feat/implement-comparison-of-matcher branch from 16df8f1 to 0737f1c Compare June 2, 2023 21:50

fix: Revert change for failure_message_when_negated

5906112

matsales28 requested a review from mcmire June 2, 2023 21:52

mcmire approved these changes Jun 5, 2023

View reviewed changes

matsales28 merged commit 95c3928 into thoughtbot:main Jun 7, 2023

misaka mentioned this pull request Sep 6, 2023

Define steps in ConsentForm nhsuk/manage-vaccinations-in-schools#482

Merged

matsales28 mentioned this pull request Dec 22, 2023

Cannot validate numericality with dynamic values #1583

Closed

mochetts mentioned this pull request Dec 26, 2023

Wrong error with matcher #1582

Closed

amalrik mentioned this pull request Dec 31, 2023

Numericality matcher not working as expected on 6.0.0 #1601

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement comparison of matcher #1552

feat: Implement comparison of matcher #1552

matsales28 commented Apr 14, 2023 •

edited

Loading

matsales28 Apr 16, 2023

mcmire May 30, 2023

matsales28 May 31, 2023

matsales28 Jun 2, 2023

mcmire Jun 5, 2023

matsales28 Apr 16, 2023

matsales28 commented Apr 28, 2023

vsppedro left a comment

tabfugnic commented May 23, 2023

matsales28 commented May 23, 2023

vsppedro commented May 24, 2023

mcmire commented May 25, 2023

mcmire left a comment

mcmire May 30, 2023

mcmire left a comment •

edited

Loading

michaelgpearce commented Jun 22, 2023

matsales28 commented Jul 7, 2023

matsales28 commented Jul 14, 2023

jeffdill2 commented Dec 15, 2023

matsales28 commented Dec 15, 2023

jeffdill2 commented Dec 16, 2023

feat: Implement comparison of matcher #1552

feat: Implement comparison of matcher #1552

Conversation

matsales28 commented Apr 14, 2023 • edited Loading

Why:

How

Compatibility changes

Compatibility changes to Submatchers and ComparisonMatcher

fix: Get failure_message_when_negated from DisalllowMatcher

Implementation of the new Matcher and specs

feat: Implement ValidateComparisonOfMatcher class

feat: Implement support for comparing between string values

feat: Implement support for comparing between date values

feat: Implement allow_nil, strict and with_message qualifiers

Disclaimer

matsales28 Apr 16, 2023

Choose a reason for hiding this comment

mcmire May 30, 2023

Choose a reason for hiding this comment

matsales28 May 31, 2023

Choose a reason for hiding this comment

matsales28 Jun 2, 2023

Choose a reason for hiding this comment

mcmire Jun 5, 2023

Choose a reason for hiding this comment

matsales28 Apr 16, 2023

Choose a reason for hiding this comment

matsales28 commented Apr 28, 2023

vsppedro left a comment

Choose a reason for hiding this comment

tabfugnic commented May 23, 2023

matsales28 commented May 23, 2023

vsppedro commented May 24, 2023

mcmire commented May 25, 2023

mcmire left a comment

Choose a reason for hiding this comment

mcmire May 30, 2023

Choose a reason for hiding this comment

mcmire left a comment • edited Loading

Choose a reason for hiding this comment

michaelgpearce commented Jun 22, 2023

matsales28 commented Jul 7, 2023

matsales28 commented Jul 14, 2023

jeffdill2 commented Dec 15, 2023

matsales28 commented Dec 15, 2023

jeffdill2 commented Dec 16, 2023

matsales28 commented Apr 14, 2023 •

edited

Loading

mcmire left a comment •

edited

Loading