Lock down new ImplicitRender behavior for 5.0 RC

actionpack/CHANGELOG.md

@@ -1,3 +1,32 @@
+*   Update default rendering policies when the controller action did
+    not explicitly indicate a response.
+
+    For API controllers, the implicit render always renders "204 No Content"
+    and does not account for any templates.
+
+    For other controllers, the following conditions are checked:
+
+    First, if a template exists for the controller action, it is rendered.
+    This template lookup takes into account the action name, locales, format,
+    variant, template handlers, etc. (see +render+ for details).
+
+    Second, if other templates exist for the controller action but is not in
+    the right format (or variant, etc.), an <tt>ActionController::UnknownFormat</tt>
+    is raised. The list of available templates is assumed to be a complete
+    enumeration of all the possible formats (or variants, etc.); that is,
+    having only HTML and JSON templates indicate that the controller action is
+    not meant to handle XML requests.
+
+    Third, if the current request is an "interactive" browser request (the user
+    navigated here by entering the URL in the address bar, submiting a form,
+    clicking on a link, etc. as opposed to an XHR or non-browser API request),
+    <tt>ActionView::UnknownFormat</tt> is raised to display a helpful error
+    message.
+
+    Finally, it falls back to the same "204 No Content" behavior as API controllers.
+
+    *Godfrey Chan*, *Jon Moss*, *Kasper Timm Hansen*, *Mike Clark*, *Matthew Draper*
+
 ## Rails 5.0.0.beta3 (February 24, 2016) ##
 
 *   Update session to have indifferent access across multiple requests.

actionpack/lib/action_controller/metal/basic_implicit_render.rb

@@ -1,5 +1,5 @@
 module ActionController
-  module BasicImplicitRender
+  module BasicImplicitRender # :nodoc:
     def send_action(method, *args)
       super.tap { default_render unless performed? }
     end

actionpack/lib/action_controller/metal/implicit_render.rb

@@ -1,29 +1,80 @@
+require 'active_support/core_ext/string/strip'
+
 module ActionController
+  # Handles implicit rendering for a controller action when it did not
+  # explicitly indicate an appropiate response via methods such as +render+,
+  # +respond_to+, +redirect+ or +head+.
+  #
+  # For API controllers, the implicit render always renders "204 No Content"
+  # and does not account for any templates.
+  #
+  # For other controllers, the following conditions are checked:
+  #
+  # First, if a template exists for the controller action, it is rendered.
+  # This template lookup takes into account the action name, locales, format,
+  # variant, template handlers, etc. (see +render+ for details).
+  #
+  # Second, if other templates exist for the controller action but is not in
+  # the right format (or variant, etc.), an <tt>ActionController::UnknownFormat</tt>
+  # is raised. The list of available templates is assumed to be a complete
+  # enumeration of all the possible formats (or variants, etc.); that is,
+  # having only HTML and JSON templates indicate that the controller action is
+  # not meant to handle XML requests.
+  #
+  # Third, if the current request is an "interactive" browser request (the user
+  # navigated here by entering the URL in the address bar, submiting a form,
+  # clicking on a link, etc. as opposed to an XHR or non-browser API request),
+  # <tt>ActionView::UnknownFormat</tt> is raised to display a helpful error
+  # message.
+  #
+  # Finally, it falls back to the same "204 No Content" behavior as API controllers.
   module ImplicitRender
 
+    # :stopdoc:
     include BasicImplicitRender
 
-    # Renders the template corresponding to the controller action, if it exists.
-    # The action name, format, and variant are all taken into account.
-    # For example, the "new" action with an HTML format and variant "phone" 
-    # would try to render the <tt>new.html+phone.erb</tt> template.
-    #
-    # If no template is found <tt>ActionController::BasicImplicitRender</tt>'s implementation is called, unless
-    # a block is passed. In that case, it will override the super implementation.
-    #
-    #   default_render do
-    #     head 404 # No template was found
-    #   end
     def default_render(*args)
       if template_exists?(action_name.to_s, _prefixes, variants: request.variant)
         render(*args)
-      else
-        if block_given?
-          yield(*args)
-        else
-          logger.info "No template found for #{self.class.name}\##{action_name}, rendering head :no_content" if logger
-          super
+      elsif any_templates?(action_name.to_s, _prefixes)
+        message = "#{self.class.name}\##{action_name} does not know how to repond " \
+          "to this request. There are other templates available for this controller " \
+          "action but none of them were suitable for this request.\n\n" \
+          "This usually happens when the client requested an unsupported format " \
+          "(e.g. requesting HTML content from a JSON endpoint or vice versa), but " \
+          "it might also be failing due to other constraints, such as locales or" \
+          "variants.\n"
+
+        if request.formats.any?
+          message << "\nRequested format(s): #{request.formats.join(", ")}"
         end
+
+        if request.variant.any?
+          message << "\nRequested variant(s): #{request.variant.join(", ")}"
+        end
+
+        raise ActionController::UnknownFormat, message
+      elsif interactive_browser_request?
+        message = "You did not define any templates for #{self.class.name}\##{action_name}. " \
+          "This is not necessarily a problem (e.g. you might be building an API endpoint " \
+          "that does not require any templates), and the controller would usually respond " \
+          "with `head :no_content` for your convenience.\n\n" \
+          "However, you appear to have navigated here from an interactive browser request – " \
+          "such as by navigating to this URL directly, clicking on a link or submitting a form. " \
+          "Rendering a `head :no_content` in this case could have resulted in unexpected UI " \
+          "behavior in the browser.\n\n" \
+          "If you expected the `head :no_content` response, you do not need to take any " \
+          "actions – requests coming from an XHR (AJAX) request or other non-browser clients " \
+          "will receive the \"204 No Content\" response as expected.\n\n" \
+          "If you did not expect this behavior, you can resolve this error by adding a " \
+          "template for this controller action (usually `#{action_name}.html.erb`) or " \
+          "otherwise indicate the appropriate response in the action using `render`, " \
+          "`redirect_to`, `head`, etc.\n"
+
+        raise ActionController::UnknownFormat, message
+      else
+        logger.info "No template found for #{self.class.name}\##{action_name}, rendering head :no_content" if logger
+        super
       end
     end
 
@@ -32,5 +83,11 @@ def method_for_action(action_name)
         "default_render"
       end
     end
+
+    private
+
+      def interactive_browser_request?
+        request.format == Mime[:html] && !request.xhr?
+      end
   end
 end

actionpack/test/controller/mime/respond_to_test.rb

@@ -160,7 +160,14 @@ def iphone_with_html_response_type_without_layout
     end
   end
 
-  def variant_with_implicit_rendering
+  def variant_with_implicit_template_rendering
+    # This has exactly one variant template defined in the file system (+mobile.html.erb),
+    # which raises the regular MissingTemplate error for other variants.
+  end
+
+  def variant_without_implicit_template_rendering
+    # This differs from the above in that it does not have any templates defined in the file
+    # system, which triggers the ImplicitRender (204 No Content) behavior.
   end
 
   def variant_with_format_and_custom_render
@@ -272,6 +279,8 @@ def set_layout
 end
 
 class RespondToControllerTest < ActionController::TestCase
+  NO_CONTENT_WARNING = "No template found for RespondToController#variant_without_implicit_template_rendering, rendering head :no_content"
+
   def setup
     super
     @request.host = "www.example.com"
@@ -616,30 +625,69 @@ def test_invalid_format
   end
 
   def test_invalid_variant
+    assert_raises(ActionController::UnknownFormat) do
+      get :variant_with_implicit_template_rendering, params: { v: :invalid }
+    end
+  end
+
+  def test_variant_not_set_regular_unknown_format
+    assert_raises(ActionController::UnknownFormat) do
+      get :variant_with_implicit_template_rendering
+    end
+  end
+
+  def test_variant_with_implicit_template_rendering
+    get :variant_with_implicit_template_rendering, params: { v: :mobile }
+    assert_equal "text/html", @response.content_type
+    assert_equal "mobile", @response.body
+  end
+
+  def test_variant_without_implicit_rendering_from_browser
+    assert_raises(ActionController::UnknownFormat) do
+      get :variant_without_implicit_template_rendering, params: { v: :does_not_matter }
+    end
+  end
+
+  def test_variant_variant_not_set_and_without_implicit_rendering_from_browser
+    assert_raises(ActionController::UnknownFormat) do
+      get :variant_without_implicit_template_rendering
+    end
+  end
+
+  def test_variant_without_implicit_rendering_from_xhr
     logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
     old_logger, ActionController::Base.logger = ActionController::Base.logger, logger
 
-    get :variant_with_implicit_rendering, params: { v: :invalid }
+    get :variant_without_implicit_template_rendering, xhr: true, params: { v: :does_not_matter }
     assert_response :no_content
-    assert_equal 1, logger.logged(:info).select{ |s| s =~ /No template found/ }.size, "Implicit head :no_content not logged"
+
+    assert_equal 1, logger.logged(:info).select{ |s| s == NO_CONTENT_WARNING }.size, "Implicit head :no_content not logged"
   ensure
     ActionController::Base.logger = old_logger
   end
 
-  def test_variant_not_set_regular_template_missing
-    get :variant_with_implicit_rendering
+  def test_variant_without_implicit_rendering_from_api
+    logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
+    old_logger, ActionController::Base.logger = ActionController::Base.logger, logger
+
+    get :variant_without_implicit_template_rendering, format: 'json', params: { v: :does_not_matter }
     assert_response :no_content
+
+    assert_equal 1, logger.logged(:info).select{ |s| s == NO_CONTENT_WARNING }.size, "Implicit head :no_content not logged"
+  ensure
+    ActionController::Base.logger = old_logger
   end
 
-  def test_variant_with_implicit_rendering
-    get :variant_with_implicit_rendering, params: { v: :implicit }
+  def test_variant_variant_not_set_and_without_implicit_rendering_from_xhr
+    logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
+    old_logger, ActionController::Base.logger = ActionController::Base.logger, logger
+
+    get :variant_without_implicit_template_rendering, xhr: true
     assert_response :no_content
-  end
 
-  def test_variant_with_implicit_template_rendering
-    get :variant_with_implicit_rendering, params: { v: :mobile }
-    assert_equal "text/html", @response.content_type
-    assert_equal "mobile", @response.body
+    assert_equal 1, logger.logged(:info).select { |s| s == NO_CONTENT_WARNING }.size, "Implicit head :no_content not logged"
+  ensure
+    ActionController::Base.logger = old_logger
   end
 
   def test_variant_with_format_and_custom_render
@@ -778,24 +826,3 @@ def test_variant_negotiation_without_block
     assert_equal "phone", @response.body
   end
 end
-
-class RespondToWithBlockOnDefaultRenderController < ActionController::Base
-  def show
-    default_render do
-      render body: 'default_render yielded'
-    end
-  end
-end
-
-class RespondToWithBlockOnDefaultRenderControllerTest < ActionController::TestCase
-  def setup
-    super
-    @request.host = "www.example.com"
-  end
-
-  def test_default_render_uses_block_when_no_template_exists
-    get :show
-    assert_equal "default_render yielded", @response.body
-    assert_equal "text/plain", @response.content_type
-  end
-end

actionpack/test/controller/render_test.rb

@@ -26,6 +26,9 @@ def with_template
 class ImplicitRenderTestController < ActionController::Base
   def empty_action
   end
+
+  def empty_action_with_template
+  end
 end
 
 class TestController < ActionController::Base
@@ -537,10 +540,28 @@ def test_access_to_logger_in_view
 class ImplicitRenderTest < ActionController::TestCase
   tests ImplicitRenderTestController
 
-  def test_implicit_no_content_response
-    get :empty_action
+  def test_implicit_no_content_response_as_browser
+    assert_raises(ActionController::UnknownFormat) do
+      get :empty_action
+    end
+  end
+
+  def test_implicit_no_content_response_as_xhr
+    get :empty_action, xhr: true
     assert_response :no_content
   end
+
+  def test_implicit_success_response_with_right_format
+    get :empty_action_with_template
+    assert_equal "<h1>Empty action rendered this implicitly.</h1>\n", @response.body
+    assert_response :success
+  end
+
+  def test_implicit_unknown_format_response
+    assert_raises(ActionController::UnknownFormat) do
+      get :empty_action_with_template, format: 'json'
+    end
+  end
 end
 
 class HeadRenderTest < ActionController::TestCase

actionpack/test/fixtures/implicit_render_test/empty_action_with_mobile_variant.html+mobile.erb

@@ -0,0 +1 @@
+mobile

actionpack/test/fixtures/implicit_render_test/empty_action_with_template.html.erb

@@ -0,0 +1 @@
+<h1>Empty action rendered this implicitly.</h1>

actionpack/test/fixtures/implicit_render_test/some_action_2.html+zomg.erb

@@ -0,0 +1 @@
+zomg

actionview/lib/action_view/lookup_context.rb

@@ -136,6 +136,11 @@ def exists?(name, prefixes = [], partial = false, keys = [], **options)
       end
       alias :template_exists? :exists?
 
+      def any?(name, prefixes = [], partial = false)
+        @view_paths.exists?(*args_for_any(name, prefixes, partial))
+      end
+      alias :any_templates? :any?
+
       # Adds fallbacks to the view paths. Useful in cases when you are rendering
       # a :file.
       def with_fallbacks
@@ -172,6 +177,32 @@ def detail_args_for(options)
         [user_details, details_key]
       end
 
+      def args_for_any(name, prefixes, partial) # :nodoc:
+        name, prefixes = normalize_name(name, prefixes)
+        details, details_key = detail_args_for_any
+        [name, prefixes, partial || false, details, details_key]
+      end
+
+      def detail_args_for_any # :nodoc:
+        @detail_args_for_any ||= begin
+          details = {}
+
+          registered_details.each do |k|
+            if k == :variants
+              details[k] = :any
+            else
+              details[k] = Accessors::DEFAULT_PROCS[k].call
+            end
+          end
+
+          if @cache
+            [details, DetailsKey.get(details)]
+          else
+            [details, nil]
+          end
+        end
+      end
+
       # Support legacy foo.erb names even though we now ignore .erb
       # as well as incorrectly putting part of the path in the template
       # name instead of the prefix.

actionview/lib/action_view/renderer/abstract_renderer.rb

@@ -15,7 +15,7 @@ module ActionView
   # that new object is called in turn. This abstracts the setup and rendering
   # into a separate classes for partials and templates.
   class AbstractRenderer #:nodoc:
-    delegate :find_template, :find_file, :template_exists?, :with_fallbacks, :with_layout_format, :formats, :to => :@lookup_context
+    delegate :find_template, :find_file, :template_exists?, :any_templates?, :with_fallbacks, :with_layout_format, :formats, :to => :@lookup_context
 
     def initialize(lookup_context)
       @lookup_context = lookup_context