search

thoughtbot / shoulda-matchers

Simple one-liner tests for common Rails functionality
https://matchers.shoulda.io
MIT License
3.51k stars 912 forks source link

validate_presence_of tries to set the value for a read_only attribute #1629

Closed professor closed 5 months ago

professor commented 5 months ago

Description

Rails 7.1 modifies raise_on_assign_to_attr_readonly link to be enabled by default. In the past, the code would silently carry on. I'm now fixing our code base to no longer write to a read_only attribute.

It appears that shoulda-matchers attempts to write to read only attributes.

Reproduction Steps

  1. Create a model 
class  MyClass <  ActiveRecord::Base
    attr_readonly :company_id
    validates :company_id, presence: true
end
  1. Create a spec
RSpec.describe MyClass, type: :model do
   describe 'validation' do
       subject { create(:my_class) }

       it { is_expected.to validate_presence_of(:company_id) }
  end
end
  1. enable raise_on_assign_to_attr_readonly 
    Rails.application.config.active_record.raise_on_assign_to_attr_readonly = true

Expected behavior

tests to pass

Actual behavior

# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setter.rb:83:in `public_send'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setter.rb:83:in `set'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setter.rb:67:in `set!'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:38:in `matches?'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:24:in `each'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:24:in `detect'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:24:in `first_passing'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:581:in `public_send'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:581:in `run'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:437:in `does_not_match?'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/disallow_value_matcher.rb:32:in `matches?'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/validation_matcher.rb:179:in `run_allow_or_disallow_matcher'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/validation_matcher.rb:103:in `disallows_value_of'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb:258:in `disallows_original_or_typecast_value?'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb:180:in `block in matches?'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb:179:in `all?'
# /usr/local/bundle/ruby/3.3.0/gems/shoulda-matchers-6.2.0/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb:179:in `matches?'
# [snipped]/spec/models/my_class.rb:8:in `block (3 levels) in <top (required)>'

System configuration

shoulda_matchers version: 6.2.0 rails version: 7.1.3.2 ruby version: 3.3.1

professor commented 5 months ago

I thought this might be a solution, but it also fails:

it { is_expected.to validate_presence_of(:company_id).on(:create) }
matsales28 commented 5 months ago

Hey @professor, thanks for opening this issue. I'll have more time to look at it this Friday. Meanwhile, would you mind trying it out with a subject that is not persisted?

Using the example you provided

RSpec.describe MyClass, type: :model do
   describe 'validation' do
       subject { build(:my_class) }

       it { is_expected.to validate_presence_of(:company_id) }
  end
end

Please let me know if that works.

professor commented 5 months ago

Yes, switching from a create to a build works.

Thank you for the suggstion.