diff options
58 files changed, 862 insertions, 390 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 7645b2b0e7..f7e9104627 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,13 @@ +* `ActionDispatch::Http::UploadedFile` now delegates `to_path` to its tempfile. + + This allows uploaded file objects to be passed directly to `File.read` + without raising a `TypeError`: + + uploaded_file = ActionDispatch::Http::UploadedFile.new(tempfile: tmp_file) + File.read(uploaded_file) + + *Aaron Kromer* + * Pass along arguments to underlying `get` method in `follow_redirect!` Now all arguments passed to `follow_redirect!` are passed to the underlying diff --git a/actionpack/lib/action_controller/metal/http_authentication.rb b/actionpack/lib/action_controller/metal/http_authentication.rb index 01676f3237..a871ccd533 100644 --- a/actionpack/lib/action_controller/metal/http_authentication.rb +++ b/actionpack/lib/action_controller/metal/http_authentication.rb @@ -56,8 +56,9 @@ module ActionController # In your integration tests, you can do something like this: # # def test_access_granted_from_xml - # @request.env['HTTP_AUTHORIZATION'] = ActionController::HttpAuthentication::Basic.encode_credentials(users(:dhh).name, users(:dhh).password) - # get "/notes/1.xml" + # authorization = ActionController::HttpAuthentication::Basic.encode_credentials(users(:dhh).name, users(:dhh).password) + # + # get "/notes/1.xml", headers: { 'HTTP_AUTHORIZATION' => authorization } # # assert_equal 200, status # end @@ -389,10 +390,9 @@ module ActionController # In your integration tests, you can do something like this: # # def test_access_granted_from_xml - # get( - # "/notes/1.xml", nil, - # 'HTTP_AUTHORIZATION' => ActionController::HttpAuthentication::Token.encode_credentials(users(:dhh).token) - # ) + # authorization = ActionController::HttpAuthentication::Token.encode_credentials(users(:dhh).token) + # + # get "/notes/1.xml", headers: { 'HTTP_AUTHORIZATION' => authorization } # # assert_equal 200, status # end diff --git a/actionpack/lib/action_controller/renderer.rb b/actionpack/lib/action_controller/renderer.rb index 49c5b782f0..2d1523f0fc 100644 --- a/actionpack/lib/action_controller/renderer.rb +++ b/actionpack/lib/action_controller/renderer.rb @@ -71,6 +71,21 @@ module ActionController end # Render templates with any options from ActionController::Base#render_to_string. + # + # The primary options are: + # * <tt>:partial</tt> - See <tt>ActionView::PartialRenderer</tt> for details. + # * <tt>:file</tt> - Renders an explicit template file. Add <tt>:locals</tt> to pass in, if so desired. + # It shouldn’t be used directly with unsanitized user input due to lack of validation. + # * <tt>:inline</tt> - Renders a ERB template string. + # * <tt>:plain</tt> - Renders provided text and sets the content type as <tt>text/plain</tt>. + # * <tt>:html</tt> - Renders the provided HTML safe string, otherwise + # performs HTML escape on the string first. Sets the content type as <tt>text/html</tt>. + # * <tt>:json</tt> - Renders the provided hash or object in JSON. You don't + # need to call <tt>.to_json<tt> on the object you want to render. + # * <tt>:body</tt> - Renders provided text and sets content type of <tt>text/plain</tt>. + # + # If no <tt>options</tt> hash is passed or if <tt>:update</tt> is specified, the default is + # to render a partial and use the second parameter as the locals hash. def render(*args) raise "missing controller" unless controller diff --git a/actionpack/lib/action_dispatch/http/upload.rb b/actionpack/lib/action_dispatch/http/upload.rb index 0b162dc7f1..827f022ca2 100644 --- a/actionpack/lib/action_dispatch/http/upload.rb +++ b/actionpack/lib/action_dispatch/http/upload.rb @@ -65,6 +65,11 @@ module ActionDispatch @tempfile.path end + # Shortcut for +tempfile.to_path+. + def to_path + @tempfile.to_path + end + # Shortcut for +tempfile.rewind+. def rewind @tempfile.rewind diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb index 33edad8bd9..077a83b112 100644 --- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb @@ -152,23 +152,13 @@ module ActionDispatch end def create_template(request, wrapper) - traces = wrapper.traces - - trace_to_show = "Application Trace" - if traces[trace_to_show].empty? && wrapper.rescue_template != "routing_error" - trace_to_show = "Full Trace" - end - - if source_to_show = traces[trace_to_show].first - source_to_show_id = source_to_show[:id] - end - DebugView.new([RESCUES_TEMPLATE_PATH], request: request, + exception_wrapper: wrapper, exception: wrapper.exception, - traces: traces, - show_source_idx: source_to_show_id, - trace_to_show: trace_to_show, + traces: wrapper.traces, + show_source_idx: wrapper.source_to_show_id, + trace_to_show: wrapper.trace_to_show, routes_inspector: routes_inspector(wrapper.exception), source_extracts: wrapper.source_extracts, line_number: wrapper.line_number, diff --git a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb index f05c69137b..fb2b2bd3b0 100644 --- a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb +++ b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb @@ -31,11 +31,12 @@ module ActionDispatch "ActionController::MissingExactTemplate" => "missing_exact_template", ) - attr_reader :backtrace_cleaner, :exception, :line_number, :file + attr_reader :backtrace_cleaner, :exception, :wrapped_causes, :line_number, :file def initialize(backtrace_cleaner, exception) @backtrace_cleaner = backtrace_cleaner @exception = original_exception(exception) + @wrapped_causes = wrapped_causes_for(exception, backtrace_cleaner) expand_backtrace if exception.is_a?(SyntaxError) || exception.cause.is_a?(SyntaxError) end @@ -66,7 +67,11 @@ module ActionDispatch full_trace_with_ids = [] full_trace.each_with_index do |trace, idx| - trace_with_id = { id: idx, trace: trace } + trace_with_id = { + exception_object_id: @exception.object_id, + id: idx, + trace: trace + } if application_trace.include?(trace) application_trace_with_ids << trace_with_id @@ -99,6 +104,18 @@ module ActionDispatch end end + def trace_to_show + if traces["Application Trace"].empty? && rescue_template != "routing_error" + "Full Trace" + else + "Application Trace" + end + end + + def source_to_show_id + (traces[trace_to_show].first || {})[:id] + end + private def backtrace @@ -113,6 +130,16 @@ module ActionDispatch end end + def causes_for(exception) + return enum_for(__method__, exception) unless block_given? + + yield exception while exception = exception.cause + end + + def wrapped_causes_for(exception, backtrace_cleaner) + causes_for(exception).map { |cause| self.class.new(backtrace_cleaner, cause) } + end + def clean_backtrace(*args) if backtrace_cleaner backtrace_cleaner.clean(backtrace, *args) diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.html.erb index e7b913bbe4..88a8e6ad83 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.html.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.html.erb @@ -1,6 +1,8 @@ -<% @source_extracts.each_with_index do |source_extract, index| %> +<% error_index = local_assigns[:error_index] || 0 %> + +<% source_extracts.each_with_index do |source_extract, index| %> <% if source_extract[:code] %> - <div class="source <%="hidden" if @show_source_idx != index%>" id="frame-source-<%=index%>"> + <div class="source <%= "hidden" if show_source_idx != index %>" id="frame-source-<%= error_index %>-<%= index %>"> <div class="info"> Extracted source (around line <strong>#<%= source_extract[:line_number] %></strong>): </div> diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb index ab57b11c7d..835ca8d260 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb @@ -1,52 +1,62 @@ -<% names = @traces.keys %> +<% names = traces.keys %> +<% error_index = local_assigns[:error_index] || 0 %> <p><code>Rails.root: <%= defined?(Rails) && Rails.respond_to?(:root) ? Rails.root : "unset" %></code></p> -<div id="traces"> +<div id="traces-<%= error_index %>"> <% names.each do |name| %> <% - show = "show('#{name.gsub(/\s/, '-')}');" - hide = (names - [name]).collect {|hide_name| "hide('#{hide_name.gsub(/\s/, '-')}');"} + show = "show('#{name.gsub(/\s/, '-')}-#{error_index}');" + hide = (names - [name]).collect {|hide_name| "hide('#{hide_name.gsub(/\s/, '-')}-#{error_index}');"} %> <a href="#" onclick="<%= hide.join %><%= show %>; return false;"><%= name %></a> <%= '|' unless names.last == name %> <% end %> - <% @traces.each do |name, trace| %> - <div id="<%= name.gsub(/\s/, '-') %>" style="display: <%= (name == @trace_to_show) ? 'block' : 'none' %>;"> - <pre><code><% trace.each do |frame| %><a class="trace-frames" data-frame-id="<%= frame[:id] %>" href="#"><%= frame[:trace] %></a><br><% end %></code></pre> + <% traces.each do |name, trace| %> + <div id="<%= "#{name.gsub(/\s/, '-')}-#{error_index}" %>" style="display: <%= (name == trace_to_show) ? 'block' : 'none' %>;"> + <code style="font-size: 11px;"> + <% trace.each do |frame| %> + <a class="trace-frames trace-frames-<%= error_index %>" data-exception-object-id="<%= frame[:exception_object_id] %>" data-frame-id="<%= frame[:id] %>" href="#"> + <%= frame[:trace] %> + </a> + <br> + <% end %> + </code> </div> <% end %> <script type="text/javascript"> - var traceFrames = document.getElementsByClassName('trace-frames'); - var selectedFrame, currentSource = document.getElementById('frame-source-0'); - - // Add click listeners for all stack frames - for (var i = 0; i < traceFrames.length; i++) { - traceFrames[i].addEventListener('click', function(e) { - e.preventDefault(); - var target = e.target; - var frame_id = target.dataset.frameId; - - if (selectedFrame) { - selectedFrame.className = selectedFrame.className.replace("selected", ""); - } - - target.className += " selected"; - selectedFrame = target; - - // Change the extracted source code - changeSourceExtract(frame_id); - }); - - function changeSourceExtract(frame_id) { - var el = document.getElementById('frame-source-' + frame_id); - if (currentSource && el) { - currentSource.className += " hidden"; - el.className = el.className.replace(" hidden", ""); - currentSource = el; + (function() { + var traceFrames = document.getElementsByClassName('trace-frames-<%= error_index %>'); + var selectedFrame, currentSource = document.getElementById('frame-source-<%= error_index %>-0'); + + // Add click listeners for all stack frames + for (var i = 0; i < traceFrames.length; i++) { + traceFrames[i].addEventListener('click', function(e) { + e.preventDefault(); + var target = e.target; + var frame_id = target.dataset.frameId; + + if (selectedFrame) { + selectedFrame.className = selectedFrame.className.replace("selected", ""); + } + + target.className += " selected"; + selectedFrame = target; + + // Change the extracted source code + changeSourceExtract(frame_id); + }); + + function changeSourceExtract(frame_id) { + var el = document.getElementById('frame-source-<%= error_index %>-' + frame_id); + if (currentSource && el) { + currentSource.className += " hidden"; + el.className = el.className.replace(" hidden", ""); + currentSource = el; + } } } - } + })(); </script> </div> 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 f154021ae6..bde26f46c2 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/diagnostics.html.erb @@ -10,7 +10,25 @@ <div id="container"> <h2><%= h @exception.message %></h2> - <%= render template: "rescues/_source" %> - <%= render template: "rescues/_trace" %> + <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx, error_index: 0 %> + <%= render "rescues/trace", traces: @traces, trace_to_show: @trace_to_show, error_index: 0 %> + + <% if @exception.cause %> + <h2>Exception Causes</h2> + <% end %> + + <% @exception_wrapper.wrapped_causes.each.with_index(1) do |wrapper, index| %> + <div class="details"> + <a class="summary" href="#" style="color: #F0F0F0; text-decoration: none; background: #C52F24; border-bottom: none;" onclick="return toggle(<%= wrapper.exception.object_id %>)"> + <%= wrapper.exception.class.name %>: <%= h wrapper.exception.message %> + </a> + </div> + + <div id="<%= wrapper.exception.object_id %>" style="display: none;"> + <%= render "rescues/source", source_extracts: wrapper.source_extracts, show_source_idx: wrapper.source_to_show_id, error_index: index %> + <%= render "rescues/trace", traces: wrapper.traces, trace_to_show: wrapper.trace_to_show, error_index: index %> + </div> + <% end %> + <%= render template: "rescues/_request_and_response" %> </div> diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb index e1b129ccc5..056d99b03f 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/invalid_statement.html.erb @@ -15,7 +15,7 @@ <% end %> </h2> - <%= render template: "rescues/_source" %> - <%= render template: "rescues/_trace" %> + <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx %> + <%= render "rescues/trace", traces: @traces, trace_to_show: @trace_to_show %> <%= render template: "rescues/_request_and_response" %> </div> diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/missing_template.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/missing_template.html.erb index 2a65fd06ad..22eb6e9b4e 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/missing_template.html.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/missing_template.html.erb @@ -5,7 +5,7 @@ <div id="container"> <h2><%= h @exception.message %></h2> - <%= render template: "rescues/_source" %> - <%= render template: "rescues/_trace" %> + <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx %> + <%= render "rescues/trace", traces: @traces, trace_to_show: @trace_to_show %> <%= render template: "rescues/_request_and_response" %> </div> diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/routing_error.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/routing_error.html.erb index 55dd5ddc7b..2b8f3f2a5e 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/routing_error.html.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/routing_error.html.erb @@ -14,7 +14,7 @@ </p> <% end %> - <%= render template: "rescues/_trace" %> + <%= render "rescues/trace", traces: @traces, trace_to_show: @trace_to_show %> <% if @routes_inspector %> <h2> diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/template_error.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/template_error.html.erb index 5060da9369..324ef1567a 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/template_error.html.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/template_error.html.erb @@ -11,10 +11,10 @@ </p> <pre><code><%= h @exception.message %></code></pre> - <%= render template: "rescues/_source" %> + <%= render "rescues/source", source_extracts: @source_extracts, show_source_idx: @show_source_idx %> <p><%= @exception.sub_template_message %></p> - <%= render template: "rescues/_trace" %> + <%= render "rescues/trace", traces: @traces, trace_to_show: @trace_to_show %> <%= render template: "rescues/_request_and_response" %> </div> diff --git a/actionpack/lib/action_dispatch/testing/request_encoder.rb b/actionpack/lib/action_dispatch/testing/request_encoder.rb index 01246b7a2e..9889f61951 100644 --- a/actionpack/lib/action_dispatch/testing/request_encoder.rb +++ b/actionpack/lib/action_dispatch/testing/request_encoder.rb @@ -34,7 +34,7 @@ module ActionDispatch end def encode_params(params) - @param_encoder.call(params) + @param_encoder.call(params) if params end def self.parser(content_type) diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb index 41812a82e1..39ede1442a 100644 --- a/actionpack/test/controller/integration_test.rb +++ b/actionpack/test/controller/integration_test.rb @@ -1079,6 +1079,20 @@ class IntegrationRequestEncodersTest < ActionDispatch::IntegrationTest end end + def test_get_request_with_json_excludes_null_query_string + with_routing do |routes| + routes.draw do + ActiveSupport::Deprecation.silence do + get ":action" => FooController + end + end + + get "/foos_json", as: :json + + assert_equal "http://www.example.com/foos_json", request.url + end + end + private def post_to_foos(as:) with_routing do |routes| diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb index 045567ff83..44b79c0e5d 100644 --- a/actionpack/test/dispatch/debug_exceptions_test.rb +++ b/actionpack/test/dispatch/debug_exceptions_test.rb @@ -26,6 +26,18 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest raise StandardError.new "error in framework" end + def raise_nested_exceptions + begin + raise "First error" + rescue + begin + raise "Second error" + rescue + raise "Third error" + end + end + end + def call(env) env["action_dispatch.show_detailed_exceptions"] = @detailed req = ActionDispatch::Request.new(env) @@ -74,6 +86,8 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest end when %r{/framework_raises} method_that_raises + when %r{/nested_exceptions} + raise_nested_exceptions else raise "puke!" end @@ -440,8 +454,8 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest get "/original_syntax_error", headers: { "action_dispatch.backtrace_cleaner" => ActiveSupport::BacktraceCleaner.new } assert_response 500 - assert_select "#Application-Trace" do - assert_select "pre code", /syntax error, unexpected/ + assert_select "#Application-Trace-0" do + assert_select "code", /syntax error, unexpected/ end end @@ -454,9 +468,9 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest assert_select "#container h2", /^Missing template/ - assert_select "#Application-Trace" - assert_select "#Framework-Trace" - assert_select "#Full-Trace" + assert_select "#Application-Trace-0" + assert_select "#Framework-Trace-0" + assert_select "#Full-Trace-0" assert_select "h2", /Request/ end @@ -467,8 +481,8 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest get "/syntax_error_into_view", headers: { "action_dispatch.backtrace_cleaner" => ActiveSupport::BacktraceCleaner.new } assert_response 500 - assert_select "#Application-Trace" do - assert_select "pre code", /syntax error, unexpected/ + assert_select "#Application-Trace-0" do + assert_select "code", /syntax error, unexpected/ end end @@ -497,13 +511,13 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest end # assert application trace refers to line that calls method_that_raises is first - assert_select "#Application-Trace" do - assert_select "pre code a:first", %r{test/dispatch/debug_exceptions_test\.rb:\d+:in `call} + assert_select "#Application-Trace-0" do + assert_select "code a:first", %r{test/dispatch/debug_exceptions_test\.rb:\d+:in `call} end # assert framework trace that threw the error is first - assert_select "#Framework-Trace" do - assert_select "pre code a:first", /method_that_raises/ + assert_select "#Framework-Trace-0" do + assert_select "code a:first", /method_that_raises/ end end end @@ -523,4 +537,39 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest assert_response 500 assert_match(/puke/, body) end + + test "debug exceptions app shows all the nested exceptions in source view" do + @app = DevelopmentApp + Rails.stub :root, Pathname.new(".") do + cleaner = ActiveSupport::BacktraceCleaner.new.tap do |bc| + bc.add_silencer { |line| line !~ %r{test/dispatch/debug_exceptions_test.rb} } + end + + get "/nested_exceptions", headers: { "action_dispatch.backtrace_cleaner" => cleaner } + + # Assert correct error + assert_response 500 + assert_select "h2", /Third error/ + + # assert source view line shows the last error + assert_select "div.source:not(.hidden)" do + assert_select "pre .line.active", /raise "Third error"/ + end + + # assert application trace refers to line that raises the last exception + assert_select "#Application-Trace-0" do + assert_select "code a:first", %r{in `rescue in rescue in raise_nested_exceptions'} + end + + # assert the second application trace refers to the line that raises the second exception + assert_select "#Application-Trace-1" do + assert_select "code a:first", %r{in `rescue in raise_nested_exceptions'} + end + + # assert the third application trace refers to the line that raises the first exception + assert_select "#Application-Trace-2" do + assert_select "code a:first", %r{in `raise_nested_exceptions'} + end + end + end end diff --git a/actionpack/test/dispatch/exception_wrapper_test.rb b/actionpack/test/dispatch/exception_wrapper_test.rb index f6e70382a8..600280d6b3 100644 --- a/actionpack/test/dispatch/exception_wrapper_test.rb +++ b/actionpack/test/dispatch/exception_wrapper_test.rb @@ -108,11 +108,27 @@ module ActionDispatch wrapper = ExceptionWrapper.new(@cleaner, exception) assert_equal({ - "Application Trace" => [ id: 0, trace: "lib/file.rb:42:in `index'" ], - "Framework Trace" => [ id: 1, trace: "/gems/rack.rb:43:in `index'" ], + "Application Trace" => [ + exception_object_id: exception.object_id, + id: 0, + trace: "lib/file.rb:42:in `index'" + ], + "Framework Trace" => [ + exception_object_id: exception.object_id, + id: 1, + trace: "/gems/rack.rb:43:in `index'" + ], "Full Trace" => [ - { id: 0, trace: "lib/file.rb:42:in `index'" }, - { id: 1, trace: "/gems/rack.rb:43:in `index'" } + { + exception_object_id: exception.object_id, + id: 0, + trace: "lib/file.rb:42:in `index'" + }, + { + exception_object_id: exception.object_id, + id: 1, + trace: "/gems/rack.rb:43:in `index'" + } ] }, wrapper.traces) end diff --git a/actionpack/test/dispatch/uploaded_file_test.rb b/actionpack/test/dispatch/uploaded_file_test.rb index 5a584b12e5..21169fcb5c 100644 --- a/actionpack/test/dispatch/uploaded_file_test.rb +++ b/actionpack/test/dispatch/uploaded_file_test.rb @@ -103,6 +103,12 @@ module ActionDispatch assert_predicate uf, :eof? end + def test_delegate_to_path_to_tempfile + tf = Class.new { def to_path; "/any/file/path" end; } + uf = Http::UploadedFile.new(tempfile: tf.new) + assert_equal "/any/file/path", uf.to_path + end + def test_respond_to? tf = Class.new { def read; yield end } uf = Http::UploadedFile.new(tempfile: tf.new) diff --git a/actionview/lib/action_view/helpers/date_helper.rb b/actionview/lib/action_view/helpers/date_helper.rb index ae993f9aa2..4de4fafde0 100644 --- a/actionview/lib/action_view/helpers/date_helper.rb +++ b/actionview/lib/action_view/helpers/date_helper.rb @@ -672,8 +672,6 @@ module ActionView # <time datetime="2010-11-04T17:55:45+01:00">November 04, 2010 17:55</time> # time_tag Date.yesterday, 'Yesterday' # => # <time datetime="2010-11-03">Yesterday</time> - # time_tag Date.today, pubdate: true # => - # <time datetime="2010-11-04" pubdate="pubdate">November 04, 2010</time> # time_tag Date.today, datetime: Date.today.strftime('%G-W%V') # => # <time datetime="2010-W44">November 04, 2010</time> # diff --git a/actionview/lib/action_view/helpers/form_helper.rb b/actionview/lib/action_view/helpers/form_helper.rb index 2d5c5684c1..07f3d98322 100644 --- a/actionview/lib/action_view/helpers/form_helper.rb +++ b/actionview/lib/action_view/helpers/form_helper.rb @@ -1658,6 +1658,7 @@ module ActionView @nested_child_index = {} @object_name, @object, @template, @options = object_name, object, template, options @default_options = @options ? @options.slice(:index, :namespace, :skip_default_ids, :allow_method_names_outside_object) : {} + @default_html_options = @default_options.except(:skip_default_ids, :allow_method_names_outside_object) convert_to_legacy_options(@options) diff --git a/actionview/lib/action_view/helpers/form_options_helper.rb b/actionview/lib/action_view/helpers/form_options_helper.rb index d02f641867..7884a8d997 100644 --- a/actionview/lib/action_view/helpers/form_options_helper.rb +++ b/actionview/lib/action_view/helpers/form_options_helper.rb @@ -820,7 +820,7 @@ module ActionView # # Please refer to the documentation of the base helper for details. def select(method, choices = nil, options = {}, html_options = {}, &block) - @template.select(@object_name, method, choices, objectify_options(options), @default_options.merge(html_options), &block) + @template.select(@object_name, method, choices, objectify_options(options), @default_html_options.merge(html_options), &block) end # Wraps ActionView::Helpers::FormOptionsHelper#collection_select for form builders: @@ -832,7 +832,7 @@ module ActionView # # Please refer to the documentation of the base helper for details. def collection_select(method, collection, value_method, text_method, options = {}, html_options = {}) - @template.collection_select(@object_name, method, collection, value_method, text_method, objectify_options(options), @default_options.merge(html_options)) + @template.collection_select(@object_name, method, collection, value_method, text_method, objectify_options(options), @default_html_options.merge(html_options)) end # Wraps ActionView::Helpers::FormOptionsHelper#grouped_collection_select for form builders: @@ -844,7 +844,7 @@ module ActionView # # Please refer to the documentation of the base helper for details. def grouped_collection_select(method, collection, group_method, group_label_method, option_key_method, option_value_method, options = {}, html_options = {}) - @template.grouped_collection_select(@object_name, method, collection, group_method, group_label_method, option_key_method, option_value_method, objectify_options(options), @default_options.merge(html_options)) + @template.grouped_collection_select(@object_name, method, collection, group_method, group_label_method, option_key_method, option_value_method, objectify_options(options), @default_html_options.merge(html_options)) end # Wraps ActionView::Helpers::FormOptionsHelper#time_zone_select for form builders: @@ -856,7 +856,7 @@ module ActionView # # Please refer to the documentation of the base helper for details. def time_zone_select(method, priority_zones = nil, options = {}, html_options = {}) - @template.time_zone_select(@object_name, method, priority_zones, objectify_options(options), @default_options.merge(html_options)) + @template.time_zone_select(@object_name, method, priority_zones, objectify_options(options), @default_html_options.merge(html_options)) end # Wraps ActionView::Helpers::FormOptionsHelper#collection_check_boxes for form builders: @@ -868,7 +868,7 @@ module ActionView # # Please refer to the documentation of the base helper for details. def collection_check_boxes(method, collection, value_method, text_method, options = {}, html_options = {}, &block) - @template.collection_check_boxes(@object_name, method, collection, value_method, text_method, objectify_options(options), @default_options.merge(html_options), &block) + @template.collection_check_boxes(@object_name, method, collection, value_method, text_method, objectify_options(options), @default_html_options.merge(html_options), &block) end # Wraps ActionView::Helpers::FormOptionsHelper#collection_radio_buttons for form builders: @@ -880,7 +880,7 @@ module ActionView # # Please refer to the documentation of the base helper for details. def collection_radio_buttons(method, collection, value_method, text_method, options = {}, html_options = {}, &block) - @template.collection_radio_buttons(@object_name, method, collection, value_method, text_method, objectify_options(options), @default_options.merge(html_options), &block) + @template.collection_radio_buttons(@object_name, method, collection, value_method, text_method, objectify_options(options), @default_html_options.merge(html_options), &block) end end end diff --git a/actionview/lib/action_view/helpers/tags/select.rb b/actionview/lib/action_view/helpers/tags/select.rb index 345484ba92..790721a0b7 100644 --- a/actionview/lib/action_view/helpers/tags/select.rb +++ b/actionview/lib/action_view/helpers/tags/select.rb @@ -8,7 +8,7 @@ module ActionView @choices = block_given? ? template_object.capture { yield || "" } : choices @choices = @choices.to_a if @choices.is_a?(Range) - @html_options = html_options.except(:skip_default_ids, :allow_method_names_outside_object) + @html_options = html_options super(object_name, method_name, template_object, options) end diff --git a/actionview/test/template/form_helper/form_with_test.rb b/actionview/test/template/form_helper/form_with_test.rb index ed1683bad2..f07e494ee3 100644 --- a/actionview/test/template/form_helper/form_with_test.rb +++ b/actionview/test/template/form_helper/form_with_test.rb @@ -464,6 +464,23 @@ class FormWithActsLikeFormForTest < FormWithTest end end + def test_form_with_with_collection_select + post = Post.new + def post.active; false; end + form_with(model: post) do |f| + concat f.collection_select(:active, [true, false], :to_s, :to_s) + end + + expected = whole_form("/posts") do + "<select name='post[active]' id='post_active'>" \ + "<option value='true'>true</option>\n" \ + "<option selected='selected' value='false'>false</option>" \ + "</select>" + end + + assert_dom_equal expected, output_buffer + end + def test_form_with_with_collection_radio_buttons post = Post.new def post.active; false; end diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md index 3ecbc2ff38..8526741383 100644 --- a/activejob/CHANGELOG.md +++ b/activejob/CHANGELOG.md @@ -1,3 +1,10 @@ +* Move `enqueue`/`enqueue_at` notifications to an around callback. + + Improves timing accuracy over the old after callback by including + time spent writing to the adapter's IO implementation. + + *Zach Kemp* + * Allow `queue` option to `assert_no_enqueued_jobs`. Example: diff --git a/activejob/lib/active_job/logging.rb b/activejob/lib/active_job/logging.rb index 3312857ac7..9ffd60ad53 100644 --- a/activejob/lib/active_job/logging.rb +++ b/activejob/lib/active_job/logging.rb @@ -27,13 +27,13 @@ module ActiveJob end end - after_enqueue do |job| + around_enqueue do |job, block| if job.scheduled_at - ActiveSupport::Notifications.instrument "enqueue_at.active_job", - adapter: job.class.queue_adapter, job: job + ActiveSupport::Notifications.instrument("enqueue_at.active_job", + adapter: job.class.queue_adapter, job: job, &block) else - ActiveSupport::Notifications.instrument "enqueue.active_job", - adapter: job.class.queue_adapter, job: job + ActiveSupport::Notifications.instrument("enqueue.active_job", + adapter: job.class.queue_adapter, job: job, &block) end end end diff --git a/activejob/test/cases/logging_test.rb b/activejob/test/cases/logging_test.rb index 1f8c4a5573..a1107a07fd 100644 --- a/activejob/test/cases/logging_test.rb +++ b/activejob/test/cases/logging_test.rb @@ -45,6 +45,14 @@ class LoggingTest < ActiveSupport::TestCase ActiveJob::Base.logger = logger end + def subscribed + [].tap do |events| + ActiveSupport::Notifications.subscribed(-> (*args) { events << args }, /enqueue.*\.active_job/) do + yield + end + end + end + def test_uses_active_job_as_tag HelloJob.perform_later "Cristian" assert_match(/\[ActiveJob\]/, @logger.messages) @@ -86,8 +94,11 @@ class LoggingTest < ActiveSupport::TestCase end def test_enqueue_job_logging - HelloJob.perform_later "Cristian" + events = subscribed { HelloJob.perform_later "Cristian" } assert_match(/Enqueued HelloJob \(Job ID: .*?\) to .*?:.*Cristian/, @logger.messages) + assert_equal(events.count, 1) + key, * = events.first + assert_equal(key, "enqueue.active_job") end def test_perform_job_logging @@ -110,15 +121,21 @@ class LoggingTest < ActiveSupport::TestCase end def test_enqueue_at_job_logging - HelloJob.set(wait_until: 24.hours.from_now).perform_later "Cristian" + events = subscribed { HelloJob.set(wait_until: 24.hours.from_now).perform_later "Cristian" } assert_match(/Enqueued HelloJob \(Job ID: .*\) to .*? at.*Cristian/, @logger.messages) + assert_equal(events.count, 1) + key, * = events.first + assert_equal(key, "enqueue_at.active_job") rescue NotImplementedError skip end def test_enqueue_in_job_logging - HelloJob.set(wait: 2.seconds).perform_later "Cristian" + events = subscribed { HelloJob.set(wait: 2.seconds).perform_later "Cristian" } assert_match(/Enqueued HelloJob \(Job ID: .*\) to .*? at.*Cristian/, @logger.messages) + assert_equal(events.count, 1) + key, * = events.first + assert_equal(key, "enqueue_at.active_job") rescue NotImplementedError skip end diff --git a/activemodel/lib/active_model/callbacks.rb b/activemodel/lib/active_model/callbacks.rb index 8fa9680cb1..fde3381df2 100644 --- a/activemodel/lib/active_model/callbacks.rb +++ b/activemodel/lib/active_model/callbacks.rb @@ -127,26 +127,28 @@ module ActiveModel private def _define_before_model_callback(klass, callback) - klass.define_singleton_method("before_#{callback}") do |*args, &block| - set_callback(:"#{callback}", :before, *args, &block) + klass.define_singleton_method("before_#{callback}") do |*args, **options, &block| + options.assert_valid_keys(:if, :unless, :prepend) + set_callback(:"#{callback}", :before, *args, options, &block) end end def _define_around_model_callback(klass, callback) - klass.define_singleton_method("around_#{callback}") do |*args, &block| - set_callback(:"#{callback}", :around, *args, &block) + klass.define_singleton_method("around_#{callback}") do |*args, **options, &block| + options.assert_valid_keys(:if, :unless, :prepend) + set_callback(:"#{callback}", :around, *args, options, &block) end end def _define_after_model_callback(klass, callback) - klass.define_singleton_method("after_#{callback}") do |*args, &block| - options = args.extract_options! + klass.define_singleton_method("after_#{callback}") do |*args, **options, &block| + options.assert_valid_keys(:if, :unless, :prepend) options[:prepend] = true conditional = ActiveSupport::Callbacks::Conditionals::Value.new { |v| v != false } options[:if] = Array(options[:if]) << conditional - set_callback(:"#{callback}", :after, *(args << options), &block) + set_callback(:"#{callback}", :after, *args, options, &block) end end end diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index d1ae41ab97..456f569718 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Don't impose primary key order if limit() has already been supplied. + + Fixes #23607 + + *Brian Christian* + * Add environment & load_config dependency to `bin/rake db:seed` to enable seed load in environments without Rails and custom DB configuration diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 3b581d6fe8..1ee52945ea 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1232,9 +1232,9 @@ module ActiveRecord # * <tt>Firm#clients.size</tt> (similar to <tt>Client.count "firm_id = #{id}"</tt>) # * <tt>Firm#clients.find</tt> (similar to <tt>Client.where(firm_id: id).find(id)</tt>) # * <tt>Firm#clients.exists?(name: 'ACME')</tt> (similar to <tt>Client.exists?(name: 'ACME', firm_id: firm.id)</tt>) - # * <tt>Firm#clients.build</tt> (similar to <tt>Client.new("firm_id" => id)</tt>) - # * <tt>Firm#clients.create</tt> (similar to <tt>c = Client.new("firm_id" => id); c.save; c</tt>) - # * <tt>Firm#clients.create!</tt> (similar to <tt>c = Client.new("firm_id" => id); c.save!</tt>) + # * <tt>Firm#clients.build</tt> (similar to <tt>Client.new(firm_id: id)</tt>) + # * <tt>Firm#clients.create</tt> (similar to <tt>c = Client.new(firm_id: id); c.save; c</tt>) + # * <tt>Firm#clients.create!</tt> (similar to <tt>c = Client.new(firm_id: id); c.save!</tt>) # * <tt>Firm#clients.reload</tt> # The declaration can also include an +options+ hash to specialize the behavior of the association. # @@ -1405,9 +1405,9 @@ module ActiveRecord # An Account class declares <tt>has_one :beneficiary</tt>, which will add: # * <tt>Account#beneficiary</tt> (similar to <tt>Beneficiary.where(account_id: id).first</tt>) # * <tt>Account#beneficiary=(beneficiary)</tt> (similar to <tt>beneficiary.account_id = account.id; beneficiary.save</tt>) - # * <tt>Account#build_beneficiary</tt> (similar to <tt>Beneficiary.new("account_id" => id)</tt>) - # * <tt>Account#create_beneficiary</tt> (similar to <tt>b = Beneficiary.new("account_id" => id); b.save; b</tt>) - # * <tt>Account#create_beneficiary!</tt> (similar to <tt>b = Beneficiary.new("account_id" => id); b.save!; b</tt>) + # * <tt>Account#build_beneficiary</tt> (similar to <tt>Beneficiary.new(account_id: id)</tt>) + # * <tt>Account#create_beneficiary</tt> (similar to <tt>b = Beneficiary.new(account_id: id); b.save; b</tt>) + # * <tt>Account#create_beneficiary!</tt> (similar to <tt>b = Beneficiary.new(account_id: id); b.save!; b</tt>) # * <tt>Account#reload_beneficiary</tt> # # === Scopes @@ -1746,8 +1746,8 @@ module ActiveRecord # * <tt>Developer#projects.size</tt> # * <tt>Developer#projects.find(id)</tt> # * <tt>Developer#projects.exists?(...)</tt> - # * <tt>Developer#projects.build</tt> (similar to <tt>Project.new("developer_id" => id)</tt>) - # * <tt>Developer#projects.create</tt> (similar to <tt>c = Project.new("developer_id" => id); c.save; c</tt>) + # * <tt>Developer#projects.build</tt> (similar to <tt>Project.new(developer_id: id)</tt>) + # * <tt>Developer#projects.create</tt> (similar to <tt>c = Project.new(developer_id: id); c.save; c</tt>) # * <tt>Developer#projects.reload</tt> # The declaration may include an +options+ hash to specialize the behavior of the association. # diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 199674f531..3be0906f2a 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -741,22 +741,13 @@ module ActiveRecord # ====== Creating an index with a specific operator class # # add_index(:developers, :name, using: 'gist', opclass: :gist_trgm_ops) - # - # generates: - # - # CREATE INDEX developers_on_name ON developers USING gist (name gist_trgm_ops) -- PostgreSQL + # # CREATE INDEX developers_on_name ON developers USING gist (name gist_trgm_ops) -- PostgreSQL # # add_index(:developers, [:name, :city], using: 'gist', opclass: { city: :gist_trgm_ops }) - # - # generates: - # - # CREATE INDEX developers_on_name_and_city ON developers USING gist (name, city gist_trgm_ops) -- PostgreSQL + # # CREATE INDEX developers_on_name_and_city ON developers USING gist (name, city gist_trgm_ops) -- PostgreSQL # # add_index(:developers, [:name, :city], using: 'gist', opclass: :gist_trgm_ops) - # - # generates: - # - # CREATE INDEX developers_on_name_and_city ON developers USING gist (name gist_trgm_ops, city gist_trgm_ops) -- PostgreSQL + # # CREATE INDEX developers_on_name_and_city ON developers USING gist (name gist_trgm_ops, city gist_trgm_ops) -- PostgreSQL # # Note: only supported by PostgreSQL # 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 07206f0d01..284b38ed7b 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -558,35 +558,6 @@ module ActiveRecord @max_allowed_packet ||= (show_variable("max_allowed_packet") - bytes_margin) end - def with_multi_statements - if supports_set_server_option? - @connection.set_server_option(Mysql2::Client::OPTION_MULTI_STATEMENTS_ON) - elsif !supports_multi_statements? - previous_flags = @config[:flags] - @config[:flags] = Mysql2::Client::MULTI_STATEMENTS - reconnect! - end - - yield - ensure - unless supports_multi_statements? - if supports_set_server_option? - @connection.set_server_option(Mysql2::Client::OPTION_MULTI_STATEMENTS_OFF) - else - @config[:flags] = previous_flags - reconnect! - end - end - end - - def supports_multi_statements? - (@config[:flags] & Mysql2::Client::MULTI_STATEMENTS) != 0 - end - - def supports_set_server_option? - @connection.respond_to?(:set_server_option) - end - def initialize_type_map(m = type_map) super diff --git a/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb b/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb index 4106ce01be..d89eeb7f54 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb @@ -62,6 +62,42 @@ module ActiveRecord @connection.abandon_results! end + def supports_set_server_option? + @connection.respond_to?(:set_server_option) + end + + def multi_statements_enabled?(flags) + if flags.is_a?(Array) + flags.include?("MULTI_STATEMENTS") + else + (flags & Mysql2::Client::MULTI_STATEMENTS) != 0 + end + end + + def with_multi_statements + previous_flags = @config[:flags] + + unless multi_statements_enabled?(previous_flags) + if supports_set_server_option? + @connection.set_server_option(Mysql2::Client::OPTION_MULTI_STATEMENTS_ON) + else + @config[:flags] = Mysql2::Client::MULTI_STATEMENTS + reconnect! + end + end + + yield + ensure + unless multi_statements_enabled?(previous_flags) + if supports_set_server_option? + @connection.set_server_option(Mysql2::Client::OPTION_MULTI_STATEMENTS_OFF) + else + @config[:flags] = previous_flags + reconnect! + end + end + end + def exec_stmt_and_free(sql, name, binds, cache_stmt: false) # make sure we carry over any changes to ActiveRecord::Base.default_timezone that have been # made since we established the connection diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 7ab9bb2d2d..fce5b66719 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -309,10 +309,10 @@ module ActiveRecord # Please check unscoped if you want to remove all previous scopes (including # the default_scope) during the execution of a block. def scoping - previous, klass.current_scope = klass.current_scope(true), self + previous, klass.current_scope = klass.current_scope(true), self unless @delegate_to_klass yield ensure - klass.current_scope = previous + klass.current_scope = previous unless @delegate_to_klass end def _exec_scope(*args, &block) # :nodoc: diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index c5562c1ff0..93f3b67686 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -550,7 +550,7 @@ module ActiveRecord end def ordered_relation - if order_values.empty? && primary_key + if order_values.empty? && primary_key && limit_value.blank? order(arel_attribute(primary_key).asc) else self diff --git a/activerecord/test/cases/adapters/mysql2/active_schema_test.rb b/activerecord/test/cases/adapters/mysql2/active_schema_test.rb index 58fa39d18c..6fc9df5083 100644 --- a/activerecord/test/cases/adapters/mysql2/active_schema_test.rb +++ b/activerecord/test/cases/adapters/mysql2/active_schema_test.rb @@ -157,14 +157,19 @@ class Mysql2ActiveSchemaTest < ActiveRecord::Mysql2TestCase end def test_indexes_in_create - ActiveRecord::Base.connection.stubs(:data_source_exists?).with(:temp).returns(false) + assert_called_with( + ActiveRecord::Base.connection, + :data_source_exists?, + [:temp], + returns: false + ) do + expected = "CREATE TEMPORARY TABLE `temp` ( INDEX `index_temp_on_zip` (`zip`)) AS SELECT id, name, zip FROM a_really_complicated_query" + actual = ActiveRecord::Base.connection.create_table(:temp, temporary: true, as: "SELECT id, name, zip FROM a_really_complicated_query") do |t| + t.index :zip + end - expected = "CREATE TEMPORARY TABLE `temp` ( INDEX `index_temp_on_zip` (`zip`)) AS SELECT id, name, zip FROM a_really_complicated_query" - actual = ActiveRecord::Base.connection.create_table(:temp, temporary: true, as: "SELECT id, name, zip FROM a_really_complicated_query") do |t| - t.index :zip + assert_equal expected, actual end - - assert_equal expected, actual end private diff --git a/activerecord/test/cases/callbacks_test.rb b/activerecord/test/cases/callbacks_test.rb index b9ba51c730..253c3099d6 100644 --- a/activerecord/test/cases/callbacks_test.rb +++ b/activerecord/test/cases/callbacks_test.rb @@ -476,4 +476,31 @@ class CallbacksTest < ActiveRecord::TestCase child.save assert child.after_save_called end + + def test_before_save_doesnt_allow_on_option + exception = assert_raises ArgumentError do + Class.new(ActiveRecord::Base) do + before_save(on: :create) {} + end + end + assert_equal "Unknown key: :on. Valid keys are: :if, :unless, :prepend", exception.message + end + + def test_around_save_doesnt_allow_on_option + exception = assert_raises ArgumentError do + Class.new(ActiveRecord::Base) do + around_save(on: :create) {} + end + end + assert_equal "Unknown key: :on. Valid keys are: :if, :unless, :prepend", exception.message + end + + def test_after_save_doesnt_allow_on_option + exception = assert_raises ArgumentError do + Class.new(ActiveRecord::Base) do + after_save(on: :create) {} + end + end + assert_equal "Unknown key: :on. Valid keys are: :if, :unless, :prepend", exception.message + end end diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index 39ffe12ec6..2f256dd36a 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -1265,6 +1265,21 @@ class FinderTest < ActiveRecord::TestCase end end + def test_first_and_last_with_limit_for_order_without_primary_key + # While Topic.first should impose an ordering by primary key, + # Topic.limit(n).first should not + + Topic.first.touch # PostgreSQL changes the default order if no order clause is used + + assert_equal Topic.limit(1).to_a.first, Topic.limit(1).first + assert_equal Topic.limit(2).to_a.first, Topic.limit(2).first + assert_equal Topic.limit(2).to_a.first(2), Topic.limit(2).first(2) + + assert_equal Topic.limit(1).to_a.last, Topic.limit(1).last + assert_equal Topic.limit(2).to_a.last, Topic.limit(2).last + assert_equal Topic.limit(2).to_a.last(2), Topic.limit(2).last(2) + end + def test_finder_with_offset_string assert_nothing_raised { Topic.offset("3").to_a } end diff --git a/activerecord/test/cases/fixtures_test.rb b/activerecord/test/cases/fixtures_test.rb index cdc9a8585f..c65523d8c1 100644 --- a/activerecord/test/cases/fixtures_test.rb +++ b/activerecord/test/cases/fixtures_test.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "cases/helper" +require "support/connection_helper" require "models/admin" require "models/admin/account" require "models/admin/randomly_named_c1" @@ -32,6 +33,8 @@ require "models/treasure" require "tempfile" class FixturesTest < ActiveRecord::TestCase + include ConnectionHelper + self.use_instantiated_fixtures = true self.use_transactional_tests = false @@ -122,16 +125,88 @@ class FixturesTest < ActiveRecord::TestCase ] } - ActiveRecord::Base.transaction do - con = ActiveRecord::Base.connection - assert_equal 1, con.open_transactions - con.insert_fixtures_set(fixtures) - assert_equal 1, con.open_transactions + assert_difference "TrafficLight.count" do + ActiveRecord::Base.transaction do + conn = ActiveRecord::Base.connection + assert_equal 1, conn.open_transactions + conn.insert_fixtures_set(fixtures) + assert_equal 1, conn.open_transactions + end end end end if current_adapter?(:Mysql2Adapter) + def test_bulk_insert_with_multi_statements_enabled + run_without_connection do |orig_connection| + ActiveRecord::Base.establish_connection( + orig_connection.merge(flags: %w[MULTI_STATEMENTS]) + ) + + fixtures = { + "traffic_lights" => [ + { "location" => "US", "state" => ["NY"], "long_state" => ["a"] }, + ] + } + + ActiveRecord::Base.connection.stub(:supports_set_server_option?, false) do + assert_nothing_raised do + conn = ActiveRecord::Base.connection + conn.execute("SELECT 1; SELECT 2;") + conn.raw_connection.abandon_results! + end + + assert_difference "TrafficLight.count" do + ActiveRecord::Base.transaction do + conn = ActiveRecord::Base.connection + assert_equal 1, conn.open_transactions + conn.insert_fixtures_set(fixtures) + assert_equal 1, conn.open_transactions + end + end + + assert_nothing_raised do + conn = ActiveRecord::Base.connection + conn.execute("SELECT 1; SELECT 2;") + conn.raw_connection.abandon_results! + end + end + end + end + + def test_bulk_insert_with_multi_statements_disabled + run_without_connection do |orig_connection| + ActiveRecord::Base.establish_connection( + orig_connection.merge(flags: []) + ) + + fixtures = { + "traffic_lights" => [ + { "location" => "US", "state" => ["NY"], "long_state" => ["a"] }, + ] + } + + ActiveRecord::Base.connection.stub(:supports_set_server_option?, false) do + assert_raises(ActiveRecord::StatementInvalid) do + conn = ActiveRecord::Base.connection + conn.execute("SELECT 1; SELECT 2;") + conn.raw_connection.abandon_results! + end + + assert_difference "TrafficLight.count" do + conn = ActiveRecord::Base.connection + conn.insert_fixtures_set(fixtures) + end + + assert_raises(ActiveRecord::StatementInvalid) do + conn = ActiveRecord::Base.connection + conn.execute("SELECT 1; SELECT 2;") + conn.raw_connection.abandon_results! + end + end + end + end + def test_insert_fixtures_set_raises_an_error_when_max_allowed_packet_is_smaller_than_fixtures_set_size conn = ActiveRecord::Base.connection mysql_margin = 2 @@ -861,9 +936,9 @@ class TransactionalFixturesOnConnectionNotification < ActiveRecord::TestCase def lock_thread=(lock_thread); end end.new - connection.expects(:begin_transaction).with(joinable: false) - - fire_connection_notification(connection) + assert_called_with(connection, :begin_transaction, [joinable: false]) do + fire_connection_notification(connection) + end end def test_notification_established_transactions_are_rolled_back @@ -891,7 +966,7 @@ class TransactionalFixturesOnConnectionNotification < ActiveRecord::TestCase private def fire_connection_notification(connection) - ActiveRecord::Base.connection_handler.stub(:retrieve_connection, connection) do + assert_called_with(ActiveRecord::Base.connection_handler, :retrieve_connection, ["book"], returns: connection) do message_bus = ActiveSupport::Notifications.instrumenter payload = { spec_name: "book", diff --git a/activerecord/test/cases/scoping/relation_scoping_test.rb b/activerecord/test/cases/scoping/relation_scoping_test.rb index f18f1ed981..544adc9b39 100644 --- a/activerecord/test/cases/scoping/relation_scoping_test.rb +++ b/activerecord/test/cases/scoping/relation_scoping_test.rb @@ -254,6 +254,11 @@ class RelationScopingTest < ActiveRecord::TestCase end end + def test_scoping_works_in_the_scope_block + expected = SpecialPostWithDefaultScope.unscoped.to_a + assert_equal expected, SpecialPostWithDefaultScope.unscoped_all + end + def test_circular_joins_with_scoping_does_not_crash posts = Post.joins(comments: :post).scoping do Post.first(10) diff --git a/activerecord/test/cases/tasks/database_tasks_test.rb b/activerecord/test/cases/tasks/database_tasks_test.rb index aefe122bdf..b9cc08c446 100644 --- a/activerecord/test/cases/tasks/database_tasks_test.rb +++ b/activerecord/test/cases/tasks/database_tasks_test.rb @@ -15,6 +15,7 @@ module ActiveRecord def charset; end def collation; end def structure_dump(*); end + def structure_load(*); end end.new ) @@ -119,8 +120,9 @@ module ActiveRecord ADAPTERS_TASKS.each do |k, v| define_method("test_#{k}_create") do with_stubbed_new do - eval("@#{v}").expects(:create) - ActiveRecord::Tasks::DatabaseTasks.create "adapter" => k + assert_called(eval("@#{v}"), :create) do + ActiveRecord::Tasks::DatabaseTasks.create "adapter" => k + end end end end @@ -141,9 +143,6 @@ module ActiveRecord def setup @configurations = { "development" => { "database" => "my-db" } } - # To refrain from connecting to a newly created empty DB in sqlite3_mem tests - ActiveRecord::Base.connection_handler.stubs(:establish_connection) - $stdout, @original_stdout = StringIO.new, $stdout $stderr, @original_stderr = StringIO.new, $stderr end @@ -155,7 +154,7 @@ module ActiveRecord def test_ignores_configurations_without_databases @configurations["development"].merge!("database" => nil) - with_stubbed_configurations do + with_stubbed_configurations_establish_connection do assert_not_called(ActiveRecord::Tasks::DatabaseTasks, :create) do ActiveRecord::Tasks::DatabaseTasks.create_all end @@ -165,7 +164,7 @@ module ActiveRecord def test_ignores_remote_databases @configurations["development"].merge!("host" => "my.server.tld") - with_stubbed_configurations do + with_stubbed_configurations_establish_connection do assert_not_called(ActiveRecord::Tasks::DatabaseTasks, :create) do ActiveRecord::Tasks::DatabaseTasks.create_all end @@ -175,7 +174,7 @@ module ActiveRecord def test_warning_for_remote_databases @configurations["development"].merge!("host" => "my.server.tld") - with_stubbed_configurations do + with_stubbed_configurations_establish_connection do ActiveRecord::Tasks::DatabaseTasks.create_all assert_match "This task only modifies local databases. my-db is on a remote host.", @@ -186,7 +185,7 @@ module ActiveRecord def test_creates_configurations_with_local_ip @configurations["development"].merge!("host" => "127.0.0.1") - with_stubbed_configurations do + with_stubbed_configurations_establish_connection do assert_called(ActiveRecord::Tasks::DatabaseTasks, :create) do ActiveRecord::Tasks::DatabaseTasks.create_all end @@ -196,7 +195,7 @@ module ActiveRecord def test_creates_configurations_with_local_host @configurations["development"].merge!("host" => "localhost") - with_stubbed_configurations do + with_stubbed_configurations_establish_connection do assert_called(ActiveRecord::Tasks::DatabaseTasks, :create) do ActiveRecord::Tasks::DatabaseTasks.create_all end @@ -206,7 +205,7 @@ module ActiveRecord def test_creates_configurations_with_blank_hosts @configurations["development"].merge!("host" => nil) - with_stubbed_configurations do + with_stubbed_configurations_establish_connection do assert_called(ActiveRecord::Tasks::DatabaseTasks, :create) do ActiveRecord::Tasks::DatabaseTasks.create_all end @@ -215,9 +214,16 @@ module ActiveRecord private - def with_stubbed_configurations + def with_stubbed_configurations_establish_connection ActiveRecord::Base.stub(:configurations, @configurations) do - yield + # To refrain from connecting to a newly created empty DB in + # sqlite3_mem tests + ActiveRecord::Base.connection_handler.stub( + :establish_connection, + nil + ) do + yield + end end end end @@ -407,11 +413,15 @@ module ActiveRecord def test_establishes_connection_for_the_given_environments_config ActiveRecord::Tasks::DatabaseTasks.stub(:create, nil) do - ActiveRecord::Base.expects(:establish_connection).with(:development) - - ActiveRecord::Tasks::DatabaseTasks.create_current( - ActiveSupport::StringInquirer.new("development") - ) + assert_called_with( + ActiveRecord::Base, + :establish_connection, + [:development] + ) do + ActiveRecord::Tasks::DatabaseTasks.create_current( + ActiveSupport::StringInquirer.new("development") + ) + end end end @@ -432,8 +442,9 @@ module ActiveRecord ADAPTERS_TASKS.each do |k, v| define_method("test_#{k}_drop") do with_stubbed_new do - eval("@#{v}").expects(:drop) - ActiveRecord::Tasks::DatabaseTasks.drop "adapter" => k + assert_called(eval("@#{v}"), :drop) do + ActiveRecord::Tasks::DatabaseTasks.drop "adapter" => k + end end end end @@ -812,8 +823,9 @@ module ActiveRecord ADAPTERS_TASKS.each do |k, v| define_method("test_#{k}_purge") do with_stubbed_new do - eval("@#{v}").expects(:purge) - ActiveRecord::Tasks::DatabaseTasks.purge "adapter" => k + assert_called(eval("@#{v}"), :purge) do + ActiveRecord::Tasks::DatabaseTasks.purge "adapter" => k + end end end end @@ -827,11 +839,14 @@ module ActiveRecord "production" => { "database" => "prod-db" } } ActiveRecord::Base.stub(:configurations, configurations) do - ActiveRecord::Tasks::DatabaseTasks.expects(:purge). - with("database" => "prod-db") - - assert_called_with(ActiveRecord::Base, :establish_connection, [:production]) do - ActiveRecord::Tasks::DatabaseTasks.purge_current("production") + assert_called_with( + ActiveRecord::Tasks::DatabaseTasks, + :purge, + ["database" => "prod-db"] + ) do + assert_called_with(ActiveRecord::Base, :establish_connection, [:production]) do + ActiveRecord::Tasks::DatabaseTasks.purge_current("production") + end end end end @@ -841,10 +856,13 @@ module ActiveRecord def test_purge_all_local_configurations configurations = { development: { "database" => "my-db" } } ActiveRecord::Base.stub(:configurations, configurations) do - ActiveRecord::Tasks::DatabaseTasks.expects(:purge). - with("database" => "my-db") - - ActiveRecord::Tasks::DatabaseTasks.purge_all + assert_called_with( + ActiveRecord::Tasks::DatabaseTasks, + :purge, + ["database" => "my-db"] + ) do + ActiveRecord::Tasks::DatabaseTasks.purge_all + end end end end @@ -855,8 +873,9 @@ module ActiveRecord ADAPTERS_TASKS.each do |k, v| define_method("test_#{k}_charset") do with_stubbed_new do - eval("@#{v}").expects(:charset) - ActiveRecord::Tasks::DatabaseTasks.charset "adapter" => k + assert_called(eval("@#{v}"), :charset) do + ActiveRecord::Tasks::DatabaseTasks.charset "adapter" => k + end end end end @@ -868,8 +887,9 @@ module ActiveRecord ADAPTERS_TASKS.each do |k, v| define_method("test_#{k}_collation") do with_stubbed_new do - eval("@#{v}").expects(:collation) - ActiveRecord::Tasks::DatabaseTasks.collation "adapter" => k + assert_called(eval("@#{v}"), :collation) do + ActiveRecord::Tasks::DatabaseTasks.collation "adapter" => k + end end end end @@ -983,8 +1003,12 @@ module ActiveRecord ADAPTERS_TASKS.each do |k, v| define_method("test_#{k}_structure_dump") do with_stubbed_new do - eval("@#{v}").expects(:structure_dump).with("awesome-file.sql", nil) - ActiveRecord::Tasks::DatabaseTasks.structure_dump({ "adapter" => k }, "awesome-file.sql") + assert_called_with( + eval("@#{v}"), :structure_dump, + ["awesome-file.sql", nil] + ) do + ActiveRecord::Tasks::DatabaseTasks.structure_dump({ "adapter" => k }, "awesome-file.sql") + end end end end @@ -996,8 +1020,13 @@ module ActiveRecord ADAPTERS_TASKS.each do |k, v| define_method("test_#{k}_structure_load") do with_stubbed_new do - eval("@#{v}").expects(:structure_load).with("awesome-file.sql", nil) - ActiveRecord::Tasks::DatabaseTasks.structure_load({ "adapter" => k }, "awesome-file.sql") + assert_called_with( + eval("@#{v}"), + :structure_load, + ["awesome-file.sql", nil] + ) do + ActiveRecord::Tasks::DatabaseTasks.structure_load({ "adapter" => k }, "awesome-file.sql") + end end end end diff --git a/activerecord/test/cases/tasks/mysql_rake_test.rb b/activerecord/test/cases/tasks/mysql_rake_test.rb index 28dcaf2f89..eeb4222d97 100644 --- a/activerecord/test/cases/tasks/mysql_rake_test.rb +++ b/activerecord/test/cases/tasks/mysql_rake_test.rb @@ -21,48 +21,60 @@ if current_adapter?(:Mysql2Adapter) end def test_establishes_connection_without_database - ActiveRecord::Base.stubs(:establish_connection) ActiveRecord::Base.stub(:connection, @connection) do - ActiveRecord::Base.expects(:establish_connection). - with("adapter" => "mysql2", "database" => nil) - ActiveRecord::Tasks::DatabaseTasks.create @configuration + assert_called_with( + ActiveRecord::Base, + :establish_connection, + [ + [ "adapter" => "mysql2", "database" => nil ], + [ "adapter" => "mysql2", "database" => "my-app-db" ], + ] + ) do + ActiveRecord::Tasks::DatabaseTasks.create @configuration + end end end def test_creates_database_with_no_default_options with_stubbed_connection_establish_connection do - @connection.expects(:create_database). - with("my-app-db", {}) - - ActiveRecord::Tasks::DatabaseTasks.create @configuration + assert_called_with(@connection, :create_database, ["my-app-db", {}]) do + ActiveRecord::Tasks::DatabaseTasks.create @configuration + end end end def test_creates_database_with_given_encoding with_stubbed_connection_establish_connection do - @connection.expects(:create_database). - with("my-app-db", charset: "latin1") - - ActiveRecord::Tasks::DatabaseTasks.create @configuration.merge("encoding" => "latin1") + assert_called_with(@connection, :create_database, ["my-app-db", charset: "latin1"]) do + ActiveRecord::Tasks::DatabaseTasks.create @configuration.merge("encoding" => "latin1") + end end end def test_creates_database_with_given_collation with_stubbed_connection_establish_connection do - @connection.expects(:create_database). - with("my-app-db", collation: "latin1_swedish_ci") - - ActiveRecord::Tasks::DatabaseTasks.create @configuration.merge("collation" => "latin1_swedish_ci") + assert_called_with( + @connection, + :create_database, + ["my-app-db", collation: "latin1_swedish_ci"] + ) do + ActiveRecord::Tasks::DatabaseTasks.create @configuration.merge("collation" => "latin1_swedish_ci") + end end end def test_establishes_connection_to_database - ActiveRecord::Base.stubs(:establish_connection) - ActiveRecord::Base.stub(:connection, @connection) do - ActiveRecord::Base.expects(:establish_connection).with(@configuration) - - ActiveRecord::Tasks::DatabaseTasks.create @configuration + assert_called_with( + ActiveRecord::Base, + :establish_connection, + [ + ["adapter" => "mysql2", "database" => nil], + [@configuration] + ] + ) do + ActiveRecord::Tasks::DatabaseTasks.create @configuration + end end end @@ -149,9 +161,9 @@ if current_adapter?(:Mysql2Adapter) def test_drops_database with_stubbed_connection_establish_connection do - @connection.expects(:drop_database).with("my-app-db") - - ActiveRecord::Tasks::DatabaseTasks.drop @configuration + assert_called_with(@connection, :drop_database, ["my-app-db"]) do + ActiveRecord::Tasks::DatabaseTasks.drop @configuration + end end end @@ -193,20 +205,22 @@ if current_adapter?(:Mysql2Adapter) def test_recreates_database_with_no_default_options with_stubbed_connection_establish_connection do - @connection.expects(:recreate_database). - with("test-db", {}) - - ActiveRecord::Tasks::DatabaseTasks.purge @configuration + assert_called_with(@connection, :recreate_database, ["test-db", {}]) do + ActiveRecord::Tasks::DatabaseTasks.purge @configuration + end end end def test_recreates_database_with_the_given_options with_stubbed_connection_establish_connection do - @connection.expects(:recreate_database). - with("test-db", charset: "latin", collation: "latin1_swedish_ci") - - ActiveRecord::Tasks::DatabaseTasks.purge @configuration.merge( - "encoding" => "latin", "collation" => "latin1_swedish_ci") + assert_called_with( + @connection, + :recreate_database, + ["test-db", charset: "latin", collation: "latin1_swedish_ci"] + ) do + ActiveRecord::Tasks::DatabaseTasks.purge @configuration.merge( + "encoding" => "latin", "collation" => "latin1_swedish_ci") + end end end @@ -232,8 +246,9 @@ if current_adapter?(:Mysql2Adapter) def test_db_retrieves_charset ActiveRecord::Base.stub(:connection, @connection) do - @connection.expects(:charset) - ActiveRecord::Tasks::DatabaseTasks.charset @configuration + assert_called(@connection, :charset) do + ActiveRecord::Tasks::DatabaseTasks.charset @configuration + end end end end @@ -249,8 +264,9 @@ if current_adapter?(:Mysql2Adapter) def test_db_retrieves_collation ActiveRecord::Base.stub(:connection, @connection) do - @connection.expects(:collation) - ActiveRecord::Tasks::DatabaseTasks.collation @configuration + assert_called_with(@connection, :collation) do + ActiveRecord::Tasks::DatabaseTasks.collation @configuration + end end end end diff --git a/activerecord/test/cases/tasks/postgresql_rake_test.rb b/activerecord/test/cases/tasks/postgresql_rake_test.rb index 8b822cd550..00005e7a0d 100644 --- a/activerecord/test/cases/tasks/postgresql_rake_test.rb +++ b/activerecord/test/cases/tasks/postgresql_rake_test.rb @@ -21,52 +21,90 @@ if current_adapter?(:PostgreSQLAdapter) end def test_establishes_connection_to_postgresql_database - ActiveRecord::Base.stubs(:establish_connection) ActiveRecord::Base.stub(:connection, @connection) do - ActiveRecord::Base.expects(:establish_connection).with( - "adapter" => "postgresql", - "database" => "postgres", - "schema_search_path" => "public" - ) - ActiveRecord::Tasks::DatabaseTasks.create @configuration + assert_called_with( + ActiveRecord::Base, + :establish_connection, + [ + [ + "adapter" => "postgresql", + "database" => "postgres", + "schema_search_path" => "public" + ], + [ + "adapter" => "postgresql", + "database" => "my-app-db" + ] + ] + ) do + ActiveRecord::Tasks::DatabaseTasks.create @configuration + end end end def test_creates_database_with_default_encoding with_stubbed_connection_establish_connection do - @connection.expects(:create_database). - with("my-app-db", @configuration.merge("encoding" => "utf8")) - - ActiveRecord::Tasks::DatabaseTasks.create @configuration + assert_called_with( + @connection, + :create_database, + ["my-app-db", @configuration.merge("encoding" => "utf8")] + ) do + ActiveRecord::Tasks::DatabaseTasks.create @configuration + end end end def test_creates_database_with_given_encoding with_stubbed_connection_establish_connection do - @connection.expects(:create_database). - with("my-app-db", @configuration.merge("encoding" => "latin")) - - ActiveRecord::Tasks::DatabaseTasks.create @configuration. - merge("encoding" => "latin") + assert_called_with( + @connection, + :create_database, + ["my-app-db", @configuration.merge("encoding" => "latin")] + ) do + ActiveRecord::Tasks::DatabaseTasks.create @configuration. + merge("encoding" => "latin") + end end end def test_creates_database_with_given_collation_and_ctype with_stubbed_connection_establish_connection do - @connection.expects(:create_database). - with("my-app-db", @configuration.merge("encoding" => "utf8", "collation" => "ja_JP.UTF8", "ctype" => "ja_JP.UTF8")) - - ActiveRecord::Tasks::DatabaseTasks.create @configuration. - merge("collation" => "ja_JP.UTF8", "ctype" => "ja_JP.UTF8") + assert_called_with( + @connection, + :create_database, + [ + "my-app-db", + @configuration.merge( + "encoding" => "utf8", + "collation" => "ja_JP.UTF8", + "ctype" => "ja_JP.UTF8" + ) + ] + ) do + ActiveRecord::Tasks::DatabaseTasks.create @configuration. + merge("collation" => "ja_JP.UTF8", "ctype" => "ja_JP.UTF8") + end end end def test_establishes_connection_to_new_database - ActiveRecord::Base.stubs(:establish_connection) ActiveRecord::Base.stub(:connection, @connection) do - ActiveRecord::Base.expects(:establish_connection).with(@configuration) - - ActiveRecord::Tasks::DatabaseTasks.create @configuration + assert_called_with( + ActiveRecord::Base, + :establish_connection, + [ + [ + "adapter" => "postgresql", + "database" => "postgres", + "schema_search_path" => "public" + ], + [ + @configuration + ] + ] + ) do + ActiveRecord::Tasks::DatabaseTasks.create @configuration + end end end @@ -139,9 +177,13 @@ if current_adapter?(:PostgreSQLAdapter) def test_drops_database with_stubbed_connection_establish_connection do - @connection.expects(:drop_database).with("my-app-db") - - ActiveRecord::Tasks::DatabaseTasks.drop @configuration + assert_called_with( + @connection, + :drop_database, + ["my-app-db"] + ) do + ActiveRecord::Tasks::DatabaseTasks.drop @configuration + end end end @@ -179,32 +221,41 @@ if current_adapter?(:PostgreSQLAdapter) def test_clears_active_connections with_stubbed_connection do ActiveRecord::Base.stub(:establish_connection, nil) do - ActiveRecord::Base.expects(:clear_active_connections!) - - ActiveRecord::Tasks::DatabaseTasks.purge @configuration + assert_called(ActiveRecord::Base, :clear_active_connections!) do + ActiveRecord::Tasks::DatabaseTasks.purge @configuration + end end end end def test_establishes_connection_to_postgresql_database - ActiveRecord::Base.stubs(:establish_connection) with_stubbed_connection do - ActiveRecord::Base.expects(:establish_connection).with( - "adapter" => "postgresql", - "database" => "postgres", - "schema_search_path" => "public" - ) - - ActiveRecord::Tasks::DatabaseTasks.purge @configuration + assert_called_with( + ActiveRecord::Base, + :establish_connection, + [ + [ + "adapter" => "postgresql", + "database" => "postgres", + "schema_search_path" => "public" + ], + [ + "adapter" => "postgresql", + "database" => "my-app-db" + ] + ] + ) do + ActiveRecord::Tasks::DatabaseTasks.purge @configuration + end end end def test_drops_database with_stubbed_connection do ActiveRecord::Base.stub(:establish_connection, nil) do - @connection.expects(:drop_database).with("my-app-db") - - ActiveRecord::Tasks::DatabaseTasks.purge @configuration + assert_called_with(@connection, :drop_database, ["my-app-db"]) do + ActiveRecord::Tasks::DatabaseTasks.purge @configuration + end end end end @@ -212,20 +263,35 @@ if current_adapter?(:PostgreSQLAdapter) def test_creates_database with_stubbed_connection do ActiveRecord::Base.stub(:establish_connection, nil) do - @connection.expects(:create_database). - with("my-app-db", @configuration.merge("encoding" => "utf8")) - - ActiveRecord::Tasks::DatabaseTasks.purge @configuration + assert_called_with( + @connection, + :create_database, + ["my-app-db", @configuration.merge("encoding" => "utf8")] + ) do + ActiveRecord::Tasks::DatabaseTasks.purge @configuration + end end end end def test_establishes_connection - ActiveRecord::Base.stubs(:establish_connection) with_stubbed_connection do - ActiveRecord::Base.expects(:establish_connection).with(@configuration) - - ActiveRecord::Tasks::DatabaseTasks.purge @configuration + assert_called_with( + ActiveRecord::Base, + :establish_connection, + [ + [ + "adapter" => "postgresql", + "database" => "postgres", + "schema_search_path" => "public" + ], + [ + @configuration + ] + ] + ) do + ActiveRecord::Tasks::DatabaseTasks.purge @configuration + end end end @@ -240,7 +306,10 @@ if current_adapter?(:PostgreSQLAdapter) class PostgreSQLDBCharsetTest < ActiveRecord::TestCase def setup - @connection = Class.new { def create_database(*); end }.new + @connection = Class.new do + def create_database(*); end + def encoding; end + end.new @configuration = { "adapter" => "postgresql", "database" => "my-app-db" @@ -249,16 +318,16 @@ if current_adapter?(:PostgreSQLAdapter) def test_db_retrieves_charset ActiveRecord::Base.stub(:connection, @connection) do - @connection.expects(:encoding) - - ActiveRecord::Tasks::DatabaseTasks.charset @configuration + assert_called(@connection, :encoding) do + ActiveRecord::Tasks::DatabaseTasks.charset @configuration + end end end end class PostgreSQLDBCollationTest < ActiveRecord::TestCase def setup - @connection = Class.new { def create_database(*); end }.new + @connection = Class.new { def collation; end }.new @configuration = { "adapter" => "postgresql", "database" => "my-app-db" @@ -267,9 +336,9 @@ if current_adapter?(:PostgreSQLAdapter) def test_db_retrieves_collation ActiveRecord::Base.stub(:connection, @connection) do - @connection.expects(:collation) - - ActiveRecord::Tasks::DatabaseTasks.collation @configuration + assert_called(@connection, :collation) do + ActiveRecord::Tasks::DatabaseTasks.collation @configuration + end end end end diff --git a/activerecord/test/cases/tasks/sqlite_rake_test.rb b/activerecord/test/cases/tasks/sqlite_rake_test.rb index 3338f96cf8..7eb062b456 100644 --- a/activerecord/test/cases/tasks/sqlite_rake_test.rb +++ b/activerecord/test/cases/tasks/sqlite_rake_test.rb @@ -88,9 +88,9 @@ if current_adapter?(:SQLite3Adapter) end def test_creates_path_from_database - Pathname.expects(:new).with(@database).returns(@path) - - ActiveRecord::Tasks::DatabaseTasks.drop @configuration, "/rails/root" + assert_called_with(Pathname, :new, [@database], returns: @path) do + ActiveRecord::Tasks::DatabaseTasks.drop @configuration, "/rails/root" + end end def test_removes_file_with_absolute_path diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index 54eb5e6783..640cdb33b4 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -253,6 +253,7 @@ class SpecialPostWithDefaultScope < ActiveRecord::Base self.inheritance_column = :disabled self.table_name = "posts" default_scope { where(id: [1, 5, 6]) } + scope :unscoped_all, -> { unscoped { all } } end class PostThatLoadsCommentsInAnAfterSaveHook < ActiveRecord::Base diff --git a/activestorage/app/jobs/active_storage/purge_job.rb b/activestorage/app/jobs/active_storage/purge_job.rb index fa15e0451d..b021b5f2d0 100644 --- a/activestorage/app/jobs/active_storage/purge_job.rb +++ b/activestorage/app/jobs/active_storage/purge_job.rb @@ -2,7 +2,7 @@ # Provides asynchronous purging of ActiveStorage::Blob records via ActiveStorage::Blob#purge_later. class ActiveStorage::PurgeJob < ActiveStorage::BaseJob - discard_on ActiveRecord::RecordNotFound + discard_on ActiveRecord::RecordNotFound, ActiveRecord::InvalidForeignKey def perform(blob) blob.purge diff --git a/activestorage/app/models/active_storage/attachment.rb b/activestorage/app/models/active_storage/attachment.rb index bb80799044..1c4dc25094 100644 --- a/activestorage/app/models/active_storage/attachment.rb +++ b/activestorage/app/models/active_storage/attachment.rb @@ -17,16 +17,16 @@ class ActiveStorage::Attachment < ActiveRecord::Base after_create_commit :analyze_blob_later, :identify_blob after_destroy_commit :purge_dependent_blob_later - # Synchronously purges the blob (deletes it from the configured service) and deletes the attachment. + # Synchronously deletes the attachment and {purges the blob}[rdoc-ref:ActiveStorage::Blob#purge]. def purge - blob.purge delete + blob.purge end - # Deletes the attachment and queues a background job to purge the blob (delete it from the configured service). + # Deletes the attachment and {enqueues a background job}[rdoc-ref:ActiveStorage::Blob#purge_later] to purge the blob. def purge_later - blob.purge_later delete + blob.purge_later end private diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 86f3dba524..bf87598a66 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -193,8 +193,8 @@ class ActiveStorage::Blob < ActiveRecord::Base end - # Deletes the file on the service that's associated with this blob. This should only be done if the blob is going to be - # deleted as well or you will essentially have a dead reference. It's recommended to use the +#purge+ and +#purge_later+ + # Deletes the files on the service associated with the blob. This should only be done if the blob is going to be + # deleted as well or you will essentially have a dead reference. It's recommended to use #purge and #purge_later # methods in most circumstances. def delete service.delete(key) @@ -203,14 +203,14 @@ class ActiveStorage::Blob < ActiveRecord::Base # Deletes the file on the service and then destroys the blob record. This is the recommended way to dispose of unwanted # blobs. Note, though, that deleting the file off the service will initiate a HTTP connection to the service, which may - # be slow or prevented, so you should not use this method inside a transaction or in callbacks. Use +#purge_later+ instead. + # be slow or prevented, so you should not use this method inside a transaction or in callbacks. Use #purge_later instead. def purge - delete destroy + delete end - # Enqueues an ActiveStorage::PurgeJob job that'll call +purge+. This is the recommended way to purge blobs when the call - # needs to be made from a transaction, a callback, or any other real-time scenario. + # Enqueues an ActiveStorage::PurgeJob to call #purge. This is the recommended way to purge blobs from a transaction, + # an Active Record callback, or in any other real-time scenario. def purge_later ActiveStorage::PurgeJob.perform_later(self) end diff --git a/activestorage/db/migrate/20170806125915_create_active_storage_tables.rb b/activestorage/db/migrate/20170806125915_create_active_storage_tables.rb index 9e31e3966a..cfaf01cd5e 100644 --- a/activestorage/db/migrate/20170806125915_create_active_storage_tables.rb +++ b/activestorage/db/migrate/20170806125915_create_active_storage_tables.rb @@ -20,6 +20,7 @@ class CreateActiveStorageTables < ActiveRecord::Migration[5.2] t.datetime :created_at, null: false t.index [ :record_type, :record_id, :name, :blob_id ], name: "index_active_storage_attachments_uniqueness", unique: true + t.foreign_key :active_storage_blobs, column: :blob_id end end end diff --git a/activestorage/lib/active_storage/attached.rb b/activestorage/lib/active_storage/attached.rb index 0ccf76f00b..b540f85fbe 100644 --- a/activestorage/lib/active_storage/attached.rb +++ b/activestorage/lib/active_storage/attached.rb @@ -6,10 +6,10 @@ module ActiveStorage # Abstract base class for the concrete ActiveStorage::Attached::One and ActiveStorage::Attached::Many # classes that both provide proxy access to the blob association for a record. class Attached - attr_reader :name, :record, :dependent + attr_reader :name, :record - def initialize(name, record, dependent:) - @name, @record, @dependent = name, record, dependent + def initialize(name, record) + @name, @record = name, record end private diff --git a/activestorage/lib/active_storage/attached/model.rb b/activestorage/lib/active_storage/attached/model.rb index aa5c769cb8..ae7f0685f2 100644 --- a/activestorage/lib/active_storage/attached/model.rb +++ b/activestorage/lib/active_storage/attached/model.rb @@ -33,7 +33,7 @@ module ActiveStorage def has_one_attached(name, dependent: :purge_later) generated_association_methods.class_eval <<-CODE, __FILE__, __LINE__ + 1 def #{name} - @active_storage_attached_#{name} ||= ActiveStorage::Attached::One.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"}) + @active_storage_attached_#{name} ||= ActiveStorage::Attached::One.new("#{name}", self) end def #{name}=(attachable) @@ -89,7 +89,7 @@ module ActiveStorage def has_many_attached(name, dependent: :purge_later) generated_association_methods.class_eval <<-CODE, __FILE__, __LINE__ + 1 def #{name} - @active_storage_attached_#{name} ||= ActiveStorage::Attached::Many.new("#{name}", self, dependent: #{dependent == :purge_later ? ":purge_later" : "false"}) + @active_storage_attached_#{name} ||= ActiveStorage::Attached::Many.new("#{name}", self) end def #{name}=(attachables) diff --git a/activestorage/test/jobs/purge_job_test.rb b/activestorage/test/jobs/purge_job_test.rb index 251022a96f..ed4100b78d 100644 --- a/activestorage/test/jobs/purge_job_test.rb +++ b/activestorage/test/jobs/purge_job_test.rb @@ -24,4 +24,14 @@ class ActiveStorage::PurgeJobTest < ActiveJob::TestCase end end end + + test "ignores attached blob" do + User.create! name: "DHH", avatar: @blob + + perform_enqueued_jobs do + assert_nothing_raised do + ActiveStorage::PurgeJob.perform_later @blob + end + end + end end diff --git a/activestorage/test/models/attached/many_test.rb b/activestorage/test/models/attached/many_test.rb index 82be88af08..334254d099 100644 --- a/activestorage/test/models/attached/many_test.rb +++ b/activestorage/test/models/attached/many_test.rb @@ -10,7 +10,7 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase @user = User.create!(name: "Josh") end - teardown { ActiveStorage::Blob.all.each(&:purge) } + teardown { ActiveStorage::Blob.all.each(&:delete) } test "attaching existing blobs to an existing record" do @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") diff --git a/activestorage/test/models/attached/one_test.rb b/activestorage/test/models/attached/one_test.rb index 01caaf0b55..3333fd9323 100644 --- a/activestorage/test/models/attached/one_test.rb +++ b/activestorage/test/models/attached/one_test.rb @@ -10,7 +10,7 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase @user = User.create!(name: "Josh") end - teardown { ActiveStorage::Blob.all.each(&:purge) } + teardown { ActiveStorage::Blob.all.each(&:delete) } test "attaching an existing blob to an existing record" do @user.avatar.attach create_blob(filename: "funky.jpg") diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index a0e207642a..c2e7aae13a 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -174,6 +174,14 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase assert_not ActiveStorage::Blob.service.exist?(variant.key) end + test "purge fails when attachments exist" do + create_blob.tap do |blob| + User.create! name: "DHH", avatar: blob + assert_raises(ActiveRecord::InvalidForeignKey) { blob.purge } + assert ActiveStorage::Blob.service.exist?(blob.key) + end + end + private def expected_url_for(blob, disposition: :inline, filename: nil) filename ||= blob.filename diff --git a/guides/source/association_basics.md b/guides/source/association_basics.md index 67844ae7a4..808aa9d691 100644 --- a/guides/source/association_basics.md +++ b/guides/source/association_basics.md @@ -600,7 +600,7 @@ NOTE: If you wish to [enforce referential integrity at the database level](/acti #### Creating Join Tables for `has_and_belongs_to_many` Associations -If you create a `has_and_belongs_to_many` association, you need to explicitly create the joining table. Unless the name of the join table is explicitly specified by using the `:join_table` option, Active Record creates the name by using the lexical book of the class names. So a join between author and book models will give the default join table name of "authors_books" because "a" outranks "b" in lexical ordering. +If you create a `has_and_belongs_to_many` association, you need to explicitly create the joining table. Unless the name of the join table is explicitly specified by using the `:join_table` option, Active Record creates the name by using the lexical order of the class names. So a join between author and book models will give the default join table name of "authors_books" because "a" outranks "b" in lexical ordering. WARNING: The precedence between model names is calculated using the `<=>` operator for `String`. This means that if the strings are of different lengths, and the strings are equal when compared up to the shortest length, then the longer string is considered of higher lexical precedence than the shorter one. For example, one would expect the tables "paper_boxes" and "papers" to generate a join table name of "papers_paper_boxes" because of the length of the name "paper_boxes", but it in fact generates a join table name of "paper_boxes_papers" (because the underscore '\_' is lexicographically _less_ than 's' in common encodings). diff --git a/railties/lib/rails/commands/notes/notes_command.rb b/railties/lib/rails/commands/notes/notes_command.rb index a0faaeff8f..64b339b3cd 100644 --- a/railties/lib/rails/commands/notes/notes_command.rb +++ b/railties/lib/rails/commands/notes/notes_command.rb @@ -28,7 +28,7 @@ module Rails def deprecation_warning return if source_annotation_directories.empty? - ActiveSupport::Deprecation.warn("`SOURCE_ANNOTATION_DIRECTORIES` will be deprecated in Rails 6.1. You can add default directories by using config.annotations.register_directories instead.") + ActiveSupport::Deprecation.warn("`SOURCE_ANNOTATION_DIRECTORIES` is deprecated and will be removed in Rails 6.1. You can add default directories by using config.annotations.register_directories instead.") end def source_annotation_directories diff --git a/railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt index a5eccf816b..f6146e7259 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt +++ b/railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt @@ -4,8 +4,9 @@ # the maximum value specified for Puma. Default is set to 5 threads for minimum # and maximum; this matches the default thread size of Active Record. # -threads_count = ENV.fetch("RAILS_MAX_THREADS") { 5 } -threads threads_count, threads_count +max_threads_count = ENV.fetch("RAILS_MAX_THREADS") { 5 } +min_threads_count = ENV.fetch("RAILS_MIN_THREADS") { max_threads_count } +threads min_threads_count, max_threads_count # Specifies the `port` that Puma will listen on to receive requests; default is 3000. # diff --git a/railties/test/application/rake/notes_test.rb b/railties/test/application/rake/notes_test.rb index d73e5cdfa3..9e22ba84b5 100644 --- a/railties/test/application/rake/notes_test.rb +++ b/railties/test/application/rake/notes_test.rb @@ -30,19 +30,22 @@ module ApplicationTests app_file "config/locales/en.yaml", "# TODO: note in yaml" app_file "app/views/home/index.ruby", "# TODO: note in ruby" - run_rake_notes do |output, lines| - assert_match(/note in erb/, output) - assert_match(/note in js/, output) - assert_match(/note in css/, output) - assert_match(/note in rake/, output) - assert_match(/note in builder/, output) - assert_match(/note in yml/, output) - assert_match(/note in yaml/, output) - assert_match(/note in ruby/, output) - - assert_equal 9, lines.size - assert_equal [4], lines.map(&:size).uniq + stderr = capture(:stderr) do + run_rake_notes do |output, lines| + assert_match(/note in erb/, output) + assert_match(/note in js/, output) + assert_match(/note in css/, output) + assert_match(/note in rake/, output) + assert_match(/note in builder/, output) + assert_match(/note in yml/, output) + assert_match(/note in yaml/, output) + assert_match(/note in ruby/, output) + + assert_equal 9, lines.size + assert_equal [4], lines.map(&:size).uniq + end end + assert_match(/DEPRECATION WARNING: This rake task is deprecated and will be removed in Rails 6.1/, stderr) end test "notes finds notes in default directories" do @@ -54,17 +57,20 @@ module ApplicationTests app_file "some_other_dir/blah.rb", "# TODO: note in some_other directory" - run_rake_notes do |output, lines| - assert_match(/note in app directory/, output) - assert_match(/note in config directory/, output) - assert_match(/note in db directory/, output) - assert_match(/note in lib directory/, output) - assert_match(/note in test directory/, output) - assert_no_match(/note in some_other directory/, output) - - assert_equal 5, lines.size - assert_equal [4], lines.map(&:size).uniq + stderr = capture(:stderr) do + run_rake_notes do |output, lines| + assert_match(/note in app directory/, output) + assert_match(/note in config directory/, output) + assert_match(/note in db directory/, output) + assert_match(/note in lib directory/, output) + assert_match(/note in test directory/, output) + assert_no_match(/note in some_other directory/, output) + + assert_equal 5, lines.size + assert_equal [4], lines.map(&:size).uniq + end end + assert_match(/DEPRECATION WARNING: This rake task is deprecated and will be removed in Rails 6.1/, stderr) end test "notes finds notes in custom directories" do @@ -76,18 +82,22 @@ module ApplicationTests app_file "some_other_dir/blah.rb", "# TODO: note in some_other directory" - run_rake_notes "SOURCE_ANNOTATION_DIRECTORIES='some_other_dir' bin/rails notes" do |output, lines| - assert_match(/note in app directory/, output) - assert_match(/note in config directory/, output) - assert_match(/note in db directory/, output) - assert_match(/note in lib directory/, output) - assert_match(/note in test directory/, output) + stderr = capture(:stderr) do + run_rake_notes "SOURCE_ANNOTATION_DIRECTORIES='some_other_dir' bin/rake notes" do |output, lines| + assert_match(/note in app directory/, output) + assert_match(/note in config directory/, output) + assert_match(/note in db directory/, output) + assert_match(/note in lib directory/, output) + assert_match(/note in test directory/, output) - assert_match(/note in some_other directory/, output) + assert_match(/note in some_other directory/, output) - assert_equal 6, lines.size - assert_equal [4], lines.map(&:size).uniq + assert_equal 6, lines.size + assert_equal [4], lines.map(&:size).uniq + end end + assert_match(/DEPRECATION WARNING: This rake task is deprecated and will be removed in Rails 6.1/, stderr) + assert_match(/DEPRECATION WARNING: `SOURCE_ANNOTATION_DIRECTORIES` is deprecated and will be removed in Rails 6.1/, stderr) end test "custom rake task finds specific notes in specific directories" do @@ -122,11 +132,14 @@ module ApplicationTests app_file "app/assets/stylesheets/application.css.scss", "// TODO: note in scss" app_file "app/assets/stylesheets/application.css.sass", "// TODO: note in sass" - run_rake_notes do |output, lines| - assert_match(/note in scss/, output) - assert_match(/note in sass/, output) - assert_equal 2, lines.size + stderr = capture(:stderr) do + run_rake_notes do |output, lines| + assert_match(/note in scss/, output) + assert_match(/note in sass/, output) + assert_equal 2, lines.size + end end + assert_match(/DEPRECATION WARNING: This rake task is deprecated and will be removed in Rails 6.1/, stderr) end test "register additional directories" do @@ -134,38 +147,27 @@ module ApplicationTests app_file "spec/models/user_spec.rb", "# TODO: note in model spec" add_to_config ' config.annotations.register_directories("spec") ' - run_rake_notes do |output, lines| - assert_match(/note in spec/, output) - assert_match(/note in model spec/, output) - assert_equal 2, lines.size + stderr = capture(:stderr) do + run_rake_notes do |output, lines| + assert_match(/note in spec/, output) + assert_match(/note in model spec/, output) + assert_equal 2, lines.size + end end + assert_match(/DEPRECATION WARNING: This rake task is deprecated and will be removed in Rails 6.1/, stderr) end private - def run_rake_notes(command = "bin/rails notes") - boot_rails - load_tasks - + def run_rake_notes(command = "bin/rake notes") Dir.chdir(app_path) do output = `#{command}` - lines = output.scan(/\[([0-9\s]+)\]\s/).flatten + + lines = output.scan(/\[([0-9\s]+)\]\s/).flatten yield output, lines end end - - def load_tasks - require "rake" - require "rdoc/task" - require "rake/testtask" - - Rails.application.load_tasks - end - - def boot_rails - require "#{app_path}/config/environment" - end end end end |