diff options
37 files changed, 361 insertions, 149 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 2df6f5fc09..9931a0de81 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,18 @@ +* Raise an `ArgumentError` if a resource custom param contains a colon (`:`). + + After this change it's not possible anymore to configure routes like this: + + ``` + routes.draw do + resources :users, param: 'name/:sneaky' + end + ``` + + Fixes #30467. + + *Josua Schmid* + + ## Rails 6.0.0.beta3 (March 11, 2019) ## * No changes. diff --git a/actionpack/lib/action_controller/metal/live.rb b/actionpack/lib/action_controller/metal/live.rb index eb43ff9c63..dd69930e25 100644 --- a/actionpack/lib/action_controller/metal/live.rb +++ b/actionpack/lib/action_controller/metal/live.rb @@ -305,7 +305,7 @@ module ActionController logger.fatal do message = +"\n#{exception.class} (#{exception.message}):\n" - message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code) + message << exception.annotated_source_code.to_s if exception.respond_to?(:annotated_source_code) message << " " << exception.backtrace.join("\n ") "#{message}\n\n" end diff --git a/actionpack/lib/action_dispatch/http/mime_type.rb b/actionpack/lib/action_dispatch/http/mime_type.rb index 962d10d81b..88b3a93211 100644 --- a/actionpack/lib/action_dispatch/http/mime_type.rb +++ b/actionpack/lib/action_dispatch/http/mime_type.rb @@ -315,7 +315,7 @@ module Mime include Singleton def initialize - super "*/*", :all + super "*/*", nil end def all?; true; end diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb index bb49bc4dda..59113e13f4 100644 --- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb @@ -146,7 +146,7 @@ module ActionDispatch message = [] message << " " message << "#{exception.class} (#{exception.message}):" - message.concat(exception.annoted_source_code) if exception.respond_to?(:annoted_source_code) + message.concat(exception.annotated_source_code) if exception.respond_to?(:annotated_source_code) message << " " message.concat(trace) diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index da3ade652e..2d2073de9a 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1141,6 +1141,10 @@ module ActionDispatch attr_reader :controller, :path, :param def initialize(entities, api_only, shallow, options = {}) + if options[:param].to_s.include?(":") + raise ArgumentError, ":param option can't contain colons" + end + @name = entities.to_s @path = (options[:path] || @name).to_s @controller = (options[:controller] || @name).to_s diff --git a/actionpack/test/controller/mime/respond_to_test.rb b/actionpack/test/controller/mime/respond_to_test.rb index 21de05b323..2f8f191828 100644 --- a/actionpack/test/controller/mime/respond_to_test.rb +++ b/actionpack/test/controller/mime/respond_to_test.rb @@ -158,6 +158,12 @@ class RespondToController < ActionController::Base end end + def handle_any_with_template + respond_to do |type| + type.any { render "test/hello_world" } + end + end + def all_types_with_layout respond_to do |type| type.html @@ -572,6 +578,13 @@ class RespondToControllerTest < ActionController::TestCase assert_equal "HTML", @response.body end + def test_handle_any_with_template + @request.accept = "*/*" + + get :handle_any_with_template + assert_equal "Hello world!", @response.body + end + def test_html_type_with_layout @request.accept = "text/html" get :all_types_with_layout diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 897d17885e..7b763ec2bd 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -3338,6 +3338,16 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest assert_equal "0c0c0b68-d24b-11e1-a861-001ff3fffe6f", @request.params[:download] end + def test_colon_containing_custom_param + ex = assert_raises(ArgumentError) { + draw do + resources :profiles, param: "username/:is_admin" + end + } + + assert_match(/:param option can't contain colon/, ex.message) + end + def test_action_from_path_is_not_frozen draw do get "search" => "search" diff --git a/actionview/lib/action_view/renderer/streaming_template_renderer.rb b/actionview/lib/action_view/renderer/streaming_template_renderer.rb index 279ef3c680..19fa399e2c 100644 --- a/actionview/lib/action_view/renderer/streaming_template_renderer.rb +++ b/actionview/lib/action_view/renderer/streaming_template_renderer.rb @@ -34,7 +34,7 @@ module ActionView return unless logger message = +"\n#{exception.class} (#{exception.message}):\n" - message << exception.annoted_source_code.to_s if exception.respond_to?(:annoted_source_code) + message << exception.annotated_source_code.to_s if exception.respond_to?(:annotated_source_code) message << " " << exception.backtrace.join("\n ") logger.fatal("#{message}\n\n") end diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index 6e3af1536a..7bc5e86edb 100644 --- a/actionview/lib/action_view/template.rb +++ b/actionview/lib/action_view/template.rb @@ -331,6 +331,7 @@ module ActionView # Make sure that the resulting String to be eval'd is in the # encoding of the code + original_source = source source = +<<-end_src def #{method_name}(local_assigns, output_buffer) @virtual_path = #{@virtual_path.inspect};#{locals_code};#{code} @@ -351,7 +352,14 @@ module ActionView raise WrongEncodingError.new(source, Encoding.default_internal) end - mod.module_eval(source, identifier, 0) + begin + mod.module_eval(source, identifier, 0) + rescue SyntaxError + # Account for when code in the template is not syntactically valid; e.g. if we're using + # ERB and the user writes <%= foo( %>, attempting to call a helper `foo` and interpolate + # the result into the template, but missing an end parenthesis. + raise SyntaxErrorInTemplate.new(self, original_source) + end end def handle_render_error(view, e) diff --git a/actionview/lib/action_view/template/error.rb b/actionview/lib/action_view/template/error.rb index 4e3c02e05e..d0ea03e228 100644 --- a/actionview/lib/action_view/template/error.rb +++ b/actionview/lib/action_view/template/error.rb @@ -109,7 +109,7 @@ module ActionView end end - def annoted_source_code + def annotated_source_code source_extract(4) end @@ -138,4 +138,24 @@ module ActionView end TemplateError = Template::Error + + class SyntaxErrorInTemplate < TemplateError #:nodoc + def initialize(template, offending_code_string) + @offending_code_string = offending_code_string + super(template) + end + + def message + <<~MESSAGE + Encountered a syntax error while rendering template: check #{@offending_code_string} + MESSAGE + end + + def annotated_source_code + @offending_code_string.split("\n").map.with_index(1) { |line, index| + indentation = " " * 4 + "#{index}:#{indentation}#{line}" + } + end + end end diff --git a/actionview/test/fixtures/test/syntax_error.html.erb b/actionview/test/fixtures/test/syntax_error.html.erb new file mode 100644 index 0000000000..4004a2b187 --- /dev/null +++ b/actionview/test/fixtures/test/syntax_error.html.erb @@ -0,0 +1,4 @@ +<%= foo( + 1, + 2, +%> diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index f0fed601f8..baff7cd83f 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -259,18 +259,24 @@ module RenderTestCases "and is followed by any combination of letters, numbers and underscores.", e.message end + def test_render_template_with_syntax_error + e = assert_raises(ActionView::Template::Error) { @view.render(template: "test/syntax_error") } + assert_match %r!syntax!, e.message + assert_equal "1: <%= foo(", e.annotated_source_code[0].strip + end + def test_render_partial_with_errors e = assert_raises(ActionView::Template::Error) { @view.render(partial: "test/raise") } assert_match %r!method.*doesnt_exist!, e.message assert_equal "", e.sub_template_message assert_equal "1", e.line_number - assert_equal "1: <%= doesnt_exist %>", e.annoted_source_code[0].strip + assert_equal "1: <%= doesnt_exist %>", e.annotated_source_code[0].strip assert_equal File.expand_path("#{FIXTURE_LOAD_PATH}/test/_raise.html.erb"), e.file_name end def test_render_error_indentation e = assert_raises(ActionView::Template::Error) { @view.render(partial: "test/raise_indentation") } - error_lines = e.annoted_source_code + error_lines = e.annotated_source_code assert_match %r!error\shere!, e.message assert_equal "11", e.line_number assert_equal " 9: <p>Ninth paragraph</p>", error_lines.second @@ -290,7 +296,7 @@ module RenderTestCases assert_match %r!method.*doesnt_exist!, e.message assert_equal "", e.sub_template_message assert_equal "1", e.line_number - assert_equal "1: <%= doesnt_exist %>", e.annoted_source_code[0].strip + assert_equal "1: <%= doesnt_exist %>", e.annotated_source_code[0].strip assert_equal File.expand_path("#{FIXTURE_LOAD_PATH}/test/_raise.html.erb"), e.file_name end diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md index 56fbfacbd7..0eba05e7a3 100644 --- a/activemodel/CHANGELOG.md +++ b/activemodel/CHANGELOG.md @@ -130,7 +130,7 @@ *Unathi Chonco* -* Add `config.active_model.i18n_full_message` in order to control whether +* Add `config.active_model.i18n_customize_full_message` in order to control whether the `full_message` error format can be overridden at the attribute or model level in the locale files. This is `false` by default. diff --git a/activemodel/lib/active_model/errors.rb b/activemodel/lib/active_model/errors.rb index d7e682d406..3a692a3e64 100644 --- a/activemodel/lib/active_model/errors.rb +++ b/activemodel/lib/active_model/errors.rb @@ -63,9 +63,9 @@ module ActiveModel MESSAGE_OPTIONS = [:message] class << self - attr_accessor :i18n_full_message # :nodoc: + attr_accessor :i18n_customize_full_message # :nodoc: end - self.i18n_full_message = false + self.i18n_customize_full_message = false attr_reader :messages, :details @@ -413,7 +413,7 @@ module ActiveModel return message if attribute == :base attribute = attribute.to_s - if self.class.i18n_full_message && @base.class.respond_to?(:i18n_scope) + if self.class.i18n_customize_full_message && @base.class.respond_to?(:i18n_scope) attribute = attribute.remove(/\[\d\]/) parts = attribute.split(".") attribute_name = parts.pop diff --git a/activemodel/lib/active_model/railtie.rb b/activemodel/lib/active_model/railtie.rb index 0ed70bd473..eb7901c7e9 100644 --- a/activemodel/lib/active_model/railtie.rb +++ b/activemodel/lib/active_model/railtie.rb @@ -13,8 +13,8 @@ module ActiveModel ActiveModel::SecurePassword.min_cost = Rails.env.test? end - initializer "active_model.i18n_full_message" do - ActiveModel::Errors.i18n_full_message = config.active_model.delete(:i18n_full_message) || false + initializer "active_model.i18n_customize_full_message" do + ActiveModel::Errors.i18n_customize_full_message = config.active_model.delete(:i18n_customize_full_message) || false end end end diff --git a/activemodel/test/cases/railtie_test.rb b/activemodel/test/cases/railtie_test.rb index ab60285e2a..507135d4e4 100644 --- a/activemodel/test/cases/railtie_test.rb +++ b/activemodel/test/cases/railtie_test.rb @@ -35,20 +35,20 @@ class RailtieTest < ActiveModel::TestCase test "i18n full message defaults to false" do @app.initialize! - assert_equal false, ActiveModel::Errors.i18n_full_message + assert_equal false, ActiveModel::Errors.i18n_customize_full_message end test "i18n full message can be disabled" do - @app.config.active_model.i18n_full_message = false + @app.config.active_model.i18n_customize_full_message = false @app.initialize! - assert_equal false, ActiveModel::Errors.i18n_full_message + assert_equal false, ActiveModel::Errors.i18n_customize_full_message end test "i18n full message can be enabled" do - @app.config.active_model.i18n_full_message = true + @app.config.active_model.i18n_customize_full_message = true @app.initialize! - assert_equal true, ActiveModel::Errors.i18n_full_message + assert_equal true, ActiveModel::Errors.i18n_customize_full_message end end diff --git a/activemodel/test/cases/validations/i18n_validation_test.rb b/activemodel/test/cases/validations/i18n_validation_test.rb index ccb565c5bd..eb03e837f1 100644 --- a/activemodel/test/cases/validations/i18n_validation_test.rb +++ b/activemodel/test/cases/validations/i18n_validation_test.rb @@ -13,8 +13,8 @@ class I18nValidationTest < ActiveModel::TestCase I18n.backend = I18n::Backend::Simple.new I18n.backend.store_translations("en", errors: { messages: { custom: nil } }) - @original_i18n_full_message = ActiveModel::Errors.i18n_full_message - ActiveModel::Errors.i18n_full_message = true + @original_i18n_customize_full_message = ActiveModel::Errors.i18n_customize_full_message + ActiveModel::Errors.i18n_customize_full_message = true end def teardown @@ -22,7 +22,7 @@ class I18nValidationTest < ActiveModel::TestCase I18n.load_path.replace @old_load_path I18n.backend = @old_backend I18n.backend.reload! - ActiveModel::Errors.i18n_full_message = @original_i18n_full_message + ActiveModel::Errors.i18n_customize_full_message = @original_i18n_customize_full_message end def test_full_message_encoding @@ -47,7 +47,7 @@ class I18nValidationTest < ActiveModel::TestCase end def test_errors_full_messages_doesnt_use_attribute_format_without_config - ActiveModel::Errors.i18n_full_message = false + ActiveModel::Errors.i18n_customize_full_message = false I18n.backend.store_translations("en", activemodel: { errors: { models: { person: { attributes: { name: { format: "%{message}" } } } } } }) @@ -58,7 +58,7 @@ class I18nValidationTest < ActiveModel::TestCase end def test_errors_full_messages_uses_attribute_format - ActiveModel::Errors.i18n_full_message = true + ActiveModel::Errors.i18n_customize_full_message = true I18n.backend.store_translations("en", activemodel: { errors: { models: { person: { attributes: { name: { format: "%{message}" } } } } } }) @@ -69,7 +69,7 @@ class I18nValidationTest < ActiveModel::TestCase end def test_errors_full_messages_uses_model_format - ActiveModel::Errors.i18n_full_message = true + ActiveModel::Errors.i18n_customize_full_message = true I18n.backend.store_translations("en", activemodel: { errors: { models: { person: { format: "%{message}" } } } }) @@ -80,7 +80,7 @@ class I18nValidationTest < ActiveModel::TestCase end def test_errors_full_messages_uses_deeply_nested_model_attributes_format - ActiveModel::Errors.i18n_full_message = true + ActiveModel::Errors.i18n_customize_full_message = true I18n.backend.store_translations("en", activemodel: { errors: { models: { 'person/contacts/addresses': { attributes: { street: { format: "%{message}" } } } } } }) @@ -91,7 +91,7 @@ class I18nValidationTest < ActiveModel::TestCase end def test_errors_full_messages_uses_deeply_nested_model_model_format - ActiveModel::Errors.i18n_full_message = true + ActiveModel::Errors.i18n_customize_full_message = true I18n.backend.store_translations("en", activemodel: { errors: { models: { 'person/contacts/addresses': { format: "%{message}" } } } }) @@ -102,7 +102,7 @@ class I18nValidationTest < ActiveModel::TestCase end def test_errors_full_messages_with_indexed_deeply_nested_attributes_and_attributes_format - ActiveModel::Errors.i18n_full_message = true + ActiveModel::Errors.i18n_customize_full_message = true I18n.backend.store_translations("en", activemodel: { errors: { models: { 'person/contacts/addresses': { attributes: { street: { format: "%{message}" } } } } } }) @@ -113,7 +113,7 @@ class I18nValidationTest < ActiveModel::TestCase end def test_errors_full_messages_with_indexed_deeply_nested_attributes_and_model_format - ActiveModel::Errors.i18n_full_message = true + ActiveModel::Errors.i18n_customize_full_message = true I18n.backend.store_translations("en", activemodel: { errors: { models: { 'person/contacts/addresses': { format: "%{message}" } } } }) @@ -124,7 +124,7 @@ class I18nValidationTest < ActiveModel::TestCase end def test_errors_full_messages_with_indexed_deeply_nested_attributes_and_i18n_attribute_name - ActiveModel::Errors.i18n_full_message = true + ActiveModel::Errors.i18n_customize_full_message = true I18n.backend.store_translations("en", activemodel: { attributes: { 'person/contacts/addresses': { country: "Country" } } @@ -136,7 +136,7 @@ class I18nValidationTest < ActiveModel::TestCase end def test_errors_full_messages_with_indexed_deeply_nested_attributes_without_i18n_config - ActiveModel::Errors.i18n_full_message = false + ActiveModel::Errors.i18n_customize_full_message = false I18n.backend.store_translations("en", activemodel: { errors: { models: { 'person/contacts/addresses': { attributes: { street: { format: "%{message}" } } } } } }) @@ -147,7 +147,7 @@ class I18nValidationTest < ActiveModel::TestCase end def test_errors_full_messages_with_i18n_attribute_name_without_i18n_config - ActiveModel::Errors.i18n_full_message = false + ActiveModel::Errors.i18n_customize_full_message = false I18n.backend.store_translations("en", activemodel: { attributes: { 'person/contacts[0]/addresses[0]': { country: "Country" } } diff --git a/activerecord/lib/active_record/associations/preloader.rb b/activerecord/lib/active_record/associations/preloader.rb index 8997579527..c7cd87f9d4 100644 --- a/activerecord/lib/active_record/associations/preloader.rb +++ b/activerecord/lib/active_record/associations/preloader.rb @@ -143,9 +143,7 @@ module ActiveRecord def preloaders_for_reflection(reflection, records, scope) records.group_by { |record| record.association(reflection.name).klass }.map do |rhs_klass, rs| - loader = preloader_for(reflection, rs).new(rhs_klass, rs, reflection, scope) - loader.run self - loader + preloader_for(reflection, rs).new(rhs_klass, rs, reflection, scope).run end end @@ -166,10 +164,18 @@ module ActiveRecord @reflection = reflection end - def run(preloader); end + def run + self + end def preloaded_records - owners.flat_map { |owner| owner.association(reflection.name).target } + @preloaded_records ||= records_by_owner.flat_map(&:last) + end + + def records_by_owner + @records_by_owner ||= owners.each_with_object({}) do |owner, result| + result[owner] = Array(owner.association(reflection.name).target) + end end private diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb index 7048ff43b8..46532f651e 100644 --- a/activerecord/lib/active_record/associations/preloader/association.rb +++ b/activerecord/lib/active_record/associations/preloader/association.rb @@ -4,26 +4,44 @@ module ActiveRecord module Associations class Preloader class Association #:nodoc: - attr_reader :preloaded_records - def initialize(klass, owners, reflection, preload_scope) @klass = klass @owners = owners @reflection = reflection @preload_scope = preload_scope @model = owners.first && owners.first.class - @preloaded_records = [] end - def run(preloader) - records = load_records do |record| - owner = owners_by_key[convert_key(record[association_key_name])] - association = owner.association(reflection.name) - association.set_inverse_instance(record) + def run + if !preload_scope || preload_scope.empty_scope? + owners.each do |owner| + associate_records_to_owner(owner, records_by_owner[owner] || []) + end + else + # Custom preload scope is used and + # the association can not be marked as loaded + # Loading into a Hash instead + records_by_owner end + self + end - owners.each do |owner| - associate_records_to_owner(owner, records[convert_key(owner[owner_key_name])] || []) + def records_by_owner + @records_by_owner ||= preloaded_records.each_with_object({}) do |record, result| + owners_by_key[convert_key(record[association_key_name])].each do |owner| + (result[owner] ||= []) << record + end + end + end + + def preloaded_records + return @preloaded_records if defined?(@preloaded_records) + return [] if owner_keys.empty? + # Some databases impose a limit on the number of ids in a list (in Oracle it's 1000) + # Make several smaller queries if necessary or make one query if the adapter supports it + slices = owner_keys.each_slice(klass.connection.in_clause_length || owner_keys.size) + @preloaded_records = slices.flat_map do |slice| + records_for(slice) end end @@ -54,13 +72,10 @@ module ActiveRecord end def owners_by_key - unless defined?(@owners_by_key) - @owners_by_key = owners.each_with_object({}) do |owner, h| - key = convert_key(owner[owner_key_name]) - h[key] = owner if key - end + @owners_by_key ||= owners.each_with_object({}) do |owner, result| + key = convert_key(owner[owner_key_name]) + (result[key] ||= []) << owner if key end - @owners_by_key end def key_conversion_required? @@ -87,23 +102,16 @@ module ActiveRecord @model.type_for_attribute(owner_key_name).type end - def load_records(&block) - return {} if owner_keys.empty? - # Some databases impose a limit on the number of ids in a list (in Oracle it's 1000) - # Make several smaller queries if necessary or make one query if the adapter supports it - slices = owner_keys.each_slice(klass.connection.in_clause_length || owner_keys.size) - @preloaded_records = slices.flat_map do |slice| - records_for(slice, &block) - end - @preloaded_records.group_by do |record| - convert_key(record[association_key_name]) + def records_for(ids) + scope.where(association_key_name => ids).load do |record| + # Processing only the first owner + # because the record is modified but not an owner + owner = owners_by_key[convert_key(record[association_key_name])].first + association = owner.association(reflection.name) + association.set_inverse_instance(record) end end - def records_for(ids, &block) - scope.where(association_key_name => ids).load(&block) - end - def scope @scope ||= build_scope end diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index db89b77629..bec1c4c94a 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -4,45 +4,57 @@ module ActiveRecord module Associations class Preloader class ThroughAssociation < Association # :nodoc: - def run(preloader) - already_loaded = owners.first.association(through_reflection.name).loaded? - through_scope = through_scope() - through_preloaders = preloader.preload(owners, through_reflection.name, through_scope) - middle_records = through_preloaders.flat_map(&:preloaded_records) - preloaders = preloader.preload(middle_records, source_reflection.name, scope) - @preloaded_records = preloaders.flat_map(&:preloaded_records) - - owners.each do |owner| - through_records = Array(owner.association(through_reflection.name).target) - - if already_loaded + PRELOADER = ActiveRecord::Associations::Preloader.new + + def initialize(*) + super + @already_loaded = owners.first.association(through_reflection.name).loaded? + end + + def preloaded_records + @preloaded_records ||= source_preloaders.flat_map(&:preloaded_records) + end + + def records_by_owner + return @records_by_owner if defined?(@records_by_owner) + source_records_by_owner = source_preloaders.map(&:records_by_owner).reduce(:merge) + through_records_by_owner = through_preloaders.map(&:records_by_owner).reduce(:merge) + + @records_by_owner = owners.each_with_object({}) do |owner, result| + through_records = through_records_by_owner[owner] || [] + + if @already_loaded if source_type = reflection.options[:source_type] through_records = through_records.select do |record| record[reflection.foreign_type] == source_type end end - else - owner.association(through_reflection.name).reset if through_scope end - result = through_records.flat_map do |record| - record.association(source_reflection.name).target + records = through_records.flat_map do |record| + source_records_by_owner[record] end - result.compact! - result.sort_by! { |rhs| preload_index[rhs] } if scope.order_values.any? - result.uniq! if scope.distinct_value - associate_records_to_owner(owner, result) - end - - unless scope.empty_scope? - middle_records.each do |owner| - owner.association(source_reflection.name).reset if owner - end + records.compact! + records.sort_by! { |rhs| preload_index[rhs] } if scope.order_values.any? + records.uniq! if scope.distinct_value + result[owner] = records end end private + def source_preloaders + @source_preloaders ||= PRELOADER.preload(middle_records, source_reflection.name, scope) + end + + def middle_records + through_preloaders.flat_map(&:preloaded_records) + end + + def through_preloaders + @through_preloaders ||= PRELOADER.preload(owners, through_reflection.name, through_scope) + end + def through_reflection reflection.through_reflection end @@ -52,8 +64,8 @@ module ActiveRecord end def preload_index - @preload_index ||= @preloaded_records.each_with_object({}).with_index do |(id, result), index| - result[id] = index + @preload_index ||= preloaded_records.each_with_object({}).with_index do |(record, result), index| + result[record] = index end end @@ -96,7 +108,7 @@ module ActiveRecord end end - scope unless scope.empty_scope? + scope end end end diff --git a/activerecord/test/cases/associations/nested_through_associations_test.rb b/activerecord/test/cases/associations/nested_through_associations_test.rb index 0b83fd8421..35da74102d 100644 --- a/activerecord/test/cases/associations/nested_through_associations_test.rb +++ b/activerecord/test/cases/associations/nested_through_associations_test.rb @@ -548,6 +548,15 @@ class NestedThroughAssociationsTest < ActiveRecord::TestCase end end + def test_through_association_preload_doesnt_reset_source_association_if_already_preloaded + blue = tags(:blue) + authors = Author.preload(posts: :first_blue_tags_2, misc_post_first_blue_tags_2: {}).to_a.sort_by(&:id) + + assert_no_queries do + assert_equal [blue], authors[2].posts.first.first_blue_tags_2 + end + end + def test_nested_has_many_through_with_conditions_on_source_associations_preload_via_joins # Pointless condition to force single-query loading assert_includes_and_joins_equal( diff --git a/activestorage/app/models/active_storage/attachment.rb b/activestorage/app/models/active_storage/attachment.rb index 13758d9179..874ba80ca8 100644 --- a/activestorage/app/models/active_storage/attachment.rb +++ b/activestorage/app/models/active_storage/attachment.rb @@ -46,3 +46,5 @@ class ActiveStorage::Attachment < ActiveRecord::Base record.attachment_reflections[name]&.options[:dependent] end end + +ActiveSupport.run_load_hooks :active_storage_attachment, ActiveStorage::Attachment diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 6ca7d49bc1..c9fbafad1f 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -193,17 +193,18 @@ class ActiveStorage::Blob < ActiveRecord::Base # # The tempfile's name is prefixed with +ActiveStorage-+ and the blob's ID. Its extension matches that of the blob. # - # By default, the tempfile is created in <tt>Dir.tmpdir</tt>. Pass +tempdir:+ to create it in a different directory: + # By default, the tempfile is created in <tt>Dir.tmpdir</tt>. Pass +tmpdir:+ to create it in a different directory: # - # blob.open(tempdir: "/path/to/tmp") do |file| + # blob.open(tmpdir: "/path/to/tmp") do |file| # # ... # end # # The tempfile is automatically closed and unlinked after the given block is executed. # # Raises ActiveStorage::IntegrityError if the downloaded data does not match the blob's checksum. - def open(tempdir: nil, &block) - ActiveStorage::Downloader.new(self, tempdir: tempdir).download_blob_to_tempfile(&block) + def open(tmpdir: nil, &block) + service.open key, checksum: checksum, + name: [ "ActiveStorage-#{id}-", filename.extension_with_delimiter ], tmpdir: tmpdir, &block end @@ -272,6 +273,6 @@ class ActiveStorage::Blob < ActiveRecord::Base { content_type: content_type } end end - - ActiveSupport.run_load_hooks(:active_storage_blob, self) end + +ActiveSupport.run_load_hooks :active_storage_blob, ActiveStorage::Blob diff --git a/activestorage/lib/active_storage/analyzer.rb b/activestorage/lib/active_storage/analyzer.rb index caa25418a5..26414ffbc2 100644 --- a/activestorage/lib/active_storage/analyzer.rb +++ b/activestorage/lib/active_storage/analyzer.rb @@ -24,14 +24,14 @@ module ActiveStorage private # Downloads the blob to a tempfile on disk. Yields the tempfile. def download_blob_to_tempfile(&block) #:doc: - blob.open tempdir: tempdir, &block + blob.open tmpdir: tmpdir, &block end def logger #:doc: ActiveStorage.logger end - def tempdir #:doc: + def tmpdir #:doc: Dir.tmpdir end end diff --git a/activestorage/lib/active_storage/downloader.rb b/activestorage/lib/active_storage/downloader.rb index 87be6efb05..4d7e832af5 100644 --- a/activestorage/lib/active_storage/downloader.rb +++ b/activestorage/lib/active_storage/downloader.rb @@ -2,24 +2,23 @@ module ActiveStorage class Downloader #:nodoc: - def initialize(blob, tempdir: nil) - @blob = blob - @tempdir = tempdir + attr_reader :service + + def initialize(service) + @service = service end - def download_blob_to_tempfile - open_tempfile do |file| - download_blob_to file - verify_integrity_of file + def open(key, checksum:, name: "ActiveStorage-", tmpdir: nil) + open_tempfile(name, tmpdir) do |file| + download key, file + verify_integrity_of file, checksum: checksum yield file end end private - attr_reader :blob, :tempdir - - def open_tempfile - file = Tempfile.open([ "ActiveStorage-#{blob.id}-", blob.filename.extension_with_delimiter ], tempdir) + def open_tempfile(name, tmpdir = nil) + file = Tempfile.open(name, tmpdir) begin yield file @@ -28,15 +27,15 @@ module ActiveStorage end end - def download_blob_to(file) + def download(key, file) file.binmode - blob.download { |chunk| file.write(chunk) } + service.download(key) { |chunk| file.write(chunk) } file.flush file.rewind end - def verify_integrity_of(file) - unless Digest::MD5.file(file).base64digest == blob.checksum + def verify_integrity_of(file, checksum:) + unless Digest::MD5.file(file).base64digest == checksum raise ActiveStorage::IntegrityError end end diff --git a/activestorage/lib/active_storage/previewer.rb b/activestorage/lib/active_storage/previewer.rb index 95a041fd16..af6bcadd4c 100644 --- a/activestorage/lib/active_storage/previewer.rb +++ b/activestorage/lib/active_storage/previewer.rb @@ -26,7 +26,7 @@ module ActiveStorage private # Downloads the blob to a tempfile on disk. Yields the tempfile. def download_blob_to_tempfile(&block) #:doc: - blob.open tempdir: tempdir, &block + blob.open tmpdir: tmpdir, &block end # Executes a system command, capturing its binary output in a tempfile. Yields the tempfile. @@ -42,7 +42,7 @@ module ActiveStorage # end # end # - # The output tempfile is opened in the directory returned by #tempdir. + # The output tempfile is opened in the directory returned by #tmpdir. def draw(*argv) #:doc: open_tempfile do |file| instrument :preview, key: blob.key do @@ -54,7 +54,7 @@ module ActiveStorage end def open_tempfile - tempfile = Tempfile.open("ActiveStorage-", tempdir) + tempfile = Tempfile.open("ActiveStorage-", tmpdir) begin yield tempfile @@ -77,7 +77,7 @@ module ActiveStorage ActiveStorage.logger end - def tempdir #:doc: + def tmpdir #:doc: Dir.tmpdir end end diff --git a/activestorage/lib/active_storage/service.rb b/activestorage/lib/active_storage/service.rb index c18fccbb1d..aac1e62e7f 100644 --- a/activestorage/lib/active_storage/service.rb +++ b/activestorage/lib/active_storage/service.rb @@ -82,6 +82,10 @@ module ActiveStorage raise NotImplementedError end + def open(*args, &block) + ActiveStorage::Downloader.new(self).open(*args, &block) + end + # Delete the file at the +key+. def delete(key) raise NotImplementedError diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 54cf9e2b8a..b7b37bacf5 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -104,14 +104,12 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase end end - test "open in a custom tempdir" do - tempdir = Dir.mktmpdir - - create_file_blob(filename: "racecar.jpg").open(tempdir: tempdir) do |file| + test "open in a custom tmpdir" do + create_file_blob(filename: "racecar.jpg").open(tmpdir: tmpdir = Dir.mktmpdir) do |file| assert file.binmode? assert_equal 0, file.pos assert_match(/\.jpg\z/, file.path) - assert file.path.starts_with?(tempdir) + assert file.path.starts_with?(tmpdir) assert_equal file_fixture("racecar.jpg").binread, file.read, "Expected downloaded file to match fixture file" end end diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 0551f0781f..f30249cb3f 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,17 @@ +* Fix bug in Range comparisons when comparing to an excluded-end Range + + Before: + + (1..10).cover?(1...11) => false + + After: + + (1..10).cover?(1...11) => true + + With the same change for `Range#include?` and `Range#===`. + + *Owen Stephens* + * Use weak references in descendants tracker to allow anonymous subclasses to be garbage collected. diff --git a/activesupport/lib/active_support/core_ext/range/compare_range.rb b/activesupport/lib/active_support/core_ext/range/compare_range.rb index 6f6d2a27bb..ea1dc29a76 100644 --- a/activesupport/lib/active_support/core_ext/range/compare_range.rb +++ b/activesupport/lib/active_support/core_ext/range/compare_range.rb @@ -3,9 +3,10 @@ module ActiveSupport module CompareWithRange # Extends the default Range#=== to support range comparisons. - # (1..5) === (1..5) # => true - # (1..5) === (2..3) # => true - # (1..5) === (2..6) # => false + # (1..5) === (1..5) # => true + # (1..5) === (2..3) # => true + # (1..5) === (1...6) # => true + # (1..5) === (2..6) # => false # # The native Range#=== behavior is untouched. # ('a'..'f') === ('c') # => true @@ -13,17 +14,20 @@ module ActiveSupport def ===(value) if value.is_a?(::Range) # 1...10 includes 1..9 but it does not include 1..10. + # 1..10 includes 1...11 but it does not include 1...12. operator = exclude_end? && !value.exclude_end? ? :< : :<= - super(value.first) && value.last.send(operator, last) + value_max = !exclude_end? && value.exclude_end? ? value.max : value.last + super(value.first) && value_max.send(operator, last) else super end end # Extends the default Range#include? to support range comparisons. - # (1..5).include?(1..5) # => true - # (1..5).include?(2..3) # => true - # (1..5).include?(2..6) # => false + # (1..5).include?(1..5) # => true + # (1..5).include?(2..3) # => true + # (1..5).include?(1...6) # => true + # (1..5).include?(2..6) # => false # # The native Range#include? behavior is untouched. # ('a'..'f').include?('c') # => true @@ -31,17 +35,20 @@ module ActiveSupport def include?(value) if value.is_a?(::Range) # 1...10 includes 1..9 but it does not include 1..10. + # 1..10 includes 1...11 but it does not include 1...12. operator = exclude_end? && !value.exclude_end? ? :< : :<= - super(value.first) && value.last.send(operator, last) + value_max = !exclude_end? && value.exclude_end? ? value.max : value.last + super(value.first) && value_max.send(operator, last) else super end end # Extends the default Range#cover? to support range comparisons. - # (1..5).cover?(1..5) # => true - # (1..5).cover?(2..3) # => true - # (1..5).cover?(2..6) # => false + # (1..5).cover?(1..5) # => true + # (1..5).cover?(2..3) # => true + # (1..5).cover?(1...6) # => true + # (1..5).cover?(2..6) # => false # # The native Range#cover? behavior is untouched. # ('a'..'f').cover?('c') # => true @@ -49,8 +56,10 @@ module ActiveSupport def cover?(value) if value.is_a?(::Range) # 1...10 covers 1..9 but it does not cover 1..10. + # 1..10 covers 1...11 but it does not cover 1...12. operator = exclude_end? && !value.exclude_end? ? :< : :<= - super(value.first) && value.last.send(operator, last) + value_max = !exclude_end? && value.exclude_end? ? value.max : value.last + super(value.first) && value_max.send(operator, last) else super end diff --git a/activesupport/lib/active_support/core_ext/string/output_safety.rb b/activesupport/lib/active_support/core_ext/string/output_safety.rb index 0b40c3d799..638152626b 100644 --- a/activesupport/lib/active_support/core_ext/string/output_safety.rb +++ b/activesupport/lib/active_support/core_ext/string/output_safety.rb @@ -135,10 +135,12 @@ module ActiveSupport #:nodoc: class SafeBuffer < String UNSAFE_STRING_METHODS = %w( capitalize chomp chop delete delete_prefix delete_suffix - downcase gsub lstrip next reverse rstrip slice squeeze strip - sub succ swapcase tr tr_s unicode_normalize upcase + downcase lstrip next reverse rstrip slice squeeze strip + succ swapcase tr tr_s unicode_normalize upcase ) + UNSAFE_STRING_METHODS_WITH_BACKREF = %w(gsub sub) + alias_method :original_concat, :concat private :original_concat @@ -253,11 +255,44 @@ module ActiveSupport #:nodoc: end end + UNSAFE_STRING_METHODS_WITH_BACKREF.each do |unsafe_method| + if unsafe_method.respond_to?(unsafe_method) + class_eval <<-EOT, __FILE__, __LINE__ + 1 + def #{unsafe_method}(*args, &block) # def gsub(*args, &block) + if block # if block + to_str.#{unsafe_method}(*args) { |*params| # to_str.gsub(*args) { |*params| + set_block_back_references(block, $~) # set_block_back_references(block, $~) + block.call(*params) # block.call(*params) + } # } + else # else + to_str.#{unsafe_method}(*args) # to_str.gsub(*args) + end # end + end # end + + def #{unsafe_method}!(*args, &block) # def gsub!(*args, &block) + @html_safe = false # @html_safe = false + if block # if block + super(*args) { |*params| # super(*args) { |*params| + set_block_back_references(block, $~) # set_block_back_references(block, $~) + block.call(*params) # block.call(*params) + } # } + else # else + super # super + end # end + end # end + EOT + end + end + private def html_escape_interpolated_argument(arg) (!html_safe? || arg.html_safe?) ? arg : CGI.escapeHTML(arg.to_s) end + + def set_block_back_references(block, match_data) + block.binding.eval("proc { |m| $~ = m }").call(match_data) + end end end diff --git a/activesupport/test/core_ext/range_ext_test.rb b/activesupport/test/core_ext/range_ext_test.rb index 4b8efb8a93..16d6a4c2f2 100644 --- a/activesupport/test/core_ext/range_ext_test.rb +++ b/activesupport/test/core_ext/range_ext_test.rb @@ -57,7 +57,7 @@ class RangeTest < ActiveSupport::TestCase end def test_should_include_other_with_exclusive_end - assert((1..10).include?(1...10)) + assert((1..10).include?(1...11)) end def test_should_compare_identical_inclusive @@ -69,7 +69,7 @@ class RangeTest < ActiveSupport::TestCase end def test_should_compare_other_with_exclusive_end - assert((1..10) === (1...10)) + assert((1..10) === (1...11)) end def test_exclusive_end_should_not_include_identical_with_inclusive_end @@ -93,6 +93,10 @@ class RangeTest < ActiveSupport::TestCase assert range.method(:include?) != range.method(:cover?) end + def test_should_cover_other_with_exclusive_end + assert((1..10).cover?(1...11)) + end + def test_overlaps_on_time time_range_1 = Time.utc(2005, 12, 10, 15, 30)..Time.utc(2005, 12, 10, 17, 30) time_range_2 = Time.utc(2005, 12, 10, 17, 00)..Time.utc(2005, 12, 10, 18, 00) diff --git a/activesupport/test/safe_buffer_test.rb b/activesupport/test/safe_buffer_test.rb index 32e1d2d5bf..b1a1c2d390 100644 --- a/activesupport/test/safe_buffer_test.rb +++ b/activesupport/test/safe_buffer_test.rb @@ -248,4 +248,22 @@ class SafeBufferTest < ActiveSupport::TestCase x = "Hello".html_safe assert_nil x[/a/, 1] end + + test "Should set back references" do + a = "foo123".html_safe + a2 = a.sub(/([a-z]+)([0-9]+)/) { $2 + $1 } + assert_equal "123foo", a2 + assert_not_predicate a2, :html_safe? + a.sub!(/([a-z]+)([0-9]+)/) { $2 + $1 } + assert_equal "123foo", a + assert_not_predicate a, :html_safe? + + b = "foo123 bar456".html_safe + b2 = b.gsub(/([a-z]+)([0-9]+)/) { $2 + $1 } + assert_equal "123foo 456bar", b2 + assert_not_predicate b2, :html_safe? + b.gsub!(/([a-z]+)([0-9]+)/) { $2 + $1 } + assert_equal "123foo 456bar", b + assert_not_predicate b, :html_safe? + end end diff --git a/guides/source/6_0_release_notes.md b/guides/source/6_0_release_notes.md index 04a02259e9..6c3091153f 100644 --- a/guides/source/6_0_release_notes.md +++ b/guides/source/6_0_release_notes.md @@ -237,6 +237,18 @@ Please refer to the [Changelog][active-model] for detailed changes. ### Notable changes +* Add a configuration option to customize format of the `ActiveModel::Errors#full_message`. + ([Pull Request](https://github.com/rails/rails/pull/32956)) + +* Add support for configuring attribute name for `has_secure_password`. + ([Pull Request](https://github.com/rails/rails/pull/26764)) + +* Add `#slice!` method to `ActiveModel::Errors`. + ([Pull Request](https://github.com/rails/rails/pull/34489)) + +* Add `ActiveModel::Errors#of_kind?` to check presence of a specific error. + ([Pull Request](https://github.com/rails/rails/pull/34866)) + Active Support -------------- diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 04ad5a56a2..86e2dd284e 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -310,7 +310,7 @@ All these configuration options are delegated to the `I18n` library. ### Configuring Active Model -* `config.active_model.i18n_full_message` is a boolean value which controls whether the `full_message` error format can be overridden at the attribute or model level in the locale files. This is `false` by default. +* `config.active_model.i18n_customize_full_message` is a boolean value which controls whether the `full_message` error format can be overridden at the attribute or model level in the locale files. This is `false` by default. ### Configuring Active Record diff --git a/guides/source/engines.md b/guides/source/engines.md index a00311bffb..0b17137270 100644 --- a/guides/source/engines.md +++ b/guides/source/engines.md @@ -1518,6 +1518,7 @@ To hook into the initialization process of one of the following classes use the | `ActiveJob::Base` | `active_job` | | `ActiveJob::TestCase` | `active_job_test_case` | | `ActiveRecord::Base` | `active_record` | +| `ActiveStorage::Attachment` | `active_storage_attachment` | | `ActiveStorage::Blob` | `active_storage_blob` | | `ActiveSupport::TestCase` | `active_support_test_case` | | `i18n` | `i18n` | diff --git a/guides/source/testing.md b/guides/source/testing.md index 93372b5c26..1fad02812b 100644 --- a/guides/source/testing.md +++ b/guides/source/testing.md @@ -1404,7 +1404,7 @@ If you find your helpers are cluttering `test_helper.rb`, you can extract them i ```ruby # lib/test/multiple_assertions.rb module MultipleAssertions - def assert_multiple_of_fourty_two(number) + def assert_multiple_of_forty_two(number) assert (number % 42 == 0), 'expected #{number} to be a multiple of 42' end end @@ -1419,8 +1419,8 @@ require 'test/multiple_assertions' class NumberTest < ActiveSupport::TestCase include MultipleAssertions - test '420 is a multiple of fourty two' do - assert_multiple_of_fourty_two 420 + test '420 is a multiple of forty two' do + assert_multiple_of_forty_two 420 end end ``` |