From efdff8a1877aa74bb513a4df95a8eaddb4bf9edd Mon Sep 17 00:00:00 2001 From: Jordan Brough 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 | 4 ++ .../connection_adapters/abstract_adapter.rb | 7 +++ .../connection_adapters/mysql_adapter.rb | 12 ++++++ .../connection_adapters/postgresql_adapter.rb | 7 +++ .../connection_adapters/sqlite_adapter.rb | 7 +++ activerecord/lib/active_record/locale/en.yml | 2 + .../lib/active_record/unique_constraints.rb | 23 +++++++++++ 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 +++++++ 12 files changed, 155 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 e2f2508..63fb95c 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 650a91b..50cb7e2 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1918,6 +1918,10 @@ module ActiveRecord #:nodoc: include AutosaveAssociation, NestedAttributes include Aggregations, Transactions, Reflection, Serialization + # UniqueConstraints needs to be included after Transactions, because we need #save_with_unique_constraints + # to rescue after the transaction has been resolved for postgres. + include UniqueConstraints + NilClass.add_whiner(self) if NilClass.respond_to?(:add_whiner) end end diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 28a59c1..7ad86bf 100755 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -192,6 +192,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) diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index 470ff8d..3075cae 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -594,6 +594,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 6389094..6234210 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -857,6 +857,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] + 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 29225b8..5b3451a 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb @@ -288,6 +288,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) #:nodoc: execute(sql, name).map do |row| diff --git a/activerecord/lib/active_record/locale/en.yml b/activerecord/lib/active_record/locale/en.yml index 9d5cb54..7f54e7d 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..a094d8d --- /dev/null +++ b/activerecord/lib/active_record/unique_constraints.rb @@ -0,0 +1,23 @@ +module ActiveRecord + module UniqueConstraints + extend ActiveSupport::Concern + + included do + alias_method_chain :save, :unique_constraints + end + + def save_with_unique_constraints(*args) #:nodoc: + 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 9b28766..468f165 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -163,4 +163,39 @@ class AdapterTest < ActiveRecord::TestCase assert_equal " LIMIT 1,7 OFFSET 7", @connection.add_limit_offset!("", :limit => sql_inject, :offset => 7) 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 f5fba2f..4c1b8e2 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -569,6 +569,21 @@ ActiveRecord::Schema.define do t.string :title 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.0.3 From d76d9179e6a78ae9b5087c889e0f8e59967d6589 Mon Sep 17 00:00:00 2001 From: Jordan Brough 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 | 32 ++++++++++++++----- activerecord/test/cases/unique_constraints_test.rb | 30 ++++++++++++++++++ 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/activerecord/lib/active_record/unique_constraints.rb b/activerecord/lib/active_record/unique_constraints.rb index a094d8d..b8ce138 100644 --- a/activerecord/lib/active_record/unique_constraints.rb +++ b/activerecord/lib/active_record/unique_constraints.rb @@ -3,21 +3,35 @@ 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 def save_with_unique_constraints(*args) #:nodoc: 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: + 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.0.3