diff options
24 files changed, 156 insertions, 112 deletions
@@ -108,7 +108,6 @@ local_gemfile = File.expand_path(".Gemfile", __dir__) instance_eval File.read local_gemfile if File.exist? local_gemfile group :test do - gem "minitest", "~> 5.10.0" gem "minitest-bisect" platforms :mri do diff --git a/Gemfile.lock b/Gemfile.lock index 23b02fe945..e54d232ce1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -306,7 +306,7 @@ GEM mini_magick (4.8.0) mini_mime (0.1.4) mini_portile2 (2.3.0) - minitest (5.10.3) + minitest (5.11.1) minitest-bisect (1.4.0) minitest-server (~> 1.0) path_expander (~> 1.0) @@ -522,7 +522,6 @@ DEPENDENCIES libxml-ruby listen (>= 3.0.5, < 3.2) mini_magick - minitest (~> 5.10.0) minitest-bisect mocha mysql2 (>= 0.4.4) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 3393534eb3..52c38edf81 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Take into account association conditions when deleting through records. + + Fixes #18424. + + *Piotr Jakubowski* + * Fix nested `has_many :through` associations on unpersisted parent instances. For example, if you have diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index ac86911ad8..7a3bef969b 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -140,6 +140,7 @@ module ActiveRecord scope = through_association.scope scope.where! construct_join_attributes(*records) + scope = scope.where(through_scope_attributes) case method when :destroy diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 9e32b69786..c28e31a3da 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -564,7 +564,7 @@ module ActiveRecord end VALID_AUTOMATIC_INVERSE_MACROS = [:has_many, :has_one, :belongs_to] - INVALID_AUTOMATIC_INVERSE_OPTIONS = [:conditions, :through, :foreign_key] + INVALID_AUTOMATIC_INVERSE_OPTIONS = [:through, :foreign_key] def add_as_source(seed) seed diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 4df3864d07..10be583ef4 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -430,7 +430,7 @@ module ActiveRecord relation = self if eager_loading? - find_with_associations { |rel, _| relation = rel } + apply_join_dependency { |rel, _| relation = rel } end conn = klass.connection @@ -533,7 +533,7 @@ module ActiveRecord skip_query_cache_if_necessary do @records = if eager_loading? - find_with_associations do |relation, join_dependency| + apply_join_dependency do |relation, join_dependency| if ActiveRecord::NullRelation === relation [] else diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 50d0f14b98..98992caa0c 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -313,7 +313,7 @@ module ActiveRecord return false if !conditions || limit_value == 0 if eager_loading? - relation = apply_join_dependency(construct_join_dependency(eager_loading: false)) + relation = apply_join_dependency(eager_loading: false) return relation.exists?(conditions) end @@ -358,24 +358,6 @@ module ActiveRecord offset_value || 0 end - def find_with_associations - # NOTE: the JoinDependency constructed here needs to know about - # any joins already present in `self`, so pass them in - # - # failing to do so means that in cases like activerecord/test/cases/associations/inner_join_association_test.rb:136 - # incorrect SQL is generated. In that case, the join dependency for - # SpecialCategorizations is constructed without knowledge of the - # preexisting join in joins_values to categorizations (by way of - # the `has_many :through` for categories). - # - join_dependency = construct_join_dependency - - relation = apply_join_dependency(join_dependency) - relation._select!(join_dependency.aliases.columns) - - yield relation, join_dependency - end - def construct_relation_for_exists(conditions) relation = except(:select, :distinct, :order)._select!(ONE_AS_ONE).limit!(1) @@ -396,17 +378,23 @@ module ActiveRecord ) end - def apply_join_dependency(join_dependency = construct_join_dependency) + def apply_join_dependency(eager_loading: true) + join_dependency = construct_join_dependency(eager_loading: eager_loading) relation = except(:includes, :eager_load, :preload).joins!(join_dependency) - if using_limitable_reflections?(join_dependency.reflections) - relation - else + if eager_loading && !using_limitable_reflections?(join_dependency.reflections) if has_limit_or_offset? limited_ids = limited_ids_for(relation) limited_ids.empty? ? relation.none! : relation.where!(primary_key => limited_ids) end - relation.except(:limit, :offset) + relation.limit_value = relation.offset_value = nil + end + + if block_given? + relation._select!(join_dependency.aliases.columns) + yield relation, join_dependency + else + relation end end diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index 29de29ceb3..56a4b7c4d1 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -1308,6 +1308,25 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase end end + def test_has_many_through_update_ids_with_conditions + author = Author.create!(name: "Bill") + category = categories(:general) + + author.update( + special_categories_with_condition_ids: [category.id], + nonspecial_categories_with_condition_ids: [category.id] + ) + + assert_equal [category.id], author.special_categories_with_condition_ids + assert_equal [category.id], author.nonspecial_categories_with_condition_ids + + author.update(nonspecial_categories_with_condition_ids: []) + author.reload + + assert_equal [category.id], author.special_categories_with_condition_ids + assert_equal [], author.nonspecial_categories_with_condition_ids + end + def test_single_has_many_through_association_with_unpersisted_parent_instance post_with_single_has_many_through = Class.new(Post) do def self.name; "PostWithSingleHasManyThrough"; end diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index 95bd987551..4769ffd64d 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -270,25 +270,27 @@ class FinderTest < ActiveRecord::TestCase end def test_exists_with_includes_limit_and_empty_result - assert_equal false, Topic.includes(:replies).limit(0).exists? - assert_equal false, Topic.includes(:replies).limit(1).where("0 = 1").exists? + assert_no_queries { assert_equal false, Topic.includes(:replies).limit(0).exists? } + assert_queries(1) { assert_equal false, Topic.includes(:replies).limit(1).where("0 = 1").exists? } end def test_exists_with_distinct_association_includes_and_limit author = Author.first - assert_equal false, author.unique_categorized_posts.includes(:special_comments).limit(0).exists? - assert_equal true, author.unique_categorized_posts.includes(:special_comments).limit(1).exists? + unique_categorized_posts = author.unique_categorized_posts.includes(:special_comments) + assert_no_queries { assert_equal false, unique_categorized_posts.limit(0).exists? } + assert_queries(1) { assert_equal true, unique_categorized_posts.limit(1).exists? } end def test_exists_with_distinct_association_includes_limit_and_order author = Author.first - assert_equal false, author.unique_categorized_posts.includes(:special_comments).order("comments.tags_count DESC").limit(0).exists? - assert_equal true, author.unique_categorized_posts.includes(:special_comments).order("comments.tags_count DESC").limit(1).exists? + unique_categorized_posts = author.unique_categorized_posts.includes(:special_comments).order("comments.tags_count DESC") + assert_no_queries { assert_equal false, unique_categorized_posts.limit(0).exists? } + assert_queries(1) { assert_equal true, unique_categorized_posts.limit(1).exists? } end def test_exists_should_reference_correct_aliases_while_joining_tables_of_has_many_through_association - developer = developers(:david) - assert_not_predicate developer.ratings.includes(comment: :post).where(posts: { id: 1 }), :exists? + ratings = developers(:david).ratings.includes(comment: :post).where(posts: { id: 1 }) + assert_queries(1) { assert_not_predicate ratings.limit(1), :exists? } end def test_exists_with_empty_table_and_no_args_given @@ -1315,12 +1317,12 @@ class FinderTest < ActiveRecord::TestCase test "#skip_query_cache! for #exists? with a limited eager load" do Topic.cache do - assert_queries(2) do + assert_queries(1) do Topic.eager_load(:replies).limit(1).exists? Topic.eager_load(:replies).limit(1).exists? end - assert_queries(4) do + assert_queries(2) do Topic.eager_load(:replies).limit(1).skip_query_cache!.exists? Topic.eager_load(:replies).limit(1).skip_query_cache!.exists? end diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index cb8686f315..27da886e1c 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -88,6 +88,9 @@ class Author < ActiveRecord::Base has_many :special_categories, through: :special_categorizations, source: :category has_one :special_category, through: :special_categorizations, source: :category + has_many :special_categories_with_conditions, -> { where(categorizations: { special: true }) }, through: :categorizations, source: :category + has_many :nonspecial_categories_with_conditions, -> { where(categorizations: { special: false }) }, through: :categorizations, source: :category + has_many :categories_like_general, -> { where(name: "General") }, through: :categorizations, source: :category, class_name: "Category" has_many :categorized_posts, through: :categorizations, source: :post diff --git a/activestorage/app/models/active_storage/variation.rb b/activestorage/app/models/active_storage/variation.rb index 0046e6870b..da4af62666 100644 --- a/activestorage/app/models/active_storage/variation.rb +++ b/activestorage/app/models/active_storage/variation.rb @@ -46,14 +46,16 @@ class ActiveStorage::Variation # Accepts an open MiniMagick image instance, like what's returned by <tt>MiniMagick::Image.read(io)</tt>, # and performs the +transformations+ against it. The transformed image instance is then returned. def transform(image) - transformations.each do |name, argument_or_subtransformations| - image.mogrify do |command| - if name.to_s == "combine_options" - argument_or_subtransformations.each do |subtransformation_name, subtransformation_argument| - pass_transform_argument(command, subtransformation_name, subtransformation_argument) + ActiveSupport::Notifications.instrument("transform.active_storage") do + transformations.each do |name, argument_or_subtransformations| + image.mogrify do |command| + if name.to_s == "combine_options" + argument_or_subtransformations.each do |subtransformation_name, subtransformation_argument| + pass_transform_argument(command, subtransformation_name, subtransformation_argument) + end + else + pass_transform_argument(command, name, argument_or_subtransformations) end - else - pass_transform_argument(command, name, argument_or_subtransformations) end end end diff --git a/activestorage/lib/active_storage/previewer.rb b/activestorage/lib/active_storage/previewer.rb index 7db9ae5956..dacab1e7df 100644 --- a/activestorage/lib/active_storage/previewer.rb +++ b/activestorage/lib/active_storage/previewer.rb @@ -43,9 +43,11 @@ module ActiveStorage # # The output tempfile is opened in the directory returned by ActiveStorage::Downloading#tempdir. def draw(*argv) #:doc: - Tempfile.open("ActiveStorage", tempdir) do |file| - capture(*argv, to: file) - yield file + ActiveSupport::Notifications.instrument("preview.active_storage") do + Tempfile.open("ActiveStorage", tempdir) do |file| + capture(*argv, to: file) + yield file + end end end diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 3f77b191f9..8378de5061 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,7 @@ +* Add missing instrumentation for `read_multi` in `ActiveSupport::Cache::Store`. + + *Ignatius Reza Lesmana* + * `assert_changes` will always assert that the expression changes, regardless of `from:` and `to:` argument combinations. diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb index 8301b8c7cb..2d038dba77 100644 --- a/activesupport/lib/active_support/cache.rb +++ b/activesupport/lib/active_support/cache.rb @@ -357,23 +357,11 @@ module ActiveSupport options = names.extract_options! options = merged_options(options) - results = {} - names.each do |name| - key = normalize_key(name, options) - version = normalize_version(name, options) - entry = read_entry(key, options) - - if entry - if entry.expired? - delete_entry(key, options) - elsif entry.mismatched?(version) - # Skip mismatched versions - else - results[name] = entry.value - end + instrument :read_multi, names, options do |payload| + read_multi_entries(names, options).tap do |results| + payload[:hits] = results.keys end end - results end # Cache Storage API to write multiple values at once. @@ -414,14 +402,19 @@ module ActiveSupport options = names.extract_options! options = merged_options(options) - read_multi(*names, options).tap do |results| - writes = {} + instrument :read_multi, names, options do |payload| + read_multi_entries(names, options).tap do |results| + payload[:hits] = results.keys + payload[:super_operation] = :fetch_multi - (names - results.keys).each do |name| - results[name] = writes[name] = yield(name) - end + writes = {} - write_multi writes, options + (names - results.keys).each do |name| + results[name] = writes[name] = yield(name) + end + + write_multi writes, options + end end end @@ -538,6 +531,28 @@ module ActiveSupport raise NotImplementedError.new end + # Reads multiple entries from the cache implementation. Subclasses MAY + # implement this method. + def read_multi_entries(names, options) + results = {} + names.each do |name| + key = normalize_key(name, options) + version = normalize_version(name, options) + entry = read_entry(key, options) + + if entry + if entry.expired? + delete_entry(key, options) + elsif entry.mismatched?(version) + # Skip mismatched versions + else + results[name] = entry.value + end + end + end + results + end + # Writes multiple entries to the cache implementation. Subclasses MAY # implement this method. def write_multi_entries(hash, options) diff --git a/activesupport/lib/active_support/cache/mem_cache_store.rb b/activesupport/lib/active_support/cache/mem_cache_store.rb index df8bc8e43e..00a3670d64 100644 --- a/activesupport/lib/active_support/cache/mem_cache_store.rb +++ b/activesupport/lib/active_support/cache/mem_cache_store.rb @@ -91,28 +91,6 @@ module ActiveSupport end end - # Reads multiple values from the cache using a single call to the - # servers for all keys. Options can be passed in the last argument. - def read_multi(*names) - options = names.extract_options! - options = merged_options(options) - - keys_to_names = Hash[names.map { |name| [normalize_key(name, options), name] }] - - raw_values = @data.get_multi(keys_to_names.keys) - values = {} - - raw_values.each do |key, value| - entry = deserialize_entry(value) - - unless entry.expired? || entry.mismatched?(normalize_version(keys_to_names[key], options)) - values[keys_to_names[key]] = entry.value - end - end - - values - end - # Increment a cached value. This method uses the memcached incr atomic # operator and can only be used on values written with the :raw option. # Calling it on a value not stored with :raw will initialize that value @@ -170,6 +148,24 @@ module ActiveSupport end end + # Reads multiple entries from the cache implementation. + def read_multi_entries(names, options) + keys_to_names = Hash[names.map { |name| [normalize_key(name, options), name] }] + + raw_values = @data.get_multi(keys_to_names.keys) + values = {} + + raw_values.each do |key, value| + entry = deserialize_entry(value) + + unless entry.expired? || entry.mismatched?(normalize_version(keys_to_names[key], options)) + values[keys_to_names[key]] = entry.value + end + end + + values + end + # Delete an entry from the cache. def delete_entry(key, options) rescue_error_with(false) { @data.delete(key) } diff --git a/activesupport/lib/active_support/testing/isolation.rb b/activesupport/lib/active_support/testing/isolation.rb index fa9bebb181..562f985f1b 100644 --- a/activesupport/lib/active_support/testing/isolation.rb +++ b/activesupport/lib/active_support/testing/isolation.rb @@ -45,7 +45,8 @@ module ActiveSupport end } end - result = Marshal.dump(dup) + test_result = defined?(Minitest::Result) ? Minitest::Result.from(self) : dup + result = Marshal.dump(test_result) end write.puts [result].pack("m") @@ -69,8 +70,9 @@ module ActiveSupport if ENV["ISOLATION_TEST"] yield + test_result = defined?(Minitest::Result) ? Minitest::Result.from(self) : dup File.open(ENV["ISOLATION_OUTPUT"], "w") do |file| - file.puts [Marshal.dump(dup)].pack("m") + file.puts [Marshal.dump(test_result)].pack("m") end exit! else diff --git a/activesupport/test/cache/cache_store_write_multi_test.rb b/activesupport/test/cache/cache_store_write_multi_test.rb index 5b6fd678c5..7d606e3f7b 100644 --- a/activesupport/test/cache/cache_store_write_multi_test.rb +++ b/activesupport/test/cache/cache_store_write_multi_test.rb @@ -19,7 +19,7 @@ end class CacheStoreWriteMultiInstrumentationTest < ActiveSupport::TestCase setup do - @cache = ActiveSupport::Cache.lookup_store(:null_store) + @cache = ActiveSupport::Cache.lookup_store(:memory_store) end test "instrumentation" do @@ -35,15 +35,15 @@ class CacheStoreWriteMultiInstrumentationTest < ActiveSupport::TestCase end test "instrumentation with fetch_multi as super operation" do - skip "fetch_multi isn't instrumented yet" + @cache.write("b", "bb") - events = with_instrumentation "write_multi" do + events = with_instrumentation "read_multi" do @cache.fetch_multi("a", "b") { |key| key * 2 } end - assert_equal %w[ cache_write_multi.active_support ], events.map(&:name) - assert_nil events[0].payload[:super_operation] - assert !events[0].payload[:hit] + assert_equal %w[ cache_read_multi.active_support ], events.map(&:name) + assert_equal :fetch_multi, events[0].payload[:super_operation] + assert_equal ["b"], events[0].payload[:hits] end private diff --git a/guides/source/action_mailer_basics.md b/guides/source/action_mailer_basics.md index cb07781d1c..fe31e3403f 100644 --- a/guides/source/action_mailer_basics.md +++ b/guides/source/action_mailer_basics.md @@ -805,7 +805,7 @@ config.action_mailer.smtp_settings = { user_name: '<username>', password: '<password>', authentication: 'plain', - enable_starttls_auto: true } + enable_starttls_auto: true } ``` Note: As of July 15, 2014, Google increased [its security measures](https://support.google.com/accounts/answer/6010255) and now blocks attempts from apps it deems less secure. You can change your Gmail settings [here](https://www.google.com/settings/security/lesssecureapps) to allow the attempts. If your Gmail account has 2-factor authentication enabled, diff --git a/guides/source/active_record_validations.md b/guides/source/active_record_validations.md index e9157f3db1..d076efcd54 100644 --- a/guides/source/active_record_validations.md +++ b/guides/source/active_record_validations.md @@ -953,7 +953,7 @@ should happen, an `Array` can be used. Moreover, you can apply both `:if` and ```ruby class Computer < ApplicationRecord validates :mouse, presence: true, - if: [Proc.new { |c| c.market.retail? }, :desktop?], + if: [Proc.new { |c| c.market.retail? }, :desktop?], unless: Proc.new { |c| c.trackpad.present? } end ``` diff --git a/guides/source/security.md b/guides/source/security.md index 916b1e32f8..ab5a5a7a31 100644 --- a/guides/source/security.md +++ b/guides/source/security.md @@ -474,7 +474,7 @@ The common admin interface works like this: it's located at www.example.com/admi * Does the admin really have to access the interface from everywhere in the world? Think about _limiting the login to a bunch of source IP addresses_. Examine request.remote_ip to find out about the user's IP address. This is not bullet-proof, but a great barrier. Remember that there might be a proxy in use, though. -* _Put the admin interface to a special sub-domain_ such as admin.application.com and make it a separate application with its own user management. This makes stealing an admin cookie from the usual domain, www.application.com, impossible. This is because of the same origin policy in your browser: An injected (XSS) script on www.application.com may not read the cookie for admin.application.com and vice-versa. +* _Put the admin interface to a special subdomain_ such as admin.application.com and make it a separate application with its own user management. This makes stealing an admin cookie from the usual domain, www.application.com, impossible. This is because of the same origin policy in your browser: An injected (XSS) script on www.application.com may not read the cookie for admin.application.com and vice-versa. User Management --------------- diff --git a/guides/source/threading_and_code_execution.md b/guides/source/threading_and_code_execution.md index 3d3d31b97e..e4febc7507 100644 --- a/guides/source/threading_and_code_execution.md +++ b/guides/source/threading_and_code_execution.md @@ -272,7 +272,7 @@ that promise is to put it as close as possible to the blocking call: Rails.application.executor.wrap do th = Thread.new do Rails.application.executor.wrap do - User # inner thread can acquire the load lock, + User # inner thread can acquire the 'load' lock, # load User, and continue end end diff --git a/guides/source/working_with_javascript_in_rails.md b/guides/source/working_with_javascript_in_rails.md index c3dff1772c..b9ea4ad47a 100644 --- a/guides/source/working_with_javascript_in_rails.md +++ b/guides/source/working_with_javascript_in_rails.md @@ -373,7 +373,7 @@ Example usage: ```html document.body.addEventListener('ajax:success', function(event) { var detail = event.detail; - var data = detail[0], status = detail[1], xhr = detail[2]; + var data = detail[0], status = detail[1], xhr = detail[2]; }) ``` diff --git a/railties/lib/rails/test_unit/reporter.rb b/railties/lib/rails/test_unit/reporter.rb index 7d3164f1eb..28b93cee5a 100644 --- a/railties/lib/rails/test_unit/reporter.rb +++ b/railties/lib/rails/test_unit/reporter.rb @@ -64,11 +64,17 @@ module Rails end def format_line(result) - "%s#%s = %.2f s = %s" % [result.class, result.name, result.time, result.result_code] + klass = result.respond_to?(:klass) ? result.klass : result.class + "%s#%s = %.2f s = %s" % [klass, result.name, result.time, result.result_code] end def format_rerun_snippet(result) - location, line = result.method(result.name).source_location + location, line = if result.respond_to?(:source_location) + result.source_location + else + result.method(result.name).source_location + end + "#{executable} #{relative_path_for(location)}:#{line}" end diff --git a/railties/test/test_unit/reporter_test.rb b/railties/test/test_unit/reporter_test.rb index ad852d0f35..91cb47779b 100644 --- a/railties/test/test_unit/reporter_test.rb +++ b/railties/test/test_unit/reporter_test.rb @@ -163,7 +163,7 @@ class TestUnitReporterTest < ActiveSupport::TestCase end def failed_test - ft = ExampleTest.new(:woot) + ft = Minitest::Result.from(ExampleTest.new(:woot)) ft.failures << begin raise Minitest::Assertion, "boo" rescue Minitest::Assertion => e @@ -176,17 +176,17 @@ class TestUnitReporterTest < ActiveSupport::TestCase error = ArgumentError.new("wups") error.set_backtrace([ "some_test.rb:4" ]) - et = ExampleTest.new(:woot) + et = Minitest::Result.from(ExampleTest.new(:woot)) et.failures << Minitest::UnexpectedError.new(error) et end def passing_test - ExampleTest.new(:woot) + Minitest::Result.from(ExampleTest.new(:woot)) end def skipped_test - st = ExampleTest.new(:woot) + st = Minitest::Result.from(ExampleTest.new(:woot)) st.failures << begin raise Minitest::Skip, "skipchurches, misstemples" rescue Minitest::Assertion => e |