From 9e300598ee86d318d5312478d398f2ebdd1db2c0 Mon Sep 17 00:00:00 2001
From: Jordan Brough <jordan@animoto.com>
Date: Tue, 9 Feb 2010 13:13:01 -0700
Subject: [PATCH 1/2] Friendly ActiveRecord error handling for db-enforced unique constraints

---
 activerecord/lib/active_record.rb                  |    1 +
 activerecord/lib/active_record/base.rb             |    1 +
 .../connection_adapters/abstract_adapter.rb        |    7 +++
 .../connection_adapters/mysql2_adapter.rb          |   12 ++++++
 .../connection_adapters/mysql_adapter.rb           |   12 ++++++
 .../connection_adapters/postgresql_adapter.rb      |   13 ++++++
 .../connection_adapters/sqlite_adapter.rb          |    7 +++
 activerecord/lib/active_record/locale/en.yml       |    2 +
 .../lib/active_record/unique_constraints.rb        |   29 ++++++++++++++
 activerecord/test/cases/adapter_test.rb            |   35 +++++++++++++++++
 activerecord/test/cases/unique_constraints_test.rb |   40 ++++++++++++++++++++
 activerecord/test/models/unique_item.rb            |    2 +
 activerecord/test/schema/schema.rb                 |   15 +++++++
 13 files changed, 176 insertions(+), 0 deletions(-)
 create mode 100644 activerecord/lib/active_record/unique_constraints.rb
 create mode 100644 activerecord/test/cases/unique_constraints_test.rb
 create mode 100644 activerecord/test/models/unique_item.rb

diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb
index 5afb978..869c90b 100644
--- a/activerecord/lib/active_record.rb
+++ b/activerecord/lib/active_record.rb
@@ -78,6 +78,7 @@ module ActiveRecord
     autoload :SessionStore
     autoload :Timestamp
     autoload :Transactions
+    autoload :UniqueConstraints
     autoload :Validations
   end
 
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index effb17b..9909663 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1940,6 +1940,7 @@ MSG
     # #save_with_autosave_associations to be wrapped inside a transaction.
     include AutosaveAssociation, NestedAttributes
     include Aggregations, Transactions, Reflection, Serialization
+    include UniqueConstraints
 
     NilClass.add_whiner(self) if NilClass.respond_to?(:add_whiner)
 
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index 3a3a73f..43ffa46 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -207,6 +207,13 @@ module ActiveRecord
         "active_record_#{open_transactions}"
       end
 
+      # given an ActiveRecord::RecordNotUnique exception and a table name this returns the index whose
+      # unique constraint was violated, or nil if it can't determine the index
+      def index_for_record_not_unique(not_unique_exception, table_name) #:nodoc:
+        # override in derived class
+        nil
+      end
+
       protected
 
         def log(sql, name = "SQL")
diff --git a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
index acf1832..47da0da 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
@@ -536,6 +536,18 @@ module ActiveRecord
         where_sql
       end
 
+      def index_for_record_not_unique(exception, table_name) #:nodoc:
+        case exception.message
+        when /Duplicate entry.*for key (\d+)/
+          index_position = $1.to_i
+          # minus two b/c mysql message is one-based + rails excludes primary key index from indexes list
+          indexes(table_name)[index_position - 2]
+        when /Duplicate entry.*for key '(\w+)'/
+          index_name = $1
+          indexes(table_name).detect { |i| i.name == index_name }
+        end
+      end
+
       protected
         def quoted_columns_for_index(column_names, options = {})
           length = options[:length] if options.is_a?(Hash)
diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb
index cdf1ebf..7c89aef 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb
@@ -646,6 +646,18 @@ module ActiveRecord
         where_sql
       end
 
+      def index_for_record_not_unique(exception, table_name) #:nodoc:
+        case exception.message
+        when /Duplicate entry.*for key (\d+)/
+          index_position = $1.to_i
+          # minus two b/c mysql message is one-based + rails excludes primary key index from indexes list
+          indexes(table_name)[index_position - 2]
+        when /Duplicate entry.*for key '(\w+)'/
+          index_name = $1
+          indexes(table_name).detect { |i| i.name == index_name }
+        end
+      end
+
       protected
         def quoted_columns_for_index(column_names, options = {})
           length = options[:length] if options.is_a?(Hash)
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index 46c0f3f..bea174b 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -708,6 +708,12 @@ module ActiveRecord
         end.compact
       end
 
+      # postgres doesn't let us query this in the middle of an aborted transaction so we sometimes need to cache beforehand
+      def cached_indexes(table_name) #:nodoc:
+        @cached_indexes ||= {}
+        @cached_indexes[table_name] ||= indexes(table_name)
+      end
+
       # Returns the list of all column definitions for a table.
       def columns(table_name, name = nil)
         # Limit, precision, and scale are all handled by the superclass.
@@ -944,6 +950,13 @@ module ActiveRecord
         sql << order_columns * ', '
       end
 
+      def index_for_record_not_unique(exception, table_name) #:nodoc:
+        if match = exception.message.match(/unique constraint "(\w+)"/)
+          index_name = match[1]
+          cached_indexes(table_name).detect { |i| i.name == index_name }
+        end
+      end
+
       protected
         # Returns the version of the connected PostgreSQL version.
         def postgresql_version
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
index f650a1b..02eebd8 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
@@ -340,6 +340,13 @@ module ActiveRecord
         "VALUES(NULL)"
       end
 
+      def index_for_record_not_unique(exception, table_name) #:nodoc:
+        if match = exception.message.match(/column(?:s)? (.*) (?:is|are) not unique/)
+          index_columns = match[1].split(', ')
+          indexes(table_name).detect { |i| i.columns == index_columns }
+        end
+      end
+
       protected
         def select(sql, name = nil, binds = []) #:nodoc:
           exec_query(sql, name, binds).to_a
diff --git a/activerecord/lib/active_record/locale/en.yml b/activerecord/lib/active_record/locale/en.yml
index 44328f6..e310fba 100644
--- a/activerecord/lib/active_record/locale/en.yml
+++ b/activerecord/lib/active_record/locale/en.yml
@@ -9,6 +9,8 @@ en:
     errors:
       messages:
         taken: "has already been taken"
+        taken_multiple: "has already been taken for %{context}"
+        taken_generic: "Unique requirement not met"
         record_invalid: "Validation failed: %{errors}"
         # Append your own errors here or at the model/attributes scope.
 
diff --git a/activerecord/lib/active_record/unique_constraints.rb b/activerecord/lib/active_record/unique_constraints.rb
new file mode 100644
index 0000000..4a6d748
--- /dev/null
+++ b/activerecord/lib/active_record/unique_constraints.rb
@@ -0,0 +1,29 @@
+module ActiveRecord
+  module UniqueConstraints
+    extend ActiveSupport::Concern
+
+    included do
+      alias_method_chain :save, :unique_constraints
+    end
+
+    # work around postgres not allowing us to query indexes in the middle of an aborted transaction
+    def cache_indexes #:nodoc:
+      connection.cached_indexes(self.class.table_name) if connection.respond_to?(:cached_indexes)
+    end
+
+    def save_with_unique_constraints(*args) #:nodoc:
+      cache_indexes
+      save_without_unique_constraints(*args)
+    rescue ActiveRecord::RecordNotUnique => e
+      index = connection.index_for_record_not_unique(e, self.class.table_name)
+      if !index
+        errors[:base] << I18n.t(:'activerecord.errors.messages.taken_generic')
+      elsif index.columns.size == 1
+        errors.add(index.columns.first, :taken, :value => attributes[index.columns.first])
+      else
+        errors.add(index.columns.first, :taken_multiple, :context => index.columns.slice(1..-1).join('/'), :value => attributes[index.columns.first])
+      end
+      false
+    end
+  end
+end
diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb
index 49b2e94..1cb8b0a 100644
--- a/activerecord/test/cases/adapter_test.rb
+++ b/activerecord/test/cases/adapter_test.rb
@@ -141,4 +141,39 @@ class AdapterTest < ActiveRecord::TestCase
       end
     end
   end
+
+  # tests for db-enforced unique constraints
+  unique_scenarios = {
+    :single         => {:sql => "INSERT INTO unique_items(uniq) VALUES('abc')",
+                        :expected => ['uniq']},
+    :single_named   => {:sql => "INSERT INTO unique_items(uniq_named) VALUES('abc')",
+                        :expected => ['uniq_named']},
+    :multiple       => {:sql => "INSERT INTO unique_items(uniq_1, uniq_2, uniq_3) VALUES('a', 'b', 'c')",
+                        :expected => ['uniq_1', 'uniq_2', 'uniq_3']},
+    :multiple_named => {:sql => "INSERT INTO unique_items(uniq_1_named, uniq_2_named, uniq_3_named) VALUES('a', 'b', 'c')",
+                        :expected => ['uniq_1_named', 'uniq_2_named', 'uniq_3_named']}
+  }
+  unique_scenarios.each do |name, data|
+    test "finds index for unique constraint #{name}" do
+      @connection.execute data[:sql]
+      exception = get_exception(ActiveRecord::RecordNotUnique) do
+        @connection.execute data[:sql]
+      end
+      # postgres can't query indexes until transaction is rolled back
+      @connection.rollback_db_transaction if current_adapter?(:PostgreSQLAdapter)
+      assert_equal data[:expected], @connection.index_for_record_not_unique(exception, 'unique_items').columns
+    end
+  end
+
+  protected
+
+  def get_exception(expected_exception)
+    begin
+      yield
+    rescue expected_exception => e
+      e
+    else
+      flunk "Expected #{expected_exception} exception"
+    end
+  end
 end
diff --git a/activerecord/test/cases/unique_constraints_test.rb b/activerecord/test/cases/unique_constraints_test.rb
new file mode 100644
index 0000000..18d1890
--- /dev/null
+++ b/activerecord/test/cases/unique_constraints_test.rb
@@ -0,0 +1,40 @@
+require "cases/helper"
+require 'models/unique_item'
+
+class UniquenessValidationTest < ActiveRecord::TestCase
+
+  def test_validate_uniqueness_on_create
+    valid_item = UniqueItem.create!(:uniq => 'abc')
+    invalid_item = UniqueItem.create(:uniq => 'abc')
+    assert_equal ["has already been taken"], invalid_item.errors[:uniq]
+  end
+
+  def test_validate_uniqueness_on_save
+    item_1 = UniqueItem.create!(:uniq => 'abc')
+    item_2 = UniqueItem.create!(:uniq => 'xyz')
+    item_2.uniq = item_1.uniq
+    item_2.save
+    assert_equal ["has already been taken"], item_2.errors[:uniq]
+  end
+
+  def test_validate_uniqueness_on_update
+    item_1 = UniqueItem.create!(:uniq => 'abc')
+    item_2 = UniqueItem.create!(:uniq => 'xyz')
+    item_2.update_attributes(:uniq => item_1.uniq)
+    assert_equal ["has already been taken"], item_2.errors[:uniq]
+  end
+
+  def test_validate_uniqueness_multiple
+    valid_item = UniqueItem.create!(:uniq_1 => 'a', :uniq_2 => 'b', :uniq_3 => 'c')
+    invalid_item = UniqueItem.create(:uniq_1 => 'a', :uniq_2 => 'b', :uniq_3 => 'c')
+    assert_equal ["has already been taken for uniq_2/uniq_3"], invalid_item.errors[:uniq_1]
+  end
+
+  def test_validate_uniqueness_generic
+    ActiveRecord::Base.connection.expects(:index_for_record_not_unique).returns(nil)
+    valid_item = UniqueItem.create!(:uniq_1 => 'a', :uniq_2 => 'b', :uniq_3 => 'c')
+    invalid_item = UniqueItem.create(:uniq_1 => 'a', :uniq_2 => 'b', :uniq_3 => 'c')
+    assert_equal ["Unique requirement not met"], invalid_item.errors[:base]
+  end
+
+end
diff --git a/activerecord/test/models/unique_item.rb b/activerecord/test/models/unique_item.rb
new file mode 100644
index 0000000..e805cb8
--- /dev/null
+++ b/activerecord/test/models/unique_item.rb
@@ -0,0 +1,2 @@
+class UniqueItem < ActiveRecord::Base
+end
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 326c336..d408154 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -668,6 +668,21 @@ ActiveRecord::Schema.define do
     t.string :name
   end
 
+  create_table :unique_items, :force => true do |t|
+    t.string :uniq
+    t.string :uniq_1
+    t.string :uniq_2
+    t.string :uniq_3
+    t.string :uniq_named
+    t.string :uniq_1_named
+    t.string :uniq_2_named
+    t.string :uniq_3_named
+  end
+  add_index :unique_items, [:uniq], :unique => true
+  add_index :unique_items, [:uniq_named], :unique => true, :name => 'unique_named_single_index'
+  add_index :unique_items, [:uniq_1, :uniq_2, :uniq_3], :unique => true
+  add_index :unique_items, [:uniq_1_named, :uniq_2_named, :uniq_3_named], :unique => true, :name => 'unique_named_multiple_index'
+
   except 'SQLite' do
     # fk_test_has_fk should be before fk_test_has_pk
     create_table :fk_test_has_fk, :force => true do |t|
-- 
1.7.2.1


From fb06e862fc2cb5d48717d6af948466fb40cb002d Mon Sep 17 00:00:00 2001
From: Jordan Brough <jordan@animoto.com>
Date: Tue, 9 Feb 2010 13:50:35 -0700
Subject: [PATCH 2/2] Handling for db-enforced unique constraints in bang methods

---
 .../lib/active_record/unique_constraints.rb        |   33 ++++++++++++++-----
 activerecord/test/cases/unique_constraints_test.rb |   30 ++++++++++++++++++
 2 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/activerecord/lib/active_record/unique_constraints.rb b/activerecord/lib/active_record/unique_constraints.rb
index 4a6d748..cb7a665 100644
--- a/activerecord/lib/active_record/unique_constraints.rb
+++ b/activerecord/lib/active_record/unique_constraints.rb
@@ -3,7 +3,9 @@ module ActiveRecord
     extend ActiveSupport::Concern
 
     included do
-      alias_method_chain :save, :unique_constraints
+      [:save, :save!].each do |method|
+        alias_method_chain method, :unique_constraints
+      end
     end
 
     # work around postgres not allowing us to query indexes in the middle of an aborted transaction
@@ -15,15 +17,28 @@ module ActiveRecord
       cache_indexes
       save_without_unique_constraints(*args)
     rescue ActiveRecord::RecordNotUnique => e
-      index = connection.index_for_record_not_unique(e, self.class.table_name)
-      if !index
-        errors[:base] << I18n.t(:'activerecord.errors.messages.taken_generic')
-      elsif index.columns.size == 1
-        errors.add(index.columns.first, :taken, :value => attributes[index.columns.first])
-      else
-        errors.add(index.columns.first, :taken_multiple, :context => index.columns.slice(1..-1).join('/'), :value => attributes[index.columns.first])
-      end
+      parse_unique_exception(e)
       false
     end
+
+    def save_with_unique_constraints! #:nodoc:
+      cache_indexes
+      save_without_unique_constraints!
+    rescue ActiveRecord::RecordNotUnique => e
+      parse_unique_exception(e)
+      raise RecordInvalid.new(self)
+    end
+
+    protected
+      def parse_unique_exception(e)
+        index = connection.index_for_record_not_unique(e, self.class.table_name)
+        if !index
+          errors[:base] << I18n.translate(:'activerecord.errors.messages.taken_generic')
+        elsif index.columns.size == 1
+          errors.add(index.columns.first, :taken, :value => attributes[index.columns.first])
+        else
+          errors.add(index.columns.first, :taken_multiple, :context => index.columns.slice(1..-1).join('/'), :value => attributes[index.columns.first])
+        end
+      end
   end
 end
diff --git a/activerecord/test/cases/unique_constraints_test.rb b/activerecord/test/cases/unique_constraints_test.rb
index 18d1890..d4489fc 100644
--- a/activerecord/test/cases/unique_constraints_test.rb
+++ b/activerecord/test/cases/unique_constraints_test.rb
@@ -9,6 +9,12 @@ class UniquenessValidationTest < ActiveRecord::TestCase
     assert_equal ["has already been taken"], invalid_item.errors[:uniq]
   end
 
+  def test_validate_uniqueness_on_create!
+    valid_item = UniqueItem.create!(:uniq => 'abc')
+    e = get_unique_exception { UniqueItem.create!(:uniq => 'abc') }
+    assert_equal ["has already been taken"], e.record.errors[:uniq]
+  end
+
   def test_validate_uniqueness_on_save
     item_1 = UniqueItem.create!(:uniq => 'abc')
     item_2 = UniqueItem.create!(:uniq => 'xyz')
@@ -17,6 +23,14 @@ class UniquenessValidationTest < ActiveRecord::TestCase
     assert_equal ["has already been taken"], item_2.errors[:uniq]
   end
 
+  def test_validate_uniqueness_on_save!
+    item_1 = UniqueItem.create!(:uniq => 'abc')
+    item_2 = UniqueItem.create!(:uniq => 'xyz')
+    item_2.uniq = item_1.uniq
+    e = get_unique_exception { item_2.save! }
+    assert_equal(["has already been taken"], e.record.errors[:uniq])
+  end
+
   def test_validate_uniqueness_on_update
     item_1 = UniqueItem.create!(:uniq => 'abc')
     item_2 = UniqueItem.create!(:uniq => 'xyz')
@@ -24,6 +38,13 @@ class UniquenessValidationTest < ActiveRecord::TestCase
     assert_equal ["has already been taken"], item_2.errors[:uniq]
   end
 
+  def test_validate_uniqueness_on_update!
+    item_1 = UniqueItem.create!(:uniq => 'abc')
+    item_2 = UniqueItem.create!(:uniq => 'xyz')
+    e = get_unique_exception { item_2.update_attributes!(:uniq => item_1.uniq) }
+    assert_equal(["has already been taken"], e.record.errors[:uniq])
+  end
+
   def test_validate_uniqueness_multiple
     valid_item = UniqueItem.create!(:uniq_1 => 'a', :uniq_2 => 'b', :uniq_3 => 'c')
     invalid_item = UniqueItem.create(:uniq_1 => 'a', :uniq_2 => 'b', :uniq_3 => 'c')
@@ -37,4 +58,13 @@ class UniquenessValidationTest < ActiveRecord::TestCase
     assert_equal ["Unique requirement not met"], invalid_item.errors[:base]
   end
 
+  protected
+    def get_unique_exception
+      yield
+    rescue ActiveRecord::RecordInvalid => e
+      e
+    else
+      flunk "Expected ActiveRecord::RecordInvalid exception"
+    end
+
 end
-- 
1.7.2.1