diff options
38 files changed, 468 insertions, 115 deletions
diff --git a/Gemfile.lock b/Gemfile.lock index 8d34d9a24b..6633c7a741 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -216,7 +216,7 @@ GEM em-socksify (0.3.2) eventmachine (>= 1.0.0.beta.4) erubi (1.8.0) - et-orbi (1.1.6) + et-orbi (1.2.1) tzinfo event_emitter (0.2.6) eventmachine (1.2.7) @@ -240,8 +240,8 @@ GEM ffi (1.10.0-java) ffi (1.10.0-x64-mingw32) ffi (1.10.0-x86-mingw32) - fugit (1.1.6) - et-orbi (~> 1.1, >= 1.1.6) + fugit (1.2.1) + et-orbi (~> 1.1, >= 1.1.8) raabro (~> 1.1) globalid (0.4.2) activesupport (>= 4.2.0) @@ -370,10 +370,10 @@ GEM thor raabro (1.1.6) racc (1.4.15) - rack (2.0.6) + rack (2.0.7) rack-cache (1.8.0) rack (>= 0.4) - rack-protection (2.0.4) + rack-protection (2.0.5) rack rack-proxy (0.6.5) rack @@ -391,7 +391,7 @@ GEM ffi (~> 1.0) rdoc (6.0.4) redcarpet (3.2.3) - redis (4.0.3) + redis (4.1.1) redis-namespace (1.6.0) redis (>= 3.0.4) regexp_parser (1.3.0) @@ -399,16 +399,16 @@ GEM declarative (< 0.1.0) declarative-option (< 0.2.0) uber (< 0.2.0) - resque (1.27.4) + resque (2.0.0) mono_logger (~> 1.0) multi_json (~> 1.0) - redis-namespace (~> 1.3) + redis-namespace (~> 1.6) sinatra (>= 0.9.2) vegas (~> 0.1.2) - resque-scheduler (4.3.1) + resque-scheduler (4.4.0) mono_logger (~> 1.0) - redis (>= 3.3, < 5) - resque (~> 1.26) + redis (>= 3.3) + resque (>= 1.26) rufus-scheduler (~> 3.2) retriable (3.1.2) rubocop (0.67.2) @@ -426,8 +426,8 @@ GEM ffi (~> 1.9) ruby_dep (1.5.0) rubyzip (1.2.2) - rufus-scheduler (3.5.2) - fugit (~> 1.1, >= 1.1.5) + rufus-scheduler (3.6.0) + fugit (~> 1.1, >= 1.1.6) safe_yaml (1.0.4) sass (3.7.2) sass-listen (~> 4.0.0) @@ -458,10 +458,10 @@ GEM faraday (~> 0.9) jwt (>= 1.5, < 3.0) multi_json (~> 1.10) - sinatra (2.0.4) + sinatra (2.0.5) mustermann (~> 1.0) rack (~> 2.0) - rack-protection (= 2.0.4) + rack-protection (= 2.0.5) tilt (~> 2.0) sneakers (2.11.0) bunny (~> 2.12) @@ -488,7 +488,7 @@ GEM thor (0.20.3) thread_safe (0.3.6) thread_safe (0.3.6-java) - tilt (2.0.8) + tilt (2.0.9) turbolinks (5.2.0) turbolinks-source (~> 5.2) turbolinks-source (5.2.0) diff --git a/actionpack/lib/action_dispatch/middleware/debug_view.rb b/actionpack/lib/action_dispatch/middleware/debug_view.rb index a03650254e..148662a48b 100644 --- a/actionpack/lib/action_dispatch/middleware/debug_view.rb +++ b/actionpack/lib/action_dispatch/middleware/debug_view.rb @@ -56,5 +56,11 @@ module ActionDispatch def protect_against_forgery? false end + + def params_valid? + @request.parameters + rescue ActionController::BadRequest + false + end end end diff --git a/actionpack/lib/action_dispatch/middleware/stack.rb b/actionpack/lib/action_dispatch/middleware/stack.rb index f0c869fba0..57e4adb457 100644 --- a/actionpack/lib/action_dispatch/middleware/stack.rb +++ b/actionpack/lib/action_dispatch/middleware/stack.rb @@ -34,7 +34,11 @@ module ActionDispatch end def build(app) - InstrumentationProxy.new(klass.new(app, *args, &block), inspect) + klass.new(app, *args, &block) + end + + def build_instrumented(app) + InstrumentationProxy.new(build(app), inspect) end end @@ -119,7 +123,14 @@ module ActionDispatch end def build(app = nil, &block) - middlewares.freeze.reverse.inject(app || block) { |a, e| e.build(a) } + instrumenting = ActiveSupport::Notifications.notifier.listening?(InstrumentationProxy::EVENT_NAME) + middlewares.freeze.reverse.inject(app || block) do |a, e| + if instrumenting + e.build_instrumented(a) + else + e.build(a) + end + end end private diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_request_and_response.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_request_and_response.html.erb index 49b1e83551..04271d8e8a 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/_request_and_response.html.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_request_and_response.html.erb @@ -6,7 +6,9 @@ <% end %> <h2 style="margin-top: 30px">Request</h2> -<p><b>Parameters</b>:</p> <pre><%= debug_params(@request.filtered_parameters) %></pre> +<% if params_valid? %> + <p><b>Parameters</b>:</p> <pre><%= debug_params(@request.filtered_parameters) %></pre> +<% end %> <div class="details"> <div class="summary"><a href="#" onclick="return toggleSessionDump()">Toggle session dump</a></div> diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_request_and_response.text.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_request_and_response.text.erb index 396768ecee..ca42a6fa8b 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/_request_and_response.text.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_request_and_response.text.erb @@ -1,5 +1,5 @@ <% - clean_params = @request.filtered_parameters.clone + clean_params = params_valid? ? @request.filtered_parameters.clone : {} clean_params.delete("action") clean_params.delete("controller") diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb index 999e84e4d6..57cdcf9aaf 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb @@ -1,7 +1,7 @@ <header> <h1> <%= @exception.class.to_s %> - <% if @request.parameters['controller'] %> + <% if params_valid? && @request.parameters['controller'] %> in <%= @request.parameters['controller'].camelize %>Controller<% if @request.parameters['action'] %>#<%= @request.parameters['action'] %><% end %> <% end %> </h1> diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.text.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.text.erb index 603de54b8b..d3265563a8 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.text.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.text.erb @@ -1,5 +1,5 @@ <%= @exception.class.to_s %><% - if @request.parameters['controller'] + if params_valid? && @request.parameters['controller'] %> in <%= @request.parameters['controller'].camelize %>Controller<% if @request.parameters['action'] %>#<%= @request.parameters['action'] %><% end %> <% end %> diff --git a/actionpack/test/controller/show_exceptions_test.rb b/actionpack/test/controller/show_exceptions_test.rb index 8724f9bcdb..1d68a359dc 100644 --- a/actionpack/test/controller/show_exceptions_test.rb +++ b/actionpack/test/controller/show_exceptions_test.rb @@ -99,7 +99,7 @@ module ShowExceptions class ShowFailsafeExceptionsTest < ActionDispatch::IntegrationTest def test_render_failsafe_exception @app = ShowExceptionsOverriddenController.action(:boom) - middleware = @app.instance_variable_get(:@middleware) + middleware = @app @exceptions_app = middleware.instance_variable_get(:@exceptions_app) middleware.instance_variable_set(:@exceptions_app, nil) $stderr = StringIO.new diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb index 5ae8a20ae4..3e57e8f4d9 100644 --- a/actionpack/test/dispatch/debug_exceptions_test.rb +++ b/actionpack/test/dispatch/debug_exceptions_test.rb @@ -620,4 +620,23 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest assert_select 'input[value="Action 2"]' end end + + test "debug exceptions app shows diagnostics when malformed query parameters are provided" do + @app = DevelopmentApp + + get "/bad_request?x[y]=1&x[y][][w]=2" + + assert_response 400 + assert_match "ActionController::BadRequest", body + end + + test "debug exceptions app shows diagnostics when malformed query parameters are provided by XHR" do + @app = DevelopmentApp + xhr_request_env = { "action_dispatch.show_exceptions" => true, "HTTP_X_REQUESTED_WITH" => "XMLHttpRequest" } + + get "/bad_request?x[y]=1&x[y][][w]=2", headers: xhr_request_env + + assert_response 400 + assert_match "ActionController::BadRequest", body + end end diff --git a/actionpack/test/dispatch/middleware_stack_test.rb b/actionpack/test/dispatch/middleware_stack_test.rb index 90f2eccd19..c534e60c74 100644 --- a/actionpack/test/dispatch/middleware_stack_test.rb +++ b/actionpack/test/dispatch/middleware_stack_test.rb @@ -121,9 +121,6 @@ class MiddlewareStackTest < ActiveSupport::TestCase end test "instruments the execution of middlewares" do - app = @stack.build(proc { |env| [200, {}, []] }) - env = {} - events = [] subscriber = proc do |*args| @@ -131,6 +128,9 @@ class MiddlewareStackTest < ActiveSupport::TestCase end ActiveSupport::Notifications.subscribed(subscriber, "process_middleware.action_dispatch") do + app = @stack.build(proc { |env| [200, {}, []] }) + + env = {} app.call(env) end diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index dc85750a22..608e417583 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -305,7 +305,7 @@ module ActionView else @template_keys = @locals.keys end - template = find_partial(@path, @template_keys) + template = find_template(@path, @template_keys) @variable ||= template.variable else if options[:cached] @@ -428,10 +428,6 @@ module ActionView @object.to_ary if @object.respond_to?(:to_ary) end - def find_partial(path, template_keys) - find_template(path, template_keys) - end - def find_template(path, locals) prefixes = path.include?(?/) ? [] : @lookup_context.prefixes @lookup_context.find_template(path, prefixes, true, locals, @details) diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index f0b5d7d95e..2235a7816f 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -33,6 +33,11 @@ module RenderTestCases assert_equal ORIGINAL_LOCALES, I18n.available_locales.map(&:to_s).sort end + def teardown + I18n.reload! + ActionController::Base.view_paths.map(&:clear_cache) + end + def test_implicit_format_comes_from_parent_template rendered_templates = JSON.parse(@controller_view.render(template: "test/mixing_formats")) assert_equal({ "format" => "HTML", @@ -677,11 +682,6 @@ class CachedViewRenderTest < ActiveSupport::TestCase assert_equal ActionView::OptimizedFileSystemResolver, view_paths.first.class setup_view(view_paths) end - - def teardown - GC.start - I18n.reload! - end end class LazyViewRenderTest < ActiveSupport::TestCase @@ -697,11 +697,6 @@ class LazyViewRenderTest < ActiveSupport::TestCase setup_view(view_paths) end - def teardown - GC.start - I18n.reload! - end - def test_render_utf8_template_with_magic_comment with_external_encoding Encoding::ASCII_8BIT do result = @view.render(template: "test/utf8_magic", formats: [:html], layouts: "layouts/yield") @@ -758,10 +753,6 @@ class CachedCollectionViewRenderTest < ActiveSupport::TestCase setup_view(view_paths) end - teardown do - I18n.reload! - end - test "template body written to cache" do customer = Customer.new("david", 1) key = cache_key(customer, "test/_customer") diff --git a/activerecord/lib/active_record/connection_adapters/abstract/savepoints.rb b/activerecord/lib/active_record/connection_adapters/abstract/savepoints.rb index 52a796b926..d6dbef3fc8 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/savepoints.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/savepoints.rb @@ -8,15 +8,15 @@ module ActiveRecord end def create_savepoint(name = current_savepoint_name) - execute("SAVEPOINT #{name}") + execute("SAVEPOINT #{name}", "TRANSACTION") end def exec_rollback_to_savepoint(name = current_savepoint_name) - execute("ROLLBACK TO SAVEPOINT #{name}") + execute("ROLLBACK TO SAVEPOINT #{name}", "TRANSACTION") end def release_savepoint(name = current_savepoint_name) - execute("RELEASE SAVEPOINT #{name}") + execute("RELEASE SAVEPOINT #{name}", "TRANSACTION") end end end diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb index c9e84e48cc..7b6321d83b 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb @@ -98,9 +98,13 @@ module ActiveRecord end def rollback_records - ite = records.uniq + ite = records.uniq(&:object_id) + already_run_callbacks = {} while record = ite.shift - record.rolledback!(force_restore_state: full_rollback?) + trigger_callbacks = record.trigger_transactional_callbacks? + should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks + already_run_callbacks[record] ||= trigger_callbacks + record.rolledback!(force_restore_state: full_rollback?, should_run_callbacks: should_run_callbacks) end ensure ite.each do |i| @@ -113,10 +117,14 @@ module ActiveRecord end def commit_records - ite = records.uniq + ite = records.uniq(&:object_id) + already_run_callbacks = {} while record = ite.shift if @run_commit_callbacks - record.committed! + trigger_callbacks = record.trigger_transactional_callbacks? + should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks + already_run_callbacks[record] ||= trigger_callbacks + record.committed!(should_run_callbacks: should_run_callbacks) else # if not running callbacks, only adds the record to the parent transaction connection.add_transaction_record(record) diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index 282b2b1838..d239ecff89 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -204,7 +204,7 @@ module ActiveRecord end def begin_db_transaction - execute "BEGIN" + execute("BEGIN", "TRANSACTION") end def begin_isolated_db_transaction(isolation) @@ -213,11 +213,11 @@ module ActiveRecord end def commit_db_transaction #:nodoc: - execute "COMMIT" + execute("COMMIT", "TRANSACTION") end def exec_rollback_db_transaction #:nodoc: - execute "ROLLBACK" + execute("ROLLBACK", "TRANSACTION") end def empty_insert_statement_value(primary_key = nil) diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb index d872bd662f..45ec79ca78 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb @@ -145,7 +145,7 @@ module ActiveRecord # Begins a transaction. def begin_db_transaction - execute "BEGIN" + execute("BEGIN", "TRANSACTION") end def begin_isolated_db_transaction(isolation) @@ -155,12 +155,12 @@ module ActiveRecord # Commits a transaction. def commit_db_transaction - execute "COMMIT" + execute("COMMIT", "TRANSACTION") end # Aborts a transaction. def exec_rollback_db_transaction - execute "ROLLBACK" + execute("ROLLBACK", "TRANSACTION") end private diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb index 46ce1a15b5..73b80e8c15 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb @@ -68,15 +68,15 @@ module ActiveRecord alias :exec_update :exec_delete def begin_db_transaction #:nodoc: - log("begin transaction", nil) { @connection.transaction } + log("begin transaction", "TRANSACTION") { @connection.transaction } end def commit_db_transaction #:nodoc: - log("commit transaction", nil) { @connection.commit } + log("commit transaction", "TRANSACTION") { @connection.commit } end def exec_rollback_db_transaction #:nodoc: - log("rollback transaction", nil) { @connection.rollback } + log("rollback transaction", "TRANSACTION") { @connection.rollback } end diff --git a/activerecord/lib/active_record/middleware/database_selector.rb b/activerecord/lib/active_record/middleware/database_selector.rb index b5b5df074c..93a1a39c3e 100644 --- a/activerecord/lib/active_record/middleware/database_selector.rb +++ b/activerecord/lib/active_record/middleware/database_selector.rb @@ -35,10 +35,10 @@ module ActiveRecord # config.active_record.database_resolver = MyResolver # config.active_record.database_resolver_context = MyResolver::MySession class DatabaseSelector - def initialize(app, resolver_klass = Resolver, context_klass = Resolver::Session, options = {}) + def initialize(app, resolver_klass = nil, context_klass = nil, options = {}) @app = app - @resolver_klass = resolver_klass - @context_klass = context_klass + @resolver_klass = resolver_klass || Resolver + @context_klass = context_klass || Resolver::Session @options = options end diff --git a/activerecord/lib/active_record/touch_later.rb b/activerecord/lib/active_record/touch_later.rb index b60cc96165..6872b7844e 100644 --- a/activerecord/lib/active_record/touch_later.rb +++ b/activerecord/lib/active_record/touch_later.rb @@ -18,6 +18,7 @@ module ActiveRecord surreptitiously_touch @_defer_touch_attrs add_to_transaction + @_new_record_before_last_commit ||= false # touch the parents as we are not calling the after_save callbacks self.class.reflect_on_all_associations(:belongs_to).each do |r| diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 9e03deede7..148bc0550c 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -333,7 +333,7 @@ module ActiveRecord # Ensure that it is not called if the object was never persisted (failed create), # but call it after the commit of a destroyed object. def committed!(should_run_callbacks: true) #:nodoc: - if should_run_callbacks && trigger_transactional_callbacks? + if should_run_callbacks @_committed_already_called = true _run_commit_without_transaction_enrollment_callbacks _run_commit_callbacks @@ -346,7 +346,7 @@ module ActiveRecord # Call the #after_rollback callbacks. The +force_restore_state+ argument indicates if the record # state should be rolled back to the beginning or just to the last savepoint. def rolledback!(force_restore_state: false, should_run_callbacks: true) #:nodoc: - if should_run_callbacks && trigger_transactional_callbacks? + if should_run_callbacks _run_rollback_callbacks _run_rollback_without_transaction_enrollment_callbacks end @@ -378,6 +378,11 @@ module ActiveRecord status end + def trigger_transactional_callbacks? # :nodoc: + (@_new_record_before_last_commit || _trigger_update_callback) && persisted? || + _trigger_destroy_callback && destroyed? + end + private attr_reader :_committed_already_called, :_trigger_update_callback, :_trigger_destroy_callback @@ -392,10 +397,7 @@ module ActiveRecord level: 0 } @_start_transaction_state[:level] += 1 - remember_new_record_before_last_commit - end - def remember_new_record_before_last_commit if _committed_already_called @_new_record_before_last_commit = false else @@ -457,10 +459,6 @@ module ActiveRecord self.class.connection.add_transaction_record(self) end - def trigger_transactional_callbacks? - @_new_record_before_last_commit && !new_record? || _trigger_update_callback || _trigger_destroy_callback - end - def has_transactional_callbacks? !_rollback_callbacks.empty? || !_commit_callbacks.empty? || !_before_commit_callbacks.empty? end diff --git a/activerecord/test/cases/adapters/postgresql/referential_integrity_test.rb b/activerecord/test/cases/adapters/postgresql/referential_integrity_test.rb index ba477c63f4..96cfabf58f 100644 --- a/activerecord/test/cases/adapters/postgresql/referential_integrity_test.rb +++ b/activerecord/test/cases/adapters/postgresql/referential_integrity_test.rb @@ -13,7 +13,7 @@ class PostgreSQLReferentialIntegrityTest < ActiveRecord::PostgreSQLTestCase end module MissingSuperuserPrivileges - def execute(sql) + def execute(sql, name = nil) if IS_REFERENTIAL_INTEGRITY_SQL.call(sql) super "BROKEN;" rescue nil # put transaction in broken state raise ActiveRecord::StatementInvalid, "PG::InsufficientPrivilege" @@ -24,7 +24,7 @@ class PostgreSQLReferentialIntegrityTest < ActiveRecord::PostgreSQLTestCase end module ProgrammerMistake - def execute(sql) + def execute(sql, name = nil) if IS_REFERENTIAL_INTEGRITY_SQL.call(sql) raise ArgumentError, "something is not right." else diff --git a/activerecord/test/cases/test_case.rb b/activerecord/test/cases/test_case.rb index 78dc0a6d9f..81f7226718 100644 --- a/activerecord/test/cases/test_case.rb +++ b/activerecord/test/cases/test_case.rb @@ -107,20 +107,12 @@ module ActiveRecord clear_log - self.ignored_sql = [/^SAVEPOINT/, /^ROLLBACK TO SAVEPOINT/, /^RELEASE SAVEPOINT/] - - attr_reader :ignore - - def initialize(ignore = Regexp.union(self.class.ignored_sql)) - @ignore = ignore - end - def call(name, start, finish, message_id, values) return if values[:cached] sql = values[:sql] self.class.log_all << sql - self.class.log << sql unless values[:name] == "SCHEMA" || ignore.match?(sql) + self.class.log << sql unless ["SCHEMA", "TRANSACTION"].include? values[:name] end end diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index 135e2cb382..100bd6a925 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/activerecord/test/cases/transaction_callbacks_test.rb @@ -551,6 +551,8 @@ class CallbacksOnMultipleActionsTest < ActiveRecord::TestCase end class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase + self.use_transactional_tests = false + class TopicWithHistory < ActiveRecord::Base self.table_name = :topics @@ -564,11 +566,22 @@ class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase end class TopicWithCallbacksOnDestroy < TopicWithHistory - after_commit(on: :destroy) { |record| record.class.history << :destroy } + after_commit(on: :destroy) { |record| record.class.history << :commit_on_destroy } + after_rollback(on: :destroy) { |record| record.class.history << :rollback_on_destroy } + + before_destroy :before_destroy_for_transaction + + private + def before_destroy_for_transaction; end end class TopicWithCallbacksOnUpdate < TopicWithHistory - after_commit(on: :update) { |record| record.class.history << :update } + after_commit(on: :update) { |record| record.class.history << :commit_on_update } + + before_save :before_save_for_transaction + + private + def before_save_for_transaction; end end def test_trigger_once_on_multiple_deletions @@ -576,10 +589,39 @@ class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase topic = TopicWithCallbacksOnDestroy.new topic.save topic_clone = TopicWithCallbacksOnDestroy.find(topic.id) + + topic.define_singleton_method(:before_destroy_for_transaction) do + topic_clone.destroy + end + topic.destroy - topic_clone.destroy - assert_equal [:destroy], TopicWithCallbacksOnDestroy.history + assert_equal [:commit_on_destroy], TopicWithCallbacksOnDestroy.history + end + + def test_rollback_on_multiple_deletions + TopicWithCallbacksOnDestroy.clear_history + topic = TopicWithCallbacksOnDestroy.new + topic.save + topic_clone = TopicWithCallbacksOnDestroy.find(topic.id) + + topic.define_singleton_method(:before_destroy_for_transaction) do + topic_clone.update!(author_name: "Test Author Clone") + topic_clone.destroy + end + + TopicWithCallbacksOnDestroy.transaction do + topic.update!(author_name: "Test Author") + topic.destroy + raise ActiveRecord::Rollback + end + + assert_not_predicate topic, :destroyed? + assert_not_predicate topic_clone, :destroyed? + assert_equal [nil, "Test Author"], topic.author_name_change_to_be_saved + assert_equal [nil, "Test Author Clone"], topic_clone.author_name_change_to_be_saved + + assert_equal [:rollback_on_destroy], TopicWithCallbacksOnDestroy.history end def test_trigger_on_update_where_row_was_deleted @@ -587,7 +629,11 @@ class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase topic = TopicWithCallbacksOnUpdate.new topic.save topic_clone = TopicWithCallbacksOnUpdate.find(topic.id) - topic.destroy + + topic_clone.define_singleton_method(:before_save_for_transaction) do + topic.destroy + end + topic_clone.author_name = "Test Author" topic_clone.save diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index ccdf2c3040..e494f66c5a 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -5,18 +5,19 @@ Before: - 'foobar'.truncate(5).frozen? - => true - 'foobar'.truncate(6).frozen? - => false + 'foobar'.truncate(5).frozen? + # => true + 'foobar'.truncate(6).frozen? + # => false After: - 'foobar'.truncate(5).frozen? - => false - 'foobar'.truncate(6).frozen? - => false + 'foobar'.truncate(5).frozen? + # => false + 'foobar'.truncate(6).frozen? + # => false *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/guides/source/6_0_release_notes.md b/guides/source/6_0_release_notes.md index 4eb5296c41..cb3ea7737c 100644 --- a/guides/source/6_0_release_notes.md +++ b/guides/source/6_0_release_notes.md @@ -467,7 +467,7 @@ Please refer to the [Changelog][active-record] for detailed changes. ([Pull Request](https://github.com/rails/rails/pull/35631)) * Add `rails db:seed:replant` that truncates tables of each database - for ther current environment and loads the seeds. + for the current environment and loads the seeds. ([Pull Request](https://github.com/rails/rails/pull/34779)) * Add `reselect` method, which is a short-hand for `unscope(:select).select(fields)`. diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 3863323bd2..3adc956ef9 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -145,7 +145,7 @@ defaults to `:debug` for all environments. The available log levels are: `:debug * `secret_key_base` is used for specifying a key which allows sessions for the application to be verified against a known secure key to prevent tampering. Applications get a random generated key in test and development environments, other environments should set one in `config/credentials.yml.enc`. -* `config.public_file_server.enabled` configures Rails to serve static files from the public directory. This option defaults to `true`, but in the production environment it is set to `false` because the server software (e.g. NGINX or Apache) used to run the application should serve static files instead. If you are running or testing your app in production mode using WEBrick (it is not recommended to use WEBrick in production) set the option to `true.` Otherwise, you won't be able to use page caching and request for files that exist under the public directory. +* `config.public_file_server.enabled` configures Rails to serve static files from the public directory. This option defaults to `true`, but in the production environment it is set to `false` because the server software (e.g. NGINX or Apache) used to run the application should serve static files instead. If you are running or testing your app in production mode using WEBrick (it is not recommended to use WEBrick in production) set the option to `true`. Otherwise, you won't be able to use page caching and request for files that exist under the public directory. * `config.session_store` specifies what class to use to store the session. Possible values are `:cookie_store` which is the default, `:mem_cache_store`, and `:disabled`. The last one tells Rails not to deal with sessions. Defaults to a cookie store with application name as the session key. Custom session stores can also be specified: diff --git a/guides/source/contributing_to_ruby_on_rails.md b/guides/source/contributing_to_ruby_on_rails.md index f86589bdf1..a6eb9907a0 100644 --- a/guides/source/contributing_to_ruby_on_rails.md +++ b/guides/source/contributing_to_ruby_on_rails.md @@ -340,7 +340,7 @@ $ TEST_DIR=generators bundle exec rake test You can run the tests for a particular file by using: ```bash -$ cd actionpack +$ cd actionview $ bundle exec ruby -w -Itest test/template/form_helper_test.rb ``` diff --git a/guides/source/testing.md b/guides/source/testing.md index 18eecf49fa..9540bb2af5 100644 --- a/guides/source/testing.md +++ b/guides/source/testing.md @@ -781,7 +781,7 @@ This can be helpful for viewing the browser at the point a test failed, or to view screenshots later for debugging. Two methods are provided: `take_screenshot` and `take_failed_screenshot`. -`take_failed_screenshot` is automatically included in `after_teardown` inside +`take_failed_screenshot` is automatically included in `before_teardown` inside Rails. The `take_screenshot` helper method can be included anywhere in your tests to diff --git a/guides/source/upgrading_ruby_on_rails.md b/guides/source/upgrading_ruby_on_rails.md index 7e4152aa51..290aabc6f9 100644 --- a/guides/source/upgrading_ruby_on_rails.md +++ b/guides/source/upgrading_ruby_on_rails.md @@ -133,6 +133,223 @@ Action Cable JavaScript API: + ActionCable.logger.enabled = false ``` +### Autoloading + +The default configuration for Rails 6 + +```ruby +# config/application.rb + +config.load_defaults "6.0" +``` + +enables `zeitwerk` autoloading mode on CRuby. In that mode, autoloading, reloading, and eager loading are managed by [Zeitwerk](https://github.com/fxn/zeitwerk). + +#### Public API + +In general, applications do not need to use the API of Zeitwerk directly. Rails sets things up according to the existing contract: `config.autoload_paths`, `config.cache_classes`, etc. + +While applications should stick to that interface, the actual Zeitwerk loader object can be accessed as + +```ruby +Rails.autoloaders.main +``` + +That may be handy if you need to preload STIs or configure a custom inflector, for example. + +#### Project Structure + +If the application being upgraded autoloads correctly, the project structure should be already mostly compatible. + +However, `classic` mode infers file names from missing constant names (`underscore`), whereas `zeitwerk` mode infers constant names from file names (`camelize`). These helpers are not always inverse of each other, in particular if acronyms are involved. For instance, `"FOO".underscore` is `"foo"`, but `"foo".camelize` is `"Foo"`, not `"FOO"`. Compatibility can be checked by setting `classic` mode first temporarily: + +```ruby +# config/application.rb + +config.load_defaults "6.0" +config.autoloader = :classic +``` + +and then running + +``` +bin/rails zeitwerk:check +``` + +When all is good, you can delete `config.autoloader = :classic`. + +#### require_dependency + +All known use cases of `require_dependency` have been eliminated, you should grep the project and delete them. + +In the case of STIs with a hierarchy of more than two levels, you can preload the leaves of the hierarchy in an initializer: + +```ruby +# config/initializers/preload_stis.rb + +# By preloading leaves, the entire hierarchy is loaded upwards following +# the references to superclasses in the class definitions. +sti_leaves = %w( + app/models/leaf1.rb + app/models/leaf2.rb + app/models/leaf3.rb +) +Rails.autoloaders.main.preload(sti_leaves) +``` + +#### Qualified names in class and module definitions + +You can now robustly use constant paths in class and module definitions: + +```ruby +# Autoloading in this class' body matches Ruby semantics now. +class Admin::UsersController < ApplicationController + # ... +end +``` + +A gotcha to be aware of is that, depending on the order of execution, the classic autoloader could sometimes be able to autoload `Foo::Wadus` in + +```ruby +class Foo::Bar + Wadus +end +``` + +That does not match Ruby semantics because `Foo` is not in the nesting, and won't work at all in `zeitwerk` mode. If you find such corner case you can use the qualified name `Foo::Wadus`: + +```ruby +class Foo::Bar + Foo::Wadus +end +``` + +or add `Foo` to the nesting: + +```ruby +module Foo + class Bar + Wadus + end +end +``` + +#### Concerns + +You can autoload and eager load from a standard structure like + +``` +app/models +app/models/concerns +``` + +In that case, `app/models/concerns` is assumed to be a root directory (because it belongs to the autoload paths), and it is ignored as namespace. So, `app/models/concerns/foo.rb` should define `Foo`, not `Concerns::Foo`. + +The `Concerns::` namespace worked with the classic autoloader as a side-effect of the implementation, but it was not really an intended behavior. An application using `Concerns::` needs to rename those classes and modules to be able to run in `zeitwerk` mode. + +#### Autoloaded Constants and Explicit Namespaces + +If a namespace is defined in a file, as `Hotel` is here: + +``` +app/models/hotel.rb # Defines Hotel. +app/models/hotel/pricing.rb # Defines Hotel::Pricing. +``` + +the `Hotel` constant has to be set using the `class` or `module` keywords. For example: + +```ruby +class Hotel +end +``` + +is good. + +Alternatives like + +```ruby +Hotel = Class.new +``` + +or + +```ruby +Hotel = Struct.new +``` + +won't work, child objects like `Hotel::Pricing` won't be found. + +This restriction only applies to explicit namespaces. Classes and modules not defining a namespace can be defined using those idioms. + +#### Spring and the `test` Environment + +Spring reloads the application code if something changes. In the `test` environment you need to enable reloading for that to work: + +```ruby +# config/environments/test.rb + +config.cache_classes = false +``` + +Otherwise you'll get this error: + +``` +reloading is disabled because config.cache_classes is true +``` + +#### Bootsnap + +Bootsnap should be at least version 1.4.2. + +In addition to that, Bootsnap needs to disable the iseq cache due to a bug in the interpreter if running Ruby 2.5. Please make sure to depend on at least Bootsnap 1.4.4 in that case. + +#### `config.add_autoload_paths_to_load_path` + +The new configuration point + +```ruby +config.add_autoload_paths_to_load_path +``` + +is `true` by default for backwards compatibility, but allows you to opt-out from adding the autoload paths to `$LOAD_PATH`. + +This makes sense in most applications, since you never should require a file in `app/models`, for example, and Zeitwerk only uses absolute file names internally. + +By opting-out you optimize `$LOAD_PATH` lookups (less directories to check), and save Bootsnap work and memory consumption, since it does not need to build an index for these directories. + +#### Thread-safety + +In classic mode constant autoloading is not thread-safe, though Rails has locks in place for example to make web requests thread-safe when autoloading is enabled, as it is common in `development` mode. + +Constant autoloading is thread-safe in `zeitwerk` mode. For example, you can now autoload in multi-threaded scripts executed by the `runner` command. + +#### Globs in config.autoload_paths + +Beware of configurations like + +```ruby +config.autoload_paths += Dir["#{config.root}/lib/**/"] +``` + +Every element of `config.autoload_paths` should represent the top-level namespace (`Object`) and they cannot be nested in consequence (with the exception of `concerns` directories explained above). + +To fix this, just remove the wildcards: + +```ruby +config.autoload_paths << "#{config.root}/lib" +``` + +#### How to Use the Classic Autoloader in Rails 6 + +Applications can load Rails 6 defaults and still use the classic autoloader by setting `config.autoloader` this way: + +```ruby +# config/application.rb + +config.load_defaults "6.0" +config.autoloader = :classic +``` + Upgrading from Rails 5.1 to Rails 5.2 ------------------------------------- diff --git a/railties/lib/rails/application/bootstrap.rb b/railties/lib/rails/application/bootstrap.rb index e3c0759f95..50685a4d7a 100644 --- a/railties/lib/rails/application/bootstrap.rb +++ b/railties/lib/rails/application/bootstrap.rb @@ -19,14 +19,14 @@ module Rails initializer :set_eager_load, group: :all do if config.eager_load.nil? - warn <<-INFO -config.eager_load is set to nil. Please update your config/environments/*.rb files accordingly: + warn <<~INFO + config.eager_load is set to nil. Please update your config/environments/*.rb files accordingly: - * development - set it to false - * test - set it to false (unless you use a tool that preloads your test environment) - * production - set it to true + * development - set it to false + * test - set it to false (unless you use a tool that preloads your test environment) + * production - set it to true -INFO + INFO config.eager_load = config.cache_classes end end diff --git a/railties/lib/rails/generators/app_base.rb b/railties/lib/rails/generators/app_base.rb index 7e3560d9d2..8782a85391 100644 --- a/railties/lib/rails/generators/app_base.rb +++ b/railties/lib/rails/generators/app_base.rb @@ -307,7 +307,7 @@ module Rails def assets_gemfile_entry return [] if options[:skip_sprockets] - GemfileEntry.version("sass-rails", "~> 5", "Use SCSS for stylesheets") + GemfileEntry.version("sass-rails", ">= 5", "Use SCSS for stylesheets") end def webpacker_gemfile_entry diff --git a/railties/lib/rails/generators/erb/scaffold/templates/show.html.erb.tt b/railties/lib/rails/generators/erb/scaffold/templates/show.html.erb.tt index 6b216001d2..d3f996188c 100644 --- a/railties/lib/rails/generators/erb/scaffold/templates/show.html.erb.tt +++ b/railties/lib/rails/generators/erb/scaffold/templates/show.html.erb.tt @@ -4,7 +4,7 @@ <p> <strong><%= attribute.human_name %>:</strong> <% if attribute.attachment? -%> - <%%= link_to @<%= singular_table_name %>.<%= attribute.column_name %>.filename, @<%= singular_table_name %>.<%= attribute.column_name %> %> + <%%= link_to @<%= singular_table_name %>.<%= attribute.column_name %>.filename, @<%= singular_table_name %>.<%= attribute.column_name %> if @<%= singular_table_name %>.<%= attribute.column_name %>.attached? %> <% elsif attribute.attachments? -%> <%% @<%= singular_table_name %>.<%= attribute.column_name %>.each do |<%= attribute.singular_name %>| %> <div><%%= link_to <%= attribute.singular_name %>.filename, <%= attribute.singular_name %> %></div> diff --git a/railties/lib/rails/generators/named_base.rb b/railties/lib/rails/generators/named_base.rb index 42e64cd11f..d6732f8ff1 100644 --- a/railties/lib/rails/generators/named_base.rb +++ b/railties/lib/rails/generators/named_base.rb @@ -187,7 +187,6 @@ module Rails def attributes_names # :doc: @attributes_names ||= attributes.each_with_object([]) do |a, names| - next if a.attachments? names << a.column_name names << "password_confirmation" if a.password_digest? names << "#{a.name}_type" if a.polymorphic? diff --git a/railties/lib/rails/generators/rails/scaffold_controller/scaffold_controller_generator.rb b/railties/lib/rails/generators/rails/scaffold_controller/scaffold_controller_generator.rb index 8b46eb88ae..e1ca54ec91 100644 --- a/railties/lib/rails/generators/rails/scaffold_controller/scaffold_controller_generator.rb +++ b/railties/lib/rails/generators/rails/scaffold_controller/scaffold_controller_generator.rb @@ -36,9 +36,15 @@ module Rails private def permitted_params - params = attributes_names.map { |name| ":#{name}" }.join(", ") - params += attributes.select(&:attachments?).map { |a| ", #{a.name}: []" }.join - params + attachments, others = attributes_names.partition { |name| attachments?(name) } + params = others.map { |name| ":#{name}" } + params += attachments.map { |name| "#{name}: []" } + params.join(", ") + end + + def attachments?(name) + attribute = attributes.find { |attr| attr.name == name } + attribute&.attachments? end end end diff --git a/railties/lib/rails/generators/test_unit/scaffold/scaffold_generator.rb b/railties/lib/rails/generators/test_unit/scaffold/scaffold_generator.rb index 6df50c3217..26002a0704 100644 --- a/railties/lib/rails/generators/test_unit/scaffold/scaffold_generator.rb +++ b/railties/lib/rails/generators/test_unit/scaffold/scaffold_generator.rb @@ -49,16 +49,21 @@ module TestUnit # :nodoc: attributes_names.map do |name| if %w(password password_confirmation).include?(name) && attributes.any?(&:password_digest?) ["#{name}", "'secret'"] - else + elsif !virtual?(name) ["#{name}", "@#{singular_table_name}.#{name}"] end - end.sort.to_h + end.compact.sort.to_h end def boolean?(name) attribute = attributes.find { |attr| attr.name == name } attribute&.type == :boolean end + + def virtual?(name) + attribute = attributes.find { |attr| attr.name == name } + attribute&.virtual? + end end end end diff --git a/railties/test/application/middleware/exceptions_test.rb b/railties/test/application/middleware/exceptions_test.rb index 17df78ed4e..5fae521937 100644 --- a/railties/test/application/middleware/exceptions_test.rb +++ b/railties/test/application/middleware/exceptions_test.rb @@ -136,5 +136,21 @@ module ApplicationTests assert_match(/boooom/, last_response.body) assert_match(/測試テスト시험/, last_response.body) end + + test "displays diagnostics message when malformed query parameters are provided" do + controller :foo, <<-RUBY + class FooController < ActionController::Base + def index + end + end + RUBY + + app.config.action_dispatch.show_exceptions = true + app.config.consider_all_requests_local = true + + get "/foo?x[y]=1&x[y][][w]=2" + assert_equal 400, last_response.status + assert_match "Invalid query parameters", last_response.body + end end end diff --git a/railties/test/generators/scaffold_controller_generator_test.rb b/railties/test/generators/scaffold_controller_generator_test.rb index 1348744b0b..8278b72a5a 100644 --- a/railties/test/generators/scaffold_controller_generator_test.rb +++ b/railties/test/generators/scaffold_controller_generator_test.rb @@ -89,6 +89,15 @@ class ScaffoldControllerGeneratorTest < Rails::Generators::TestCase end end + def test_controller_permit_attachments_attributes_only + run_generator ["Message", "photos:attachments"] + + assert_file "app/controllers/messages_controller.rb" do |content| + assert_match(/def message_params/, content) + assert_match(/params\.require\(:message\)\.permit\(photos: \[\]\)/, content) + end + end + def test_helper_are_invoked_with_a_pluralized_name run_generator assert_file "app/helpers/users_helper.rb", /module UsersHelper/ diff --git a/railties/test/generators/scaffold_generator_test.rb b/railties/test/generators/scaffold_generator_test.rb index bfa52a1beb..fa9a42215b 100644 --- a/railties/test/generators/scaffold_generator_test.rb +++ b/railties/test/generators/scaffold_generator_test.rb @@ -487,6 +487,36 @@ class ScaffoldGeneratorTest < Rails::Generators::TestCase assert_match(/^\W{4}<%= form\.file_field :video %>/, content) assert_match(/^\W{4}<%= form\.file_field :photos, multiple: true %>/, content) end + + assert_file "app/views/messages/show.html.erb" do |content| + assert_match(/^\W{2}<%= link_to @message\.video\.filename, @message\.video if @message\.video\.attached\? %>/, content) + assert_match(/^\W{4}<div><%= link_to photo\.filename, photo %>/, content) + end + + assert_file "test/system/messages_test.rb" do |content| + assert_no_match(/fill_in "Video"/, content) + assert_no_match(/fill_in "Photos"/, content) + end + end + + def test_scaffold_generator_rich_text + run_generator ["message", "content:rich_text"] + + assert_file "app/models/message.rb", /rich_text :content/ + + assert_file "app/controllers/messages_controller.rb" do |content| + assert_instance_method :message_params, content do |m| + assert_match(/permit\(:content\)/, m) + end + end + + assert_file "app/views/messages/_form.html.erb" do |content| + assert_match(/^\W{4}<%= form\.rich_text_area :content %>/, content) + end + + assert_file "test/system/messages_test.rb" do |content| + assert_no_match(/fill_in "Content"/, content) + end end def test_scaffold_generator_database |