Do not call table_exists? in configuration code

[fabn]

I need to precompile assets in a db less environment. I'm not able to do that because this line trigger database connection and when building assets this make assets compilation raise a PG::ConnectionBad error.

Tomorrow I'll try to see if configuring that value in PT initializers fixes the issue, by the way table_exists? should be avoided.

[jaredbeck]

I'd like to say you should set PaperTrail.config.track_associations = false (in an initializer) but I don't think that'll work because of the ||=.

    def track_associations
      @track_associations ||= PaperTrail::VersionAssociation.table_exists?
    end

In the above, even if @track_associations is false, it's still going to call table_exists?, isn't it?

[fabn]

You're right. The solution can be to change that line in

@track_associations ||= PaperTrail::VersionAssociation.connected? && PaperTrail::VersionAssociation.table_exist?

It should not have any side effect, what do you think?

[jaredbeck]

I'm not familiar with the connected? method. It sounds like it could solve your issue, but isn't there still the issue of the person who explicitly sets PaperTrail.config.track_associations = false in an initializer? I think we also need to replace the ||= with a check for nil, like this:

if @track_associations.nil?
  @track_associations = PaperTrail::VersionAssociation.connected? && 
    PaperTrail::VersionAssociation.table_exist?
end

[batter]

I'd like to say you should set PaperTrail.config.track_associations = false (in an initializer) but I don't think that'll work because of the ||=

Excellent point. I never noticed that even though it seems obvious now. I suppose that value shouldn't be memoized. I'm thinking that the accessor method should look more like this:

def track_associations
  @track_associations && PaperTrail::VersionAssociation.table_exists?
end

[fabn]

I'm not familiar with the connected? method. It sounds like it could solve your issue

Here's the implementation I don't know if can cause some issue in your code.

If your code is called before any connection is actually opened the expression PaperTrail::VersionAssociation.connected? && PaperTrail::VersionAssociation.table_exist? could be false even when the table does exist.

Maybe you can ignore my suggestion and change the method to be

def track_associations
  # rescue false will ignore any database error raised by this code, e.g. during assets compilation.
  @track_associations && PaperTrail::VersionAssociation.table_exists? rescue false
end

In this way both issues are solved.

[batter]

The only concern I have with the connected? method is that when I hop into a console session, by default in newer versions of Rails (4.x+ I'm guessing), by default ActiveRecord doesn't eager load in the database connection for tables anymore, so in a new session, PaperTrail::VersionAssociation.connected? would return false until it was invoked for the first time, correct?

@fabn - I'm guessing you are running your rake assets:precompile command with a RAILS_ENV=production flag. I believe you can prevent your issues by ensuring that a database exists with the name you have specified for your production database in your config/database.yml file? I have seen issues like that in the past when I didn't have a production level database.

Re: using rescue would likely work... I always like to avoid that if possible but in this case it may not be the worst idea. That being said, with the implementation I suggested you could use the suggestion from @jaredbeck to have an initializer set config.track_associations = false and it would avoid the problems. Neither of those solutions work out of the box without a rescue statement though which is unideal..

[fabn]

The only concern I have with the connected? method is that when I hop into a console session, by default in newer versions of Rails (4.x+ I'm guessing), by default ActiveRecord doesn't eager load in the database connection for tables anymore, so in a new session, PaperTrail::VersionAssociation.connected? would return false until it was invoked for the first time, correct?

Yes, for that reason I think connected? should not be used. I don't know what side effects can cause having track_associations return false (or true) according to database connection status.

For this reason it's better to avoid this.

@fabn - I'm guessing you are running your rake assets:precompile command with a RAILS_ENV=production flag. I believe you can prevent your issues by ensuring that a database exists with the name you have specified for your production database in your config/database.yml file? I have seen issues like that in the past when I didn't have a production level database.

Yes, I'm playing with docker and I'd like to build my images with assets ready and everything else ready.

Rails (>= 4) code allows precompilation without an active database connection and I think gems should follow this convention.

Re: using rescue would likely work... I always like to avoid that if possible but in this case it may not be the worst idea. That being said, with the implementation I suggested you could use the suggestion from @jaredbeck to have an initializer set config.track_associations = false and it would avoid the problems. Neither of those solutions work out of the box without a rescue statement though which is unideal..

I'll try, meanwhile I'm using this hack to allow assets compilation in db less env, ugly but working...

module PaperTrail
  class Version < ActiveRecord::Base
    # See https://github.com/airblade/paper_trail/issues/636
    include PaperTrail::VersionConcern rescue nil
  end
end

[seanlinsley]

Active Admin has also run into this issue. The solution was to not reference database information until it's actually needed: activeadmin/activeadmin@ba2d10f

I read through the code, and it's not clear to me why track_associations is asking whether the table exists in the first place. It's only used here:

module PaperTrail
  module VersionConcern
    extend ::ActiveSupport::Concern

    included do
      belongs_to :item, :polymorphic => true

      # Since the test suite has test coverage for this, we want to declare
      # the association when the test suite is running. This makes it pass when
      # DB is not initialized prior to test runs such as when we run on Travis
      # CI (there won't be a db in `test/dummy/db/`).
      if PaperTrail.config.track_associations?
        has_many :version_associations, :dependent => :destroy
      end

And here:

      # Saves associations if the join table for `VersionAssociation` exists.
      def save_associations(version)
        return unless PaperTrail.config.track_associations?
        self.class.reflect_on_all_associations(:belongs_to).each do |assoc|
          assoc_version_args = {
              :version_id => version.id,
              :foreign_key_name => assoc.foreign_key
          }

Neither of which should care if the table exists. If the table doesn't exist, the has_many won't throw an error. And save_associations should only be called if the table exists anyway.

So it seems like we should be fine with this: (which also allows people to set it to false)

    def track_associations
      defined?(@track_associations) ? @track_associations : true
    end

[jaredbeck]

I will try to summarize the discussion so far.

Never check database, default to true

def track_associations
  defined?(@track_associations) ? @track_associations : true
end

I think this would require configuration by users who do not track associations, or else they would get an error in save_associations.

Never check database, default to false

def track_associations
  if @track_associations.nil?
    warn "PaperTrail.config.track_associations is not set. Defaulting to false."
    false
  else
    @track_associations
  end
end

This is a breaking change, and would require configuration by everyone, but it's probably the simplest solution.

Check the database if it is not configured

def track_associations
  @track_associations.nil? ?
    PaperTrail::VersionAssociation.table_exists? :
    @track_associations
end

This is not a breaking change, and allows people who need to precompile assets without a database to configure track_associations = false. This seems like the best solution. I have created a PR: #640

Check the database if not configured, but connected?

def track_associations
  @track_associations.nil? ?
    PaperTrail::VersionAssociation.connected? && 
      PaperTrail::VersionAssociation.table_exists? :
    @track_associations
end

This does not seem to work because of load order. It seems that PaperTrail::Version is loaded before the database is connected, so the version_associations association is not defined.

[jaredbeck]

Fixed by 90dc72c, hopefully to be included in a 4.0.1 release.