Use group rather than distinct or .uniq when using tagged_with(any:true). Fixes PostgreSQL errors

CHANGELOG.md

@@ -10,6 +10,7 @@ As such, a _Feature_ would map to either major or minor. A _bug fix_ to a patch.
 * Features
 * Fixes
   * [@jonseaberg #499 fix for race condition when multiple processes try to add the same tag](https://github.com/mbleigh/acts-as-taggable-on/pull/499)
+  * [@leklund #496 Fix for distinct and postgresql json columns errors](https://github.com/mbleigh/acts-as-taggable-on/pull/496)
 * Performance
 * Misc
 

lib/acts_as_taggable_on/acts_as_taggable_on/core.rb

@@ -152,6 +152,7 @@ def tagged_with(tags, options = {})
           end
 
           joins << tagging_join
+          group = "#{table_name}.#{primary_key}"
         else
           tags = ActsAsTaggableOn::Tag.named_any(tag_list)
 
@@ -179,7 +180,7 @@ def tagged_with(tags, options = {})
           end
         end
 
-        group = [] # Rails interprets this as a no-op in the group() call below
+        group ||= [] # Rails interprets this as a no-op in the group() call below
         if options.delete(:order_by_matching_tag_count)
           select_clause = "#{table_name}.*, COUNT(#{taggings_alias}.tag_id) AS #{taggings_alias}_count"
           group_columns = ActsAsTaggableOn::Tag.using_postgresql? ? grouped_column_names_for(self) : "#{table_name}.#{primary_key}"
@@ -208,8 +209,6 @@ def tagged_with(tags, options = {})
             having(having).
             order(order_by.join(', ')).
             readonly(false)
-
-        ((context and tag_types.one?) && options.delete(:any)) ? request : request.uniq
       end
 
       def is_taggable?

spec/acts_as_taggable_on/taggable_spec.rb

@@ -217,6 +217,13 @@
     expect(TaggableModel.tagged_with('ruby').group(:created_at).count.count).to eq(1)
   end
 
+  it "shouldn't generate a query with DISTINCT by default" do
+    @taggable.skill_list = "ruby, rails, css"
+    @taggable.save
+
+    TaggableModel.tagged_with('ruby').to_sql.should_not match /DISTINCT/
+  end
+
   it 'should be able to find by tag with context' do
     @taggable.skill_list = 'ruby, rails, css'
     @taggable.tag_list = 'bob, charlie'
@@ -236,6 +243,27 @@
     expect(TaggableModel.tagged_with('ruby').to_a).to eq(TaggableModel.tagged_with('Ruby').to_a)
   end
 
+  it "should be able to find by tags with other joins in the query" do
+    @taggable.skill_list = "ruby, rails, css"
+    @taggable.tag_list = "bob, charlie"
+    @taggable.save
+
+    TaggableModel.group(:created_at).tagged_with(['bob','css'], :any => true).first.should == @taggable
+
+    bob = TaggableModel.create(:name => "Bob", :tag_list => "ruby, rails, css")
+    frank = TaggableModel.create(:name => "Frank", :tag_list => "ruby, rails")
+    charlie = TaggableModel.create(:name => "Charlie", :skill_list => "ruby, java")
+
+    # Test for explicit distinct in select
+    bob.untaggable_models.create!
+    frank.untaggable_models.create!
+    charlie.untaggable_models.create!
+
+    TaggableModel.select("distinct(taggable_models.id), taggable_models.*").joins(:untaggable_models).tagged_with(['css','java'], :any => true).to_a.sort.should == [bob,charlie].sort
+
+    TaggableModel.select("distinct(taggable_models.id), taggable_models.*").joins(:untaggable_models).tagged_with(['rails','ruby'], :any => false).to_a.sort.should == [bob,frank].sort
+  end
+
   unless ActsAsTaggableOn::Tag.using_sqlite?
     it 'should not care about case for unicode names' do
       ActsAsTaggableOn.strict_case_match = false
@@ -754,3 +782,35 @@
     end
   end
 end
+
+
+if ActsAsTaggableOn::Tag.using_postgresql?
+  describe "Taggable model with json columns" do
+    before(:each) do
+      clean_database!
+      @taggable = TaggableModelWithJson.new(:name => "Bob Jones")
+      @taggables = [@taggable, TaggableModelWithJson.new(:name => "John Doe")]
+    end
+
+    it "should be able to find by tag with context" do
+      @taggable.skill_list = "ruby, rails, css"
+      @taggable.tag_list = "bob, charlie"
+      @taggable.save
+
+      TaggableModelWithJson.tagged_with("ruby").first.should == @taggable
+      TaggableModelWithJson.tagged_with("ruby, css").first.should == @taggable
+      TaggableModelWithJson.tagged_with("bob", :on => :skills).first.should_not == @taggable
+      TaggableModelWithJson.tagged_with("bob", :on => :tags).first.should == @taggable
+    end
+
+    it "should be able to find tagged with any tag" do
+      bob = TaggableModelWithJson.create(:name => "Bob", :tag_list => "fitter, happier, more productive", :skill_list => "ruby, rails, css")
+      frank = TaggableModelWithJson.create(:name => "Frank", :tag_list => "weaker, depressed, inefficient", :skill_list => "ruby, rails, css")
+      steve = TaggableModelWithJson.create(:name => 'Steve', :tag_list => 'fitter, happier, more productive', :skill_list => 'c++, java, ruby')
+
+      TaggableModelWithJson.tagged_with(["ruby", "java"], :order => 'taggable_model_with_jsons.name', :any => true).to_a.should == [bob, frank, steve]
+      TaggableModelWithJson.tagged_with(["c++", "fitter"], :order => 'taggable_model_with_jsons.name', :any => true).to_a.should == [bob, steve]
+      TaggableModelWithJson.tagged_with(["depressed", "css"], :order => 'taggable_model_with_jsons.name', :any => true).to_a.should == [bob, frank]
+    end
+  end
+end

spec/internal/app/models/models.rb

@@ -75,3 +75,8 @@ class OrderedTaggableModel < ActiveRecord::Base
   acts_as_ordered_taggable
   acts_as_ordered_taggable_on :colours
 end
+
+class TaggableModelWithJson < ActiveRecord::Base
+  acts_as_taggable
+  acts_as_taggable_on :skills
+end

spec/internal/db/schema.rb

@@ -73,4 +73,15 @@
     t.column :name, :string
     t.column :type, :string
   end
+
+  create_table :taggable_model_with_jsons, :force => true do |t|
+    t.column :name, :string
+    t.column :type, :string
+    if self.connection.adapter_name == 'PostgreSQL' && self.connection.execute("SHOW SERVER_VERSION").first["server_version"].to_f >= 9.2
+      type = :json
+    else
+      type = :string
+    end
+    t.column :opts, type
+  end
 end

spec/schema.rb

@@ -0,0 +1,82 @@
+ActiveRecord::Schema.define :version => 0 do
+  create_table :tags, :force => true do |t|
+    t.string :name
+    t.integer :taggings_count, :default => 0
+  end
+  add_index "tags", ["name"], name: "index_tags_on_name", unique: true
+
+  create_table :taggings, :force => true do |t|
+    t.references :tag
+
+    # You should make sure that the column created is
+    # long enough to store the required class names.
+    t.references :taggable, :polymorphic => true
+    t.references :tagger, :polymorphic => true
+
+    # Limit is created to prevent MySQL error on index
+    # length for MyISAM table type: http://bit.ly/vgW2Ql
+    t.string :context, :limit => 128
+
+    t.datetime :created_at
+  end
+  add_index "taggings",
+            ["tag_id", "taggable_id", "taggable_type", "context", "tagger_id", "tagger_type"],
+            unique: true, name: "taggings_idx"
+
+  # above copied from
+  # generators/acts_as_taggable_on/migration/migration_generator
+
+  create_table :taggable_models, :force => true do |t|
+    t.column :name, :string
+    t.column :type, :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
+  end
+
+  create_table :untaggable_models, :force => true do |t|
+    t.column :taggable_model_id, :integer
+    t.column :name, :string
+  end
+
+  create_table :cached_models, :force => true do |t|
+    t.column :name, :string
+    t.column :type, :string
+    t.column :cached_tag_list, :string
+  end
+
+  create_table :other_cached_models, :force => true do |t|
+    t.column :name, :string
+    t.column :type, :string
+    t.column :cached_language_list, :string
+    t.column :cached_status_list, :string
+    t.column :cached_glass_list, :string
+  end
+
+  create_table :users, :force => true do |t|
+    t.column :name, :string
+  end
+
+  create_table :other_taggable_models, :force => true do |t|
+    t.column :name, :string
+    t.column :type, :string
+  end
+
+  create_table :ordered_taggable_models, :force => true do |t|
+    t.column :name, :string
+    t.column :type, :string
+  end
+
+  create_table :taggable_model_with_jsons, :force => true do |t|
+    t.column :name, :string
+    t.column :type, :string
+    if self.connection.adapter_name == 'PostgreSQL' && self.connection.execute("SHOW SERVER_VERSION").first["server_version"].to_f >= 9.2
+      type = :json
+    else
+      type = :string
+    end
+    t.column :opts, type
+  end
+end

spec/spec_helper.rb

@@ -83,7 +83,7 @@ def logger_off
 def clean_database!
   models = [ActsAsTaggableOn::Tag, ActsAsTaggableOn::Tagging, TaggableModel, OtherTaggableModel,
             InheritingTaggableModel, AlteredInheritingTaggableModel, User, UntaggableModel,
-            OrderedTaggableModel]
+            OrderedTaggableModel, TaggableModelWithJson]
   models.each do |model|
     #Sqlite don't support truncate
     if ActsAsTaggableOn::Utils.using_sqlite?