diff options
Diffstat (limited to 'activerecord')
24 files changed, 215 insertions, 33 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 3fa24f3837..fb64156b78 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,56 @@ +* ActiveRecord::RecordNotFound modified to store model name, primary_key and + id of the caller model. It allows the catcher of this exception to make + a better decision to what to do with it. For example consider this simple + example: + + class SomeAbstractController < ActionController::Base + rescue_from ActiveRecord::RecordNotFound, with: :redirect_to_404 + + private def redirect_to_404(e) + return redirect_to(posts_url) if e.model == 'Post' + raise + end + end + + *Sameer Rahmani* + +* Deprecate the keys for association `restrict_dependent_destroy` errors in favor + of new key names. + + Previously `has_one` and `has_many` associations were using the + `one` and `many` keys respectively. Both of these keys have special + meaning in I18n (they are considered to be pluralizations) so by + renaming them to `has_one` and `has_many` we make the messages more explicit + and most importantly they don't clash with linguistical systems that need to + validate translation keys (and their pluralizations). + + The `:'restrict_dependent_destroy.one'` key should be replaced with + `:'restrict_dependent_destroy.has_one'`, and `:'restrict_dependent_destroy.many'` + with `:'restrict_dependent_destroy.has_many'`. + + *Roque Pinel*, *Christopher Dell* + +* Fix state being carried over from previous transaction. + + Considering the following example where `name` is a required attribute. + Before we had `new_record?` returning `true` for a persisted record: + + author = Author.create! name: 'foo' + author.name = nil + author.save # => false + author.new_record? # => true + + Fixes #20824. + + *Roque Pinel* + +* Correctly ignore `mark_for_destruction` when `autosave` isn't set to `true` + when validating associations. + + Fixes #20882. + + *Sean Griffin* + * Fix a bug where counter_cache doesn't always work with polymorphic relations. diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb index 19e9ffcb3c..b5a8c81fe4 100644 --- a/activerecord/lib/active_record/associations/collection_proxy.rb +++ b/activerecord/lib/active_record/associations/collection_proxy.rb @@ -971,7 +971,7 @@ module ActiveRecord alias_method :append, :<< def prepend(*args) - raise NoMethodError, "prepend on association is not defined. Please use << or append" + raise NoMethodError, "prepend on association is not defined. Please use <<, push or append" end # Equivalent to +delete_all+. The difference is that returns +self+, instead diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index ca27c9fdde..9f6c832c1b 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -16,7 +16,14 @@ module ActiveRecord when :restrict_with_error unless empty? record = klass.human_attribute_name(reflection.name).downcase - owner.errors.add(:base, :"restrict_dependent_destroy.many", record: record) + message = owner.errors.generate_message(:base, :'restrict_dependent_destroy.many', record: record, raise: true) rescue nil + if message + ActiveSupport::Deprecation.warn(<<-MESSAGE.squish) + The error key `:'restrict_dependent_destroy.many'` has been deprecated and will be removed in Rails 5.1. + Please use `:'restrict_dependent_destroy.has_many'` instead. + MESSAGE + end + owner.errors.add(:base, message || :'restrict_dependent_destroy.has_many', record: record) throw(:abort) end diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index 41a75b820e..5a92bc5e8a 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -12,7 +12,14 @@ module ActiveRecord when :restrict_with_error if load_target record = klass.human_attribute_name(reflection.name).downcase - owner.errors.add(:base, :"restrict_dependent_destroy.one", record: record) + message = owner.errors.generate_message(:base, :'restrict_dependent_destroy.one', record: record, raise: true) rescue nil + if message + ActiveSupport::Deprecation.warn(<<-MESSAGE.squish) + The error key `:'restrict_dependent_destroy.one'` has been deprecated and will be removed in Rails 5.1. + Please use `:'restrict_dependent_destroy.has_one'` instead. + MESSAGE + end + owner.errors.add(:base, message || :'restrict_dependent_destroy.has_one', record: record) throw(:abort) end diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 5f38bd51f6..dbb0e2fab2 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -325,7 +325,7 @@ module ActiveRecord # the parent, <tt>self</tt>, if it wasn't. Skips any <tt>:autosave</tt> # enabled records if they're marked_for_destruction? or destroyed. def association_valid?(reflection, record) - return true if record.destroyed? || record.marked_for_destruction? + return true if record.destroyed? || (reflection.options[:autosave] && record.marked_for_destruction?) validation_context = self.validation_context unless [:create, :update].include?(self.validation_context) unless valid = record.valid?(validation_context) diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 6d3a21a3dc..56227ddd80 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -107,6 +107,18 @@ module ActiveRecord @prepared_statements = false end + class Version + include Comparable + + def initialize(version_string) + @version = version_string.split('.').map(&:to_i) + end + + def <=>(version_string) + @version <=> version_string.split('.').map(&:to_i) + end + end + class BindCollector < Arel::Collectors::Bind def compile(bvs, conn) casted_binds = conn.prepare_binds_for_database(bvs) diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index 2027492f29..af156c9c78 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -307,7 +307,7 @@ module ActiveRecord # # http://bugs.mysql.com/bug.php?id=39170 def supports_transaction_isolation? - version[0] >= 5 + version >= '5.0.0' end def supports_indexes_in_create? @@ -319,11 +319,11 @@ module ActiveRecord end def supports_views? - version[0] >= 5 + version >= '5.0.0' end def supports_datetime_with_precision? - (version[0] == 5 && version[1] >= 6) || version[0] >= 6 + version >= '5.6.4' end def native_database_types @@ -386,6 +386,14 @@ module ActiveRecord 0 end + def quoted_date(value) + if supports_datetime_with_precision? + super + else + super.sub(/\.\d{6}\z/, '') + end + end + # REFERENTIAL INTEGRITY ==================================== def disable_referential_integrity #:nodoc: @@ -938,7 +946,7 @@ module ActiveRecord end def version - @version ||= full_version.scan(/^(\d+)\.(\d+)\.(\d+)/).flatten.map(&:to_i) + @version ||= Version.new(full_version.match(/^\d+\.\d+\.\d+/)[0]) end def mariadb? @@ -946,7 +954,7 @@ module ActiveRecord end def supports_rename_index? - mariadb? ? false : (version[0] == 5 && version[1] >= 7) || version[0] >= 6 + mariadb? ? false : version >= '5.7.6' end def configure_connection diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 7c809b088c..358039723f 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -65,18 +65,6 @@ module ActiveRecord boolean: { name: "boolean" } } - class Version - include Comparable - - def initialize(version_string) - @version = version_string.split('.').map(&:to_i) - end - - def <=>(version_string) - @version <=> version_string.split('.').map(&:to_i) - end - end - class StatementPool < ConnectionAdapters::StatementPool private diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index b82488a59c..ffce2173ec 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -162,11 +162,13 @@ module ActiveRecord } record = statement.execute([id], self, connection).first unless record - raise RecordNotFound, "Couldn't find #{name} with '#{primary_key}'=#{id}" + raise RecordNotFound.new("Couldn't find #{name} with '#{primary_key}'=#{id}", + name, primary_key, id) end record rescue RangeError - raise RecordNotFound, "Couldn't find #{name} with an out of range value for '#{primary_key}'" + raise RecordNotFound.new("Couldn't find #{name} with an out of range value for '#{primary_key}'", + name, primary_key) end def find_by(*args) # :nodoc: @@ -199,7 +201,7 @@ module ActiveRecord end def find_by!(*args) # :nodoc: - find_by(*args) or raise RecordNotFound.new("Couldn't find #{name}") + find_by(*args) or raise RecordNotFound.new("Couldn't find #{name}", name) end def initialize_generated_modules # :nodoc: diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb index 0f1759abaa..d589620f8a 100644 --- a/activerecord/lib/active_record/errors.rb +++ b/activerecord/lib/active_record/errors.rb @@ -47,6 +47,15 @@ module ActiveRecord # Raised when Active Record cannot find record by given id or set of ids. class RecordNotFound < ActiveRecordError + attr_reader :model, :primary_key, :id + + def initialize(message = nil, model = nil, primary_key = nil, id = nil) + @primary_key = primary_key + @model = model + @id = id + + super(message) + end end # Raised by ActiveRecord::Base.save! and ActiveRecord::Base.create! methods when record cannot be diff --git a/activerecord/lib/active_record/locale/en.yml b/activerecord/lib/active_record/locale/en.yml index 8a3c27e6da..0b35027b2b 100644 --- a/activerecord/lib/active_record/locale/en.yml +++ b/activerecord/lib/active_record/locale/en.yml @@ -16,8 +16,8 @@ en: messages: record_invalid: "Validation failed: %{errors}" restrict_dependent_destroy: - one: "Cannot delete record because a dependent %{record} exists" - many: "Cannot delete record because dependent %{record} exist" + has_one: "Cannot delete record because a dependent %{record} exists" + has_many: "Cannot delete record because dependent %{record} exist" # Append your own errors here or at the model/attributes scope. # You can define own errors for models or model attributes. diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index a6b76b25bf..c5a1488588 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -561,7 +561,9 @@ module ActiveRecord end def raise_nested_attributes_record_not_found!(association_name, record_id) - raise RecordNotFound, "Couldn't find #{self.class._reflect_on_association(association_name).klass.name} with ID=#{record_id} for #{self.class.name} with ID=#{id}" + model = self.class._reflect_on_association(association_name).klass.name + raise RecordNotFound.new("Couldn't find #{model} with ID=#{record_id} for #{self.class.name} with ID=#{id}", + model, 'id', record_id) end end end diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 9fef55adea..009b2bad57 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -85,7 +85,8 @@ module ActiveRecord def find_by!(arg, *args) where(arg, *args).take! rescue RangeError - raise RecordNotFound, "Couldn't find #{@klass.name} with an out of range value" + raise RecordNotFound.new("Couldn't find #{@klass.name} with an out of range value", + @klass.name) end # Gives a record (or N records if a parameter is supplied) without any implied diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 3131723828..887d7a5903 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -204,9 +204,8 @@ module ActiveRecord # # Note that "TRUNCATE" is also a MySQL DDL statement! module ClassMethods - # See ActiveRecord::Transactions::ClassMethods for detailed documentation. + # See the ConnectionAdapters::DatabaseStatements#transaction API docs. def transaction(options = {}, &block) - # See the ConnectionAdapters::DatabaseStatements#transaction API docs. connection.transaction(options, &block) end @@ -380,6 +379,10 @@ module ActiveRecord raise ActiveRecord::Rollback unless status end status + ensure + if @transaction_state && @transaction_state.committed? + clear_transaction_record_state + end end protected diff --git a/activerecord/test/cases/adapters/mysql/quoting_test.rb b/activerecord/test/cases/adapters/mysql/quoting_test.rb index a296cf9d31..ca476947fc 100644 --- a/activerecord/test/cases/adapters/mysql/quoting_test.rb +++ b/activerecord/test/cases/adapters/mysql/quoting_test.rb @@ -12,4 +12,18 @@ class MysqlQuotingTest < ActiveRecord::MysqlTestCase def test_type_cast_false assert_equal 0, @conn.type_cast(false) end + + def test_quoted_date_precision_for_gte_564 + @conn.stubs(:full_version).returns('5.6.4') + @conn.remove_instance_variable(:@version) + t = Time.now.change(usec: 1) + assert_match(/\.000001\z/, @conn.quoted_date(t)) + end + + def test_quoted_date_precision_for_lt_564 + @conn.stubs(:full_version).returns('5.6.3') + @conn.remove_instance_variable(:@version) + t = Time.now.change(usec: 1) + refute_match(/\.000001\z/, @conn.quoted_date(t)) + end end diff --git a/activerecord/test/cases/adapters/mysql2/quoting_test.rb b/activerecord/test/cases/adapters/mysql2/quoting_test.rb new file mode 100644 index 0000000000..a49cbba4b4 --- /dev/null +++ b/activerecord/test/cases/adapters/mysql2/quoting_test.rb @@ -0,0 +1,21 @@ +require "cases/helper" + +class Mysql2QuotingTest < ActiveRecord::Mysql2TestCase + setup do + @connection = ActiveRecord::Base.connection + end + + test 'quoted date precision for gte 5.6.4' do + @connection.stubs(:full_version).returns('5.6.4') + @connection.remove_instance_variable(:@version) + t = Time.now.change(usec: 1) + assert_match(/\.000001\z/, @connection.quoted_date(t)) + end + + test 'quoted date precision for lt 5.6.4' do + @connection.stubs(:full_version).returns('5.6.3') + @connection.remove_instance_variable(:@version) + t = Time.now.change(usec: 1) + refute_match(/\.000001\z/, @connection.quoted_date(t)) + end +end diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index f4bacbea2e..c2ef59d0f7 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -1426,6 +1426,26 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert firm.companies.exists?(:name => 'child') end + def test_restrict_with_error_is_deprecated_using_key_many + I18n.backend = I18n::Backend::Simple.new + I18n.backend.store_translations :en, activerecord: { errors: { messages: { restrict_dependent_destroy: { many: 'message for deprecated key' } } } } + + firm = RestrictedWithErrorFirm.create!(name: 'restrict') + firm.companies.create(name: 'child') + + assert !firm.companies.empty? + + assert_deprecated { firm.destroy } + + assert !firm.errors.empty? + + assert_equal 'message for deprecated key', firm.errors[:base].first + assert RestrictedWithErrorFirm.exists?(name: 'restrict') + assert firm.companies.exists?(name: 'child') + ensure + I18n.backend.reload! + end + def test_restrict_with_error firm = RestrictedWithErrorFirm.create!(:name => 'restrict') firm.companies.create(:name => 'child') diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index b7cb5a064f..5a8afaf4d2 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -178,6 +178,25 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert firm.account.present? end + def test_restrict_with_error_is_deprecated_using_key_one + I18n.backend = I18n::Backend::Simple.new + I18n.backend.store_translations :en, activerecord: { errors: { messages: { restrict_dependent_destroy: { one: 'message for deprecated key' } } } } + + firm = RestrictedWithErrorFirm.create!(name: 'restrict') + firm.create_account(credit_limit: 10) + + assert_not_nil firm.account + + assert_deprecated { firm.destroy } + + assert !firm.errors.empty? + assert_equal 'message for deprecated key', firm.errors[:base].first + assert RestrictedWithErrorFirm.exists?(name: 'restrict') + assert firm.account.present? + ensure + I18n.backend.reload! + end + def test_restrict_with_error firm = RestrictedWithErrorFirm.create!(:name => 'restrict') firm.create_account(:credit_limit => 10) diff --git a/activerecord/test/cases/attributes_test.rb b/activerecord/test/cases/attributes_test.rb index eeda9335ad..bd74821d87 100644 --- a/activerecord/test/cases/attributes_test.rb +++ b/activerecord/test/cases/attributes_test.rb @@ -142,7 +142,7 @@ module ActiveRecord end if current_adapter?(:PostgreSQLAdapter) - test "arrays types can be specified" do + test "array types can be specified" do klass = Class.new(OverloadedType) do attribute :my_array, :string, limit: 50, array: true attribute :my_int_array, :integer, array: true diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 6b69b96e21..37ec3f1106 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -1154,6 +1154,13 @@ class TestAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCase def test_should_not_load_the_associated_model assert_queries(1) { @pirate.catchphrase = 'Arr'; @pirate.save! } end + + def test_mark_for_destruction_is_ignored_without_autosave_true + ship = ShipWithoutNestedAttributes.new(name: "The Black Flag") + ship.parts.build.mark_for_destruction + + assert_not ship.valid? + end end class TestAutosaveAssociationOnAHasOneThroughAssociation < ActiveRecord::TestCase diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index 12c793c408..d9d0f929db 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -47,7 +47,8 @@ end def mysql_56? current_adapter?(:MysqlAdapter, :Mysql2Adapter) && - ActiveRecord::Base.connection.send(:version).join(".") >= "5.6.0" + ActiveRecord::Base.connection.send(:version) >= '5.6.0' && + ActiveRecord::Base.connection.send(:version) < '5.7.0' end def mysql_enforcing_gtid_consistency? diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index 4e163ca4a6..29a6ec7522 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -175,6 +175,13 @@ class TransactionTest < ActiveRecord::TestCase assert topic.new_record?, "#{topic.inspect} should be new record" end + def test_transaction_state_is_cleared_when_record_is_persisted + author = Author.create! name: 'foo' + author.name = nil + assert_not author.save + assert_not author.new_record? + end + def test_update_should_rollback_on_failure author = Author.find(1) posts_count = author.posts.size diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb index fcc533380b..95172e4d3e 100644 --- a/activerecord/test/models/ship.rb +++ b/activerecord/test/models/ship.rb @@ -22,6 +22,7 @@ end class ShipWithoutNestedAttributes < ActiveRecord::Base self.table_name = "ships" has_many :prisoners, inverse_of: :ship, foreign_key: :ship_id + has_many :parts, class_name: "ShipPart", foreign_key: :ship_id validates :name, presence: true end diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb index f81ffe1d90..d17270021a 100644 --- a/activerecord/test/models/topic.rb +++ b/activerecord/test/models/topic.rb @@ -32,7 +32,7 @@ class Topic < ActiveRecord::Base end end - has_many :replies, :dependent => :destroy, :foreign_key => "parent_id" + has_many :replies, dependent: :destroy, foreign_key: "parent_id", autosave: true has_many :approved_replies, -> { approved }, class_name: 'Reply', foreign_key: "parent_id", counter_cache: 'replies_count' has_many :unique_replies, :dependent => :destroy, :foreign_key => "parent_id" |