Merge pull request #911 from tonyta/column-cache-override

Fix User-Defined Column Cache Override

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/altered_inheriting_taggable_model.rb

@@ -1,3 +1,5 @@
+require_relative 'taggable_model'
+
 class AlteredInheritingTaggableModel < TaggableModel
   acts_as_taggable_on :parts
 end

spec/internal/app/models/cached_model_with_array.rb

@@ -2,4 +2,10 @@
   class CachedModelWithArray < ActiveRecord::Base
     acts_as_taggable
   end
+  if postgresql_support_json?
+    class TaggableModelWithJson < ActiveRecord::Base
+      acts_as_taggable
+      acts_as_taggable_on :skills
+    end
+  end
 end

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/app/models/company.rb

@@ -12,4 +12,4 @@ def find_or_create_tags_from_list_with_context(tag_list, context)
       super
     end
   end
-end
+end

spec/internal/app/models/inheriting_taggable_model.rb

@@ -1,2 +1,4 @@
+require_relative 'taggable_model'
+
 class InheritingTaggableModel < TaggableModel
 end

spec/internal/app/models/market.rb

@@ -1,2 +1,2 @@
 class Market < ActsAsTaggableOn::Tag
-end
+end

spec/internal/app/models/non_standard_id_taggable_model.rb

@@ -1,5 +1,5 @@
 class NonStandardIdTaggableModel < ActiveRecord::Base
-  self.primary_key = 'an_id'
+  self.primary_key = :an_id
   acts_as_taggable
   acts_as_taggable_on :languages
   acts_as_taggable_on :skills

spec/internal/app/models/student.rb

@@ -1,2 +1,4 @@
+require_relative 'user'
+
 class Student < User
 end

spec/internal/app/models/taggable_model.rb

@@ -6,6 +6,7 @@ class TaggableModel < ActiveRecord::Base
   has_many :untaggable_models
 
   attr_reader :tag_list_submethod_called
+
   def tag_list=(v)
     @tag_list_submethod_called = true
     super

spec/internal/app/models/user.rb

@@ -1,3 +1,3 @@
 class User < ActiveRecord::Base
   acts_as_tagger
-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
-

spec/support/database.rb

@@ -28,9 +28,9 @@
     ActiveRecord::Base.establish_connection(config)
   end
 
-  load(File.dirname(__FILE__) + '/../internal/db/schema.rb')
-  load(File.dirname(__FILE__) + '/../internal/app/models/models.rb')
+  require File.dirname(__FILE__) + '/../internal/db/schema.rb'
+  Dir[File.dirname(__dir__) + '/internal/app/models/*.rb'].each { |f| require f }
 
 else
   fail "Please create #{database_yml} first to configure your database. Take a look at: #{database_yml}.sample"
-end
+end