Avoid overriding user-defined columns cache methods

Previously, ActsAsTaggableOn::Taggable::Cache intercepts calls to
columns by using instance_eval. Unfortunately, if the user had defined
columns before invoking acts-as-taggable-on, their method would be
overridden.

This commit ensures the intercept happens as part of the inheritance
ancestor walk-up, before any user-defined methods.

Author: tonyta

CHANGELOG.md

@@ -5,6 +5,8 @@ Each change should fall into categories that would affect whether the release is
 As such, a _Feature_ would map to either major or minor. A _bug fix_ to a patch.  And _misc_ is either minor or patch, the difference being kind of fuzzy for the purposes of history.  Adding tests would be patch level.
 
 ### [Master / Unreleased](https://github.com/mbleigh/acts-as-taggable-on/compare/v6.0.1...master)
+  * Fixes
+    * [@tonyta Avoid overriding user-defined columns cache methods](https://github.com/mbleigh/acts-as-taggable-on/pull/911)
   * Misc
     * [@gssbzn Remove legacy code for an empty query and replace it with ` ActiveRecord::none`](https://github.com/mbleigh/acts-as-taggable-on/pull/906)
 

lib/acts_as_taggable_on/taggable/cache.rb

@@ -3,47 +3,51 @@ module Cache
     def self.included(base)
       # When included, conditionally adds tag caching methods when the model
       #   has any "cached_#{tag_type}_list" column
-      base.instance_eval do
-        # @private
-        def _has_tags_cache_columns?(db_columns)
-          db_column_names = db_columns.map(&:name)
-          tag_types.any? do |context|
-            db_column_names.include?("cached_#{context.to_s.singularize}_list")
-          end
+      base.extend Columns
+    end
+
+    module Columns
+      # ActiveRecord::Base.columns makes a database connection and caches the
+      #   calculated columns hash for the record as @columns.  Since we don't
+      #   want to add caching methods until we confirm the presence of a
+      #   caching column, and we don't want to force opening a database
+      #   connection when the class is loaded, here we intercept and cache
+      #   the call to :columns as @acts_as_taggable_on_cache_columns
+      #   to mimic the underlying behavior.  While processing this first
+      #   call to columns, we do the caching column check and dynamically add
+      #   the class and instance methods
+      #   FIXME: this method cannot compile in rubinius
+      def columns
+        @acts_as_taggable_on_cache_columns ||= begin
+          db_columns = super
+          _add_tags_caching_methods if _has_tags_cache_columns?(db_columns)
+          db_columns
         end
+      end
 
-        # @private
-        def _add_tags_caching_methods
-          send :include, ActsAsTaggableOn::Taggable::Cache::InstanceMethods
-          extend ActsAsTaggableOn::Taggable::Cache::ClassMethods
+      def reset_column_information
+        super
+        @acts_as_taggable_on_cache_columns = nil
+      end
 
-          before_save :save_cached_tag_list
+      private
 
-          initialize_tags_cache
+      # @private
+      def _has_tags_cache_columns?(db_columns)
+        db_column_names = db_columns.map(&:name)
+        tag_types.any? do |context|
+          db_column_names.include?("cached_#{context.to_s.singularize}_list")
         end
+      end
 
-        # ActiveRecord::Base.columns makes a database connection and caches the
-        #   calculated columns hash for the record as @columns.  Since we don't
-        #   want to add caching methods until we confirm the presence of a
-        #   caching column, and we don't want to force opening a database
-        #   connection when the class is loaded, here we intercept and cache
-        #   the call to :columns as @acts_as_taggable_on_cache_columns
-        #   to mimic the underlying behavior.  While processing this first
-        #   call to columns, we do the caching column check and dynamically add
-        #   the class and instance methods
-        #   FIXME: this method cannot compile in rubinius
-        def columns
-          @acts_as_taggable_on_cache_columns ||= begin
-            db_columns = super
-            _add_tags_caching_methods if _has_tags_cache_columns?(db_columns)
-            db_columns
-          end
-        end
+      # @private
+      def _add_tags_caching_methods
+        send :include, ActsAsTaggableOn::Taggable::Cache::InstanceMethods
+        extend ActsAsTaggableOn::Taggable::Cache::ClassMethods
 
-        def reset_column_information
-          super
-          @acts_as_taggable_on_cache_columns = nil
-        end
+        before_save :save_cached_tag_list
+
+        initialize_tags_cache
       end
     end
 

spec/acts_as_taggable_on/caching_spec.rb

@@ -75,6 +75,12 @@
       CachedModel.reset_column_information
       expect(CachedModel.instance_variable_get(:@acts_as_taggable_on_cache_columns)).to eql(nil)
     end
+
+    it 'should not override a user-defined columns method' do
+      expect(ColumnsOverrideModel.columns.map(&:name)).not_to include('ignored_column')
+      ColumnsOverrideModel.acts_as_taggable
+      expect(ColumnsOverrideModel.columns.map(&:name)).not_to include('ignored_column')
+    end
   end
 
   describe 'with a custom delimiter' do

spec/internal/app/models/columns_override_model.rb

@@ -0,0 +1,5 @@
+class ColumnsOverrideModel < ActiveRecord::Base
+  def self.columns
+    super.reject { |c| c.name == 'ignored_column' }
+  end
+end

spec/internal/db/schema.rb

@@ -36,6 +36,12 @@
     t.column :type, :string
   end
 
+  create_table :columns_override_models, force: true do |t|
+    t.column :name, :string
+    t.column :type, :string
+    t.column :ignored_column, :string
+  end
+
   create_table :non_standard_id_taggable_models, primary_key: 'an_id', force: true do |t|
     t.column :name, :string
     t.column :type, :string

spec/spec_helper.rb

@@ -18,4 +18,3 @@
 RSpec.configure do |config|
   config.raise_errors_for_deprecations!
 end
-