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

Define missing attribute methods from method_missing #50594

Merged
merged 1 commit into from Jan 5, 2024

Conversation

byroot
Copy link
Member

@byroot byroot commented Jan 5, 2024

Ref: mastodon/mastodon#27622

If applications don't eager load and use the old Rails 6.1 Marshal format, they can load Active Record instances from caches without calling init_internals hence attribute methods and aliases are nnot defined yet, leading to NoMethodError

Initially I was hopping to fully get rid of the define_attribute_methods call in init_internals in favor of this new method missing, as it would remove work from the happy path, unfortunately that isn't possible because if a generated method overrides a default method inherited from object, method_missing won't be called. e.g. Kernel#format

We may want to get rid of this extra code once we remove support for the 6.1 marshal format.

cc @ClearlyClaire
FYI: @nvasilevski

Ref: mastodon/mastodon#27622

If applications don't eager load and use the old Rails 6.1 Marshal
format, they can load Active Record instances from caches without
calling `init_internals` hence attribute methods and aliases are
nnot defined yet, leading to `NoMethodError`

Initially I was hopping to fully get rid of the `define_attribute_methods`
call in `init_internals` in favor of this new method missing, as it would
remove work from the happy path, unfortunately that isn't possible
because if a generated method overrides a default method inherited from
object, `method_missing` won't be called. e.g. `Kernel#format`

We may want to get rid of this extra code once we remove support
for the 6.1 marshal format.
@casperisfine casperisfine force-pushed the ar-marshal-define-attribute-methods branch from 3aca935 to a0993f8 Compare January 5, 2024 15:56
@byroot byroot merged commit d429bfb into rails:main Jan 5, 2024
4 checks passed
@byroot
Copy link
Member Author

byroot commented Jan 5, 2024

Backported to 7-1-stable.

@skipkayhil
Copy link
Member

@byroot did the backport not get pushed up? I'm not seeing it on 7-1-stable

@byroot
Copy link
Member Author

byroot commented Jan 7, 2024

yeah I forgot to push 😫 . It's now at f7255ec

@rafaelfranca
Copy link
Member

@byroot I think isn't backported yet. I don't see this code in the branch. https://github.com/rails/rails/commits/7-1-stable/activerecord/lib/active_record/model_schema.rb

There is a changelog entry for it, but no other code change went, and the commit message is wrong.

@byroot
Copy link
Member Author

byroot commented Jan 11, 2024

Hum, I must have borked my merge, I will have a look.

casperisfine pushed a commit to Shopify/rails that referenced this pull request Jan 11, 2024
Ref: rails#50594

Define missing attribute methods from `method_missing`
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jan 11, 2024
Ref: rails#50594

Define missing attribute methods from `method_missing`
byroot added a commit that referenced this pull request Jan 11, 2024
Mistakenly added in #50594
@stephankaag
Copy link

stephankaag commented Feb 28, 2024

@byroot @rafaelfranca
This change changed / broke the following scenario where AR tries to load the schema of a non-existing table.

I noticed because motor-admin-rails uses this pattern with the audited gem.

Setup:

class Audit < ApplicationRecord
  belongs_to :auditable, polymorphic: true
end

class MotorAudit < Audit
  self.table_name = 'motor_audits'
end

class Article < ApplicationRecord
  has_many :audits, as: :auditable, class_name: "MotorAudit", inverse_of: :auditable
end

create_table :motor_audits do |t|
  t.column :auditable_id, :string
  t.column :auditable_type, :string
end

create_table :articles do |t|
  t.timestamps
end

Rails 7.1.3.2:

> article = Article.create
  TRANSACTION (0.1ms)  begin transaction
  Article Create (0.8ms)  INSERT INTO "articles" ("created_at", "updated_at") VALUES (?, ?) RETURNING "id"  [["created_at", "2024-02-28 09:09:01.693949"], ["updated_at", "2024-02-28 09:09:01.693949"]]
  TRANSACTION (0.1ms)  commit transaction
 => #<Article:0x0000000107e57588 id: 4, created_at: Wed, 28 Feb 2024 09:09:01.693949000 UTC +00:00, updated_at: Wed, 28 Feb 2024 09:09:01.693949000 UTC +00:00> 
> article.audits.create
  TRANSACTION (0.0ms)  begin transaction
  MotorAudit Create (0.2ms)  INSERT INTO "motor_audits" ("auditable_id", "auditable_type") VALUES (?, ?) RETURNING "id"  [["auditable_id", "4"], ["auditable_type", "Article"]]
  TRANSACTION (0.1ms)  commit transaction
 => #<MotorAudit:0x00000001078ffa40 id: 2, auditable_id: "4", auditable_type: "Article"> 

Rails 7.2.0.alpha:

> article = Article.create
  TRANSACTION (0.0ms)  begin transaction
  Article Create (0.8ms)  INSERT INTO "articles" ("created_at", "updated_at") VALUES (?, ?) RETURNING "id"  [["created_at", "2024-02-28 09:09:57.515359"], ["updated_at", "2024-02-28 09:09:57.515359"]]
  TRANSACTION (0.1ms)  commit transaction
 => #<Article:0x0000000105e944a8 id: 5, created_at: Wed, 28 Feb 2024 09:09:57.515359000 UTC +00:00, updated_at: Wed, 28 Feb 2024 09:09:57.515359000 UTC +00:00> 
> article.audits.create
ruby-3.2.2/bundler/gems/rails-649e99bf9267/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:484:in `table_structure': Could not find table 'audits' (ActiveRecord::StatementInvalid)

@byroot
Copy link
Member Author

byroot commented Feb 28, 2024

If the table doesn't exist, shouldn't Audit have self.abstract_class = true?

If that doesn't solve your problem, please open an issue with a repro script, and I'll figure a solution.

@stephankaag
Copy link

If the table doesn't exist, shouldn't Audit have self.abstract_class = true?

If that doesn't solve your problem, please open an issue with a repro script, and I'll figure a solution.

Not sure. But at least I wanted to mention that the behaviour is now changed.

@byroot
Copy link
Member Author

byroot commented Feb 28, 2024

But at least I wanted to mention that the behaviour is now changed.

Of course, but to be clear, if that behavior wasn't supported nor documented in the first place but just happened to work by accident, it's not a concern.

@stephankaag
Copy link

Pinging @omohokcoj as motor-admin-rails maintainer.

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

Successfully merging this pull request may close these issues.

None yet

4 participants