fix bug #506 - .uniq breaks with pg and json columns

fix bug #506 - .uniq breaks with pg and json columns

updated specs for 'select distinct' and pg issues

* new model/table with a json column for postgresql specific testing
* add tests for explicit distinct in a select and for all queries to
  test if tagged_with adds a distinct clause on the query.
  http://github.com/mbleigh/acts-as-taggable-on/issues/357
* add tests for taggable_model_with_json
* exclude test should sort the array result and expected array.
  PostgreSQL won't always return the two expected results in
  the expected order so the test failed intermittently.

use group vs distinct when options[:any] is true

update CHANGELOG

check pg version and only use json if >=9.2

move change to master section

move table with json to spec/internal/db/schema.rb

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?