From 83ca4a18f726e7860fde469372888fe5a122dc21 Mon Sep 17 00:00:00 2001 From: st0012 Date: Wed, 24 Apr 2019 19:55:16 +0800 Subject: [PATCH 01/37] Add a section to introduce `pluck`'s eager loading behavior --- guides/source/active_record_querying.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/guides/source/active_record_querying.md b/guides/source/active_record_querying.md index e40f16e62d..5fb030fad4 100644 --- a/guides/source/active_record_querying.md +++ b/guides/source/active_record_querying.md @@ -1821,6 +1821,21 @@ Client.limit(1).pluck(:name) # => ["David"] ``` +NOTE: You should also know that using `pluck` will trigger eager loading if the relation object contains include values, even if the eager loading is not necessary for the query. For example: + +```ruby +# store association for reusing it +assoc = Company.includes(:account) +assoc.pluck(:id) +# SELECT "companies"."id" FROM "companies" LEFT OUTER JOIN "accounts" ON "accounts"."id" = "companies"."account_id" +``` + +One way to avoid this is to `unscope` the includes: + +```ruby +assoc.unscope(:includes).pluck(:id) +``` + ### `ids` `ids` can be used to pluck all the IDs for the relation using the table's primary key. From be34af0c531c840f1f8a36e72f8d315a0ca74f3d Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 21 May 2019 19:39:44 -0700 Subject: [PATCH 02/37] Wrap actionview cache expiry in a mutex --- actionview/lib/action_view/cache_expiry.rb | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/actionview/lib/action_view/cache_expiry.rb b/actionview/lib/action_view/cache_expiry.rb index 820afc093d..3d8ffeaefb 100644 --- a/actionview/lib/action_view/cache_expiry.rb +++ b/actionview/lib/action_view/cache_expiry.rb @@ -16,18 +16,21 @@ module ActionView @watched_dirs = nil @watcher_class = watcher @watcher = nil + @mutex = Mutex.new end def clear_cache_if_necessary - watched_dirs = dirs_to_watch - if watched_dirs != @watched_dirs - @watched_dirs = watched_dirs - @watcher = @watcher_class.new([], watched_dirs) do - clear_cache + @mutex.synchronize do + watched_dirs = dirs_to_watch + if watched_dirs != @watched_dirs + @watched_dirs = watched_dirs + @watcher = @watcher_class.new([], watched_dirs) do + clear_cache + end + @watcher.execute + else + @watcher.execute_if_updated end - @watcher.execute - else - @watcher.execute_if_updated end end From a4229a534ff237443b445de9aec0310c0f388b56 Mon Sep 17 00:00:00 2001 From: Younes SERRAJ Date: Wed, 22 May 2019 10:21:59 +0200 Subject: [PATCH 03/37] Fix select_tag so that is doesn't change options when include_blank is set --- actionview/CHANGELOG.md | 3 +++ actionview/lib/action_view/helpers/form_tag_helper.rb | 3 ++- actionview/test/template/form_tag_helper_test.rb | 7 +++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index dcd3e33c46..a1e94384bd 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,3 +1,6 @@ +* Fix `select_tag` so that it doesn't change `options` when `include_blank` is present. + + *Younes SERRAJ* Please check [6-0-stable](https://github.com/rails/rails/blob/6-0-stable/actionview/CHANGELOG.md) for previous changes. diff --git a/actionview/lib/action_view/helpers/form_tag_helper.rb b/actionview/lib/action_view/helpers/form_tag_helper.rb index 5d4ff36425..c93ead9653 100644 --- a/actionview/lib/action_view/helpers/form_tag_helper.rb +++ b/actionview/lib/action_view/helpers/form_tag_helper.rb @@ -137,7 +137,8 @@ module ActionView html_name = (options[:multiple] == true && !name.to_s.ends_with?("[]")) ? "#{name}[]" : name if options.include?(:include_blank) - include_blank = options.delete(:include_blank) + include_blank = options[:include_blank] + options = options.except(:include_blank) options_for_blank_options_tag = { value: "" } if include_blank == true diff --git a/actionview/test/template/form_tag_helper_test.rb b/actionview/test/template/form_tag_helper_test.rb index 9ece9f3ad1..70c5ae6771 100644 --- a/actionview/test/template/form_tag_helper_test.rb +++ b/actionview/test/template/form_tag_helper_test.rb @@ -301,6 +301,13 @@ class FormTagHelperTest < ActionView::TestCase assert_dom_equal expected, actual end + def test_select_tag_with_include_blank_doesnt_change_options + options = { include_blank: true, prompt: "string" } + expected_options = options.dup + select_tag "places", raw(""), options + expected_options.each { |k, v| assert_equal v, options[k] } + end + def test_select_tag_with_include_blank_false actual = select_tag "places", raw(""), include_blank: false expected = %() From f3bb3f1c4e2a879b56c7cd22dcd4ec41da17760b Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 22 May 2019 16:59:06 -0700 Subject: [PATCH 04/37] Delete evented_file_update_checker existing_parent This is no longer used as of caa3cc8868206f8109e0d633efb09d31e94ef635 --- .../lib/active_support/evented_file_update_checker.rb | 7 ------- activesupport/test/evented_file_update_checker_test.rb | 8 -------- 2 files changed, 15 deletions(-) diff --git a/activesupport/lib/active_support/evented_file_update_checker.rb b/activesupport/lib/active_support/evented_file_update_checker.rb index 3893b0de0e..3f6fd58f5e 100644 --- a/activesupport/lib/active_support/evented_file_update_checker.rb +++ b/activesupport/lib/active_support/evented_file_update_checker.rb @@ -187,13 +187,6 @@ module ActiveSupport lcsp end - # Returns the deepest existing ascendant, which could be the argument itself. - def existing_parent(dir) - dir.ascend do |ascendant| - break ascendant if ascendant.directory? - end - end - # Filters out directories which are descendants of others in the collection (stable). def filter_out_descendants(dirs) return dirs if dirs.length < 2 diff --git a/activesupport/test/evented_file_update_checker_test.rb b/activesupport/test/evented_file_update_checker_test.rb index b2d5eb94c2..a6bd2ade31 100644 --- a/activesupport/test/evented_file_update_checker_test.rb +++ b/activesupport/test/evented_file_update_checker_test.rb @@ -156,14 +156,6 @@ class EventedFileUpdateCheckerPathHelperTest < ActiveSupport::TestCase assert_nil @ph.longest_common_subpath([]) end - test "#existing_parent returns the most specific existing ascendant" do - wd = Pathname.getwd - - assert_equal wd, @ph.existing_parent(wd) - assert_equal wd, @ph.existing_parent(wd.join("non-existing/directory")) - assert_equal pn("/"), @ph.existing_parent(pn("/non-existing/directory")) - end - test "#filter_out_descendants returns the same collection if there are no descendants (empty)" do assert_equal [], @ph.filter_out_descendants([]) end From f1084f2d0eaf8405284b2da19e8ac2c2eb7063ad Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 22 May 2019 16:58:01 -0700 Subject: [PATCH 05/37] Use existing tmpdir in evented_file_update_test The common include of this test creates a tmpdir, we should use that for consistency. --- .../test/evented_file_update_checker_test.rb | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/activesupport/test/evented_file_update_checker_test.rb b/activesupport/test/evented_file_update_checker_test.rb index b2d5eb94c2..ce09fa1586 100644 --- a/activesupport/test/evented_file_update_checker_test.rb +++ b/activesupport/test/evented_file_update_checker_test.rb @@ -78,31 +78,29 @@ class EventedFileUpdateCheckerTest < ActiveSupport::TestCase end test "updated should become true when nonexistent directory is added later" do - Dir.mktmpdir do |dir| - watched_dir = File.join(dir, "app") - unwatched_dir = File.join(dir, "node_modules") - not_exist_watched_dir = File.join(dir, "test") + watched_dir = File.join(tmpdir, "app") + unwatched_dir = File.join(tmpdir, "node_modules") + not_exist_watched_dir = File.join(tmpdir, "test") - Dir.mkdir(watched_dir) - Dir.mkdir(unwatched_dir) + Dir.mkdir(watched_dir) + Dir.mkdir(unwatched_dir) - checker = new_checker([], watched_dir => ".rb", not_exist_watched_dir => ".rb") { } + checker = new_checker([], watched_dir => ".rb", not_exist_watched_dir => ".rb") { } - FileUtils.touch(File.join(watched_dir, "a.rb")) - wait - assert_predicate checker, :updated? - assert checker.execute_if_updated + FileUtils.touch(File.join(watched_dir, "a.rb")) + wait + assert_predicate checker, :updated? + assert checker.execute_if_updated - Dir.mkdir(not_exist_watched_dir) - wait - assert_predicate checker, :updated? - assert checker.execute_if_updated + Dir.mkdir(not_exist_watched_dir) + wait + assert_predicate checker, :updated? + assert checker.execute_if_updated - FileUtils.touch(File.join(unwatched_dir, "a.rb")) - wait - assert_not_predicate checker, :updated? - assert_not checker.execute_if_updated - end + FileUtils.touch(File.join(unwatched_dir, "a.rb")) + wait + assert_not_predicate checker, :updated? + assert_not checker.execute_if_updated end end From 2fd6c3799dd04dcae0948acd25c40dded1459756 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 22 May 2019 16:42:34 -0700 Subject: [PATCH 06/37] Fix EventedFileUpdateChecker through a symlink On MacOS, Dir.tmpdir gives me a folder inside "/var/folders/". However, /var is a symlink to /private/var. Previously, the nonexistent directory test would fail because it was initialized with /var/folders/... but the filenames from listen would be the realpaths. This commit normalizes the dirs by calling realpath on them if they exist. This is done on boot!, so it will work with newly directories through the symlink. --- .../evented_file_update_checker.rb | 7 +++++++ .../test/evented_file_update_checker_test.rb | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/activesupport/lib/active_support/evented_file_update_checker.rb b/activesupport/lib/active_support/evented_file_update_checker.rb index 3893b0de0e..15b84537cc 100644 --- a/activesupport/lib/active_support/evented_file_update_checker.rb +++ b/activesupport/lib/active_support/evented_file_update_checker.rb @@ -107,6 +107,7 @@ module ActiveSupport private def boot! + normalize_dirs! Listen.to(*@dtw, &method(:changed)).start end @@ -114,6 +115,12 @@ module ActiveSupport Listen.stop end + def normalize_dirs! + @dirs.transform_keys! do |dir| + dir.exist? ? dir.realpath : dir + end + end + def changed(modified, added, removed) unless updated? @updated.make_true if (modified + added + removed).any? { |f| watching?(f) } diff --git a/activesupport/test/evented_file_update_checker_test.rb b/activesupport/test/evented_file_update_checker_test.rb index ce09fa1586..bec2643b45 100644 --- a/activesupport/test/evented_file_update_checker_test.rb +++ b/activesupport/test/evented_file_update_checker_test.rb @@ -77,6 +77,24 @@ class EventedFileUpdateCheckerTest < ActiveSupport::TestCase Process.wait(pid) end + test "should detect changes through symlink" do + actual_dir = File.join(tmpdir, "actual") + linked_dir = File.join(tmpdir, "linked") + + Dir.mkdir(actual_dir) + FileUtils.ln_s(actual_dir, linked_dir) + + checker = new_checker([], linked_dir => ".rb") { } + + assert_not_predicate checker, :updated? + + FileUtils.touch(File.join(actual_dir, "a.rb")) + wait + + assert_predicate checker, :updated? + assert checker.execute_if_updated + end + test "updated should become true when nonexistent directory is added later" do watched_dir = File.join(tmpdir, "app") unwatched_dir = File.join(tmpdir, "node_modules") From 5c14eb0b4ea3bde067bc8fce7ed99f5f10ab16ff Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Fri, 24 May 2019 22:54:20 +0200 Subject: [PATCH 07/37] Address 639d7be. Readd changelog line; remove needless explicit return. --- activesupport/CHANGELOG.md | 1 + activesupport/lib/active_support/core_ext/module/delegation.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 3d1dd41085..955eb7eef9 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -25,4 +25,5 @@ *Jordan Thomas* + Please check [6-0-stable](https://github.com/rails/rails/blob/6-0-stable/activesupport/CHANGELOG.md) for previous changes. diff --git a/activesupport/lib/active_support/core_ext/module/delegation.rb b/activesupport/lib/active_support/core_ext/module/delegation.rb index 2f88010d27..b8996ecb10 100644 --- a/activesupport/lib/active_support/core_ext/module/delegation.rb +++ b/activesupport/lib/active_support/core_ext/module/delegation.rb @@ -297,7 +297,7 @@ class Module rescue NoMethodError if #{target}.nil? if #{allow_nil == true} - return nil + nil else raise DelegationError, "\#{method} delegated to #{target}, but #{target} is nil" end From 2c8e2b71cd00f17ef8273f4389331ddce89da0f9 Mon Sep 17 00:00:00 2001 From: Edu Depetris Date: Sat, 25 May 2019 12:41:52 -0300 Subject: [PATCH 08/37] Change comments --- .../lib/active_support/core_ext/hash/deep_transform_values.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/activesupport/lib/active_support/core_ext/hash/deep_transform_values.rb b/activesupport/lib/active_support/core_ext/hash/deep_transform_values.rb index 720a1f67c8..18acb1e70c 100644 --- a/activesupport/lib/active_support/core_ext/hash/deep_transform_values.rb +++ b/activesupport/lib/active_support/core_ext/hash/deep_transform_values.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true class Hash - # Returns a new hash with all keys converted by the block operation. - # This includes the keys from the root hash and from all + # Returns a new hash with all values converted by the block operation. + # This includes the values from the root hash and from all # nested hashes and arrays. # # hash = { person: { name: 'Rob', age: '28' } } From a1c269fbdf1f10aa038c8b58e6ba48e9f5f3f864 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Sun, 26 May 2019 23:02:24 +0200 Subject: [PATCH 09/37] Skip needless spaces from generated app layout. --- .../app/templates/app/views/layouts/application.html.erb.tt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railties/lib/rails/generators/rails/app/templates/app/views/layouts/application.html.erb.tt b/railties/lib/rails/generators/rails/app/templates/app/views/layouts/application.html.erb.tt index 9a7267c783..b8c1f21c0b 100644 --- a/railties/lib/rails/generators/rails/app/templates/app/views/layouts/application.html.erb.tt +++ b/railties/lib/rails/generators/rails/app/templates/app/views/layouts/application.html.erb.tt @@ -6,7 +6,7 @@ <%%= csp_meta_tag %> <%- if options[:skip_javascript] -%> - <%%= stylesheet_link_tag 'application', media: 'all' %> + <%%= stylesheet_link_tag 'application', media: 'all' %> <%- else -%> <%- unless options[:skip_turbolinks] -%> <%%= stylesheet_link_tag 'application', media: 'all', 'data-turbolinks-track': 'reload' %> From b55f5a3ed9134ed86993fcda3ea9b6fb2e97f09e Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 27 May 2019 06:41:31 +0900 Subject: [PATCH 10/37] Extract `readonly_attribute?` --- activerecord/lib/active_record/attribute_methods.rb | 6 +----- activerecord/lib/active_record/persistence.rb | 2 +- activerecord/lib/active_record/readonly_attributes.rb | 4 ++++ 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index 220043c061..6e4f76aa73 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -437,7 +437,7 @@ module ActiveRecord def attributes_for_update(attribute_names) attribute_names &= self.class.column_names attribute_names.delete_if do |name| - readonly_attribute?(name) + self.class.readonly_attribute?(name) end end @@ -460,10 +460,6 @@ module ActiveRecord end end - def readonly_attribute?(name) - self.class.readonly_attributes.include?(name) - end - def pk_attribute?(name) name == @primary_key end diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index a58c5fd48a..b7bc4b3b8e 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -939,7 +939,7 @@ module ActiveRecord end def verify_readonly_attribute(name) - raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name) + raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attribute?(name) end def _raise_record_not_destroyed diff --git a/activerecord/lib/active_record/readonly_attributes.rb b/activerecord/lib/active_record/readonly_attributes.rb index 7bc26993d5..c851ed52c3 100644 --- a/activerecord/lib/active_record/readonly_attributes.rb +++ b/activerecord/lib/active_record/readonly_attributes.rb @@ -19,6 +19,10 @@ module ActiveRecord def readonly_attributes _attr_readonly end + + def readonly_attribute?(name) # :nodoc: + _attr_readonly.include?(name) + end end end end From 6302e56d6c1fec048f6438d9f1ac6a8cfaed7eb9 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Sun, 26 May 2019 13:26:19 -0700 Subject: [PATCH 11/37] Use WeakRef to avoid leaking connection pools Prior to 3e2e8eeb9ea552bd4782538cf9348455f3d0e14a the Reaper thread would hold a reference to connection pools indefinitely, preventing the connection pool from being garbage collected, and also leaking the Thread. Since 3e2e8eeb9ea552bd4782538cf9348455f3d0e14a, there is only one Reaper Thread for all pools, however all pools are still stored in a class variable, preventing them from being garbage collected. This commit instead holds reference to the pools through a WeakRef. This way, connection pools referenced elsewhere will be reaped, any others will be able to be garbage collected. I don't love resorting to WeakRef to solve this, but I believe it's the simplest way to accomplish the the desired behaviour. --- .../abstract/connection_pool.rb | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index 4ff3cb0071..7628ef5537 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -3,6 +3,7 @@ require "thread" require "concurrent/map" require "monitor" +require "weakref" module ActiveRecord # Raised when a connection could not be obtained within the connection @@ -294,28 +295,37 @@ module ActiveRecord @frequency = frequency end - @@mutex = Mutex.new - @@pools = {} + @mutex = Mutex.new + @pools = {} - def self.register_pool(pool, frequency) # :nodoc: - @@mutex.synchronize do - if @@pools.key?(frequency) - @@pools[frequency] << pool - else - @@pools[frequency] = [pool] + class << self + def register_pool(pool, frequency) # :nodoc: + @mutex.synchronize do + unless @pools.key?(frequency) + @pools[frequency] = [] + spawn_thread(frequency) + end + @pools[frequency] << WeakRef.new(pool) + end + end + + private + + def spawn_thread(frequency) Thread.new(frequency) do |t| loop do sleep t - @@mutex.synchronize do - @@pools[frequency].each do |p| + @mutex.synchronize do + @pools[frequency].select!(&:weakref_alive?) + @pools[frequency].each do |p| p.reap p.flush + rescue WeakRef::RefError end end end end end - end end def run From f09ca65e0f298b39feecf99347deaf2acd2acc10 Mon Sep 17 00:00:00 2001 From: Akira Matsuda Date: Tue, 28 May 2019 05:38:07 +0900 Subject: [PATCH 12/37] ruby < 2.5 is no longer supported --- .../test/core_ext/object/duplicable_test.rb | 9 ++------- activesupport/test/dependencies_test.rb | 17 +++++++---------- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/activesupport/test/core_ext/object/duplicable_test.rb b/activesupport/test/core_ext/object/duplicable_test.rb index 5203434ae6..c9af2cb624 100644 --- a/activesupport/test/core_ext/object/duplicable_test.rb +++ b/activesupport/test/core_ext/object/duplicable_test.rb @@ -6,13 +6,8 @@ require "active_support/core_ext/object/duplicable" require "active_support/core_ext/numeric/time" class DuplicableTest < ActiveSupport::TestCase - if RUBY_VERSION >= "2.5.0" - RAISE_DUP = [method(:puts)] - ALLOW_DUP = ["1", "symbol_from_string".to_sym, Object.new, /foo/, [], {}, Time.now, Class.new, Module.new, BigDecimal("4.56"), nil, false, true, 1, 2.3, Complex(1), Rational(1)] - else - RAISE_DUP = [method(:puts), Complex(1), Rational(1)] - ALLOW_DUP = ["1", "symbol_from_string".to_sym, Object.new, /foo/, [], {}, Time.now, Class.new, Module.new, BigDecimal("4.56"), nil, false, true, 1, 2.3] - end + RAISE_DUP = [method(:puts)] + ALLOW_DUP = ["1", "symbol_from_string".to_sym, Object.new, /foo/, [], {}, Time.now, Class.new, Module.new, BigDecimal("4.56"), nil, false, true, 1, 2.3, Complex(1), Rational(1)] def test_duplicable rubinius_skip "* Method#dup is allowed at the moment on Rubinius\n" \ diff --git a/activesupport/test/dependencies_test.rb b/activesupport/test/dependencies_test.rb index d4e709137e..003a0dbccb 100644 --- a/activesupport/test/dependencies_test.rb +++ b/activesupport/test/dependencies_test.rb @@ -481,17 +481,14 @@ class DependenciesTest < ActiveSupport::TestCase end end - # This raises only on 2.5.. (warns on ..2.4) - if RUBY_VERSION > "2.5" - def test_access_thru_and_upwards_fails - with_autoloading_fixtures do - assert_not defined?(ModuleFolder) - assert_raise(NameError) { ModuleFolder::Object } - assert_raise(NameError) { ModuleFolder::NestedClass::Object } - end - ensure - remove_constants(:ModuleFolder) + def test_access_thru_and_upwards_fails + with_autoloading_fixtures do + assert_not defined?(ModuleFolder) + assert_raise(NameError) { ModuleFolder::Object } + assert_raise(NameError) { ModuleFolder::NestedClass::Object } end + ensure + remove_constants(:ModuleFolder) end def test_non_existing_const_raises_name_error_with_fully_qualified_name From 2e3d374b53da24ef8c8b72ac1ff727e7eff69cf9 Mon Sep 17 00:00:00 2001 From: Corprew Reed Date: Mon, 27 May 2019 18:43:41 -0700 Subject: [PATCH 13/37] changes 'Week day' to 'day of week' 'Week day' has a specific meaning in English -- see https://en.wiktionary.org/wiki/weekday for details -- that is not meant here. 'Day of week' is more appropriate. [ci skip] --- guides/source/configuring.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 0b54683b5a..cc64c7eac6 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -69,7 +69,7 @@ These configuration methods are to be called on a `Rails::Railtie` object, such * `config.cache_classes` controls whether or not application classes and modules should be reloaded on each request. Defaults to `false` in development mode, and `true` in test and production modes. * `config.beginning_of_week` sets the default beginning of week for the -application. Accepts a valid week day symbol (e.g. `:monday`). +application. Accepts a valid day of week as a symbol (e.g. `:monday`). * `config.cache_store` configures which cache store to use for Rails caching. Options include one of the symbols `:memory_store`, `:file_store`, `:mem_cache_store`, `:null_store`, `:redis_cache_store`, or an object that implements the cache API. Defaults to `:file_store`. From 430d931738f15fc621e9461df56683d54de9b843 Mon Sep 17 00:00:00 2001 From: yaojie Date: Tue, 28 May 2019 12:28:19 +0800 Subject: [PATCH 14/37] remove unused requires from debug_exceptions --- actionpack/lib/action_dispatch/middleware/debug_exceptions.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb index 0b15c94122..59113e13f4 100644 --- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb @@ -4,8 +4,6 @@ require "action_dispatch/http/request" require "action_dispatch/middleware/exception_wrapper" require "action_dispatch/routing/inspector" -require "active_support/actionable_error" - require "action_view" require "action_view/base" From 82ee7a0fa93e6f9ec69804734c8981217587dae5 Mon Sep 17 00:00:00 2001 From: Corprew Reed Date: Mon, 27 May 2019 23:44:53 -0700 Subject: [PATCH 15/37] stringify_keys and symbolize_keys have stable results. Rails 6 uses the `Hash.transform_keys` found in Ruby 2.5 and later, and that method enumerates keys based on insertion order. Calling `symbolize_keys`, `stringify_keys`, and their bang variants will result in the same hash every time -- the value for any key where a collision occurs is the last assigned in that enumeration In the docs for Hash -- https://ruby-doc.org/core-2.5.0/Hash.html > Hashes enumerate their values in the order that the corresponding keys were inserted. --- guides/source/active_support_core_extensions.md | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/guides/source/active_support_core_extensions.md b/guides/source/active_support_core_extensions.md index 1a057832d4..d46f8fb74d 100644 --- a/guides/source/active_support_core_extensions.md +++ b/guides/source/active_support_core_extensions.md @@ -2633,14 +2633,12 @@ The method `stringify_keys` returns a hash that has a stringified version of the # => {"" => nil, "1" => 1, "a" => :a} ``` -In case of key collision, one of the values will be chosen. The chosen value may not always be the same given the same hash: +In case of key collision, the value will be the one most recently inserted into the hash. ```ruby {"a" => 1, a: 2}.stringify_keys -# The result could either be +# The result will be # => {"a"=>2} -# or -# => {"a"=>1} ``` This method may be useful for example to easily accept both symbols and strings as options. For instance `ActionView::Helpers::FormHelper` defines: @@ -2677,14 +2675,12 @@ The method `symbolize_keys` returns a hash that has a symbolized version of the WARNING. Note in the previous example only one key was symbolized. -In case of key collision, one of the values will be chosen. The chosen value may not always be the same given the same hash: +In case of key collision, the value will be the one most recently inserted into the hash. ```ruby {"a" => 1, a: 2}.symbolize_keys -# The result could either be +# The result will be # => {:a=>2} -# or -# => {:a=>1} ``` This method may be useful for example to easily accept both symbols and strings as options. For instance `ActionController::UrlRewriter` defines From bc837892e6b17ed9e8aa58c6de539af8fa4f1526 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 27 May 2019 22:57:06 +0900 Subject: [PATCH 16/37] Allow symbol (i.e. quoted identifier) as safe SQL string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pluck(:id)` / `order(:id)` are very common use case, and passed symbol (i.e. quoted identifier) is obviously safe argument, but `:id.to_s.split(/\s*,\s*/).all? { |part| permit.match?(part) }` is useless and a bit expensive operation for each such safe symbols (will make extra 2 mutable strings, 1 array, 1 proc). This avoids the expensive operation to such safe symbols, it makes `pluck(:id)` / `order(:id)` itself about 9% faster. https://gist.github.com/kamipo/11d428b57f3629a72ae89c6f21721326 Before (93e640735e9363672b770b8d1c5a35f9e464f806): ``` Warming up -------------------------------------- users.pluck(:id) 1.217k i/100ms users.order(:id).to_sql 1.848k i/100ms Calculating ------------------------------------- users.pluck(:id) 12.239k (± 8.2%) i/s - 60.850k in 5.013839s users.order(:id).to_sql 19.111k (± 7.5%) i/s - 96.096k in 5.064450s ``` After (this change): ``` Warming up -------------------------------------- users.pluck(:id) 1.293k i/100ms users.order(:id).to_sql 2.036k i/100ms Calculating ------------------------------------- users.pluck(:id) 13.257k (± 6.9%) i/s - 65.943k in 5.002568s users.order(:id).to_sql 20.957k (± 7.6%) i/s - 105.872k in 5.086102s ``` --- activerecord/lib/active_record/attribute_methods.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index 6e4f76aa73..fd32eaaf3a 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -185,12 +185,14 @@ module ActiveRecord /ix def disallow_raw_sql!(args, permit: COLUMN_NAME) # :nodoc: - unexpected = args.reject do |arg| - Arel.arel_node?(arg) || + unexpected = nil + args.each do |arg| + next if arg.is_a?(Symbol) || Arel.arel_node?(arg) || arg.to_s.split(/\s*,\s*/).all? { |part| permit.match?(part) } + (unexpected ||= []) << arg end - return if unexpected.none? + return unless unexpected if allow_unsafe_raw_sql == :deprecated ActiveSupport::Deprecation.warn( From 497e52f8c2d9d5bda485df5d1ff396935d23e43f Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Tue, 28 May 2019 22:46:51 +0900 Subject: [PATCH 17/37] Don't round off subseconds unless necessary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, if `:datetime` type has a precision, type casting always does round off subseconds regardless of whether necessary or not, it is a bit slow. Since #34970, `t.timestamps` with `precision: 6` by default, so `pluck(:created_at)` in newly created app will always be affected by the round off. This avoids the round off if possible, it makes `pluck(:created_at)` about 25% faster. https://gist.github.com/kamipo/e029539f2632aee9f5e711fe66fc8842 Before (0a87d7c9ddb95cf7568baf889ff4091469ba9af4 with postgresql adapter): ``` Warming up -------------------------------------- users.pluck(:id) 344.000 i/100ms users.pluck(:name) 336.000 i/100ms users.pluck(:created_at) 206.000 i/100ms Calculating ------------------------------------- users.pluck(:id) 3.620k (± 8.5%) i/s - 18.232k in 5.077316s users.pluck(:name) 3.579k (± 9.4%) i/s - 17.808k in 5.020230s users.pluck(:created_at) 2.069k (± 8.0%) i/s - 10.300k in 5.019284s ``` Before (0a87d7c9ddb95cf7568baf889ff4091469ba9af4 with mysql2 adapter): ``` Warming up -------------------------------------- users.pluck(:id) 245.000 i/100ms users.pluck(:name) 240.000 i/100ms users.pluck(:created_at) 180.000 i/100ms Calculating ------------------------------------- users.pluck(:id) 2.548k (± 9.4%) i/s - 12.740k in 5.066574s users.pluck(:name) 2.513k (± 8.0%) i/s - 12.480k in 5.011260s users.pluck(:created_at) 1.771k (±11.2%) i/s - 8.820k in 5.084473s ``` After (this change with postgresql adapter): ``` Warming up -------------------------------------- users.pluck(:id) 348.000 i/100ms users.pluck(:name) 357.000 i/100ms users.pluck(:created_at) 254.000 i/100ms Calculating ------------------------------------- users.pluck(:id) 3.628k (± 8.2%) i/s - 18.096k in 5.024748s users.pluck(:name) 3.624k (±12.4%) i/s - 17.850k in 5.020959s users.pluck(:created_at) 2.567k (± 7.0%) i/s - 12.954k in 5.081153s ``` After (this change with mysql2 adapter): ``` Warming up -------------------------------------- users.pluck(:id) 268.000 i/100ms users.pluck(:name) 265.000 i/100ms users.pluck(:created_at) 207.000 i/100ms Calculating ------------------------------------- users.pluck(:id) 2.586k (±10.9%) i/s - 12.864k in 5.050546s users.pluck(:name) 2.543k (±10.2%) i/s - 12.720k in 5.067726s users.pluck(:created_at) 2.263k (±14.5%) i/s - 10.971k in 5.004039s ``` --- .../lib/active_model/type/helpers/time_value.rb | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/activemodel/lib/active_model/type/helpers/time_value.rb b/activemodel/lib/active_model/type/helpers/time_value.rb index 735b9a75a6..dab196d653 100644 --- a/activemodel/lib/active_model/type/helpers/time_value.rb +++ b/activemodel/lib/active_model/type/helpers/time_value.rb @@ -22,10 +22,17 @@ module ActiveModel end def apply_seconds_precision(value) - return value unless precision && value.respond_to?(:usec) - number_of_insignificant_digits = 6 - precision + return value unless precision && value.respond_to?(:nsec) + + number_of_insignificant_digits = 9 - precision round_power = 10**number_of_insignificant_digits - value.change(usec: value.usec - value.usec % round_power) + rounded_off_nsec = value.nsec % round_power + + if rounded_off_nsec > 0 + value.change(nsec: value.nsec - rounded_off_nsec) + else + value + end end def type_cast_for_schema(value) From 17afd4b982f32761f1725b07cda37b7db03dbb33 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Tue, 28 May 2019 23:52:18 +0900 Subject: [PATCH 18/37] `:datetime` and `:time` columns allows `:precision` option [ci skip] --- .../connection_adapters/abstract/schema_statements.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index f97842b3f5..107d1ce855 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -518,14 +518,15 @@ module ActiveRecord # Available options are (none of these exists by default): # * :limit - # Requests a maximum column length. This is the number of characters for a :string column - # and number of bytes for :text, :binary and :integer columns. + # and number of bytes for :text, :binary, and :integer columns. # This option is ignored by some backends. # * :default - # The column's default value. Use +nil+ for +NULL+. # * :null - # Allows or disallows +NULL+ values in the column. # * :precision - - # Specifies the precision for the :decimal and :numeric columns. + # Specifies the precision for the :decimal, :numeric, + # :datetime, and :time columns. # * :scale - # Specifies the scale for the :decimal and :numeric columns. # * :collation - From 2f96cfbb546dd6fdd3073f1895d4feb3aff20fb6 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 29 May 2019 00:26:58 +0900 Subject: [PATCH 19/37] Address intermittent CI failure due to unfilled schema columns cache https://buildkite.com/rails/rails/builds/61358#a78ee50e-30b5-48a2-858f-63eba287d919/1290-1298 --- .../has_and_belongs_to_many_associations_test.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb index 0e9dafeee6..25cfa0a723 100644 --- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -550,7 +550,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase developer = project.developers.first - assert_no_queries do + assert_queries(0) do assert_predicate project.developers, :loaded? assert_includes project.developers, developer end @@ -745,7 +745,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase def test_get_ids_for_loaded_associations developer = developers(:david) developer.projects.reload - assert_no_queries do + assert_queries(0) do developer.project_ids developer.project_ids end @@ -873,7 +873,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase def test_has_and_belongs_to_many_associations_on_new_records_use_null_relations projects = Developer.new.projects - assert_no_queries do + assert_queries(0) do assert_equal [], projects assert_equal [], projects.where(title: "omg") assert_equal [], projects.pluck(:title) From 9dce7d9268a30d8f7d94567f749fa8b63bee093e Mon Sep 17 00:00:00 2001 From: Petrik Date: Tue, 28 May 2019 18:12:25 +0200 Subject: [PATCH 20/37] Fix comment for "broken" inverse_of associations [ci skip] --- activerecord/test/models/face.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/test/models/face.rb b/activerecord/test/models/face.rb index e900fd40fb..45ccc442ba 100644 --- a/activerecord/test/models/face.rb +++ b/activerecord/test/models/face.rb @@ -6,7 +6,7 @@ class Face < ActiveRecord::Base belongs_to :polymorphic_man, polymorphic: true, inverse_of: :polymorphic_face # Oracle identifier length is limited to 30 bytes or less, `polymorphic` renamed `poly` belongs_to :poly_man_without_inverse, polymorphic: true - # These is a "broken" inverse_of for the purposes of testing + # These are "broken" inverse_of associations for the purposes of testing belongs_to :horrible_man, class_name: "Man", inverse_of: :horrible_face belongs_to :horrible_polymorphic_man, polymorphic: true, inverse_of: :horrible_polymorphic_face From f47be7bd96827af9f0b9b165666c6626cefc008f Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 29 May 2019 03:49:23 +0900 Subject: [PATCH 21/37] Address intermittent CI failure in cascaded_eager_loading_test.rb https://buildkite.com/rails/rails/builds/61362#99165d42-172d-4ad5-bf72-b29d8cd44f3e/995-1006 --- .../cascaded_eager_loading_test.rb | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb index 49f754be63..cbe48a374f 100644 --- a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb +++ b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb @@ -37,8 +37,8 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase def test_eager_association_loading_with_hmt_does_not_table_name_collide_when_joining_associations authors = Author.joins(:posts).eager_load(:comments).where(posts: { tags_count: 1 }).order(:id).to_a - assert_equal 3, assert_no_queries { authors.size } - assert_equal 10, assert_no_queries { authors[0].comments.size } + assert_equal 3, assert_queries(0) { authors.size } + assert_equal 10, assert_queries(0) { authors[0].comments.size } end def test_eager_association_loading_grafts_stashed_associations_to_correct_parent @@ -103,14 +103,14 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase firms = Firm.all.merge!(includes: { account: { firm: :account } }, order: "companies.id").to_a assert_equal 2, firms.size assert_equal firms.first.account, firms.first.account.firm.account - assert_equal companies(:first_firm).account, assert_no_queries { firms.first.account.firm.account } - assert_equal companies(:first_firm).account.firm.account, assert_no_queries { firms.first.account.firm.account } + assert_equal companies(:first_firm).account, assert_queries(0) { firms.first.account.firm.account } + assert_equal companies(:first_firm).account.firm.account, assert_queries(0) { firms.first.account.firm.account } end def test_eager_association_loading_with_has_many_sti topics = Topic.all.merge!(includes: :replies, order: "topics.id").to_a first, second, = topics(:first).replies.size, topics(:second).replies.size - assert_no_queries do + assert_queries(0) do assert_equal first, topics[0].replies.size assert_equal second, topics[1].replies.size end @@ -131,13 +131,13 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase replies = Reply.all.merge!(includes: :topic, order: "topics.id").to_a assert_includes replies, topics(:second) assert_not_includes replies, topics(:first) - assert_equal topics(:first), assert_no_queries { replies.first.topic } + assert_equal topics(:first), assert_queries(0) { replies.first.topic } end def test_eager_association_loading_with_multiple_stis_and_order author = Author.all.merge!(includes: { posts: [ :special_comments, :very_special_comment ] }, order: ["authors.name", "comments.body", "very_special_comments_posts.body"], where: "posts.id = 4").first assert_equal authors(:david), author - assert_no_queries do + assert_queries(0) do author.posts.first.special_comments author.posts.first.very_special_comment end @@ -146,7 +146,7 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase def test_eager_association_loading_of_stis_with_multiple_references authors = Author.all.merge!(includes: { posts: { special_comments: { post: [ :special_comments, :very_special_comment ] } } }, order: "comments.body, very_special_comments_posts.body", where: "posts.id = 4").to_a assert_equal [authors(:david)], authors - assert_no_queries do + assert_queries(0) do authors.first.posts.first.special_comments.first.post.special_comments authors.first.posts.first.special_comments.first.post.very_special_comment end @@ -155,14 +155,14 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase def test_eager_association_loading_where_first_level_returns_nil authors = Author.all.merge!(includes: { post_about_thinking: :comments }, order: "authors.id DESC").to_a assert_equal [authors(:bob), authors(:mary), authors(:david)], authors - assert_no_queries do + assert_queries(0) do authors[2].post_about_thinking.comments.first end end def test_preload_through_missing_records post = Post.where.not(author_id: Author.select(:id)).preload(author: { comments: :post }).first! - assert_no_queries { assert_nil post.author } + assert_queries(0) { assert_nil post.author } end def test_eager_association_loading_with_missing_first_record @@ -172,12 +172,12 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase def test_eager_association_loading_with_recursive_cascading_four_levels_has_many_through source = Vertex.all.merge!(includes: { sinks: { sinks: { sinks: :sinks } } }, order: "vertices.id").first - assert_equal vertices(:vertex_4), assert_no_queries { source.sinks.first.sinks.first.sinks.first } + assert_equal vertices(:vertex_4), assert_queries(0) { source.sinks.first.sinks.first.sinks.first } end def test_eager_association_loading_with_recursive_cascading_four_levels_has_and_belongs_to_many sink = Vertex.all.merge!(includes: { sources: { sources: { sources: :sources } } }, order: "vertices.id DESC").first - assert_equal vertices(:vertex_1), assert_no_queries { sink.sources.first.sources.first.sources.first.sources.first } + assert_equal vertices(:vertex_1), assert_queries(0) { sink.sources.first.sources.first.sources.first.sources.first } end def test_eager_association_loading_with_cascaded_interdependent_one_level_and_two_levels From d380ffe216ce3b44d9678524bcabeb16379430df Mon Sep 17 00:00:00 2001 From: "yuuji.yaginuma" Date: Wed, 29 May 2019 07:57:05 +0900 Subject: [PATCH 22/37] Remove `frozen_string_literal` magic comment from template file The other template files do not add `frozen_string_literal`, so should behave the same. Ref: #30342, #30348. --- .../lib/rails/generators/test_unit/templates/mailbox_test.rb.tt | 2 -- 1 file changed, 2 deletions(-) diff --git a/actionmailbox/lib/rails/generators/test_unit/templates/mailbox_test.rb.tt b/actionmailbox/lib/rails/generators/test_unit/templates/mailbox_test.rb.tt index 0b51f29fe4..3e215b4d0b 100644 --- a/actionmailbox/lib/rails/generators/test_unit/templates/mailbox_test.rb.tt +++ b/actionmailbox/lib/rails/generators/test_unit/templates/mailbox_test.rb.tt @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "test_helper" class <%= class_name %>MailboxTest < ActionMailbox::TestCase From 784664f85b684ec3749c36f6f57a355631920cab Mon Sep 17 00:00:00 2001 From: "yuuji.yaginuma" Date: Wed, 29 May 2019 11:46:32 +0900 Subject: [PATCH 23/37] Bring `after_bundle` back to API document [ci skip] At class level `:nodoc:` all elements are prevented. Instead, use `:stopdoc:` / `:startdoc:` to make `after_bundle` appear. --- railties/lib/rails/generators/rails/app/app_generator.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/railties/lib/rails/generators/rails/app/app_generator.rb b/railties/lib/rails/generators/rails/app/app_generator.rb index f2f46d6e25..7336e235f6 100644 --- a/railties/lib/rails/generators/rails/app/app_generator.rb +++ b/railties/lib/rails/generators/rails/app/app_generator.rb @@ -243,7 +243,10 @@ module Rails # can change in Ruby 1.8.7 when we FileUtils.cd. RAILS_DEV_PATH = File.expand_path("../../../../../..", __dir__) - class AppGenerator < AppBase # :nodoc: + class AppGenerator < AppBase + + # :stopdoc: + WEBPACKS = %w( react vue angular elm stimulus ) add_shared_options_for "application" @@ -492,6 +495,8 @@ module Rails "rails new #{arguments.map(&:usage).join(' ')} [options]" end + # :startdoc: + private # Define file as an alias to create_file for backwards compatibility. From f430ea90d71b79f57eb067279ddd6f0bb40905ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20R=C3=B6der?= Date: Wed, 29 May 2019 11:47:45 +0200 Subject: [PATCH 24/37] Remove wrong default value for `cache_versioning` in documentation of `cache_version` `ActiveRecord::Base.cache_versioning` it `true` by default since Rails 5.2 as stated correctly in the documentation for the `ActiveRecord::Base.cache_versioning` class attribute. Remove the wrong and duplicated documentation of the default value for `cache_versioning` from `cache_version`. --- activerecord/lib/active_record/integration.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/integration.rb b/activerecord/lib/active_record/integration.rb index 573a823dbc..4a97061731 100644 --- a/activerecord/lib/active_record/integration.rb +++ b/activerecord/lib/active_record/integration.rb @@ -93,7 +93,7 @@ module ActiveRecord # cache_version, but this method can be overwritten to return something else. # # Note, this method will return nil if ActiveRecord::Base.cache_versioning is set to - # +false+ (which it is by default until Rails 6.0). + # +false+. def cache_version return unless cache_versioning From f0445213d8f23c982d1c080a750b33d4855d46e4 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Thu, 30 May 2019 00:00:46 +0900 Subject: [PATCH 25/37] Address intermittent CI failure due to non-determined sort order https://buildkite.com/rails/rails/builds/61384#ad441461-87d8-4bdc-a71f-61921fe2df2e/993-1004 --- activerecord/test/cases/associations/eager_test.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 9ed25ca7c2..f7aad9d775 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -1245,7 +1245,7 @@ class EagerAssociationTest < ActiveRecord::TestCase Post.all.merge!(select: "posts.*, authors.name as author_name", includes: :comments, joins: :author, order: "posts.id").to_a end assert_equal "David", posts[0].author_name - assert_equal posts(:welcome).comments, assert_no_queries { posts[0].comments } + assert_equal posts(:welcome).comments.sort_by(&:id), assert_no_queries { posts[0].comments.sort_by(&:id) } end def test_eager_loading_with_conditions_on_join_model_preloads @@ -1257,8 +1257,8 @@ class EagerAssociationTest < ActiveRecord::TestCase end def test_preload_belongs_to_uses_exclusive_scope - people = Person.males.merge(includes: :primary_contact).to_a - assert_not_equal people.length, 0 + people = Person.males.includes(:primary_contact).to_a + assert_equal 2, people.length people.each do |person| assert_no_queries { assert_not_nil person.primary_contact } assert_equal Person.find(person.id).primary_contact, person.primary_contact @@ -1267,16 +1267,17 @@ class EagerAssociationTest < ActiveRecord::TestCase def test_preload_has_many_uses_exclusive_scope people = Person.males.includes(:agents).to_a + assert_equal 2, people.length people.each do |person| - assert_equal Person.find(person.id).agents, person.agents + assert_equal Person.find(person.id).agents.sort_by(&:id), person.agents.sort_by(&:id) end end def test_preload_has_many_using_primary_key - expected = Firm.first.clients_using_primary_key.to_a + expected = Firm.first.clients_using_primary_key.sort_by(&:id) firm = Firm.includes(:clients_using_primary_key).first assert_no_queries do - assert_equal expected, firm.clients_using_primary_key + assert_equal expected, firm.clients_using_primary_key.sort_by(&:id) end end From 165785e8cf43456816c8152ca4295640b21d1765 Mon Sep 17 00:00:00 2001 From: George Claghorn Date: Thu, 30 May 2019 18:20:55 -0400 Subject: [PATCH 26/37] Skip image analysis on ImageMagick error --- activestorage/lib/active_storage/analyzer/image_analyzer.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/activestorage/lib/active_storage/analyzer/image_analyzer.rb b/activestorage/lib/active_storage/analyzer/image_analyzer.rb index c8bc8fe953..bd1bef3076 100644 --- a/activestorage/lib/active_storage/analyzer/image_analyzer.rb +++ b/activestorage/lib/active_storage/analyzer/image_analyzer.rb @@ -43,6 +43,9 @@ module ActiveStorage rescue LoadError logger.info "Skipping image analysis because the mini_magick gem isn't installed" {} + rescue MiniMagick::Error => error + logger.error "Skipping image analysis due to an ImageMagick error: #{error.message}" + {} end def rotated_image?(image) From 5ce1f66d7ac50492f4cd1e6bd0170b2c888ec73d Mon Sep 17 00:00:00 2001 From: George Claghorn Date: Thu, 30 May 2019 22:33:29 -0400 Subject: [PATCH 27/37] Add a changelog entry for 165785e --- activestorage/CHANGELOG.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 351d8687a4..2d9fe56858 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,6 +1,15 @@ +* Image analysis is skipped if ImageMagick returns an error. + + `ActiveStorage::Analyzer::ImageAnalyzer#metadata` would previously raise a + `MiniMagick::Error`, which caused persistent `ActiveStorage::AnalyzeJob` + failures. It now logs the error and returns `{}`, resulting in no metadata + being added to the offending image blob. + + *George Claghorn* + * Method calls on singular attachments return `nil` when no file is attached. - Previously, assuming the following User model, `user.avatar.filename` would + Previously, assuming the following User model, `user.avatar.filename` would raise a `Module::DelegationError` if no avatar was attached: ```ruby @@ -8,7 +17,7 @@ has_one_attached :avatar end ``` - + They now return `nil`. *Matthew Tanous* From 9fd02d181a0706cd3a98e08b2caa7c9ed3c39b50 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 30 May 2019 16:56:29 -0700 Subject: [PATCH 28/37] Fail parallel tests if workers exit early Previously, if a test worker exited early, the in-flight test it was supposed to run wasn't reported as a failure. If all workers exited immediately, this would be reported as ex. Finished in 1.708349s, 39.2192 runs/s, 79.0237 assertions/s. 67 runs, 135 assertions, 0 failures, 0 errors, 2 skips This commit validates that all workers finish running tests by ensuring that the queue is empty after they exit. This works because we signal the workers to exit by pushing nil onto the queue, so that there should be a number of items left in the queue matching potentially missed tests. --- .../active_support/testing/parallelization.rb | 8 ++++++++ railties/test/application/test_runner_test.rb | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/activesupport/lib/active_support/testing/parallelization.rb b/activesupport/lib/active_support/testing/parallelization.rb index e760bf5ce3..f50a5e0554 100644 --- a/activesupport/lib/active_support/testing/parallelization.rb +++ b/activesupport/lib/active_support/testing/parallelization.rb @@ -27,6 +27,10 @@ module ActiveSupport @queue << o end + def length + @queue.length + end + def pop; @queue.pop; end end @@ -109,6 +113,10 @@ module ActiveSupport def shutdown @queue_size.times { @queue << nil } @pool.each { |pid| Process.waitpid pid } + + if @queue.length > 0 + raise "Queue not empty, but all workers have finished. This probably means that a worker crashed and #{@queue.length} tests were missed." + end end private diff --git a/railties/test/application/test_runner_test.rb b/railties/test/application/test_runner_test.rb index 1ab45abcd0..7fc918898b 100644 --- a/railties/test/application/test_runner_test.rb +++ b/railties/test/application/test_runner_test.rb @@ -564,6 +564,24 @@ module ApplicationTests assert_no_match "create_table(:users)", output end + def test_run_in_parallel_with_process_worker_crash + exercise_parallelization_regardless_of_machine_core_count(with: :processes) + + file_name = app_file("test/models/parallel_test.rb", <<-RUBY) + require 'test_helper' + + class ParallelTest < ActiveSupport::TestCase + def test_crash + Kernel.exit 1 + end + end + RUBY + + output = run_test_command(file_name) + + assert_match %r{Queue not empty, but all workers have finished. This probably means that a worker crashed and 1 tests were missed.}, output + end + def test_run_in_parallel_with_threads exercise_parallelization_regardless_of_machine_core_count(with: :threads) From 5f2bc3a6a00a2277481de7241f7a2066e886b84d Mon Sep 17 00:00:00 2001 From: Guilherme Goettems Schneider Date: Fri, 31 May 2019 16:59:47 -0300 Subject: [PATCH 29/37] Fix table comment also being applied to the primary key column --- activerecord/CHANGELOG.md | 4 ++++ .../connection_adapters/abstract/schema_statements.rb | 2 +- activerecord/test/cases/comment_test.rb | 5 +++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 8c5342edb2..2b5fc1c2f8 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* Fix table comment also being applied to the primary key column + + *Guilherme Goettems Schneider* + * Allow generated `create_table` migrations to include or skip timestamps. *Michael Duchemin* diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 107d1ce855..bec22c9b03 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -302,7 +302,7 @@ module ActiveRecord if pk.is_a?(Array) td.primary_keys pk else - td.primary_key pk, options.fetch(:id, :primary_key), options + td.primary_key pk, options.fetch(:id, :primary_key), options.except(:comment) end end diff --git a/activerecord/test/cases/comment_test.rb b/activerecord/test/cases/comment_test.rb index 584e03d196..f2f28462d1 100644 --- a/activerecord/test/cases/comment_test.rb +++ b/activerecord/test/cases/comment_test.rb @@ -44,6 +44,11 @@ if ActiveRecord::Base.connection.supports_comments? @connection.drop_table "blank_comments", if_exists: true end + def test_primary_key_comment + column = Commented.columns_hash["id"] + assert_nil column.comment + end + def test_column_created_in_block column = Commented.columns_hash["name"] assert_equal :string, column.type From ea5f509643d6d9c468a9b26f6c45bd4e40fd67cf Mon Sep 17 00:00:00 2001 From: "yuuji.yaginuma" Date: Wed, 17 Apr 2019 15:37:16 +0900 Subject: [PATCH 30/37] Change `ActionDispatch::Response#content_type` returning Content-Type header as it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since #35709, `Response#conten_type` returns only MIME type correctly. It is a documented behavior that this method only returns MIME type, so this change seems appropriate. https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249 But unfortunately, some users expect this method to return all Content-Type that does not contain charset. This seems to be breaking changes. We can change this behavior with the deprecate cycle. But, in that case, a method needs that include Content-Type with additional parameters. And that method name is probably the `content_type` seems to properly. So I changed the new behavior to more appropriate `media_type` method. And `Response#content_type` changed (as the method name) to return Content-Type header as it is. Fixes #35709. [Rafael Mendonça França & Yuuji Yaginuma ] --- actionpack/lib/action_controller/metal.rb | 2 +- .../action_controller/metal/mime_responds.rb | 2 +- .../lib/action_controller/metal/renderers.rb | 2 +- .../lib/action_controller/metal/rendering.rb | 2 +- .../lib/action_dispatch/http/response.rb | 9 ++- .../lib/action_dispatch/testing/assertions.rb | 2 +- .../action_dispatch/testing/test_response.rb | 2 +- .../test/controller/content_type_test.rb | 28 +++---- .../test/controller/integration_test.rb | 8 +- .../controller/localized_templates_test.rb | 2 +- .../test/controller/metal/renderers_test.rb | 4 +- .../test/controller/mime/respond_to_test.rb | 78 +++++++++---------- actionpack/test/controller/render_js_test.rb | 2 +- .../test/controller/render_json_test.rb | 14 ++-- actionpack/test/controller/render_xml_test.rb | 4 +- actionpack/test/controller/renderers_test.rb | 4 +- .../test/controller/show_exceptions_test.rb | 8 +- .../test/dispatch/debug_exceptions_test.rb | 30 +++---- actionpack/test/dispatch/response_test.rb | 36 +++++++-- .../test/actionpack/controller/render_test.rb | 24 +++--- guides/source/6_0_release_notes.md | 4 + guides/source/testing.md | 2 +- guides/source/upgrading_ruby_on_rails.md | 22 ++++++ 23 files changed, 171 insertions(+), 120 deletions(-) diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb index b9088e6d86..9ca0bbb9db 100644 --- a/actionpack/lib/action_controller/metal.rb +++ b/actionpack/lib/action_controller/metal.rb @@ -148,7 +148,7 @@ module ActionController attr_internal :response, :request delegate :session, to: "@_request" delegate :headers, :status=, :location=, :content_type=, - :status, :location, :content_type, to: "@_response" + :status, :location, :content_type, :media_type, to: "@_response" def initialize @_request = nil diff --git a/actionpack/lib/action_controller/metal/mime_responds.rb b/actionpack/lib/action_controller/metal/mime_responds.rb index bf5e7a433f..5c6f7fe396 100644 --- a/actionpack/lib/action_controller/metal/mime_responds.rb +++ b/actionpack/lib/action_controller/metal/mime_responds.rb @@ -205,7 +205,7 @@ module ActionController #:nodoc: yield collector if block_given? if format = collector.negotiate_format(request) - if content_type && content_type != format + if media_type && media_type != format raise ActionController::RespondToMismatchError end _process_format(format) diff --git a/actionpack/lib/action_controller/metal/renderers.rb b/actionpack/lib/action_controller/metal/renderers.rb index b81d3ef539..a251c29d23 100644 --- a/actionpack/lib/action_controller/metal/renderers.rb +++ b/actionpack/lib/action_controller/metal/renderers.rb @@ -157,7 +157,7 @@ module ActionController json = json.to_json(options) unless json.kind_of?(String) if options[:callback].present? - if content_type.nil? || content_type == Mime[:json] + if media_type.nil? || media_type == Mime[:json] self.content_type = Mime[:js] end diff --git a/actionpack/lib/action_controller/metal/rendering.rb b/actionpack/lib/action_controller/metal/rendering.rb index 7d0a944381..7f7c736965 100644 --- a/actionpack/lib/action_controller/metal/rendering.rb +++ b/actionpack/lib/action_controller/metal/rendering.rb @@ -73,7 +73,7 @@ module ActionController end def _set_rendered_content_type(format) - if format && !response.content_type + if format && !response.media_type self.content_type = format.to_s end end diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index 69798f99e0..61e3a870ab 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/actionpack/lib/action_dispatch/http/response.rb @@ -243,8 +243,13 @@ module ActionDispatch # :nodoc: end # Content type of response. - # It returns just MIME type and does NOT contain charset part. def content_type + type = super + type&.empty? ? nil : type + end + + # Media type of response. + def media_type parsed_content_type_header.mime_type end @@ -458,7 +463,7 @@ module ActionDispatch # :nodoc: end def assign_default_content_type_and_charset! - return if content_type + return if media_type ct = parsed_content_type_header set_content_type(ct.mime_type || Mime[:html].to_s, diff --git a/actionpack/lib/action_dispatch/testing/assertions.rb b/actionpack/lib/action_dispatch/testing/assertions.rb index 08c2969685..dcaf914ac9 100644 --- a/actionpack/lib/action_dispatch/testing/assertions.rb +++ b/actionpack/lib/action_dispatch/testing/assertions.rb @@ -14,7 +14,7 @@ module ActionDispatch include Rails::Dom::Testing::Assertions def html_document - @html_document ||= if @response.content_type.to_s.end_with?("xml") + @html_document ||= if @response.media_type.to_s.end_with?("xml") Nokogiri::XML::Document.parse(@response.body) else Nokogiri::HTML::Document.parse(@response.body) diff --git a/actionpack/lib/action_dispatch/testing/test_response.rb b/actionpack/lib/action_dispatch/testing/test_response.rb index 6f7c86fdcf..f1dd4099c5 100644 --- a/actionpack/lib/action_dispatch/testing/test_response.rb +++ b/actionpack/lib/action_dispatch/testing/test_response.rb @@ -19,7 +19,7 @@ module ActionDispatch end def response_parser - @response_parser ||= RequestEncoder.parser(content_type) + @response_parser ||= RequestEncoder.parser(media_type) end end end diff --git a/actionpack/test/controller/content_type_test.rb b/actionpack/test/controller/content_type_test.rb index 636b025f2c..aeb0d07195 100644 --- a/actionpack/test/controller/content_type_test.rb +++ b/actionpack/test/controller/content_type_test.rb @@ -66,68 +66,68 @@ class ContentTypeTest < ActionController::TestCase def test_render_defaults get :render_defaults assert_equal "utf-8", @response.charset - assert_equal Mime[:text], @response.content_type + assert_equal Mime[:text], @response.media_type end def test_render_changed_charset_default with_default_charset "utf-16" do get :render_defaults assert_equal "utf-16", @response.charset - assert_equal Mime[:text], @response.content_type + assert_equal Mime[:text], @response.media_type end end # :ported: def test_content_type_from_body get :render_content_type_from_body - assert_equal Mime[:rss], @response.content_type + assert_equal Mime[:rss], @response.media_type assert_equal "utf-8", @response.charset end # :ported: def test_content_type_from_render get :render_content_type_from_render - assert_equal Mime[:rss], @response.content_type + assert_equal Mime[:rss], @response.media_type assert_equal "utf-8", @response.charset end # :ported: def test_charset_from_body get :render_charset_from_body - assert_equal Mime[:text], @response.content_type + assert_equal Mime[:text], @response.media_type assert_equal "utf-16", @response.charset end # :ported: def test_nil_charset_from_body get :render_nil_charset_from_body - assert_equal Mime[:text], @response.content_type + assert_equal Mime[:text], @response.media_type assert_equal "utf-8", @response.charset, @response.headers.inspect end def test_nil_default_for_erb with_default_charset nil do get :render_default_for_erb - assert_equal Mime[:html], @response.content_type + assert_equal Mime[:html], @response.media_type assert_nil @response.charset, @response.headers.inspect end end def test_default_for_erb get :render_default_for_erb - assert_equal Mime[:html], @response.content_type + assert_equal Mime[:html], @response.media_type assert_equal "utf-8", @response.charset end def test_default_for_builder get :render_default_for_builder - assert_equal Mime[:xml], @response.content_type + assert_equal Mime[:xml], @response.media_type assert_equal "utf-8", @response.charset end def test_change_for_builder get :render_change_for_builder - assert_equal Mime[:html], @response.content_type + assert_equal Mime[:html], @response.media_type assert_equal "utf-8", @response.charset end @@ -148,22 +148,22 @@ class AcceptBasedContentTypeTest < ActionController::TestCase def test_render_default_content_types_for_respond_to @request.accept = Mime[:html].to_s get :render_default_content_types_for_respond_to - assert_equal Mime[:html], @response.content_type + assert_equal Mime[:html], @response.media_type @request.accept = Mime[:js].to_s get :render_default_content_types_for_respond_to - assert_equal Mime[:js], @response.content_type + assert_equal Mime[:js], @response.media_type end def test_render_default_content_types_for_respond_to_with_template @request.accept = Mime[:xml].to_s get :render_default_content_types_for_respond_to - assert_equal Mime[:xml], @response.content_type + assert_equal Mime[:xml], @response.media_type end def test_render_default_content_types_for_respond_to_with_overwrite @request.accept = Mime[:rss].to_s get :render_default_content_types_for_respond_to - assert_equal Mime[:xml], @response.content_type + assert_equal Mime[:xml], @response.media_type end end diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb index 4dddd98f9f..cce229b30d 100644 --- a/actionpack/test/controller/integration_test.rb +++ b/actionpack/test/controller/integration_test.rb @@ -522,11 +522,11 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest with_test_route_set do get "/get", headers: { "Accept" => "application/json" }, xhr: true assert_equal "application/json", request.accept - assert_equal "application/json", response.content_type + assert_equal "application/json", response.media_type get "/get", headers: { "HTTP_ACCEPT" => "application/json" }, xhr: true assert_equal "application/json", request.accept - assert_equal "application/json", response.content_type + assert_equal "application/json", response.media_type end end @@ -986,7 +986,7 @@ class IntegrationRequestEncodersTest < ActionDispatch::IntegrationTest def test_encoding_as_json post_to_foos as: :json do assert_response :success - assert_equal "application/json", request.content_type + assert_equal "application/json", request.media_type assert_equal "application/json", request.accepts.first.to_s assert_equal :json, request.format.ref assert_equal({ "foo" => "fighters" }, request.request_parameters) @@ -1025,7 +1025,7 @@ class IntegrationRequestEncodersTest < ActionDispatch::IntegrationTest post_to_foos as: :wibble do assert_response :success assert_equal "/foos_wibble", request.path - assert_equal "text/wibble", request.content_type + assert_equal "text/wibble", request.media_type assert_equal "text/wibble", request.accepts.first.to_s assert_equal :wibble, request.format.ref assert_equal Hash.new, request.request_parameters # Unregistered MIME Type can't be parsed. diff --git a/actionpack/test/controller/localized_templates_test.rb b/actionpack/test/controller/localized_templates_test.rb index d84a76fb46..5c5cef66d5 100644 --- a/actionpack/test/controller/localized_templates_test.rb +++ b/actionpack/test/controller/localized_templates_test.rb @@ -43,6 +43,6 @@ class LocalizedTemplatesTest < ActionController::TestCase I18n.locale = :it get :hello_world assert_equal "Ciao Mondo", @response.body - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end end diff --git a/actionpack/test/controller/metal/renderers_test.rb b/actionpack/test/controller/metal/renderers_test.rb index 5f0d125128..f6558f1354 100644 --- a/actionpack/test/controller/metal/renderers_test.rb +++ b/actionpack/test/controller/metal/renderers_test.rb @@ -38,13 +38,13 @@ class RenderersMetalTest < ActionController::TestCase get :one assert_response :success assert_equal({ a: "b" }.to_json, @response.body) - assert_equal "application/json", @response.content_type + assert_equal "application/json", @response.media_type end def test_render_xml get :two assert_response :success assert_equal(" ", @response.body) - assert_equal "text/plain", @response.content_type + assert_equal "text/plain", @response.media_type end end diff --git a/actionpack/test/controller/mime/respond_to_test.rb b/actionpack/test/controller/mime/respond_to_test.rb index 2f8f191828..fc16c639fb 100644 --- a/actionpack/test/controller/mime/respond_to_test.rb +++ b/actionpack/test/controller/mime/respond_to_test.rb @@ -423,12 +423,12 @@ class RespondToControllerTest < ActionController::TestCase def test_using_defaults @request.accept = "*/*" get :using_defaults - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "Hello world!", @response.body @request.accept = "application/xml" get :using_defaults - assert_equal "application/xml", @response.content_type + assert_equal "application/xml", @response.media_type assert_equal "

Hello world!

\n", @response.body end @@ -449,12 +449,12 @@ class RespondToControllerTest < ActionController::TestCase def test_using_defaults_with_type_list @request.accept = "*/*" get :using_defaults_with_type_list - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "Hello world!", @response.body @request.accept = "application/xml" get :using_defaults_with_type_list - assert_equal "application/xml", @response.content_type + assert_equal "application/xml", @response.media_type assert_equal "

Hello world!

\n", @response.body end @@ -468,7 +468,7 @@ class RespondToControllerTest < ActionController::TestCase def test_using_non_conflicting_nested_js_then_js @request.accept = "*/*" get :using_non_conflicting_nested_js_then_js - assert_equal "text/javascript", @response.content_type + assert_equal "text/javascript", @response.media_type assert_equal "JS", @response.body end @@ -499,12 +499,12 @@ class RespondToControllerTest < ActionController::TestCase def test_custom_types @request.accept = "application/fancy-xml" get :custom_type_handling - assert_equal "application/fancy-xml", @response.content_type + assert_equal "application/fancy-xml", @response.media_type assert_equal "Fancy XML", @response.body @request.accept = "text/html" get :custom_type_handling - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "HTML", @response.body end @@ -595,7 +595,7 @@ class RespondToControllerTest < ActionController::TestCase @request.accept = "application/json" get :json_with_callback assert_equal "/**/alert(JS)", @response.body - assert_equal "text/javascript", @response.content_type + assert_equal "text/javascript", @response.media_type end def test_xhr @@ -605,13 +605,13 @@ class RespondToControllerTest < ActionController::TestCase def test_custom_constant get :custom_constant_handling, format: "mobile" - assert_equal "text/x-mobile", @response.content_type + assert_equal "text/x-mobile", @response.media_type assert_equal "Mobile", @response.body end def test_custom_constant_handling_without_block get :custom_constant_handling_without_block, format: "mobile" - assert_equal "text/x-mobile", @response.content_type + assert_equal "text/x-mobile", @response.media_type assert_equal "Mobile", @response.body end @@ -664,7 +664,7 @@ class RespondToControllerTest < ActionController::TestCase assert_equal '
Hello future from Firefox!
', @response.body get :iphone_with_html_response_type, format: "iphone" - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal '
Hello iPhone future from iPhone!
', @response.body end @@ -672,7 +672,7 @@ class RespondToControllerTest < ActionController::TestCase @request.accept = "text/iphone" get :iphone_with_html_response_type assert_equal '
Hello iPhone future from iPhone!
', @response.body - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end def test_invalid_format @@ -702,7 +702,7 @@ class RespondToControllerTest < ActionController::TestCase def test_variant_with_implicit_template_rendering get :variant_with_implicit_template_rendering, params: { v: :mobile } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "mobile", @response.body end @@ -756,137 +756,137 @@ class RespondToControllerTest < ActionController::TestCase def test_variant_with_format_and_custom_render get :variant_with_format_and_custom_render, params: { v: :phone } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "mobile", @response.body end def test_multiple_variants_for_format get :multiple_variants_for_format, params: { v: :tablet } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "tablet", @response.body end def test_no_variant_in_variant_setup get :variant_plus_none_for_format - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "none", @response.body end def test_variant_inline_syntax get :variant_inline_syntax - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "none", @response.body get :variant_inline_syntax, params: { v: :phone } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "phone", @response.body end def test_variant_inline_syntax_with_format get :variant_inline_syntax, format: :js - assert_equal "text/javascript", @response.content_type + assert_equal "text/javascript", @response.media_type assert_equal "js", @response.body end def test_variant_inline_syntax_without_block get :variant_inline_syntax_without_block, params: { v: :phone } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "phone", @response.body end def test_variant_any get :variant_any, params: { v: :phone } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "phone", @response.body get :variant_any, params: { v: :tablet } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "any", @response.body get :variant_any, params: { v: :phablet } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "any", @response.body end def test_variant_any_any get :variant_any_any - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "any", @response.body get :variant_any_any, params: { v: :phone } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "phone", @response.body get :variant_any_any, params: { v: :yolo } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "any", @response.body end def test_variant_inline_any get :variant_any, params: { v: :phone } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "phone", @response.body get :variant_inline_any, params: { v: :tablet } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "any", @response.body get :variant_inline_any, params: { v: :phablet } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "any", @response.body end def test_variant_inline_any_any get :variant_inline_any_any, params: { v: :phone } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "phone", @response.body get :variant_inline_any_any, params: { v: :yolo } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "any", @response.body end def test_variant_any_implicit_render get :variant_any_implicit_render, params: { v: :tablet } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "tablet", @response.body get :variant_any_implicit_render, params: { v: :phablet } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "phablet", @response.body end def test_variant_any_with_none get :variant_any_with_none - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "none or phone", @response.body get :variant_any_with_none, params: { v: :phone } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "none or phone", @response.body end def test_format_any_variant_any get :format_any_variant_any, format: :js, params: { v: :tablet } - assert_equal "text/javascript", @response.content_type + assert_equal "text/javascript", @response.media_type assert_equal "tablet", @response.body end def test_variant_negotiation_inline_syntax get :variant_inline_syntax_without_block, params: { v: [:tablet, :phone] } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "phone", @response.body end def test_variant_negotiation_block_syntax get :variant_plus_none_for_format, params: { v: [:tablet, :phone] } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "phone", @response.body end def test_variant_negotiation_without_block get :variant_inline_syntax_without_block, params: { v: [:tablet, :phone] } - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type assert_equal "phone", @response.body end end diff --git a/actionpack/test/controller/render_js_test.rb b/actionpack/test/controller/render_js_test.rb index 1efc0b9de1..7ef35c2396 100644 --- a/actionpack/test/controller/render_js_test.rb +++ b/actionpack/test/controller/render_js_test.rb @@ -26,7 +26,7 @@ class RenderJSTest < ActionController::TestCase def test_render_vanilla_js get :render_vanilla_js_hello, xhr: true assert_equal "alert('hello')", @response.body - assert_equal "text/javascript", @response.content_type + assert_equal "text/javascript", @response.media_type end def test_should_render_js_partial diff --git a/actionpack/test/controller/render_json_test.rb b/actionpack/test/controller/render_json_test.rb index 82c1ba26cb..aba7593c15 100644 --- a/actionpack/test/controller/render_json_test.rb +++ b/actionpack/test/controller/render_json_test.rb @@ -80,7 +80,7 @@ class RenderJsonTest < ActionController::TestCase def test_render_json_nil get :render_json_nil assert_equal "null", @response.body - assert_equal "application/json", @response.content_type + assert_equal "application/json", @response.media_type end def test_render_json_render_to_string @@ -91,7 +91,7 @@ class RenderJsonTest < ActionController::TestCase def test_render_json get :render_json_hello_world assert_equal '{"hello":"world"}', @response.body - assert_equal "application/json", @response.content_type + assert_equal "application/json", @response.media_type end def test_render_json_with_status @@ -103,31 +103,31 @@ class RenderJsonTest < ActionController::TestCase def test_render_json_with_callback get :render_json_hello_world_with_callback, xhr: true assert_equal '/**/alert({"hello":"world"})', @response.body - assert_equal "text/javascript", @response.content_type + assert_equal "text/javascript", @response.media_type end def test_render_json_with_custom_content_type get :render_json_with_custom_content_type, xhr: true assert_equal '{"hello":"world"}', @response.body - assert_equal "text/javascript", @response.content_type + assert_equal "text/javascript", @response.media_type end def test_render_symbol_json get :render_symbol_json assert_equal '{"hello":"world"}', @response.body - assert_equal "application/json", @response.content_type + assert_equal "application/json", @response.media_type end def test_render_json_with_render_to_string get :render_json_with_render_to_string assert_equal '{"hello":"partial html"}', @response.body - assert_equal "application/json", @response.content_type + assert_equal "application/json", @response.media_type end def test_render_json_forwards_extra_options get :render_json_with_extra_options assert_equal '{"a":"b"}', @response.body - assert_equal "application/json", @response.content_type + assert_equal "application/json", @response.media_type end def test_render_json_calls_to_json_from_object diff --git a/actionpack/test/controller/render_xml_test.rb b/actionpack/test/controller/render_xml_test.rb index a72d14e4bb..7d61076e7c 100644 --- a/actionpack/test/controller/render_xml_test.rb +++ b/actionpack/test/controller/render_xml_test.rb @@ -92,11 +92,11 @@ class RenderXmlTest < ActionController::TestCase def test_should_render_xml_but_keep_custom_content_type get :render_xml_with_custom_content_type - assert_equal "application/atomsvc+xml", @response.content_type + assert_equal "application/atomsvc+xml", @response.media_type end def test_should_use_implicit_content_type get :implicit_content_type, format: "atom" - assert_equal Mime[:atom], @response.content_type + assert_equal Mime[:atom], @response.media_type end end diff --git a/actionpack/test/controller/renderers_test.rb b/actionpack/test/controller/renderers_test.rb index d92de6f5d5..96cce664a4 100644 --- a/actionpack/test/controller/renderers_test.rb +++ b/actionpack/test/controller/renderers_test.rb @@ -73,7 +73,7 @@ class RenderersTest < ActionController::TestCase assert_raise ActionView::MissingTemplate do get :respond_to_mime, format: "csv" end - assert_equal Mime[:csv], @response.content_type + assert_equal Mime[:csv], @response.media_type assert_equal "", @response.body end @@ -83,7 +83,7 @@ class RenderersTest < ActionController::TestCase end @request.accept = "text/csv" get :respond_to_mime, format: "csv" - assert_equal Mime[:csv], @response.content_type + assert_equal Mime[:csv], @response.media_type assert_equal "c,s,v", @response.body ensure ActionController::Renderers.remove :csv diff --git a/actionpack/test/controller/show_exceptions_test.rb b/actionpack/test/controller/show_exceptions_test.rb index 1d68a359dc..2cd2114db6 100644 --- a/actionpack/test/controller/show_exceptions_test.rb +++ b/actionpack/test/controller/show_exceptions_test.rb @@ -76,7 +76,7 @@ module ShowExceptions @app = ShowExceptionsOverriddenController.action(:boom) get "/", headers: { "HTTP_ACCEPT" => "application/json" } assert_response :internal_server_error - assert_equal "application/json", response.content_type.to_s + assert_equal "application/json", response.media_type assert_equal({ status: 500, error: "Internal Server Error" }.to_json, response.body) end @@ -84,7 +84,7 @@ module ShowExceptions @app = ShowExceptionsOverriddenController.action(:boom) get "/", headers: { "HTTP_ACCEPT" => "application/xml" } assert_response :internal_server_error - assert_equal "application/xml", response.content_type.to_s + assert_equal "application/xml", response.media_type assert_equal({ status: 500, error: "Internal Server Error" }.to_xml, response.body) end @@ -92,7 +92,7 @@ module ShowExceptions @app = ShowExceptionsOverriddenController.action(:boom) get "/", headers: { "HTTP_ACCEPT" => "text/csv" } assert_response :internal_server_error - assert_equal "text/html", response.content_type.to_s + assert_equal "text/html", response.media_type end end @@ -106,7 +106,7 @@ module ShowExceptions get "/", headers: { "HTTP_ACCEPT" => "text/json" } assert_response :internal_server_error - assert_equal "text/plain", response.content_type.to_s + assert_equal "text/plain", response.media_type ensure middleware.instance_variable_set(:@exceptions_app, @exceptions_app) $stderr = STDERR diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb index 3e57e8f4d9..68817ccdea 100644 --- a/actionpack/test/dispatch/debug_exceptions_test.rb +++ b/actionpack/test/dispatch/debug_exceptions_test.rb @@ -208,7 +208,7 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest assert_response 500 assert_no_match(/
/, body) assert_no_match(//, body) - assert_equal "text/plain", response.content_type + assert_equal "text/plain", response.media_type assert_match(/RuntimeError\npuke/, body) Rails.stub :root, Pathname.new(".") do @@ -222,31 +222,31 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest get "/not_found", headers: xhr_request_env assert_response 404 assert_no_match(//, body) - assert_equal "text/plain", response.content_type + assert_equal "text/plain", response.media_type assert_match(/#{AbstractController::ActionNotFound.name}/, body) get "/method_not_allowed", headers: xhr_request_env assert_response 405 assert_no_match(//, body) - assert_equal "text/plain", response.content_type + assert_equal "text/plain", response.media_type assert_match(/ActionController::MethodNotAllowed/, body) get "/unknown_http_method", headers: xhr_request_env assert_response 405 assert_no_match(//, body) - assert_equal "text/plain", response.content_type + assert_equal "text/plain", response.media_type assert_match(/ActionController::UnknownHttpMethod/, body) get "/bad_request", headers: xhr_request_env assert_response 400 assert_no_match(//, body) - assert_equal "text/plain", response.content_type + assert_equal "text/plain", response.media_type assert_match(/ActionController::BadRequest/, body) get "/parameter_missing", headers: xhr_request_env assert_response 400 assert_no_match(//, body) - assert_equal "text/plain", response.content_type + assert_equal "text/plain", response.media_type assert_match(/ActionController::ParameterMissing/, body) end @@ -257,37 +257,37 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest assert_response 500 assert_no_match(/
/, body) assert_no_match(//, body) - assert_equal "application/json", response.content_type + assert_equal "application/json", response.media_type assert_match(/RuntimeError: puke/, body) get "/not_found", headers: { "action_dispatch.show_exceptions" => true }, as: :json assert_response 404 assert_no_match(//, body) - assert_equal "application/json", response.content_type + assert_equal "application/json", response.media_type assert_match(/#{AbstractController::ActionNotFound.name}/, body) get "/method_not_allowed", headers: { "action_dispatch.show_exceptions" => true }, as: :json assert_response 405 assert_no_match(//, body) - assert_equal "application/json", response.content_type + assert_equal "application/json", response.media_type assert_match(/ActionController::MethodNotAllowed/, body) get "/unknown_http_method", headers: { "action_dispatch.show_exceptions" => true }, as: :json assert_response 405 assert_no_match(//, body) - assert_equal "application/json", response.content_type + assert_equal "application/json", response.media_type assert_match(/ActionController::UnknownHttpMethod/, body) get "/bad_request", headers: { "action_dispatch.show_exceptions" => true }, as: :json assert_response 400 assert_no_match(//, body) - assert_equal "application/json", response.content_type + assert_equal "application/json", response.media_type assert_match(/ActionController::BadRequest/, body) get "/parameter_missing", headers: { "action_dispatch.show_exceptions" => true }, as: :json assert_response 400 assert_no_match(//, body) - assert_equal "application/json", response.content_type + assert_equal "application/json", response.media_type assert_match(/ActionController::ParameterMissing/, body) end @@ -298,7 +298,7 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest assert_response 500 assert_match(/
/, body) assert_match(//, body) - assert_equal "text/html", response.content_type + assert_equal "text/html", response.media_type assert_match(/puke/, body) end @@ -307,7 +307,7 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest get "/index.xml", headers: { "action_dispatch.show_exceptions" => true } assert_response 500 - assert_equal "application/xml", response.content_type + assert_equal "application/xml", response.media_type assert_match(/RuntimeError: puke/, body) end @@ -321,7 +321,7 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest get "/index", headers: { "action_dispatch.show_exceptions" => true }, as: :wibble assert_response 500 - assert_equal "application/json", response.content_type + assert_equal "application/json", response.media_type assert_match(/RuntimeError: puke/, body) ensure diff --git a/actionpack/test/dispatch/response_test.rb b/actionpack/test/dispatch/response_test.rb index 7758b0406a..33cf86a081 100644 --- a/actionpack/test/dispatch/response_test.rb +++ b/actionpack/test/dispatch/response_test.rb @@ -290,8 +290,8 @@ class ResponseTest < ActiveSupport::TestCase resp.to_a assert_equal("utf-16", resp.charset) - assert_equal(Mime[:xml], resp.content_type) - + assert_equal(Mime[:xml], resp.media_type) + assert_equal("application/xml; charset=utf-16", resp.content_type) assert_equal("application/xml; charset=utf-16", resp.headers["Content-Type"]) end @@ -503,8 +503,8 @@ class ResponseIntegrationTest < ActionDispatch::IntegrationTest assert_response :success assert_equal("utf-16", @response.charset) - assert_equal(Mime[:xml], @response.content_type) - + assert_equal(Mime[:xml], @response.media_type) + assert_equal("application/xml; charset=utf-16", @response.content_type) assert_equal("application/xml; charset=utf-16", @response.headers["Content-Type"]) end @@ -519,8 +519,8 @@ class ResponseIntegrationTest < ActionDispatch::IntegrationTest assert_response :success assert_equal("utf-16", @response.charset) - assert_equal(Mime[:xml], @response.content_type) - + assert_equal(Mime[:xml], @response.media_type) + assert_equal("application/xml; charset=utf-16", @response.content_type) assert_equal("application/xml; charset=utf-16", @response.headers["Content-Type"]) end @@ -553,7 +553,26 @@ class ResponseIntegrationTest < ActionDispatch::IntegrationTest assert_response :success assert_equal("text/csv; charset=utf-16; header=present", @response.headers["Content-Type"]) - assert_equal("text/csv", @response.content_type) + assert_equal("text/csv; charset=utf-16; header=present", @response.content_type) + assert_equal("text/csv", @response.media_type) + assert_equal("utf-16", @response.charset) + end + + test "response Content-Type with optional parameters that set before charset" do + @app = lambda { |env| + [ + 200, + { "Content-Type" => "text/csv; header=present; charset=utf-16" }, + ["Hello"] + ] + } + + get "/" + assert_response :success + + assert_equal("text/csv; header=present; charset=utf-16", @response.headers["Content-Type"]) + assert_equal("text/csv; header=present; charset=utf-16", @response.content_type) + assert_equal("text/csv", @response.media_type) assert_equal("utf-16", @response.charset) end @@ -570,7 +589,8 @@ class ResponseIntegrationTest < ActionDispatch::IntegrationTest assert_response :success assert_equal('text/csv; header=present; charset="utf-16"', @response.headers["Content-Type"]) - assert_equal("text/csv", @response.content_type) + assert_equal('text/csv; header=present; charset="utf-16"', @response.content_type) + assert_equal("text/csv", @response.media_type) assert_equal("utf-16", @response.charset) end end diff --git a/actionview/test/actionpack/controller/render_test.rb b/actionview/test/actionpack/controller/render_test.rb index c8ce7366d1..9b1a720636 100644 --- a/actionview/test/actionpack/controller/render_test.rb +++ b/actionview/test/actionpack/controller/render_test.rb @@ -1003,14 +1003,14 @@ class RenderTest < ActionController::TestCase def test_render_xml get :render_xml_hello assert_equal "\n

Hello David

\n

This is grand!

\n\n", @response.body - assert_equal "application/xml", @response.content_type + assert_equal "application/xml", @response.media_type end # :ported: def test_render_xml_as_string_template get :render_xml_hello_as_string_template assert_equal "\n

Hello David

\n

This is grand!

\n\n", @response.body - assert_equal "application/xml", @response.content_type + assert_equal "application/xml", @response.media_type end # :ported: @@ -1039,7 +1039,7 @@ class RenderTest < ActionController::TestCase def test_rendered_format_without_format get :inline_rendered_format_without_format assert_equal "test", @response.body - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end def test_partials_list @@ -1077,7 +1077,7 @@ class RenderTest < ActionController::TestCase def test_accessing_local_assigns_in_inline_template get :accessing_local_assigns_in_inline_template, params: { local_name: "Local David" } assert_equal "Goodbye, Local David", @response.body - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end def test_should_implicitly_render_html_template_from_xhr_request @@ -1264,13 +1264,13 @@ class RenderTest < ActionController::TestCase def test_partial_only get :partial_only assert_equal "only partial", @response.body - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end def test_should_render_html_formatted_partial get :partial assert_equal "partial html", @response.body - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end def test_render_html_formatted_partial_even_with_other_mime_time_in_accept @@ -1279,20 +1279,20 @@ class RenderTest < ActionController::TestCase get :partial_html_erb assert_equal "partial.html.erb", @response.body.strip - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end def test_should_render_html_partial_with_formats get :partial_formats_html assert_equal "partial html", @response.body - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end def test_render_to_string_partial get :render_to_string_with_partial assert_equal "only partial", @controller.instance_variable_get(:@partial_only) assert_equal "Hello: david", @controller.instance_variable_get(:@partial_with_locals) - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end def test_render_to_string_with_template_and_html_partial @@ -1300,21 +1300,21 @@ class RenderTest < ActionController::TestCase assert_equal "**only partial**\n", @controller.instance_variable_get(:@text) assert_equal "only partial\n", @controller.instance_variable_get(:@html) assert_equal "only html partial\n", @response.body - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end def test_render_to_string_and_render_with_different_formats get :render_to_string_and_render_with_different_formats assert_equal "only partial\n", @controller.instance_variable_get(:@html) assert_equal "**only partial**\n", @response.body - assert_equal "text/plain", @response.content_type + assert_equal "text/plain", @response.media_type end def test_render_template_within_a_template_with_other_format get :render_template_within_a_template_with_other_format expected = "only html partial

This is grand!

" assert_equal expected, @response.body.strip - assert_equal "text/html", @response.content_type + assert_equal "text/html", @response.media_type end def test_partial_with_counter diff --git a/guides/source/6_0_release_notes.md b/guides/source/6_0_release_notes.md index fa45e7240d..e421ae1ac7 100644 --- a/guides/source/6_0_release_notes.md +++ b/guides/source/6_0_release_notes.md @@ -215,6 +215,10 @@ Please refer to the [Changelog][action-pack] for detailed changes. ### Notable changes +* Change `ActionDispatch::Response#content_type` returning Content-Type + header as it is. + ([Pull Request](https://github.com/rails/rails/pull/36034)) + * Raise an `ArgumentError` if a resource param contains a colon. ([Pull Request](https://github.com/rails/rails/pull/35236)) diff --git a/guides/source/testing.md b/guides/source/testing.md index 9540bb2af5..41bc54b924 100644 --- a/guides/source/testing.md +++ b/guides/source/testing.md @@ -1144,7 +1144,7 @@ test "ajax request" do get article_url(article), xhr: true assert_equal 'hello world', @response.body - assert_equal "text/javascript", @response.content_type + assert_equal "text/javascript", @response.media_type end ``` diff --git a/guides/source/upgrading_ruby_on_rails.md b/guides/source/upgrading_ruby_on_rails.md index b8a5c39f39..1110592d5e 100644 --- a/guides/source/upgrading_ruby_on_rails.md +++ b/guides/source/upgrading_ruby_on_rails.md @@ -134,6 +134,28 @@ Action Cable JavaScript API: + ActionCable.logger.enabled = false ``` +### `ActionDispatch::Response#content_type` now returned Content-Type header as it is. + +Previously, `ActionDispatch::Response#content_type` returned value does NOT contain charset part. +This behavior changed to returned Content-Type header containing charset part as it is. + +If you want just MIME type, please use `ActionDispatch::Response#media_type` instead. + +Before: + +```ruby +resp = ActionDispatch::Response.new(200, "Content-Type" => "text/csv; header=present; charset=utf-16") +resp.content_type #=> "text/csv; header=present" +``` + +After: + +```ruby +resp = ActionDispatch::Response.new(200, "Content-Type" => "text/csv; header=present; charset=utf-16") +resp.content_type #=> "text/csv; header=present; charset=utf-16" +resp.media_type #=> "text/csv" +``` + ### Autoloading The default configuration for Rails 6 From 613060d106f2d3bf7350bab540c952b1567ad66b Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 29 May 2019 06:45:34 +0900 Subject: [PATCH 31/37] Avoid making extra 5 arrays in each `save` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each `save` calls `all_timestamp_attributes_in_model` to fill timestamp columns. Allthough the `all_timestamp_attributes_in_model` returns the same value every time, the `all_timestamp_attributes_in_model` makes extra 5 arrays every time. This avoids the making extra 5 arrays by memoizing the result, it makes `save` economical and a bit faster. https://gist.github.com/kamipo/1ddad2235073f508637bf9a72d64bb83 Before (2a015f6c0be0593a624b0c800e5335319ac4c660): ``` {["~/rails/activerecord/lib/active_record/timestamp.rb", 76, :T_ARRAY]=>[1000, 0, 341, 0, 1, 13640], ["~/rails/activerecord/lib/active_record/timestamp.rb", 64, :T_ARRAY]=>[1000, 0, 341, 0, 1, 13640], ["~/rails/activerecord/lib/active_record/timestamp.rb", 80, :T_ARRAY]=>[1000, 0, 341, 0, 1, 13640], ["~/rails/activerecord/lib/active_record/timestamp.rb", 68, :T_ARRAY]=>[1000, 0, 341, 0, 1, 13640], ["~/rails/activerecord/lib/active_record/timestamp.rb", 73, :T_ARRAY]=>[1000, 0, 341, 0, 1, 13640]} Warming up -------------------------------------- User.create * 10 36.000 i/100ms Calculating ------------------------------------- User.create * 10 353.644 (± 7.4%) i/s - 1.764k in 5.021876s ``` After (this change): ``` {["~/rails/activerecord/lib/active_record/timestamp.rb", 83, :T_ARRAY]=>[1, 0, 1, 1, 1, 40], ["~/rails/activerecord/lib/active_record/timestamp.rb", 87, :T_ARRAY]=>[1, 0, 1, 1, 1, 40], ["~/rails/activerecord/lib/active_record/timestamp.rb", 64, :T_ARRAY]=>[1, 1, 1, 1, 1, 0], ["~/rails/activerecord/lib/active_record/timestamp.rb", 69, :T_ARRAY]=>[1, 1, 1, 1, 1, 0], ["~/rails/activerecord/lib/active_record/timestamp.rb", 74, :T_ARRAY]=>[1, 1, 1, 1, 1, 0]} Warming up -------------------------------------- User.create * 10 37.000 i/100ms Calculating ------------------------------------- User.create * 10 380.063 (± 7.1%) i/s - 1.924k in 5.097917s ``` --- activerecord/lib/active_record/timestamp.rb | 46 +++++++++++++-------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/activerecord/lib/active_record/timestamp.rb b/activerecord/lib/active_record/timestamp.rb index 04a1c03474..a5862ae06b 100644 --- a/activerecord/lib/active_record/timestamp.rb +++ b/activerecord/lib/active_record/timestamp.rb @@ -59,19 +59,26 @@ module ActiveRecord attribute_names.index_with(time || current_time_from_proper_timezone) end + def timestamp_attributes_for_create_in_model + @timestamp_attributes_for_create_in_model ||= + (timestamp_attributes_for_create & column_names).freeze + end + + def timestamp_attributes_for_update_in_model + @timestamp_attributes_for_update_in_model ||= + (timestamp_attributes_for_update & column_names).freeze + end + + def all_timestamp_attributes_in_model + @all_timestamp_attributes_in_model ||= + (timestamp_attributes_for_create_in_model + timestamp_attributes_for_update_in_model).freeze + end + + def current_time_from_proper_timezone + default_timezone == :utc ? Time.now.utc : Time.now + end + private - def timestamp_attributes_for_create_in_model - timestamp_attributes_for_create.select { |c| column_names.include?(c) } - end - - def timestamp_attributes_for_update_in_model - timestamp_attributes_for_update.select { |c| column_names.include?(c) } - end - - def all_timestamp_attributes_in_model - timestamp_attributes_for_create_in_model + timestamp_attributes_for_update_in_model - end - def timestamp_attributes_for_create ["created_at", "created_on"] end @@ -80,8 +87,11 @@ module ActiveRecord ["updated_at", "updated_on"] end - def current_time_from_proper_timezone - default_timezone == :utc ? Time.now.utc : Time.now + def reload_schema_from_cache + @timestamp_attributes_for_create_in_model = nil + @timestamp_attributes_for_update_in_model = nil + @all_timestamp_attributes_in_model = nil + super end end @@ -124,19 +134,19 @@ module ActiveRecord end def timestamp_attributes_for_create_in_model - self.class.send(:timestamp_attributes_for_create_in_model) + self.class.timestamp_attributes_for_create_in_model end def timestamp_attributes_for_update_in_model - self.class.send(:timestamp_attributes_for_update_in_model) + self.class.timestamp_attributes_for_update_in_model end def all_timestamp_attributes_in_model - self.class.send(:all_timestamp_attributes_in_model) + self.class.all_timestamp_attributes_in_model end def current_time_from_proper_timezone - self.class.send(:current_time_from_proper_timezone) + self.class.current_time_from_proper_timezone end def max_updated_column_timestamp From 22274d320e36a4750fd83b096fb3a490c96559d6 Mon Sep 17 00:00:00 2001 From: "yuuji.yaginuma" Date: Sun, 2 Jun 2019 07:26:42 +0900 Subject: [PATCH 32/37] Simplify `ActionDispatch::Response#content_type` --- actionpack/lib/action_dispatch/http/response.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index 61e3a870ab..e5cffccf35 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/actionpack/lib/action_dispatch/http/response.rb @@ -244,8 +244,7 @@ module ActionDispatch # :nodoc: # Content type of response. def content_type - type = super - type&.empty? ? nil : type + super.presence end # Media type of response. From 86384ea88968f38e79728fbe584a7de518acfed0 Mon Sep 17 00:00:00 2001 From: Yasuo Honda Date: Sun, 2 Jun 2019 06:22:07 +0000 Subject: [PATCH 33/37] Address test_pluck_columns_with_same_name failure due to nondeterministic sort order ```ruby $ bundle exec rake test_postgresql ... snip ... Failure: CalculationsTest#test_pluck_columns_with_same_name [/home/yahonda/git/rails/activerecord/test/cases/calculations_test.rb:842]: --- expected +++ actual @@ -1 +1 @@ -[["The First Topic", "The Second Topic of the day"], ["The Third Topic of the day", "The Fourth Topic of the day"]] +[["The Third Topic of the day", "The Fourth Topic of the day"], ["The First Topic", "The Second Topic of the day"]] ``` --- activerecord/test/cases/calculations_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 0d13174e12..e42af3686e 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -837,7 +837,7 @@ class CalculationsTest < ActiveRecord::TestCase def test_pluck_columns_with_same_name expected = [["The First Topic", "The Second Topic of the day"], ["The Third Topic of the day", "The Fourth Topic of the day"]] - actual = Topic.joins(:replies) + actual = Topic.joins(:replies).order(:id) .pluck("topics.title", "replies_topics.title") assert_equal expected, actual end From a04a0330a56a5eae53fd7ab3ebcab692acf2ea06 Mon Sep 17 00:00:00 2001 From: Abhay Nikam Date: Sun, 2 Jun 2019 20:23:10 +0530 Subject: [PATCH 34/37] Remove unnecessary require pathname from actionpack controller specs --- actionpack/test/controller/render_js_test.rb | 1 - actionpack/test/controller/render_json_test.rb | 1 - actionpack/test/controller/render_xml_test.rb | 1 - 3 files changed, 3 deletions(-) diff --git a/actionpack/test/controller/render_js_test.rb b/actionpack/test/controller/render_js_test.rb index 7ef35c2396..da8f6e8062 100644 --- a/actionpack/test/controller/render_js_test.rb +++ b/actionpack/test/controller/render_js_test.rb @@ -2,7 +2,6 @@ require "abstract_unit" require "controller/fake_models" -require "pathname" class RenderJSTest < ActionController::TestCase class TestController < ActionController::Base diff --git a/actionpack/test/controller/render_json_test.rb b/actionpack/test/controller/render_json_test.rb index aba7593c15..82c6aaafe5 100644 --- a/actionpack/test/controller/render_json_test.rb +++ b/actionpack/test/controller/render_json_test.rb @@ -3,7 +3,6 @@ require "abstract_unit" require "controller/fake_models" require "active_support/logger" -require "pathname" class RenderJsonTest < ActionController::TestCase class JsonRenderable diff --git a/actionpack/test/controller/render_xml_test.rb b/actionpack/test/controller/render_xml_test.rb index 7d61076e7c..28d8e281ab 100644 --- a/actionpack/test/controller/render_xml_test.rb +++ b/actionpack/test/controller/render_xml_test.rb @@ -2,7 +2,6 @@ require "abstract_unit" require "controller/fake_models" -require "pathname" class RenderXmlTest < ActionController::TestCase class XmlRenderable From c3ca9b00e3c5f839311c55549f25f7afe8120f9d Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 3 Jun 2019 03:40:17 +0900 Subject: [PATCH 35/37] Refactor `create_table`'s options separation `create_table` and `t.column` have the same named options (e.g. `:comment`, `:primary_key`), so it should be separated table options from column options. Related #36373. --- .../abstract/schema_definitions.rb | 3 +-- .../abstract/schema_statements.rb | 20 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index 688eea75e8..dbd533b4b3 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -264,8 +264,7 @@ module ActiveRecord if_not_exists: false, options: nil, as: nil, - comment: nil, - ** + comment: nil ) @conn = conn @columns_hash = {} diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index bec22c9b03..cf57af5473 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -291,25 +291,25 @@ module ActiveRecord # SELECT * FROM orders INNER JOIN line_items ON order_id=orders.id # # See also TableDefinition#column for details on how to create columns. - def create_table(table_name, **options) - td = create_table_definition(table_name, options) + def create_table(table_name, id: :primary_key, primary_key: nil, force: nil, **options) + td = create_table_definition( + table_name, options.extract!(:temporary, :if_not_exists, :options, :as, :comment) + ) - if options[:id] != false && !options[:as] - pk = options.fetch(:primary_key) do - Base.get_primary_key table_name.to_s.singularize - end + if id && !td.as + pk = primary_key || Base.get_primary_key(table_name.to_s.singularize) if pk.is_a?(Array) td.primary_keys pk else - td.primary_key pk, options.fetch(:id, :primary_key), options.except(:comment) + td.primary_key pk, id, options end end yield td if block_given? - if options[:force] - drop_table(table_name, options.merge(if_exists: true)) + if force + drop_table(table_name, force: force, if_exists: true) end result = execute schema_creation.accept td @@ -321,7 +321,7 @@ module ActiveRecord end if supports_comments? && !supports_comments_in_create? - if table_comment = options[:comment].presence + if table_comment = td.comment.presence change_table_comment(table_name, table_comment) end From 90d6237762da832d0c6302b31fd68d3efb771c64 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 3 Jun 2019 05:47:10 +0900 Subject: [PATCH 36/37] Fix `subscribed` with no pattern to subscribe all messages This is a regression for #36184. And also, add new `monotonic` argument to the last of the method signature rather than the first. --- .../lib/active_support/notifications.rb | 14 ++++++-------- .../lib/active_support/notifications/fanout.rb | 6 +++--- activesupport/test/notifications_test.rb | 18 ++++++++++++++++++ 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/activesupport/lib/active_support/notifications.rb b/activesupport/lib/active_support/notifications.rb index 555c0ad1d3..a7a6112b0f 100644 --- a/activesupport/lib/active_support/notifications.rb +++ b/activesupport/lib/active_support/notifications.rb @@ -231,18 +231,16 @@ module ActiveSupport # ActiveSupport::Notifications.subscribe(/render/) do |event| # @event = event # end - def subscribe(*args, &block) - pattern, callback = *args - notifier.subscribe(pattern, callback, false, &block) + def subscribe(pattern = nil, callback = nil, &block) + notifier.subscribe(pattern, callback, monotonic: false, &block) end - def monotonic_subscribe(*args, &block) - pattern, callback = *args - notifier.subscribe(pattern, callback, true, &block) + def monotonic_subscribe(pattern = nil, callback = nil, &block) + notifier.subscribe(pattern, callback, monotonic: true, &block) end - def subscribed(callback, pattern, monotonic: false, &block) - subscriber = notifier.subscribe(pattern, callback, monotonic) + def subscribed(callback, pattern = nil, monotonic: false, &block) + subscriber = notifier.subscribe(pattern, callback, monotonic: monotonic) yield ensure unsubscribe(subscriber) diff --git a/activesupport/lib/active_support/notifications/fanout.rb b/activesupport/lib/active_support/notifications/fanout.rb index c37bec4ee5..aa602329ec 100644 --- a/activesupport/lib/active_support/notifications/fanout.rb +++ b/activesupport/lib/active_support/notifications/fanout.rb @@ -20,8 +20,8 @@ module ActiveSupport super end - def subscribe(pattern = nil, callable = nil, monotonic = false, &block) - subscriber = Subscribers.new(monotonic, pattern, callable || block) + def subscribe(pattern = nil, callable = nil, monotonic: false, &block) + subscriber = Subscribers.new(pattern, callable || block, monotonic) synchronize do if String === pattern @string_subscribers[pattern] << subscriber @@ -84,7 +84,7 @@ module ActiveSupport end module Subscribers # :nodoc: - def self.new(monotonic, pattern, listener) + def self.new(pattern, listener, monotonic) subscriber_class = monotonic ? MonotonicTimed : Timed if listener.respond_to?(:start) && listener.respond_to?(:finish) diff --git a/activesupport/test/notifications_test.rb b/activesupport/test/notifications_test.rb index 3b98749f1b..02b90b0297 100644 --- a/activesupport/test/notifications_test.rb +++ b/activesupport/test/notifications_test.rb @@ -113,6 +113,24 @@ module Notifications assert_equal expected, events end + def test_subscribed_all_messages + name = "foo" + name2 = name * 2 + expected = [name, name2, name] + + events = [] + callback = lambda { |*_| events << _.first } + ActiveSupport::Notifications.subscribed(callback) do + ActiveSupport::Notifications.instrument(name) + ActiveSupport::Notifications.instrument(name2) + ActiveSupport::Notifications.instrument(name) + end + assert_equal expected, events + + ActiveSupport::Notifications.instrument(name) + assert_equal expected, events + end + def test_subscribing_to_instrumentation_while_inside_it # the repro requires that there are no evented subscribers for the "foo" event, # so we have to duplicate some of the setup code From 49f31043be571381fd66dbc22b123d5625af64de Mon Sep 17 00:00:00 2001 From: Guilherme Goettems Schneider Date: Mon, 3 Jun 2019 08:05:36 -0300 Subject: [PATCH 37/37] Fix invalid schema dump when primary key column has a comment Before this fix it would either generate an invalid schema, passing `comment` option twice to `create_table`, or it move the comment from primary key column to the table if table had no comment when the dump was generated. The situation now is that a comment on primary key will be ignored (not present on schema). Fixes #29966 --- activerecord/CHANGELOG.md | 6 +++++ .../abstract/schema_dumper.rb | 2 +- activerecord/test/cases/comment_test.rb | 22 ++++++++++++++++++- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 2b5fc1c2f8..6f08b1b8fe 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Fix invalid schema when primary key column has a comment + + Fixes #29966 + + *Guilherme Goettems Schneider* + * Fix table comment also being applied to the primary key column *Guilherme Goettems Schneider* diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb index 622e00fffb..fb56e712be 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb @@ -15,7 +15,7 @@ module ActiveRecord def column_spec_for_primary_key(column) return {} if default_primary_key?(column) spec = { id: schema_type(column).inspect } - spec.merge!(prepare_column_options(column).except!(:null)) + spec.merge!(prepare_column_options(column).except!(:null, :comment)) spec[:default] ||= "nil" if explicit_primary_key_default?(column) spec end diff --git a/activerecord/test/cases/comment_test.rb b/activerecord/test/cases/comment_test.rb index f2f28462d1..25e2f20676 100644 --- a/activerecord/test/cases/comment_test.rb +++ b/activerecord/test/cases/comment_test.rb @@ -14,6 +14,9 @@ if ActiveRecord::Base.connection.supports_comments? class BlankComment < ActiveRecord::Base end + class PkCommented < ActiveRecord::Base + end + setup do @connection = ActiveRecord::Base.connection @@ -35,8 +38,13 @@ if ActiveRecord::Base.connection.supports_comments? t.index :absent_comment end + @connection.create_table("pk_commenteds", comment: "Table comment", id: false, force: true) do |t| + t.integer :id, comment: "Primary key comment", primary_key: true + end + Commented.reset_column_information BlankComment.reset_column_information + PkCommented.reset_column_information end teardown do @@ -44,7 +52,7 @@ if ActiveRecord::Base.connection.supports_comments? @connection.drop_table "blank_comments", if_exists: true end - def test_primary_key_comment + def test_default_primary_key_comment column = Commented.columns_hash["id"] assert_nil column.comment end @@ -169,5 +177,17 @@ if ActiveRecord::Base.connection.supports_comments? column = Commented.columns_hash["name"] assert_nil column.comment end + + def test_comment_on_primary_key + column = PkCommented.columns_hash["id"] + assert_equal "Primary key comment", column.comment + assert_equal "Table comment", @connection.table_comment("pk_commenteds") + end + + def test_schema_dump_with_primary_key_comment + output = dump_table_schema "pk_commenteds" + assert_match %r[create_table "pk_commenteds",.*\s+comment: "Table comment"], output + assert_no_match %r[create_table "pk_commenteds",.*\s+comment: "Primary key comment"], output + end end end