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

avoid modify of frozen string in validatable.rb #5465

Closed
wants to merge 4 commits into from

Conversation

mameier
Copy link
Contributor

@mameier mameier commented Feb 2, 2022

frozen string was modified in assert_validations_api. Use multiline string instead.

The error is hard to reproduce but trivial:
As frozen_string_literal is set, the concatenation of the error message in line 50 is modifying a frozen string.
It's easy to use a multiline string instead.

mameier and others added 3 commits February 2, 2022 14:12
frozen string was modified in assert_validations_api. Use multiline string instead.
Devise checks existence of timestamps at various places by testing i.e.

    confirmation_sent_at && confirmation_sent_at.utc >= ...

If an ORM returns empty stings on no timestamp (i.e. when using JSON) this fails. Check response to :utc instead (works for several Time / Date / DateTime values.
@p8
Copy link
Contributor

p8 commented Apr 22, 2022

Hey @mameier, rhis does seem like a valid fix to me.
I think commit 2e521bf is unrelated and should be dropped from this PR.

Also I think we need to assert the error message here, to make sure it's a proper fix:
https://github.com/heartcombo/devise/blob/main/test/models/validatable_test.rb#L113-L115

@mameier
Copy link
Contributor Author

mameier commented Apr 23, 2022

Hey @p8 , I admit that commit 2e521bf is a different issue. I did'n mean to include it in this pull request.
I moved the commit to a new branch but don'n know how to remove it from this pull request on github.

carlosantoniodasilva added a commit that referenced this pull request Mar 1, 2023
This was raising a `FrozenError` on Ruby < 3 where interpolated strings
were considered frozen. This [changed in Ruby 3], since such strings are
dynamic there's no point in freezing them by default.

The test wasn't catching this because `FrozenError` actually inherits
from `RuntimeError`:

>> FrozenError.ancestors
=> [FrozenError, RuntimeError, StandardError, Exception, Object ...]

So the exception check passed. Now we're also checking for the error
message to ensure it raised the exception we really expected there.

Closes #5465

[changed in Ruby 3] https://bugs.ruby-lang.org/issues/17104
@carlosantoniodasilva
Copy link
Member

The behavior has changed slightly in Ruby 3 so that interpolated strings are no longer frozen:

% cat x.rb
# frozen_string_literal: true

blah = "BLAH"

raise "Could not use :validatable module since #{blah} does not respond " <<
      "to the following methods: LOL"
# on Ruby 2.7.7
% be ruby x.rb
Traceback (most recent call last):
x.rb:5:in `<main>': can't modify frozen String: "Could not use :validatable module since BLAH does not respond " (FrozenError)

# on Ruby 3.2.0
% be ruby x.rb
x.rb:5:in `<main>': Could not use :validatable module since BLAH does not respond to the following methods: LOL (RuntimeError)

See https://bugs.ruby-lang.org/issues/17104 and https://scriptday.com/blog/2020/09/16/ruby-3-0-interpolated-strings-are-no-longer-frozen for some references.

Funny enough, our tests were passing because FrozenError actually inherits from RuntimeError:

>> FrozenError.ancestors
=> [FrozenError, RuntimeError, StandardError, Exception, Object ...]

But if I change the test to check for the message, it passes on 3.2 but fails on Ruby 2.7 as expected:

Failure:
ValidatableTest#test_should_not_be_included_in_objects_with_invalid_API [test/models/validatable_test.rb:118]:
Expected /Could not use :validatable module since .* does not respond to the following methods: validates_presence_of/ to match # encoding: ASCII-8BIT
#    valid: true
"can't modify frozen String: \"Could not use :validatable module since #<Class:0x00000001315d98c8> does not respond \"".

In order to keep it compatible to the Ruby versions we currently support, I have cherry-picked the commit and expanded the tests in #5563. Thanks @mameier.

carlosantoniodasilva pushed a commit that referenced this pull request Mar 1, 2023
Expand tests to check for the actual validatable exception message

This was raising a `FrozenError` on Ruby < 3 where interpolated strings
were considered frozen. This [changed in Ruby 3], since such strings are
dynamic there's no point in freezing them by default.

The test wasn't catching this because `FrozenError` actually inherits
from `RuntimeError`:

>> FrozenError.ancestors
=> [FrozenError, RuntimeError, StandardError, Exception, Object ...]

So the exception check passed. Now we're also checking for the error
message to ensure it raised the exception we really expected there.

Closes #5465

[changed in Ruby 3] https://bugs.ruby-lang.org/issues/17104
carlosantoniodasilva pushed a commit that referenced this pull request Mar 1, 2023
Expand tests to check for the actual validatable exception message

This was raising a `FrozenError` on Ruby < 3 where interpolated strings
were considered frozen. This [changed in Ruby 3], since such strings are
dynamic there's no point in freezing them by default.

The test wasn't catching this because `FrozenError` actually inherits
from `RuntimeError`:

>> FrozenError.ancestors
=> [FrozenError, RuntimeError, StandardError, Exception, Object ...]

So the exception check passed. Now we're also checking for the error
message to ensure it raised the exception we really expected there.

Closes #5465

[changed in Ruby 3] https://bugs.ruby-lang.org/issues/17104
carlosantoniodasilva added a commit that referenced this pull request Mar 1, 2023
Expand tests to check for the actual validatable exception message

This was raising a `FrozenError` on Ruby < 3 where interpolated strings
were considered frozen. This [changed in Ruby 3], since such strings are
dynamic there's no point in freezing them by default.

The test wasn't catching this because `FrozenError` actually inherits
from `RuntimeError`:

>> FrozenError.ancestors
=> [FrozenError, RuntimeError, StandardError, Exception, Object ...]

So the exception check passed. Now we're also checking for the error
message to ensure it raised the exception we really expected there.

Closes #5465

[changed in Ruby 3] https://bugs.ruby-lang.org/issues/17104

Co-authored-by: Martin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants