Fix issues with Rails 4.2.x when not using STI for version models

[TobiasBales]

Fixes #488, #483

[bramswenson]

👍 As it turns out I'm still suffering this issue with non-STI setup, but only when using reloading features ie spring.

[bramswenson]

Oh, this is not a Rails version issue btw as my project is still Rails 3.2.x and suffers the same issue exactly.

[bramswenson]

Trying to understand why this fixes the issue... @TobiasBales Do you think it was the timing of PaperTrail::Version.module_eval that caused rails to not see it as "abstract" until to late on code reload?

[TobiasBales]

Not really a timing thing.
Aparently now the paper_trail source get's reloaded in the development environment when saving a file (not sure what exactly changed that) which means the class gets discarded from the cache and rebuild from the paper_trail sources.
The initializer is not interpreted again since they are one time only, no module_eval, no abstract class -> exceptions.

The other option is to just use

require 'paper_trail/version_concern'
class MyVersionVersion < ActiveRecord::Base
  include PaperTrail::VersionConcern
 end

instead of an inheritance chain (which is kind of pointless with the VersionConcern anyway)
and just define your own versions which derive from ActiveRecord::Base

[batter]

So this is only an issue w/ PaperTrail 4.x? I'm guessing the cause is the loading of the version class via a Rails::Engine when possible (see #347), as that is what necessitates adding the PaperTrail::Rails::Engine.eager_load! to a custom initializer with the new version.

I don't even know how useful it is anymore but it sounds like maybe there should be a config option to allow users to select whether they want to eager load or lazily load the model. Fixing one issue has broken another :frown:

@TobiasBales - Won't you still need the PaperTrail::Rails::Engine.eager_load! on your initializer, even with these changes?

[batter]

Anyone have an answer for me?

[ngelx]

It works for me. Rails 4.2, ruby 2.1.2

@batter - Initialiser is not needed at all. Custom version should be something like:

class SomeCustomVersion < PaperTrail::AbstractVersion
  # custom behaviour, e.g:
  self.table_name = :some_custom_versions
end

Thanks both for your help and dedication.

[johnnymo87]

I'm on Rails 4.1.5, Ruby 2.1.0, and this did not work for me, although I don't know why. Instead, I had to define ...

if Rails.env.development?
  PaperTrail::Version.module_eval do
    self.abstract_class = true
  end
end

... over every custom versions class.

[ngelx]

@johnnymo87 sorry, but i must ask. have you changed the gemfile according?

gem 'paper_trail', github: 'TobiasBales/paper_trail', branch: 'fix-rails-4.2.0'

I'm not sure if that fix is for rails 4.2 and above, but I think not.

[johnnymo87]

@ngelx Yes. Just as a sanity-check, I also did bundle open paper_trail, and inspected that Tobias's new code was in fact there.

But I tried the other thing he mentioned in here ...

require 'paper_trail/version_concern'

module PaperTrail
  class MyCustomVersion < ::ActiveRecord::Base
    include PaperTrail::VersionConcern
    self.table_name = :my_custom_versions
    self.sequence_name = :my_custom_versions_id_seq
  end
end

... and this worked as well.

[jaredbeck]

Thanks @TobiasBales, this PR would address the particular problem of the abstract_class flag. However, I think this is a symptom of a larger problem, the practice of using an initializer to reopen PaperTrail::Version. As you say,

.. Rails now reloads gemfiles as well as application code .. the preferred method was to use an initializer which isn't reloaded ..

If whatever we put in the initializer will be lost when we reload!, then the real problem is the initializer. We must address the real problem, not just this particular symptom (the abstract_class flag).

Instead of the initializer, applications that need to reopen PaperTrail::Version should be encouraged to have their own base class that includes VersionConcern, just as you have done here Tobias. To clarify, you would put your AbstractVersion in your own codebase.

I just tried this out with a new rails app and reload! seems to work:

# app/models/abstract_version.rb
class AbstractVersion < ::ActiveRecord::Base
  include PaperTrail::VersionConcern
  self.abstract_class = true
end

# app/models/banana_version.rb
class BananaVersion < AbstractVersion
  self.table_name = :banana_versions
end

# app/models/banana.rb
class Banana < ::ActiveRecord::Base
  has_paper_trail class_name: 'BananaVersion'
end

# in console
Loading development environment (Rails 4.2.2)
Banana.create(grams: 125) 
=> #<Banana id: 7, grams: 125>
reload!
Reloading...
=> true
Banana.create(grams: 125)
=> #<Banana id: 8, grams: 125>

Tobias and Ben, please take a look at my complete experiment and let me know if I've missed anything.

If that all sounds good, I recommend that we close this PR and update the documentation, replacing any mention of an initializer with the suggestions above.

[batter]

@jaredbeck - Thanks for taking a detailed look at this. I will pull down the repo you created and play with it ASAP when I get chance, and this is great to know because I believe a lot of the issues people are seeing when they are playing around with 4.0.0.rc1 stem from the fact that there seems to be an issue with the custom initializer changes they make to their versions class not working as expected or not "sticking" so to speak.

My question is, what about users that don't want to use a custom version class of their own, but simply want to modify or add to the way that the standard PaperTrail::Version class behaves? (See #546, #526, etc). Does it make more sense to tell them to put their custom class changes in app/models/paper_trail/version.rb? Am I understanding correctly that the PaperTrail::Version class is being unloaded from memory and then reloaded into memory as part of the Gemfile reloading that is occurring in rails by default now? And does this behavior occur only when the server is being run in development or test mode?

I guess I still don't fully understand when reload! is called in the rails lifecycle. I take it this is a new(ish) feature?

[batter]

@jaredbeck - Exciting news. I was just playing around with your dummy app, trying to figure out a way to get methods and adjustments to PaperTrail::Version to stick, even after a call to reload!.

Originally I attempted this:

# config/initializers/paper_trail.rb
PaperTrail::Rails::Engine.eager_load!

module PaperTrail
  class Version < ActiveRecord::Base
    def self.foo
      'test'
    end
  end
end

$ rails c
Loading development environment in sandbox (Rails 4.2.2)
> PaperTrail::Version.foo
=> 'test'
> reload!
Reloading...
=> true
> PaperTrail::Version.foo
NoMethodError: undefined method `foo' for #<Class:0x007fb8c6807290>...

But then I got it to work by trying this:

# config/initializers/paper_trail.rb
module PaperTrail
  module VersionConcern
    module ClassMethods
      def foo
        'test'
      end
    end
  end
end

$ rails c
Loading development environment in sandbox (Rails 4.2.2)
> PaperTrail::Version.foo
=> 'test'
> reload!
Reloading...
=> true
> PaperTrail::Version.foo
=> 'test'

So perhaps this is the key to allowing users to openly modify their classes. The downside to this is that it impacts every class that includes PaperTrail::VersionConcern, but I don't think that's such a huge problem because users can always override what they don't want / need on their subclasses. Does this make sense or should we be looking for an alternative solution?

[brennovich]

It is one of the ActiveSupport::Concern magics, handle dependencies https://github.com/airblade/paper_trail/blob/master/lib/paper_trail/version_concern.rb#L5 :)

[jaredbeck]

My question is, what about users that don't want to use a custom version class of their own, but simply want to modify or add to the way that the standard PaperTrail::Version class behaves?

Yes, I think we should continue to support that in PaperTrail 4.x. I'd like to have a discussion about deprecating PaperTrail::Version and requiring users to have their own class (as carrierwave, devise, sidekiq, and cancan do), but we should postpone that discussion to PaperTrail 5.0.

Does it make more sense to tell them to put their custom class changes in app/models/paper_trail/version.rb?

That sounds good, app/models makes more sense for an AR model than config/initializers :)

Am I understanding correctly that the PaperTrail::Version class is being unloaded from memory and then reloaded into memory as part of the Gemfile reloading that is occurring in rails by default now? And does this behavior occur only when the server is being run in development or test mode?

Yes, that's my understanding as well. I suspect this behavior is controlled by rails' cache_classes flag.

.. I still don't fully understand when reload! is called in the rails lifecycle.

I believe reload! is only available in the rails console, though I suspect the underlying mechanism is the same as the cache_classes flag.

I got it to work by [reopening ClassMethods in the initializer]

I have two concerns:

1. Will this technique (reopening ClassMethods in the initializer) work for the abstract_class flag?
2. If we continue to recommend reopening anything in an initializer, it will be tempting for users to put code outside ClassMethods and continue to run into this issue. This can be mitigated with documentation, but it still feels like we're leading people in the wrong direction.

[jaredbeck]

Will this technique (reopening ClassMethods in the initializer) work for the abstract_class flag?

Ben, I don't see a way to set abstract_class by reopening VersionConcern. It would have to go in the included do block, but ActiveSupport::Concern does not allow multiple included do blocks.

Instead of the initializer, applications that need to reopen PaperTrail::Version should be encouraged to [instead] have their own base class that includes VersionConcern, just as you have done here Tobias. To clarify, you would put your AbstractVersion in your own codebase.

Have you, Tobias, or anyone else had a chance to try this technique? Please let us know.

[jaredbeck]

Thanks @TobiasBales for the PR, and everyone who joined the discussion. Tobias, your solution would work for the particular problem of the abstract_class flag, but the larger problem is the technique of reopening PaperTrail::Version in an initializer. Going forward, we're planning to recommend that people reopen or redefine PaperTrail::Version in their app/models. Here are the proposed recommendations:

#557

Please take a minute to check it out and we can continue this discussion there. Thanks!