Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent ActiveSupport::Duration.build(string) comparison bug #37013

Merged
merged 1 commit into from Aug 27, 2019

Conversation

alexeiemam
Copy link
Contributor

@alexeiemam alexeiemam commented Aug 22, 2019

Summary

This change prevents ActiveSupport::Duration instances being created by ActiveSupport::Duration.build(value) unless value is of type ::Numeric (raises TypeError)

This addresses the errant set of behaviours described in #37012 where ActiveSupport::Duration comparisons would fail confusingly or return unexpected results when comparing durations built from strings.

e.g.

## Setup
2.6.3 :001 > small_duration_from_string = ActiveSupport::Duration.build('9')
 => 9.0 seconds 
2.6.3 :002 > large_duration_from_string = ActiveSupport::Duration.build('100000000000000')
 => 3168873 years, 10 months, 6 days, 8 hours, 4 minutes, and 4.0 seconds 
2.6.3 :003 > small_duration_from_int = ActiveSupport::Duration.build(9)
 => 9.0 seconds 
## Comparison
2.6.3 :004 > large_duration_from_string > small_duration_from_string
 => false 
2.6.3 :005 > small_duration_from_string == small_duration_from_int
 => false 
2.6.3 :006 > small_duration_from_int < large_duration_from_string
Traceback (most recent call last):
        1: from (irb):6
ArgumentError (comparison of ActiveSupport::Duration::Scalar with ActiveSupport::Duration failed)
2.6.3 :007 > large_duration_from_string > small_duration_from_int
Traceback (most recent call last):
        2: from (irb):7
        1: from (irb):7:in `rescue in irb_binding'
ArgumentError (comparison of String with ActiveSupport::Duration failed)

Other Information

@alexeiemam alexeiemam changed the title Improve Duration.build String input support Improve Duration.build String input support / Fix resultant comparison bug Aug 22, 2019
@alexeiemam alexeiemam changed the title Improve Duration.build String input support / Fix resultant comparison bug Fix ActiveSupport::Duration.build(string) comparison bug Aug 23, 2019
@alexeiemam alexeiemam force-pushed the issue-37012 branch 4 times, most recently from 2f1b410 to 1a41988 Compare Aug 23, 2019
@alexeiemam
Copy link
Contributor Author

@alexeiemam alexeiemam commented Aug 23, 2019

@eileencodes @tenderlove @pixeltrix any blocking issues problems with this Pull Request?

@pixeltrix
Copy link
Contributor

@pixeltrix pixeltrix commented Aug 27, 2019

@alexeiemam my preference would be certainly for build not having to deal with string/numeric conversion. This is based on the fact that we don't support doing things like '30'.seconds so it doesn't make sense to accept strings in build.

In fact had I realised that build wasn't marked as :nodoc: I would've asked for it in the original PR that added the method, however that ship has sailed so lets not overload this method with too much responsibility, thanks.

@alexeiemam
Copy link
Contributor Author

@alexeiemam alexeiemam commented Aug 27, 2019

@pixeltrix: Would it be appropriate, then, to raise a type error on initialize if the passed value is not Numeric?
To remove the possibility of creating a Duration object that looks like a duck, swims like a duck and quacks like a duck but barks like a fish.

@pixeltrix
Copy link
Contributor

@pixeltrix pixeltrix commented Aug 27, 2019

@pixeltrix: Would it be appropriate, then, to raise a type error on initialize if the passed value is not Numeric?
To remove the possibility of creating a Duration object that looks like a duck, swims like a duck and quacks like a duck but barks like a fish.

It's arguable that we should deprecate it first but given the fact that it's broken anyway and either gives a result based on comparing two strings using > or blows up with ArgumentError I think we can go straight to raising a TypeError with a appropriate message like:

>> ActiveSupport::Duration.build('1')
TypeError (can't build an ActiveSupport::Duration from a String)

@alexeiemam
Copy link
Contributor Author

@alexeiemam alexeiemam commented Aug 27, 2019

@pixeltrix have modified the code, now raises error on .build rather than perform conversion.

@alexeiemam alexeiemam changed the title Fix ActiveSupport::Duration.build(string) comparison bug Prevent ActiveSupport::Duration.build(string) comparison bug Aug 27, 2019
end

assert_kind_of TypeError, string_build_response
assert_equal string_build_response.message, "can't build an `ActiveSupport::Duration` from a `String`"
Copy link
Member

@eugeneius eugeneius Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests could be clearer and shorter with assert_raises:

    error = assert_raises(TypeError) do
      ActiveSupport::Duration.build("9")
    end

    assert_equal "can't build an `ActiveSupport::Duration` from a `String`", error.message

Copy link
Contributor Author

@alexeiemam alexeiemam Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eugeneius: Updated in f8dbaf0

Copy link
Contributor

@pixeltrix pixeltrix left a comment

Can you also add an entry to the Active Support CHANGELOG.md and squash it down to a single commit so that we don't have the redundant changes in the Rails commit history, thanks.

@@ -181,6 +181,10 @@ def years(value) #:nodoc:
# ActiveSupport::Duration.build(2716146).parts # => {:months=>1, :days=>1}
#
def build(value)
unless value.is_a?(::Numeric)
raise TypeError, "can't build an `#{self.name}` from a `#{value.class.name}`"
Copy link
Contributor

@pixeltrix pixeltrix Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the backticks from the message please - the other message doesn't have them and core Ruby exception messages don't so I'd rather stick to that pattern (though I accept there are places elsewhere in Rails they are added).

Copy link
Contributor Author

@alexeiemam alexeiemam Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pixeltrix: Changed in 94fa3e8

Copy link
Contributor Author

@alexeiemam alexeiemam Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pixeltrix: squashed into b8b7e85

ActiveSupport::Duration.build("9")
end

assert_equal error.message, "can't build an ActiveSupport::Duration from a String"
Copy link
Member

@eugeneius eugeneius Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These assertions are backwards; the expected value should be passed first. See a46b2f8.

Copy link
Contributor Author

@alexeiemam alexeiemam Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eugeneius: changed in 94fa3e8

Copy link
Contributor Author

@alexeiemam alexeiemam Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eugeneius: Squashed into b8b7e85

@alexeiemam alexeiemam force-pushed the issue-37012 branch 3 times, most recently from 94fa3e8 to b8b7e85 Compare Aug 27, 2019
    Prevent `ActiveSupport::Duration.build(value)` from creating instances of
    `ActiveSupport::Duration` unless `value` is of type `Numeric`.

    Addresses the errant set of behaviours described in rails#37012 where
    `ActiveSupport::Duration` comparisons would fail confusingly
    or return unexpected results when comparing durations built from instances of `String`.

    Before:

        small_duration_from_string = ActiveSupport::Duration.build('9')
        large_duration_from_string = ActiveSupport::Duration.build('100000000000000')
        small_duration_from_int = ActiveSupport::Duration.build(9)

        large_duration_from_string > small_duration_from_string
            => false

        small_duration_from_string == small_duration_from_int
            => false

        small_duration_from_int < large_duration_from_string
            => ArgumentError (comparison of ActiveSupport::Duration::Scalar
                    with ActiveSupport::Duration failed)

        large_duration_from_string > small_duration_from_int
            => ArgumentError (comparison of String with ActiveSupport::Duration failed)

    After:

        small_duration_from_string = ActiveSupport::Duration.build('9')
            => TypeError (can't build an `ActiveSupport::Duration` from a `String`)
@alexeiemam
Copy link
Contributor Author

@alexeiemam alexeiemam commented Aug 27, 2019

@pixeltrix @eugeneius
Rebased against current HEAD of master for avoidance of merge conflicts.

@pixeltrix pixeltrix merged commit 45fa326 into rails:master Aug 27, 2019
2 checks passed
@pixeltrix
Copy link
Contributor

@pixeltrix pixeltrix commented Aug 27, 2019

@alexeiemam thanks for your contribution! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants