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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fixes Rails 7.1 raise_on_assign_to_attr_readonly #1468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ recommendations of [keepachangelog.com](http://keepachangelog.com/).

### Fixed

- None
- [#1467](https://github.com/paper-trail-gem/paper_trail/issues/1467) - Rails 7.1
enables ActiveRecord.raise_on_assign_to_attr_readonly so that writing to a
attr_readonly raises an exception. Fixes paper trail to gracefully handle this
situation.

### Dependencies

Expand Down
2 changes: 1 addition & 1 deletion lib/paper_trail/reifier.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

This is the approach in #1473. Any concerns?

Copy link

Choose a reason for hiding this comment

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

I talked about this with someone today and came to the conclusion that option 1 is actually the best. Option 3 happens to work sometimes for folks. However, if you consider the business logic decision to add attr_readonly to an attribute at a certain/later point in time now affects your ability to reify a record.

In my opinion, reifying a record is a break-the-glass operation that should just always work (similar to update_column). At the very least, the assignment should just always work. Devs can then call save(validate: false) if they want to persist the an invalid record.

As for the implementation of option 1, I think papertrail will have to do some tricks to temporarily empty out ([]) the readonly array for a model while it's doing it assignment for the reify. Or, it can use private apis to set the attributes directly, bypassing the raise. Or, maybe Rails will take a patch to allow a bypass block similar to no_touching.

elsif model.respond_to?("#{k}=")
model.send("#{k}=", v)
elsif version.logger
Expand Down
1 change: 1 addition & 0 deletions spec/dummy_app/app/models/wotsit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

class Wotsit < ApplicationRecord
attr_readonly :id
has_paper_trail

belongs_to :widget, optional: true
Expand Down
39 changes: 33 additions & 6 deletions spec/models/wotsit_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,39 @@
require "spec_helper"

RSpec.describe Wotsit, versioning: true do
it "update! records timestamps" do
wotsit = described_class.create!(name: "wotsit")
wotsit.update!(name: "changed")
reified = wotsit.versions.last.reify
expect(reified.created_at).not_to(be_nil)
expect(reified.updated_at).not_to(be_nil)
context "when handling attr_readonly attributes" do
context "without raise_on_assign_to_attr_readonly" do
before do
# Rails 7.1 first introduces this setting, and framework_defaults 7.0 has it as false
if ActiveRecord.respond_to?(:raise_on_assign_to_attr_readonly)
ActiveRecord.raise_on_assign_to_attr_readonly = false
end
end

it "update! records timestamps" do
wotsit = described_class.create!(name: "wotsit")
wotsit.update!(name: "changed")
reified = wotsit.versions.last.reify
expect(reified.created_at).not_to(be_nil)
expect(reified.updated_at).not_to(be_nil)
end
end

if ActiveRecord.respond_to?(:raise_on_assign_to_attr_readonly)
context "with raise_on_assign_to_attr_readonly enabled" do
before do
ActiveRecord.raise_on_assign_to_attr_readonly = true
end

it "update! records timestamps" do
wotsit = described_class.create!(name: "wotsit")
wotsit.update!(name: "changed")
reified = wotsit.versions.last.reify
expect(reified.created_at).not_to(be_nil)
expect(reified.updated_at).not_to(be_nil)
end
end
end
end

it "update! does not raise error" do
Expand Down
Loading