diff options
24 files changed, 130 insertions, 75 deletions
diff --git a/Gemfile.lock b/Gemfile.lock index 4a0fba9024..77c234814a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -214,7 +214,7 @@ GEM em-socksify (0.3.2) eventmachine (>= 1.0.0.beta.4) erubi (1.7.0) - et-orbi (1.0.8) + et-orbi (1.1.2) tzinfo event_emitter (0.2.6) eventmachine (1.2.5) @@ -472,7 +472,7 @@ GEM turbolinks (5.1.1) turbolinks-source (~> 5.1) turbolinks-source (5.1.0) - tzinfo (1.2.3) + tzinfo (1.2.5) thread_safe (~> 0.1) tzinfo-data (1.2017.2) tzinfo (>= 1.0.0) diff --git a/actioncable/test/channel/stream_test.rb b/actioncable/test/channel/stream_test.rb index 5c1f308f53..b4b118846d 100644 --- a/actioncable/test/channel/stream_test.rb +++ b/actioncable/test/channel/stream_test.rb @@ -180,7 +180,7 @@ module ActionCable::StreamTests run_in_eventmachine do connection = open_connection expected = { "identifier" => { "channel" => MultiChatChannel.name }.to_json, "type" => "confirm_subscription" } - assert_called(connection.websocket, :transmit, [expected.to_json]) do + assert_called_with(connection.websocket, :transmit, [expected.to_json]) do receive(connection, command: "subscribe", channel: MultiChatChannel.name, identifiers: {}) wait_for_async end diff --git a/actionview/test/template/form_helper/form_with_test.rb b/actionview/test/template/form_helper/form_with_test.rb index 6b65d740eb..ed1683bad2 100644 --- a/actionview/test/template/form_helper/form_with_test.rb +++ b/actionview/test/template/form_helper/form_with_test.rb @@ -2347,13 +2347,4 @@ class FormWithActsLikeFormForTest < FormWithTest ensure I18n.locale = old_locale end - - def with_default_enforce_utf8(value) - old_value = ActionView::Helpers::FormTagHelper.default_enforce_utf8 - ActionView::Helpers::FormTagHelper.default_enforce_utf8 = value - - yield - ensure - ActionView::Helpers::FormTagHelper.default_enforce_utf8 = old_value - end end diff --git a/activejob/lib/active_job/exceptions.rb b/activejob/lib/active_job/exceptions.rb index 31bbb18d7f..96b1e262f2 100644 --- a/activejob/lib/active_job/exceptions.rb +++ b/activejob/lib/active_job/exceptions.rb @@ -30,8 +30,8 @@ module ActiveJob # class RemoteServiceJob < ActiveJob::Base # retry_on CustomAppException # defaults to 3s wait, 5 attempts # retry_on AnotherCustomAppException, wait: ->(executions) { executions * 2 } - # retry_on(YetAnotherCustomAppException) do |job, exception| - # ExceptionNotifier.caught(exception) + # retry_on(YetAnotherCustomAppException) do |job, error| + # ExceptionNotifier.caught(error) # end # retry_on ActiveRecord::Deadlocked, wait: 5.seconds, attempts: 3 # retry_on Net::OpenTimeout, wait: :exponentially_longer, attempts: 10 @@ -67,8 +67,8 @@ module ActiveJob # # class SearchIndexingJob < ActiveJob::Base # discard_on ActiveJob::DeserializationError - # discard_on(CustomAppException) do |job, exception| - # ExceptionNotifier.caught(exception) + # discard_on(CustomAppException) do |job, error| + # ExceptionNotifier.caught(error) # end # # def perform(record) diff --git a/activemodel/lib/active_model/type/boolean.rb b/activemodel/lib/active_model/type/boolean.rb index bcdbab0343..f6c6efbc87 100644 --- a/activemodel/lib/active_model/type/boolean.rb +++ b/activemodel/lib/active_model/type/boolean.rb @@ -20,6 +20,10 @@ module ActiveModel :boolean end + def serialize(value) # :nodoc: + cast(value) + end + private def cast_value(value) diff --git a/activemodel/lib/active_model/validations/numericality.rb b/activemodel/lib/active_model/validations/numericality.rb index 31750ba78e..0478915be7 100644 --- a/activemodel/lib/active_model/validations/numericality.rb +++ b/activemodel/lib/active_model/validations/numericality.rb @@ -19,9 +19,11 @@ module ActiveModel end def validate_each(record, attr_name, value) - before_type_cast = :"#{attr_name}_before_type_cast" + came_from_user = :"#{attr_name}_came_from_user?" - raw_value = record.send(before_type_cast) if record.respond_to?(before_type_cast) && record.send(before_type_cast) != value + if record.respond_to?(came_from_user) && record.public_send(came_from_user) + raw_value = record.read_attribute_before_type_cast(attr_name) + end raw_value ||= value if record_attribute_changed_in_place?(record, attr_name) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index dda7d19915..5cefb5dfd7 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Fix parent record should not get saved with duplicate children records. + + Fixes #32940. + + *Santosh Wadghule* + * Fix logic on disabling commit callbacks so they are not called unexpectedly when errors occur. *Brian Durand* diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 9575cc24c8..a405f05e0b 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -392,7 +392,7 @@ module ActiveRecord records -= records_to_destroy end - records.each do |record| + records.each_with_index do |record, index| next if record.destroyed? saved = true @@ -401,9 +401,11 @@ module ActiveRecord if autosave saved = association.insert_record(record, false) elsif !reflection.nested? - association_saved = association.insert_record(record) if reflection.validate? - saved = association_saved + valid = association_valid?(reflection, record, index) + saved = valid ? association.insert_record(record, false) : false + else + association.insert_record(record) end end elsif autosave diff --git a/activerecord/lib/active_record/callbacks.rb b/activerecord/lib/active_record/callbacks.rb index 0a29c5134e..b6852bfc71 100644 --- a/activerecord/lib/active_record/callbacks.rb +++ b/activerecord/lib/active_record/callbacks.rb @@ -128,7 +128,7 @@ module ActiveRecord # end # end # - # So you specify the object you want messaged on a given callback. When that callback is triggered, the object has + # So you specify the object you want to be messaged on a given callback. When that callback is triggered, the object has # a method by the name of the callback messaged. You can make these callbacks more flexible by passing in other # initialization data such as the name of the attribute to work with: # diff --git a/activerecord/lib/arel/visitors/postgresql.rb b/activerecord/lib/arel/visitors/postgresql.rb index 108ee431ee..c5110fa89c 100644 --- a/activerecord/lib/arel/visitors/postgresql.rb +++ b/activerecord/lib/arel/visitors/postgresql.rb @@ -5,7 +5,7 @@ module Arel # :nodoc: all class PostgreSQL < Arel::Visitors::ToSql CUBE = "CUBE" ROLLUP = "ROLLUP" - GROUPING_SET = "GROUPING SET" + GROUPING_SETS = "GROUPING SETS" LATERAL = "LATERAL" private @@ -67,7 +67,7 @@ module Arel # :nodoc: all end def visit_Arel_Nodes_GroupingSet(o, collector) - collector << GROUPING_SET + collector << GROUPING_SETS grouping_array_or_grouping_element o, collector end diff --git a/activerecord/test/cases/adapters/postgresql/money_test.rb b/activerecord/test/cases/adapters/postgresql/money_test.rb index be3590e8dd..61e75e772d 100644 --- a/activerecord/test/cases/adapters/postgresql/money_test.rb +++ b/activerecord/test/cases/adapters/postgresql/money_test.rb @@ -6,7 +6,9 @@ require "support/schema_dumping_helper" class PostgresqlMoneyTest < ActiveRecord::PostgreSQLTestCase include SchemaDumpingHelper - class PostgresqlMoney < ActiveRecord::Base; end + class PostgresqlMoney < ActiveRecord::Base + validates :depth, numericality: true + end setup do @connection = ActiveRecord::Base.connection @@ -35,6 +37,7 @@ class PostgresqlMoneyTest < ActiveRecord::PostgreSQLTestCase def test_default assert_equal BigDecimal("150.55"), PostgresqlMoney.column_defaults["depth"] assert_equal BigDecimal("150.55"), PostgresqlMoney.new.depth + assert_equal "$150.55", PostgresqlMoney.new.depth_before_type_cast end def test_money_values diff --git a/activerecord/test/cases/arel/visitors/postgres_test.rb b/activerecord/test/cases/arel/visitors/postgres_test.rb index ba37afecfb..ba9cfcfc64 100644 --- a/activerecord/test/cases/arel/visitors/postgres_test.rb +++ b/activerecord/test/cases/arel/visitors/postgres_test.rb @@ -229,7 +229,7 @@ module Arel it "should know how to visit with array arguments" do node = Arel::Nodes::GroupingSet.new([@table[:name], @table[:bool]]) compile(node).must_be_like %{ - GROUPING SET( "users"."name", "users"."bool" ) + GROUPING SETS( "users"."name", "users"."bool" ) } end @@ -237,7 +237,7 @@ module Arel group = Arel::Nodes::GroupingElement.new([@table[:name], @table[:bool]]) node = Arel::Nodes::GroupingSet.new(group) compile(node).must_be_like %{ - GROUPING SET( "users"."name", "users"."bool" ) + GROUPING SETS( "users"."name", "users"."bool" ) } end @@ -246,7 +246,7 @@ module Arel group2 = Arel::Nodes::GroupingElement.new([@table[:bool], @table[:created_at]]) node = Arel::Nodes::GroupingSet.new([group1, group2]) compile(node).must_be_like %{ - GROUPING SET( ( "users"."name" ), ( "users"."bool", "users"."created_at" ) ) + GROUPING SETS( ( "users"."name" ), ( "users"."bool", "users"."created_at" ) ) } end end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 7355f4cd62..cf93d42942 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -27,6 +27,8 @@ require "models/member_detail" require "models/organization" require "models/guitar" require "models/tuning_peg" +require "models/topic" +require "models/reply" class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase def test_autosave_validation @@ -557,6 +559,22 @@ class TestDefaultAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCa assert_equal no_of_clients + 1, Client.count end + def test_parent_should_not_get_saved_with_duplicate_children_records + Topic.delete_all + + content = "Best content" + reply1 = ValidateUniqueContentReply.new(content: content) + reply2 = ValidateUniqueContentReply.new(content: content) + + topic = Topic.new(validate_unique_content_replies: [reply1, reply2]) + + assert_not topic.save + assert topic.errors.any? + + assert_equal 0, Topic.count + assert_equal 0, ValidateUniqueContentReply.count + end + def test_invalid_build new_client = companies(:first_firm).clients_of_firm.build assert_not_predicate new_client, :persisted? diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index fcfab074a2..d216fe16fa 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -14,7 +14,6 @@ require "models/computer" require "models/project" require "models/default" require "models/auto_id" -require "models/boolean" require "models/column_name" require "models/subscriber" require "models/comment" @@ -716,48 +715,6 @@ class BasicsTest < ActiveRecord::TestCase assert_equal expected_attributes, category.attributes end - def test_boolean - b_nil = Boolean.create("value" => nil) - nil_id = b_nil.id - b_false = Boolean.create("value" => false) - false_id = b_false.id - b_true = Boolean.create("value" => true) - true_id = b_true.id - - b_nil = Boolean.find(nil_id) - assert_nil b_nil.value - b_false = Boolean.find(false_id) - assert_not_predicate b_false, :value? - b_true = Boolean.find(true_id) - assert_predicate b_true, :value? - end - - def test_boolean_without_questionmark - b_true = Boolean.create("value" => true) - true_id = b_true.id - - subclass = Class.new(Boolean).find true_id - superclass = Boolean.find true_id - - assert_equal superclass.read_attribute(:has_fun), subclass.read_attribute(:has_fun) - end - - def test_boolean_cast_from_string - b_blank = Boolean.create("value" => "") - blank_id = b_blank.id - b_false = Boolean.create("value" => "0") - false_id = b_false.id - b_true = Boolean.create("value" => "1") - true_id = b_true.id - - b_blank = Boolean.find(blank_id) - assert_nil b_blank.value - b_false = Boolean.find(false_id) - assert_not_predicate b_false, :value? - b_true = Boolean.find(true_id) - assert_predicate b_true, :value? - end - def test_new_record_returns_boolean assert_equal false, Topic.new.persisted? assert_equal true, Topic.find(1).persisted? diff --git a/activerecord/test/cases/boolean_test.rb b/activerecord/test/cases/boolean_test.rb new file mode 100644 index 0000000000..ab9f974e2c --- /dev/null +++ b/activerecord/test/cases/boolean_test.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require "cases/helper" +require "models/boolean" + +class BooleanTest < ActiveRecord::TestCase + def test_boolean + b_nil = Boolean.create!(value: nil) + b_false = Boolean.create!(value: false) + b_true = Boolean.create!(value: true) + + assert_nil Boolean.find(b_nil.id).value + assert_not_predicate Boolean.find(b_false.id), :value? + assert_predicate Boolean.find(b_true.id), :value? + end + + def test_boolean_without_questionmark + b_true = Boolean.create!(value: true) + + subclass = Class.new(Boolean).find(b_true.id) + superclass = Boolean.find(b_true.id) + + assert_equal superclass.read_attribute(:has_fun), subclass.read_attribute(:has_fun) + end + + def test_boolean_cast_from_string + b_blank = Boolean.create!(value: "") + b_false = Boolean.create!(value: "0") + b_true = Boolean.create!(value: "1") + + assert_nil Boolean.find(b_blank.id).value + assert_not_predicate Boolean.find(b_false.id), :value? + assert_predicate Boolean.find(b_true.id), :value? + end + + def test_find_by_boolean_string + b_false = Boolean.create!(value: "false") + b_true = Boolean.create!(value: "true") + + assert_equal b_false, Boolean.find_by(value: "false") + assert_equal b_true, Boolean.find_by(value: "true") + end +end diff --git a/activerecord/test/models/reply.rb b/activerecord/test/models/reply.rb index d1cee58788..d178413fd7 100644 --- a/activerecord/test/models/reply.rb +++ b/activerecord/test/models/reply.rb @@ -16,6 +16,11 @@ end class SillyUniqueReply < UniqueReply end +class ValidateUniqueContentReply < Reply + belongs_to :topic, foreign_key: "parent_id" + validates :content, uniqueness: true +end + class WrongReply < Reply validate :errors_on_empty_content validate :title_is_wrong_create, on: :create diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb index 72699046f9..8368405cef 100644 --- a/activerecord/test/models/topic.rb +++ b/activerecord/test/models/topic.rb @@ -47,6 +47,7 @@ class Topic < ActiveRecord::Base has_many :unique_replies, dependent: :destroy, foreign_key: "parent_id" has_many :silly_unique_replies, dependent: :destroy, foreign_key: "parent_id" + has_many :validate_unique_content_replies, dependent: :destroy, foreign_key: "parent_id" serialize :content diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 134d3bb2d9..d36c951292 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -167,6 +167,8 @@ class ActiveStorage::Blob < ActiveRecord::Base end # Downloads the blob to a tempfile on disk. Yields the tempfile. + # + # Raises ActiveStorage::IntegrityError if the downloaded data does not match the blob's checksum. def open(tempdir: nil, &block) ActiveStorage::Downloader.new(self, tempdir: tempdir).download_blob_to_tempfile(&block) end diff --git a/activestorage/lib/active_storage/downloader.rb b/activestorage/lib/active_storage/downloader.rb index 0e7039e104..2aa56a729a 100644 --- a/activestorage/lib/active_storage/downloader.rb +++ b/activestorage/lib/active_storage/downloader.rb @@ -10,6 +10,7 @@ module ActiveStorage def download_blob_to_tempfile open_tempfile do |file| download_blob_to file + verify_integrity_of file yield file end end @@ -34,6 +35,12 @@ module ActiveStorage file.rewind end + def verify_integrity_of(file) + unless Digest::MD5.file(file).base64digest == blob.checksum + raise ActiveStorage::IntegrityError + end + end + def tempfile_extension_with_delimiter blob.filename.extension_with_delimiter end diff --git a/activestorage/lib/active_storage/errors.rb b/activestorage/lib/active_storage/errors.rb index f099b13f5b..bedcd080c4 100644 --- a/activestorage/lib/active_storage/errors.rb +++ b/activestorage/lib/active_storage/errors.rb @@ -4,4 +4,8 @@ module ActiveStorage class InvariableError < StandardError; end class UnpreviewableError < StandardError; end class UnrepresentableError < StandardError; end + + # Raised when uploaded or downloaded data does not match a precomputed checksum. + # Indicates that a network error or a software bug caused data corruption. + class IntegrityError < StandardError; end end diff --git a/activestorage/lib/active_storage/service.rb b/activestorage/lib/active_storage/service.rb index 949969fc95..da1af4f745 100644 --- a/activestorage/lib/active_storage/service.rb +++ b/activestorage/lib/active_storage/service.rb @@ -3,8 +3,6 @@ require "active_storage/log_subscriber" module ActiveStorage - class IntegrityError < StandardError; end - # Abstract class serving as an interface for concrete services. # # The available services are: diff --git a/activestorage/lib/tasks/activestorage.rake b/activestorage/lib/tasks/activestorage.rake index 296e91afa1..ac254d717f 100644 --- a/activestorage/lib/tasks/activestorage.rake +++ b/activestorage/lib/tasks/activestorage.rake @@ -1,6 +1,9 @@ # frozen_string_literal: true namespace :active_storage do + # Prevent migration installation task from showing up twice. + Rake::Task["install:migrations"].clear_comments + desc "Copy over the migration needed to the application" task install: :environment do if Rake::Task.task_defined?("active_storage:install:migrations") diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index a013b7a924..2d1857041d 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -84,7 +84,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase assert_equal "a" * 64.kilobytes, chunks.second end - test "open" do + test "open with integrity" do create_file_blob(filename: "racecar.jpg").open do |file| assert file.binmode? assert_equal 0, file.pos @@ -93,6 +93,16 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase end end + test "open without integrity" do + create_blob(data: "Hello, world!").tap do |blob| + blob.update! checksum: Digest::MD5.base64digest("Goodbye, world!") + + assert_raises ActiveStorage::IntegrityError do + blob.open { |file| flunk "Expected integrity check to fail" } + end + end + end + test "open in a custom tempdir" do tempdir = Dir.mktmpdir diff --git a/activesupport/lib/active_support/i18n_railtie.rb b/activesupport/lib/active_support/i18n_railtie.rb index ce8bfbfd8c..93bde57f6a 100644 --- a/activesupport/lib/active_support/i18n_railtie.rb +++ b/activesupport/lib/active_support/i18n_railtie.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require "active_support" -require "active_support/file_update_checker" require "active_support/core_ext/array/wrap" # :enddoc: |