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

Fixes Rails 7.1 raise_on_assign_to_attr_readonly #1468

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

professor
Copy link

@professor professor commented Apr 29, 2024

Fixes #1467

Check the following boxes:

  • Wrote good commit messages.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

I manually tested this PR with these commands:

DB=sqlite  bundle exec appraisal rails-7.1 rspec /Users/todd.sedano/workspace/paper_trail/spec/models/wotsit_spec.rb
DB=sqlite  bundle exec appraisal rails-7.0 rspec /Users/todd.sedano/workspace/paper_trail/spec/models/wotsit_spec.rb
DB=sqlite  bundle exec appraisal rails-6.1 rspec /Users/todd.sedano/workspace/paper_trail/spec/models/wotsit_spec.rb

lib/paper_trail/reifier.rb Outdated Show resolved Hide resolved
lib/paper_trail/reifier.rb Outdated Show resolved Hide resolved
lib/paper_trail/reifier.rb Outdated Show resolved Hide resolved
spec/models/wotsit_spec.rb Outdated Show resolved Hide resolved
@professor professor force-pushed the rails-7.1-bug-fix-attr-readonly branch 3 times, most recently from 22b411a to af8073b Compare April 29, 2024 18:05
@professor professor force-pushed the rails-7.1-bug-fix-attr-readonly branch from af8073b to bddd0b1 Compare May 1, 2024 22:19
@@ -92,7 +92,7 @@ def init_unversioned_attrs(attrs, model)
# @api private
def reify_attribute(k, v, model, version)
if model.has_attribute?(k)
model[k.to_sym] = v
model[k.to_sym] = v unless model.class.readonly_attribute?(k.to_s)
Copy link

Choose a reason for hiding this comment

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

I think this is pretty dangerous since it silently throws away assignments of readonly attributes without the developer knowing about it. This is the exact problem that config.active_record.raise_on_assign_to_attr_readonly was looking to address.

I see some viable options:

  1. We consider paper_trail's reify operation as a "dangerous" operation that automatically disable attr_readonly declarations (somehow) and forces the attributes to be rewritten.
  2. We take the stance you can't have both worlds. As in, you can't reify a record if it's going to set a readonly attribute. This is basically not changing anything.
  3. We only set the value iff it's a different value. This allows the reify to work in some cases and fail (loudly) in others. This seems a little odd to me, but still, it's an option.

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

Successfully merging this pull request may close these issues.

reify tries to set the value for a read_only attribute
3 participants