From d5867a01a82d14216541c8bfc38e466b02580376 Mon Sep 17 00:00:00 2001 From: Caleb Thompson Date: Sat, 13 Apr 2013 00:11:04 -0400 Subject: Fix freeze applying to cloned objects Previously, freezing a cloned ActiveRecord object froze the original too. By cloning `@attributes` before freezing, we prevent cloned objects (which in Ruby share state of ivars) from being effected by `#freeze`. Resolves issue #4936, which has further information on this issue, as well as steps to reproduce. * Add a test case for `#freeze` not causing `cloned.frozen?` to be true. * Clone @attributes before freezing in `ActiveRecord::Core`, then reassign the cloned, frozen hash to the frozen model's `@attributes` ivar. /cc @steveklabnik --- activerecord/lib/active_record/core.rb | 6 ++++-- activerecord/test/cases/clone_test.rb | 7 +++++++ 2 files changed, 11 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index 733d4e1c67..9e45e6e474 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -307,9 +307,11 @@ module ActiveRecord id.hash end - # Freeze the attributes hash such that associations are still accessible, even on destroyed records. + # Clone and freeze the attributes hash such that associations are still + # accessible, even on destroyed records, but cloned models will not be + # frozen. def freeze - @attributes.freeze + @attributes = @attributes.clone.freeze self end diff --git a/activerecord/test/cases/clone_test.rb b/activerecord/test/cases/clone_test.rb index d91646efca..5e43082c33 100644 --- a/activerecord/test/cases/clone_test.rb +++ b/activerecord/test/cases/clone_test.rb @@ -29,5 +29,12 @@ module ActiveRecord topic.author_name = 'Aaron' assert_equal 'Aaron', cloned.author_name end + + def test_freezing_a_cloned_model_does_not_freeze_clone + cloned = Topic.new + clone = cloned.clone + cloned.freeze + assert_not clone.frozen? + end end end -- cgit v1.2.3 From 2976558bc3a02019b605a1d70fda76ba5e9b5df2 Mon Sep 17 00:00:00 2001 From: bondarev Date: Mon, 25 Feb 2013 19:05:22 +0400 Subject: Support transactions in Migrator.run --- activerecord/CHANGELOG.md | 4 ++++ activerecord/lib/active_record/migration.rb | 17 ++++++++++++----- activerecord/test/cases/migration_test.rb | 26 ++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 062c548f20..79f9232050 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,9 @@ ## Rails 4.0.0 (unreleased) ## +* Run migrate:down & migrate:up in transaction if database supports + + *Alexander Bondarev* + * Added Statement Cache to allow the caching of a single statement. The cache works by duping the relation returned from yielding a statement, which allows skipping the AST building phase for following executes. The cache returns results in array format. diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 451104106c..d2f10cf2ac 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -867,11 +867,18 @@ module ActiveRecord alias :current :current_migration def run - target = migrations.detect { |m| m.version == @target_version } - raise UnknownMigrationVersionError.new(@target_version) if target.nil? - unless (up? && migrated.include?(target.version.to_i)) || (down? && !migrated.include?(target.version.to_i)) - target.migrate(@direction) - record_version_state_after_migrating(target.version) + migration = migrations.detect { |m| m.version == @target_version } + raise UnknownMigrationVersionError.new(@target_version) if migration.nil? + unless (up? && migrated.include?(migration.version.to_i)) || (down? && !migrated.include?(migration.version.to_i)) + begin + ddl_transaction(migration) do + migration.migrate(@direction) + record_version_state_after_migrating(migration.version) + end + rescue => e + canceled_msg = use_transaction?(migration) ? ", the migration canceled" : "" + raise StandardError, "An error has occurred#{canceled_msg}:\n\n#{e}", e.backtrace + end end end diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index f8afb7c591..b26ffa0e79 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -258,6 +258,32 @@ class MigrationTest < ActiveRecord::TestCase "On error, the Migrator should revert schema changes but it did not." end + def test_migrator_one_up_with_exception_and_rollback_using_run + unless ActiveRecord::Base.connection.supports_ddl_transactions? + skip "not supported on #{ActiveRecord::Base.connection.class}" + end + + assert_not Person.column_methods_hash.include?(:last_name) + + migration = Class.new(ActiveRecord::Migration) { + def version; 100 end + def migrate(x) + add_column "people", "last_name", :string + raise 'Something broke' + end + }.new + + migrator = ActiveRecord::Migrator.new(:up, [migration], 100) + + e = assert_raise(StandardError) { migrator.run } + + assert_equal "An error has occurred, the migration canceled:\n\nSomething broke", e.message + + Person.reset_column_information + assert_not Person.column_methods_hash.include?(:last_name), + "On error, the Migrator should revert schema changes but it did not." + end + def test_migration_without_transaction unless ActiveRecord::Base.connection.supports_ddl_transactions? skip "not supported on #{ActiveRecord::Base.connection.class}" -- cgit v1.2.3 From 0e34a7efa6992e6771d34dc8163b79dd7df52667 Mon Sep 17 00:00:00 2001 From: Chris Constantine Date: Thu, 18 Apr 2013 12:24:14 -0700 Subject: Fix loading of fixtures when the column type is a postgres array of strings. - A string in an array of strings that has a quote char (') needs to have that quote char escaped if the array is getting wrapped in quote chars. --- .../lib/active_record/connection_adapters/postgresql/quoting.rb | 2 +- activerecord/test/cases/adapters/postgresql/array_test.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb index 6329733abc..40a3b82839 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb @@ -32,7 +32,7 @@ module ActiveRecord when 'point' then super(PostgreSQLColumn.point_to_string(value)) else if column.array - "'#{PostgreSQLColumn.array_to_string(value, column, self)}'" + "'#{PostgreSQLColumn.array_to_string(value, column, self).gsub(/'/, "''")}'" else super end diff --git a/activerecord/test/cases/adapters/postgresql/array_test.rb b/activerecord/test/cases/adapters/postgresql/array_test.rb index 8774bf626f..61a3a2ba0f 100644 --- a/activerecord/test/cases/adapters/postgresql/array_test.rb +++ b/activerecord/test/cases/adapters/postgresql/array_test.rb @@ -81,6 +81,12 @@ class PostgresqlArrayTest < ActiveRecord::TestCase assert_cycle(['1',nil,nil]) end + def test_insert_fixture + tag_values = ["val1", "val2", "val3_with_'_multiple_quote_'_chars"] + @connection.insert_fixture({"tags" => tag_values}, "pg_arrays" ) + assert_equal(PgArray.last.tags, tag_values) + end + private def assert_cycle array # test creation -- cgit v1.2.3 From 1d9309a5b2391b9f839dd23a213031267d03a281 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Fri, 19 Apr 2013 11:22:32 -0300 Subject: Improve the error message --- activerecord/lib/active_record/migration.rb | 2 +- activerecord/test/cases/migration_test.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index d2f10cf2ac..2d2ca4c8e8 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -876,7 +876,7 @@ module ActiveRecord record_version_state_after_migrating(migration.version) end rescue => e - canceled_msg = use_transaction?(migration) ? ", the migration canceled" : "" + canceled_msg = use_transaction?(migration) ? ", this migration was canceled" : "" raise StandardError, "An error has occurred#{canceled_msg}:\n\n#{e}", e.backtrace end end diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index b26ffa0e79..193ffb26e3 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -277,7 +277,7 @@ class MigrationTest < ActiveRecord::TestCase e = assert_raise(StandardError) { migrator.run } - assert_equal "An error has occurred, the migration canceled:\n\nSomething broke", e.message + assert_equal "An error has occurred, this migration was canceled:\n\nSomething broke", e.message Person.reset_column_information assert_not Person.column_methods_hash.include?(:last_name), -- cgit v1.2.3 From 9de28419b1e6312e911eea0c315f326f498b195c Mon Sep 17 00:00:00 2001 From: Johnny Holton Date: Wed, 10 Apr 2013 23:29:19 -0400 Subject: destroys association records before saving/inserting new association records fixes bug introduced by #3329 These are the conditions necessary to reproduce the bug: - For an association, autosave => true. - An association record is being destroyed - A new association record is being created. - There is a unique index one of the association's fields. - The record being created has the same value as the record being destroyed on the indexed field. Before, the deletion of records was postponed until after all insertions/saves. Therefore the new record with the identical value in the indexed field caused a non-unique value error to be thrown at the database level. With this fix, the deletions happen first, before the insertions/saves. Therefore the record with the duplicate value is gone from the database before the new record is created, thereby avoiding the non-uniuqe value error. --- activerecord/CHANGELOG.md | 7 +++++++ activerecord/lib/active_record/autosave_association.rb | 17 ++++++++--------- activerecord/test/cases/autosave_association_test.rb | 14 ++++++++++++++ 3 files changed, 29 insertions(+), 9 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 278da322f2..9e5a9a15c3 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,12 @@ ## Rails 4.0.0 (unreleased) ## +* fixes bug introduced by #3329. Now, when autosaving associations, + deletions happen before inserts and saves. This prevents a 'duplicate + unique value' database error that would occur if a record being created had + the same value on a unique indexed field as that of a record being destroyed. + + *Johnny Holton* + * Run `rake migrate:down` & `rake migrate:up` in transaction if database supports. *Alexander Bondarev* diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 44323ce9db..a98cacc78c 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -334,15 +334,18 @@ module ActiveRecord autosave = reflection.options[:autosave] if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave) - records_to_destroy = [] + + if autosave + records_to_destroy = records.select(&:marked_for_destruction?) + records_to_destroy.each { |record| association.destroy(record) } + records -= records_to_destroy + end + records.each do |record| - next if record.destroyed? saved = true - if autosave && record.marked_for_destruction? - records_to_destroy << record - elsif autosave != false && (@new_record_before_save || record.new_record?) + if autosave != false && (@new_record_before_save || record.new_record?) if autosave saved = association.insert_record(record, false) else @@ -354,10 +357,6 @@ module ActiveRecord raise ActiveRecord::Rollback unless saved end - - records_to_destroy.each do |record| - association.destroy(record) - end end # reconstruct the scope now that we know the owner's id diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 536ff4882c..552bbd3f52 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -764,6 +764,20 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase assert_equal 2, @pirate.birds.reload.length end + def test_should_save_new_record_that_has_same_value_as_existing_record_marked_for_destruction_on_field_that_has_unique_index + Bird.connection.add_index :birds, :name, unique: true + + 3.times { |i| @pirate.birds.create(name: "birds_#{i}") } + + @pirate.birds[0].mark_for_destruction + @pirate.birds.build(name: @pirate.birds[0].name) + @pirate.save! + + assert_equal 3, @pirate.birds.reload.length + ensure + Bird.connection.remove_index :birds, column: :name + end + # Add and remove callbacks tests for association collections. %w{ method proc }.each do |callback_type| define_method("test_should_run_add_callback_#{callback_type}s_for_has_many") do -- cgit v1.2.3 From 0920d4fccbfc41b6ccdae7070758fc2133280409 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 19 Apr 2013 16:57:06 +0100 Subject: Revert "Merge pull request #10183 from jholton/fix_association_auto_save" This reverts commit e8727d37fc49d5bf9976c3cb5c46badb92cf4ced, reversing changes made to d098e1c24bc145e0cc14532348436e14dc46d375. Reason: it broke the mysql build --- activerecord/CHANGELOG.md | 7 ------- activerecord/lib/active_record/autosave_association.rb | 17 +++++++++-------- activerecord/test/cases/autosave_association_test.rb | 14 -------------- 3 files changed, 9 insertions(+), 29 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 9e5a9a15c3..278da322f2 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,12 +1,5 @@ ## Rails 4.0.0 (unreleased) ## -* fixes bug introduced by #3329. Now, when autosaving associations, - deletions happen before inserts and saves. This prevents a 'duplicate - unique value' database error that would occur if a record being created had - the same value on a unique indexed field as that of a record being destroyed. - - *Johnny Holton* - * Run `rake migrate:down` & `rake migrate:up` in transaction if database supports. *Alexander Bondarev* diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index a98cacc78c..44323ce9db 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -334,18 +334,15 @@ module ActiveRecord autosave = reflection.options[:autosave] if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave) - - if autosave - records_to_destroy = records.select(&:marked_for_destruction?) - records_to_destroy.each { |record| association.destroy(record) } - records -= records_to_destroy - end - + records_to_destroy = [] records.each do |record| + next if record.destroyed? saved = true - if autosave != false && (@new_record_before_save || record.new_record?) + if autosave && record.marked_for_destruction? + records_to_destroy << record + elsif autosave != false && (@new_record_before_save || record.new_record?) if autosave saved = association.insert_record(record, false) else @@ -357,6 +354,10 @@ module ActiveRecord raise ActiveRecord::Rollback unless saved end + + records_to_destroy.each do |record| + association.destroy(record) + end end # reconstruct the scope now that we know the owner's id diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 552bbd3f52..536ff4882c 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -764,20 +764,6 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase assert_equal 2, @pirate.birds.reload.length end - def test_should_save_new_record_that_has_same_value_as_existing_record_marked_for_destruction_on_field_that_has_unique_index - Bird.connection.add_index :birds, :name, unique: true - - 3.times { |i| @pirate.birds.create(name: "birds_#{i}") } - - @pirate.birds[0].mark_for_destruction - @pirate.birds.build(name: @pirate.birds[0].name) - @pirate.save! - - assert_equal 3, @pirate.birds.reload.length - ensure - Bird.connection.remove_index :birds, column: :name - end - # Add and remove callbacks tests for association collections. %w{ method proc }.each do |callback_type| define_method("test_should_run_add_callback_#{callback_type}s_for_has_many") do -- cgit v1.2.3