diff options
25 files changed, 231 insertions, 68 deletions
diff --git a/.travis.yml b/.travis.yml index 0fdea1367c..d7544fecb6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -91,7 +91,7 @@ matrix: env: - "GEM=ar:mysql2 MYSQL=mariadb" addons: - mariadb: 10.2 + mariadb: 10.3 - rvm: 2.5.1 env: - "GEM=ar:sqlite3_mem" diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 5cefb5dfd7..d101b81e28 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* Bump minimum SQLite version to 3.8 + + *Yasuo Honda* + * Fix parent record should not get saved with duplicate children records. Fixes #32940. diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb index d2934c830a..44596f4424 100644 --- a/activerecord/lib/active_record/associations/association.rb +++ b/activerecord/lib/active_record/associations/association.rb @@ -275,6 +275,7 @@ module ActiveRecord def build_record(attributes) reflection.build_association(attributes) do |record| initialize_attributes(record, attributes) + yield(record) if block_given? end end diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index d61d105544..7f1df9a21d 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -105,9 +105,7 @@ module ActiveRecord if attributes.is_a?(Array) attributes.collect { |attr| build(attr, &block) } else - add_to_target(build_record(attributes)) do |record| - yield(record) if block_given? - end + add_to_target(build_record(attributes, &block)) end end @@ -361,8 +359,7 @@ module ActiveRecord attributes.collect { |attr| _create_record(attr, raise, &block) } else transaction do - add_to_target(build_record(attributes)) do |record| - yield(record) if block_given? + add_to_target(build_record(attributes, &block)) do |record| insert_record(record, true, raise) { @_was_loaded = loaded? @association_ids = nil diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 59929b8c4e..617956c768 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -90,7 +90,7 @@ module ActiveRecord def build_record(attributes) ensure_not_nested - record = super(attributes) + record = super inverse = source_reflection.inverse_of if inverse diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index d211884135..390bfd8b08 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -106,7 +106,7 @@ module ActiveRecord end end - def _create_record(attributes, raise_error = false) + def _create_record(attributes, raise_error = false, &block) unless owner.persisted? raise ActiveRecord::RecordNotSaved, "You cannot call create unless the parent is saved" end diff --git a/activerecord/lib/active_record/associations/singular_association.rb b/activerecord/lib/active_record/associations/singular_association.rb index ead89bfe6c..cfab16a745 100644 --- a/activerecord/lib/active_record/associations/singular_association.rb +++ b/activerecord/lib/active_record/associations/singular_association.rb @@ -17,9 +17,8 @@ module ActiveRecord replace(record) end - def build(attributes = {}) - record = build_record(attributes) - yield(record) if block_given? + def build(attributes = {}, &block) + record = build_record(attributes, &block) set_new_record(record) record end @@ -62,9 +61,8 @@ module ActiveRecord replace(record) end - def _create_record(attributes, raise_error = false) - record = build_record(attributes) - yield(record) if block_given? + def _create_record(attributes, raise_error = false, &block) + record = build_record(attributes, &block) saved = record.save set_new_record(record) raise RecordInvalid.new(record) if !saved && raise_error diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 5afb0bc068..15e6565e69 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -114,7 +114,7 @@ module ActiveRecord attributes[inverse.foreign_key] = target.id end - super(attributes) + super end end end diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 22d195c9a4..6d2f75a3ae 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -13,33 +13,37 @@ module ActiveRecord class_attribute :aggregate_reflections, instance_writer: false, default: {} end - def self.create(macro, name, scope, options, ar) - klass = \ - case macro - when :composed_of - AggregateReflection - when :has_many - HasManyReflection - when :has_one - HasOneReflection - when :belongs_to - BelongsToReflection - else - raise "Unsupported Macro: #{macro}" - end + class << self + def create(macro, name, scope, options, ar) + reflection = reflection_class_for(macro).new(name, scope, options, ar) + options[:through] ? ThroughReflection.new(reflection) : reflection + end - reflection = klass.new(name, scope, options, ar) - options[:through] ? ThroughReflection.new(reflection) : reflection - end + def add_reflection(ar, name, reflection) + ar.clear_reflections_cache + name = name.to_s + ar._reflections = ar._reflections.except(name).merge!(name => reflection) + end - def self.add_reflection(ar, name, reflection) - ar.clear_reflections_cache - name = name.to_s - ar._reflections = ar._reflections.except(name).merge!(name => reflection) - end + def add_aggregate_reflection(ar, name, reflection) + ar.aggregate_reflections = ar.aggregate_reflections.merge(name.to_s => reflection) + end - def self.add_aggregate_reflection(ar, name, reflection) - ar.aggregate_reflections = ar.aggregate_reflections.merge(name.to_s => reflection) + private + def reflection_class_for(macro) + case macro + when :composed_of + AggregateReflection + when :has_many + HasManyReflection + when :has_one + HasOneReflection + when :belongs_to + BelongsToReflection + else + raise "Unsupported Macro: #{macro}" + end + end end # \Reflection enables the ability to examine the associations and aggregations of diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 82adb19f5b..c5d5fca672 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -328,10 +328,12 @@ module ActiveRecord # but call it after the commit of a destroyed object. def committed!(should_run_callbacks: true) #:nodoc: if should_run_callbacks && (destroyed? || persisted?) + @_committed_already_called = true _run_commit_without_transaction_enrollment_callbacks _run_commit_callbacks end ensure + @_committed_already_called = false force_clear_transaction_record_state end @@ -380,6 +382,7 @@ module ActiveRecord end private + attr_reader :_committed_already_called, :_trigger_update_callback, :_trigger_destroy_callback # Save the new record state and id of a record so it can be restored later if a transaction fails. def remember_transaction_record_state @@ -390,6 +393,15 @@ module ActiveRecord frozen?: frozen?, ) @_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) + 1 + remember_new_record_before_last_commit + end + + def remember_new_record_before_last_commit + if _committed_already_called + @_new_record_before_last_commit = false + else + @_new_record_before_last_commit = @_start_transaction_state[:new_record] + end end # Clear the new record state and id of a record. @@ -421,22 +433,16 @@ module ActiveRecord end end - # Determine if a record was created or destroyed in a transaction. State should be one of :new_record or :destroyed. - def transaction_record_state(state) - @_start_transaction_state[state] - end - # Determine if a transaction included an action for :create, :update, or :destroy. Used in filtering callbacks. def transaction_include_any_action?(actions) actions.any? do |action| case action when :create - transaction_record_state(:new_record) - when :destroy - defined?(@_trigger_destroy_callback) && @_trigger_destroy_callback + persisted? && @_new_record_before_last_commit when :update - !(transaction_record_state(:new_record) || destroyed?) && - (defined?(@_trigger_update_callback) && @_trigger_update_callback) + !(@_new_record_before_last_commit || destroyed?) && _trigger_update_callback + when :destroy + _trigger_destroy_callback end end end diff --git a/activerecord/test/cases/adapters/mysql2/virtual_column_test.rb b/activerecord/test/cases/adapters/mysql2/virtual_column_test.rb index ffde8ed4d8..8494acee3b 100644 --- a/activerecord/test/cases/adapters/mysql2/virtual_column_test.rb +++ b/activerecord/test/cases/adapters/mysql2/virtual_column_test.rb @@ -18,6 +18,7 @@ if ActiveRecord::Base.connection.supports_virtual_columns? t.string :name t.virtual :upper_name, type: :string, as: "UPPER(`name`)" t.virtual :name_length, type: :integer, as: "LENGTH(`name`)", stored: true + t.virtual :name_octet_length, type: :integer, as: "OCTET_LENGTH(`name`)", stored: true end VirtualColumn.create(name: "Rails") end @@ -55,7 +56,8 @@ if ActiveRecord::Base.connection.supports_virtual_columns? def test_schema_dumping output = dump_table_schema("virtual_columns") assert_match(/t\.virtual\s+"upper_name",\s+type: :string,\s+as: "(?:UPPER|UCASE)\(`name`\)"$/i, output) - assert_match(/t\.virtual\s+"name_length",\s+type: :integer,\s+as: "LENGTH\(`name`\)",\s+stored: true$/i, output) + assert_match(/t\.virtual\s+"name_length",\s+type: :integer,\s+as: "(?:octet_length|length)\(`name`\)",\s+stored: true$/i, output) + assert_match(/t\.virtual\s+"name_octet_length",\s+type: :integer,\s+as: "(?:octet_length|length)\(`name`\)",\s+stored: true$/i, output) end end end diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index 0facc286da..46fa36d7dd 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -737,6 +737,18 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase [:added, :before, "Roger"], [:added, :after, "Roger"] ], log.last(4) + + post.people_with_callbacks.build { |person| person.first_name = "Ted" } + assert_equal [ + [:added, :before, "Ted"], + [:added, :after, "Ted"] + ], log.last(2) + + post.people_with_callbacks.create { |person| person.first_name = "Sam" } + assert_equal [ + [:added, :before, "Sam"], + [:added, :after, "Sam"] + ], log.last(2) end def test_dynamic_find_should_respect_association_include diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index 05941c75ac..c0be45eee7 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/activerecord/test/cases/transaction_callbacks_test.rb @@ -139,6 +139,23 @@ class TransactionCallbacksTest < ActiveRecord::TestCase assert_equal [], reply.history end + def test_only_call_after_commit_on_destroy_after_transaction_commits_for_destroyed_new_record + new_record = TopicWithCallbacks.new(title: "New topic", written_on: Date.today) + add_transaction_execution_blocks new_record + + new_record.destroy + assert_equal [:commit_on_destroy], new_record.history + end + + def test_save_in_after_create_commit_wont_invoke_extra_after_create_commit + new_record = TopicWithCallbacks.new(title: "New topic", written_on: Date.today) + add_transaction_execution_blocks new_record + new_record.after_commit_block(:create) { |r| r.save! } + + new_record.save! + assert_equal [:commit_on_create, :commit_on_update], new_record.history + end + def test_only_call_after_commit_on_create_and_doesnt_leaky r = ReplyWithCallbacks.new(content: "foo") r.save_on_after_create = true diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index c8911fe611..4aa551781b 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,8 @@ +* Add the ability to reflect on defined attachments using the existing + ActiveRecord reflection mechanism. + + *Kevin Deisz* + * Variant arguments of `false` or `nil` will no longer be passed to the processor. For example, the following will not have the monochrome variation applied: diff --git a/activestorage/lib/active_storage/attached/macros.rb b/activestorage/lib/active_storage/attached/macros.rb index f99cf35680..6ad9fc43d7 100644 --- a/activestorage/lib/active_storage/attached/macros.rb +++ b/activestorage/lib/active_storage/attached/macros.rb @@ -48,6 +48,12 @@ module ActiveStorage else before_destroy { public_send(name).detach } end + + ActiveRecord::Reflection.add_attachment_reflection( + self, + name, + ActiveRecord::Reflection.create(:has_one_attached, name, nil, { dependent: dependent }, self) + ) end # Specifies the relation between multiple attachments and the model. @@ -105,6 +111,12 @@ module ActiveStorage else before_destroy { public_send(name).detach } end + + ActiveRecord::Reflection.add_attachment_reflection( + self, + name, + ActiveRecord::Reflection.create(:has_many_attached, name, nil, { dependent: dependent }, self) + ) end end end diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb index 99588cdd4b..519b9ae283 100644 --- a/activestorage/lib/active_storage/engine.rb +++ b/activestorage/lib/active_storage/engine.rb @@ -10,6 +10,8 @@ require "active_storage/previewer/video_previewer" require "active_storage/analyzer/image_analyzer" require "active_storage/analyzer/video_analyzer" +require "active_storage/reflection" + module ActiveStorage class Engine < Rails::Engine # :nodoc: isolate_namespace ActiveStorage @@ -95,5 +97,12 @@ module ActiveStorage end end end + + initializer "active_storage.reflection" do + ActiveSupport.on_load(:active_record) do + include Reflection::ActiveRecordExtensions + ActiveRecord::Reflection.singleton_class.prepend(Reflection::ReflectionExtension) + end + end end end diff --git a/activestorage/lib/active_storage/previewer.rb b/activestorage/lib/active_storage/previewer.rb index bff5e42d41..13657bf13e 100644 --- a/activestorage/lib/active_storage/previewer.rb +++ b/activestorage/lib/active_storage/previewer.rb @@ -44,16 +44,17 @@ module ActiveStorage # # The output tempfile is opened in the directory returned by #tempdir. def draw(*argv) #:doc: - ActiveSupport::Notifications.instrument("preview.active_storage") do - open_tempfile_for_drawing do |file| + open_tempfile do |file| + instrument :preview, key: blob.key do capture(*argv, to: file) - yield file end + + yield file end end - def open_tempfile_for_drawing - tempfile = Tempfile.open("ActiveStorage", tempdir) + def open_tempfile + tempfile = Tempfile.open("ActiveStorage-", tempdir) begin yield tempfile @@ -62,6 +63,10 @@ module ActiveStorage end end + def instrument(operation, payload = {}, &block) + ActiveSupport::Notifications.instrument "#{operation}.active_storage", payload, &block + end + def capture(*argv, to:) to.binmode IO.popen(argv, err: File::NULL) { |out| IO.copy_stream(out, to) } diff --git a/activestorage/lib/active_storage/reflection.rb b/activestorage/lib/active_storage/reflection.rb new file mode 100644 index 0000000000..04a1b20882 --- /dev/null +++ b/activestorage/lib/active_storage/reflection.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +module ActiveStorage + module Reflection + # Holds all the metadata about a has_one_attached attachment as it was + # specified in the Active Record class. + class HasOneAttachedReflection < ActiveRecord::Reflection::MacroReflection #:nodoc: + def macro + :has_one_attached + end + end + + # Holds all the metadata about a has_many_attached attachment as it was + # specified in the Active Record class. + class HasManyAttachedReflection < ActiveRecord::Reflection::MacroReflection #:nodoc: + def macro + :has_many_attached + end + end + + module ReflectionExtension # :nodoc: + def add_attachment_reflection(ar, name, reflection) + ar.attachment_reflections.merge!(name.to_s => reflection) + end + + private + def reflection_class_for(macro) + case macro + when :has_one_attached + HasOneAttachedReflection + when :has_many_attached + HasManyAttachedReflection + else + super + end + end + end + + module ActiveRecordExtensions + extend ActiveSupport::Concern + + included do + class_attribute :attachment_reflections, instance_writer: false, default: {} + end + + module ClassMethods + # Returns an array of reflection objects for all the attachments in the + # class. + def reflect_on_all_attachments + attachment_reflections.values + end + + # Returns the reflection object for the named +attachment+. + # + # User.reflect_on_attachment(:avatar) + # # => the avatar reflection + # + def reflect_on_attachment(attachment) + attachment_reflections[attachment.to_s] + end + end + end + end +end diff --git a/activestorage/test/models/reflection_test.rb b/activestorage/test/models/reflection_test.rb new file mode 100644 index 0000000000..da866ca996 --- /dev/null +++ b/activestorage/test/models/reflection_test.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require "test_helper" + +class ActiveStorage::ReflectionTest < ActiveSupport::TestCase + test "allows reflecting for all attachment" do + expected_classes = + User.reflect_on_all_attachments.all? do |reflection| + reflection.is_a?(ActiveStorage::Reflection::HasOneAttachedReflection) || + reflection.is_a?(ActiveStorage::Reflection::HasManyAttachedReflection) + end + + assert expected_classes + end + + test "allows reflecting on a singular has_one_attached attachment" do + reflection = User.reflect_on_attachment(:avatar) + + assert_equal :avatar, reflection.name + assert_equal :has_one_attached, reflection.macro + end + + test "allows reflecting on a singular has_many_attached attachment" do + reflection = User.reflect_on_attachment(:highlights) + + assert_equal :highlights, reflection.name + assert_equal :has_many_attached, reflection.macro + end +end diff --git a/activesupport/lib/active_support/core_ext/enumerable.rb b/activesupport/lib/active_support/core_ext/enumerable.rb index 7713c52cb1..d87d63f287 100644 --- a/activesupport/lib/active_support/core_ext/enumerable.rb +++ b/activesupport/lib/active_support/core_ext/enumerable.rb @@ -7,11 +7,15 @@ module Enumerable # Enumerable#sum was added in Ruby 2.4, but it only works with Numeric elements # when we omit an identity. + # :stopdoc: + # We can't use Refinements here because Refinements with Module which will be prepended # doesn't work well https://bugs.ruby-lang.org/issues/13446 - alias :_original_sum_with_required_identity :sum # :nodoc: + alias :_original_sum_with_required_identity :sum private :_original_sum_with_required_identity + # :startdoc: + # Calculates a sum from the elements. # # payments.sum { |p| p.price * p.tax_rate } diff --git a/guides/source/active_storage_overview.md b/guides/source/active_storage_overview.md index 91ad089d40..0c465d67cf 100644 --- a/guides/source/active_storage_overview.md +++ b/guides/source/active_storage_overview.md @@ -248,13 +248,13 @@ end Call `avatar.attach` to attach an avatar to an existing user: ```ruby -Current.user.avatar.attach(params[:avatar]) +user.avatar.attach(params[:avatar]) ``` Call `avatar.attached?` to determine whether a particular user has an avatar: ```ruby -Current.user.avatar.attached? +user.avatar.attached? ``` ### `has_many_attached` diff --git a/guides/source/configuring.md b/guides/source/configuring.md index d4aa6546a7..4d8883a7bd 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -165,7 +165,7 @@ pipeline is enabled. It is set to `true` by default. * `config.assets.precompile` allows you to specify additional assets (other than `application.css` and `application.js`) which are to be precompiled when `rake assets:precompile` is run. -* `config.assets.unknown_asset_fallback` allows you to modify the behavior of the asset pipeline when an asset is not in the pipeline, if you use sprockets-rails 3.2.0 or newer. Defaults to `true`. +* `config.assets.unknown_asset_fallback` allows you to modify the behavior of the asset pipeline when an asset is not in the pipeline, if you use sprockets-rails 3.2.0 or newer. Defaults to `false`. * `config.assets.prefix` defines the prefix where assets are served from. Defaults to `/assets`. diff --git a/guides/source/layouts_and_rendering.md b/guides/source/layouts_and_rendering.md index d7072a766b..ba3ef4679b 100644 --- a/guides/source/layouts_and_rendering.md +++ b/guides/source/layouts_and_rendering.md @@ -1210,7 +1210,7 @@ Partials are very useful in rendering collections. When you pass a collection to When a partial is called with a pluralized collection, then the individual instances of the partial have access to the member of the collection being rendered via a variable named after the partial. In this case, the partial is `_product`, and within the `_product` partial, you can refer to `product` to get the instance that is being rendered. -There is also a shorthand for this. Assuming `@products` is a collection of `product` instances, you can simply write this in the `index.html.erb` to produce the same result: +There is also a shorthand for this. Assuming `@products` is a collection of `Product` instances, you can simply write this in the `index.html.erb` to produce the same result: ```html+erb <h1>Products</h1> diff --git a/railties/lib/rails/commands/server/server_command.rb b/railties/lib/rails/commands/server/server_command.rb index 77b6c1f65d..5593840c71 100644 --- a/railties/lib/rails/commands/server/server_command.rb +++ b/railties/lib/rails/commands/server/server_command.rb @@ -293,10 +293,10 @@ module Rails Run `rails server --help` for more options. MSG else - suggestions = Rails::Command::Spellchecker.suggest(server, from: RACK_SERVERS) + suggestion = Rails::Command::Spellchecker.suggest(server, from: RACK_SERVERS) <<~MSG - Could not find server "#{server}". Maybe you meant #{suggestions.inspect}? + Could not find server "#{server}". Maybe you meant #{suggestion.inspect}? Run `rails server --help` for more options. MSG end diff --git a/railties/test/generators_test.rb b/railties/test/generators_test.rb index a16a2d3f0a..f98c1f78f7 100644 --- a/railties/test/generators_test.rb +++ b/railties/test/generators_test.rb @@ -49,12 +49,6 @@ class GeneratorsTest < Rails::Generators::TestCase I18n.default_locale = orig_default_locale end - def test_generator_multiple_suggestions - name = :tas - output = capture(:stdout) { Rails::Generators.invoke name } - assert_match 'Maybe you meant "task"?', output - end - def test_help_when_a_generator_with_required_arguments_is_invoked_without_arguments output = capture(:stdout) { Rails::Generators.invoke :model, [] } assert_match(/Description:/, output) |