Rails 7.2 compatibility: Add README.md note about custom audit table names

[pond]

Adds information in the README.md about changes that must be made to client code using a custom table name for audit data, which becomes necessary in Rails 7.2 but is harmless in earlier versions.

This PR was intended to have code and tests, arising from Rails 7.2's new behaviour wherein (so long as Audited::Audit is not marked abstract) it tries to access an audits table. See https://github.com/rails/rails/issues/52715. The rest of this PR description is only of interest for readers wondering why this can't be done automatically.

Merging this would close https://github.com/collectiveidea/audited/issues/725 as unfixable.

Option 1: def self.inherited inside Audited::Audit

The Rails team suggested just having Audited::Audit self-mark itself as abstract upon inheritance. Notwithstanding arguments about it being unusual for the act of inheritance to silently mutate its superclass, the approach doesn't work because the Audited gem might want the base class to be concrete since the default behaviour uses the base class table name. The example given is:

class CustomAudit < Audited::Audit
  def some_custom_behavior
    "Hiya!"
  end
end

If we were to blindly mark Audited::Audit as abstract, all such client code (the majority use case) would fail because ActiveRecord will start looking for a table name of custom_audits. We want it to keep using the base class's table name, as it would when using STI.

Side note - Audited doesn't rely on STI; it inherits, but has no type column in its table. Per https://api.rubyonrails.org/classes/ActiveRecord/Inheritance.html, "If you don’t have a type column defined in your table, single-table inheritance won’t be triggered. In that case, it’ll work just like normal subclasses with no special magic for differentiating between them or reloading the right type with find." (my emphasis). Any comments by anyone about needing to set 'abstract' specifically in relation to STI are therefore irrelevant, since Audited is using documented, defined non-STI inheritance.

This means we need to check the table name of the inheriting subclass, to find out if it differs from the superclass default. However, doing this inside def self.inherited breaks since the class definition is mid-way through; ActiveRecord explodes (quite reasonably). We need to wait until after the subclass is fully defined and then enquire. The solution to that - and a surprisingly clean-reading and high performance one at that, given what it's actually doing under the hood! - is a Tracepoint.

It looks something like this.

def self.inherited(subclass)
  TracePoint.trace(:end) do | tracepoint |
    if subclass == tracepoint.self
      subclass_table_name = tracepoint.self.table_name rescue nil
      self.abstract_class = (subclass_table_name.present? && subclass_table_name != self.table_name)

      tracepoint.disable
    end
  end

  super
end

The rescue nil is required because if you ask for the table name at this point when one is not being defined in the subclass, ActiveRecord again crashes out, but it's a benign failure. Nonetheless, while this does work there are obvious issues:

• Having a class self-mutate on inheritance is nasty and definitely violates the principle of least surprise.
• It's borderline untestable - the second anything anywhere in the test suite inherits, the base class might (or might not, depending on table name) mutate, polluting further tests.
• The fact we can use Tracepoints to do this is remarkable, but it's also really hideous and smells heavily of fragility.
• The fact we have to quietly ignore ActiveRecord raising an exception when trying to read a not-yet-configured table name is obviously a very bad sign and could cause unknown issues later in execution, and/or on earlier or later ActiveRecord gem versions.

Option 2: Custom writer for the audit_class configuration entry

This was a world of pain! On the surface it's very simple, but we go deep, deep down the rabbit hole due to our old nemesis, the autoloader. The core of the issue isn't a custom writer - it's the default reader. In the Audited module, we have this (in-method comments removed for brevity):

def audit_class
  @audit_class = @audit_class.safe_constantize if @audit_class.is_a?(String)
  @audit_class ||= Audited::Audit
end

This is a reasonable implementation. The custom class is usually configured in config/initializers/audited.rb, per README.md, but the autoloader hasn't loaded models yet. As per the documented example, the class name is given as a string - trying to reference the class directly would provoke NameError at this stage in initialization. The current gem implementation then, on reading, constantly safe_constantize's that. So long as the inheriting model has yet to be autoloaded, this will yield nil and the Audited gem will act as if there's no configured custom class. Any gem code at all that reads through the audit_class accessor "too early" will believe that Audited::Audit is the in-use audit model.

Worse, in tests, it was often the case that even at the point where a Rails application's initializer is running and actually calling the write accessor for that config value, the Audited module was defined but Audited::Audit was not. This led to the following merry dance in Audited:

def audit_class=(subclass_or_string)
  old_audit_class = @audit_class
  @audit_class = subclass_or_string

  if defined?(Audited::Audit) && old_audit_class.to_s != @audit_class.to_s
    if Rails.initialized?
      Audited::Audit.check_subclass_custom_table_name!
    else
      Rails.application.config.after_initialize do
        Audited::Audit.check_subclass_custom_table_name!
      end
    end
  end
end

...with that "if initialized" check being present to account for the potential case where someone might be setting the custom class later, for any reason - a belt & braces approach. The above calls a new class method in Audited::Audit, which I'd written in full including comments - before I realised this was a losing battle! - as:

# Check the configured audit class's table name and, if it is different
# from the default, mark this assumed-base class abstract. This prevents
# ActiveRecord from thinking STI is in use and requiring the base class's
# inferred storage table (Audit -> 'audits') to exist. Rails 7.2 and later
# require this, else an exception is raised when a custom name is used.
#
def self.check_subclass_custom_table_name!
  subclass_table_name = Audited.audit_class.table_name rescue nil
  self.abstract_class = (subclass_table_name != nil && subclass_table_name.to_s != 'audits')
end

OK, so if Audited and Audited::Audit are both loaded and parsed before someone sets the custom class, everything will work. But what if that's not the case, as happened at times in dev test? Well, we can also address this from the "other side" in Audited::Audit by adding a line underneath the above method:

self.check_subclass_custom_table_name! unless Audited.audit_class.nil?

...and that worked fine in our real-world case but not in a minimal test case (the application in the Rails issue); here, we get back to the problem that AuditTrail is not yet autoloaded and the Audited.audit_class reader therefore returns nil at this instant in execution.

We're tying ourselves in knots with increasingly convoluted code, all of which feels like a hack; adding more layers to try and work around every possible load order feels like madness; this isn't a good approach.

Option 3: Hack the subclass

Inside Audited::Audit we decide to forget any notion of abstract classes. Rails 7.2 is really, really keen on using Audited::Audit's table name - so let's set it!

def self.inherited(subclass)
  subclass.class_eval do
    def table_name=(name)
      self.superclass.table_name = name
      super
    end
  end

  super
end

The code above runs at, essentially, the instant that the parser is running through class CustomAudit < Audited::Audit, but before the class body is being parsed. This means we override CustomAudit's table_name writer before the class has a chance to set a custom table name, if it's going to. Should it do so, the Audited::Audit base class table name is set to match. It feels fragile but works, however, it does make testing difficult because - again - we are essentially mutating global properties inside Audited::Audit just because of some seemingly innocuous action inside a subclass where a dev would normally never expect such a side effect.

Behaviour is now close to standard; things are running in sort-of-STI mode all the time, just like they were before. Really, this is the most likely-to-work approach except:

• The least-surprise violation again; setting a property in a subclass mutates the superclass too. That's not clever.

• It's also nasty redefining the table_name= method in the subclass because it might for whatever reason already have its own overriding implementation and we'd squash that.

Conclusion

• https://github.com/rails/rails/issues/52715 claims a fix is easy - make self abstract in def self.inherited - but that is incorrect.
• Any fixes we might make in the gem are hacky and/or risky as well as being complex and each having caveats.
• In the end I gave up and figured the best I could do was warn people how to patch around it in README.md. Any client of Audited on Rails 7.2 or later using a custom table name will have to figure out what's wrong and fix it manually.

[pond]

Any traction on this? It now amounts to just an improvement to README.md.