devise icon indicating copy to clipboard operation
devise copied to clipboard

avoid modify of frozen string in validatable.rb

Open mameier opened this issue 2 years ago • 2 comments

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 avatar Feb 02 '22 13:02 mameier

Hey @mameier, rhis does seem like a valid fix to me. I think commit 2e521bffcd93752cd08c32b41a05e3298b51eb02 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

p8 avatar Apr 22 '22 18:04 p8

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.

mameier avatar Apr 23 '22 12:04 mameier

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 https://github.com/heartcombo/devise/pull/5563. Thanks @mameier.

carlosantoniodasilva avatar Mar 01 '23 21:03 carlosantoniodasilva