Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

when a template is missing for the default render, do head no_content instead #19377

Merged
merged 1 commit into from Apr 6, 2015

Conversation

@sb8244
Copy link
Contributor

sb8244 commented Mar 17, 2015

Fixes #19036

#19036 (comment)

I want to do a comparison like:

    def default_render(*args)
      if template_exists?(action, _prefixes)
        render(*args)
      else
        head :no_content
      end
    end

However, there is no way (that I know of) to access the action at this point, it is simply the method. I opted for rescuing MissingTemplate as an iteration 1 that I will look into refactoring. Ideally, the logic for rendering default_render vs head :no_content will happen in ImplicitRender#send_action and not in default_render.

@@ -8,6 +8,8 @@ def send_action(method, *args)

def default_render(*args)
render(*args)
rescue ActionView::MissingTemplate

This comment has been minimized.

Copy link
@sb8244

sb8244 Mar 17, 2015

Author Contributor

Concerns around this being slow / an exceptional state when it doesn't need to be. It should be once-per-request code path, and it was exceptional but being handled.

I'm happy to check if the template exists or not, just need to find out how to turn a method into the correct template name.

@rafaelfranca
Copy link
Member

rafaelfranca commented Mar 17, 2015

Ideally, the logic for rendering default_render vs head :no_content will happen in ImplicitRender#send_action and not in default_render.

Yes, this is the best solution here. Lets split default_render in two methods, one that calls render if the template exists and other that call head :no_content if it doesn't.

Also it is missing a test for implicit render with non-defined actions.

@sb8244
Copy link
Contributor Author

sb8244 commented Mar 17, 2015

@rafaelfranca in send_action, how do you get access to the action_name from the method? I need this to do template_exists as the method/action_name may differ.

@rafaelfranca
Copy link
Member

rafaelfranca commented Mar 17, 2015

It will hardly differ. method_name is action_name, unless you don't have an action with that name and your controller responds to action_missing. If that is the case the performed? will return true since action_missing will set the response body and we will not call default_render anyway.

@sb8244
Copy link
Contributor Author

sb8244 commented Mar 17, 2015

@rafaelfranca I think that variants through off the method_name vs action_name. For example, this spec fails because it thinks a template does not exist and renders a head :no_content instead of the mobile variant.

  def test_variant_with_implicit_rendering
    @request.variant = :mobile
    get :variant_with_implicit_rendering
    assert_equal "text/html", @response.content_type
    assert_equal "mobile", @response.body
  end
@sb8244
Copy link
Contributor Author

sb8244 commented Mar 18, 2015

Looking into variant implicit rendering more. It looks like the default behavior is to render the variant name. I can not find where in actionpack or actionview this behavior is defined at.

Should the default for variants be to render the variant name, or head :no_content?

@sb8244
Copy link
Contributor Author

sb8244 commented Apr 2, 2015

@rafaelfranca do you have any feedback on how to approach the implicit variant rendering?

@kaspth
Copy link
Member

kaspth commented Apr 3, 2015

@sb8244 what does render the variant name mean? Can you show a code example?

@sb8244
Copy link
Contributor Author

sb8244 commented Apr 3, 2015

Sure @kaspth

  def test_variant_with_implicit_rendering
    @request.variant = :mobile
    get :variant_with_implicit_rendering
    assert_equal "text/html", @response.content_type
    assert_equal "mobile", @response.body
  end

This test is expecting that the behavior of an implicit rendering of a variant is the variant name. However, it seems like this behavior is going to change based on this PR, so that the default behavior is a HEAD no-content.

ret
return ret if performed?

if template_exists?(method, _prefixes)

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 4, 2015

Member

Because of this do we still need method_for_action down below?

Alternatively, we might be able to implement this as:

def default_head(*)
  logger.info "No template found for #{self.class.name}#{action_name}, rendering head :no_content"
  head :no_content
end

def method_for_action(action_name)
  if method = super 
    method
  elsif template_exists?(action_name.to_s, _prefixes)
    "default_render"
  else
    "default_head"
  end
end

This comment has been minimized.

Copy link
@sb8244

sb8244 Apr 5, 2015

Author Contributor

method_for_action is important when you are rendering a method that doesn't exist (super is nil in that case). Implementing it like you proposed is how it should be handled, although template_exists? is not picking up variants correctly here as well as in send_action.

This comment has been minimized.

Copy link
@sb8244

sb8244 Apr 5, 2015

Author Contributor

I propose as leaving method_for_action alone. The current method body is:

    def method_for_action(action_name)
      super || if template_exists?(action_name.to_s, _prefixes)
        "default_render"
      end
    end

The reason I say this is that the default behavior of default_head should apply when there is a valid method that doesn't call render but not in the case of there being no method at all. This could lead to more complex errors such as getting head :no_content that doesn't seem to apply to a method.

@kaspth
Copy link
Member

kaspth commented Apr 4, 2015

Ah, I see where I went astray. The default render for variants just renders the correct template, say show.html+mobile.erb.

That's why you can't find any rendering of the variant name defined in Action Pack or Action View. The reason you're seeing assert_equal "mobile", @response.body in the test, is that the template rendered by the test only contains the word "mobile". You can see it here: https://github.com/sb8244/rails/blob/issue-19036/actionpack/test/fixtures/respond_to/variant_with_implicit_rendering.html%2Bmobile.erb

To answer your question head :no_content is what we want.

@@ -608,16 +608,20 @@ def test_invalid_format
end

def test_invalid_variant
logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
ActionController::Base.logger = logger

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 4, 2015

Member

This test overwrites the logger, which will leek into other tests. You have to capture the original value and then set it again after the tests:

def test_invalid_variant
  logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
  old_logger, ActionController::Base.logger = ActionController::Base.logger, logger

  # tests...
ensure
  ActionController::Base.logger = old_logger
end
get :variant_with_implicit_rendering
end
get :variant_with_implicit_rendering
assert_equal 204, @response.status

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 4, 2015

Member

Can we suffice with assert_response :no_content here? If so, don't bother with the other assertions in this test.

This comment has been minimized.

Copy link
@sb8244

sb8244 Apr 5, 2015

Author Contributor

nice! Yea that works great.

get :variant_with_implicit_rendering
end
get :variant_with_implicit_rendering
assert_equal 204, @response.status

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 4, 2015

Member

Same goes here.

@kaspth
Copy link
Member

kaspth commented Apr 4, 2015

@sb8244 Left a few notes. We also need an entry at the top of Action Pack's CHANGELOG.md file. Finally squash all your commits into one.

Once those things have been addressed, we should be good to go, 👍.

@kaspth kaspth added actionpack and removed actionview labels Apr 4, 2015
@kaspth kaspth added this to the 5.0.0 milestone Apr 4, 2015
@kaspth
Copy link
Member

kaspth commented Apr 4, 2015

@rafaelfranca do we need a deprecation warning for this too or an upgrade flag (raise instead of no content)?

We might even be able to remove the missing template error completely.

@sb8244
Copy link
Contributor Author

sb8244 commented Apr 5, 2015

@kaspth in the case of mobile template being rendered, wouldn't we want that in this specific test? There is a template that exists, so head :no_content would be unexpected behavior. If the variant doesn't have a template, then it should be a head no_content.

@kaspth
Copy link
Member

kaspth commented Apr 5, 2015

@sb8244 sorry, for not being clearer. What you're saying is what we want: render the variant template if it's there or head :no_content.

@sb8244
Copy link
Contributor Author

sb8244 commented Apr 5, 2015

Awesome, I'm digging through template_exists? now to figure out how to properly pass the variant. It's not getting used currently so template_exists?(method, _prefixes) is returning false.

@sb8244
Copy link
Contributor Author

sb8244 commented Apr 5, 2015

I've determined these will work:

template_exists?(method, _prefixes, false, {}, { variants: request.variant })

or set .variants

lookup_context.variants = request.variant

Any preference?

@kaspth
Copy link
Member

kaspth commented Apr 5, 2015

@sb8244 do we need either of those? The way I see it variants are already handled.

For instance the original tests would render the correct template if the correct variant was set and otherwise raise MissingTemplate.
It seems the variant is extracted in the resolver too:

handler, format, variant = extract_handler_and_format_and_variant(template, formats)
.

@sb8244
Copy link
Contributor Author

sb8244 commented Apr 5, 2015

The variants are not handled in the case of implicit rendering in the proposed change. It worked previously because the following code snippet always default_renders unless performed?. Because it doesn't check for a template, it will either render correctly (letting the resolver do its thing) or raise the MissingTemplate error.

ret = super
default_render unless performed?
ret

In the proposed change, it only default_renders if the template exists, so the implicit rendering of variant doesn't get handled.

@kaspth
Copy link
Member

kaspth commented Apr 5, 2015

Ah, I see. Well, I don't like mutating the @_lookup_context, so let's go with the first option.

Can we change template_exists? to use **options instead of options = {}:

def exists?(name, prefixes = [], partial = false, keys = [], options = {})
.

This way we should be able to call it like:

template_exists?(method, _prefixes, variants: @_request.variant)
end

def default_render(*args)
render(*args)
end

def default_head(*)

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 5, 2015

Member

We should extend method_for_action to use this too, right? Otherwise we should just put this back in the else clause above.

This comment has been minimized.

Copy link
@sb8244

sb8244 Apr 5, 2015

Author Contributor

I made a comment about that on the original comment you did. I think we should not change method_for_action because it isn't in the use case and could very easily lead to silent confusion in certain cases. I'll pull it back into the send_action method.

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 5, 2015

Member

Yeah, I thought about this some more and you're right, 👍.

@sb8244 sb8244 force-pushed the sb8244:issue-19036 branch to 305e4ca Apr 5, 2015
dhh added a commit that referenced this pull request Apr 6, 2015
when a template is missing for the default render, do head no_content instead
@dhh dhh merged commit ca4417d into rails:master Apr 6, 2015
@sb8244
Copy link
Contributor Author

sb8244 commented Apr 6, 2015

Woot first Rails commit! See yall at Rails conf.

On Monday, April 6, 2015, David Heinemeier Hansson notifications@github.com
wrote:

Merged #19377 #19377.


Reply to this email directly or view it on GitHub
#19377 (comment).

Steve Bussey •SalesLoft

•Software Engineer

e steve.bussey@salesloft.com w salesloft.com
http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=Email-Signature-Rescue&utm_medium=Email-Signature&utm_campaign=Email-Signature-Rescue

a 3423 Piedmont Rd NE, Atlanta, GA 30305

[image: Facebook] https://www.facebook.com/SalesLoft [image: Twitter]
https://twitter.com/SalesLoft [image: Linkedin]
https://www.linkedin.com/company/salesloft [image: wordpress]
http://blog.salesloft.com/blog/ [image:
http://salesloft.com/prospector/#video]
http://salesloft.com/prospector/#video [image: Software Engineer Logo]
http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=Email-Signature-Rescue&utm_medium=Email-Signature&utm_campaign=Email-Signature-Rescue

eileencodes added a commit that referenced this pull request Apr 6, 2015
After merging #19377 ActionPack tests were missing a require for
`ActiveSupport::LogSubscriber::TestHelper` and change didn't take
into account that logger could be nil. Added the require and only log to
info if logger exists.

This wasn't caught earlier because these tests only run after a merge.
@eileencodes
Copy link
Member

eileencodes commented Apr 6, 2015

Congrats on your first merged Rails commit @sb8244! 😄

This caused a minor bug in ActionPack (don't feel bad we've all caused bugs 🐛). actionpack/test/controller/mime/respond_to_test.rb tests only run after merge so CI didn't catch it earlier. Fixed in 188934c.

@clarkware
Copy link

clarkware commented Feb 7, 2016

I'd love to find a way to bring back the "Missing template" exception for a regular browser request to fix #23077. As a start, I spent some time looking through this merge and how it compares to the behavior in Rails 4. The primary difference is that default_render now checks template_exists?. This short-circuits the template existence checks that are already part of the render method in ActionView::TemplateRenderer, including raising ActionView::MissingTemplate if the template isn't found.

For browser requests, it would be ideal to just call render straight away and let ActionView::TemplateRenderer do the checking. If the template for the default render isn't found, then ActionView::MissingTemplate gets raised and the error shows up in the browser just as before. Here's a quick hack that demonstrates the idea:

def default_render(*args)
   if request.format == Mime[:html]
     render(*args)
  end

  # current implementation for all other formats
  # which returns a 204 if the template is missing
end

I realize I'm really late to this party and may be missing subtleties that were addressed in the original discussion. Is there general interest in a pull request along these lines that raises a MissingTemplate exception for browser requests (the default behavior pre-Rails 5) rather than returning a 204?

Any help or guidance is appreciated.

@sb8244 sb8244 deleted the sb8244:issue-19036 branch Feb 7, 2016
@kaspth
Copy link
Member

kaspth commented Feb 7, 2016

@clarkware yeah, do open a pull request. ❤️

Your suggestion also comports with @matthewd's here: #19036 (comment) and #19036 (comment)

@matthewd
Copy link
Member

matthewd commented Feb 7, 2016

Yeah, I think the only potential challenge will be in finding the right way of defining an interactive_browser_request?. Maybe 'html && !xhr' is sufficient? It feels like something we should have prior art on, but nothing specific is coming to mind.

@sb8244
Copy link
Contributor Author

sb8244 commented Feb 7, 2016

That is my concern as well. For instance, a potentially interesting side
effect of the 204 is that you can GET request without the browser changing
pages. This could allow graceful fallback for scriptless support. Maybe
that isn't a use case, but it made me pause and consider what
an interactive request definition might have to overcome.

Steve Bussey • SalesLoft

• Software Engineer

e steve.bussey@salesloft.com w salesloft.com
http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=Email-Signature-Rescue&utm_medium=Email-Signature&utm_campaign=Email-Signature-Rescue

a 3423 Piedmont Rd NE, Atlanta, GA 30305

[image: Facebook] https://www.facebook.com/SalesLoft [image: Twitter]
https://twitter.com/SalesLoft [image: Linkedin]
https://www.linkedin.com/company/salesloft [image: wordpress]
http://blog.salesloft.com/blog/ [image:
http://salesloft.com/prospector/#video]
http://salesloft.com/prospector/#video [image: Software Engineer Logo]
http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=Email-Signature-Rescue&utm_medium=Email-Signature&utm_campaign=Email-Signature-Rescue

@clarkware
Copy link

clarkware commented Feb 8, 2016

Thanks for the input! Here's a first cut at a pull request: #23564

chancancode added a commit that referenced this pull request Feb 23, 2016
1. Conceptually revert #20276

   The feature was implemented for the `responders` gem. In the end,
   they did not need that feature, and have found a better fix (see
   heartcombo/responders#131).

   `ImplicitRender` is the place where Rails specifies our default
   policies for the case where the user did not explicitly tell us
   what to render, essentially describing a set of heuristics. If
   the gem (or the user) knows exactly what they want, they could
   just perform the correct `render` to avoid falling through to
   here, as `responders` did (the user called `respond_with`).

   Reverting the patch allows us to avoid exploding the complexity
   and defining “the fallback for a fallback” policies.

2. `respond_to` and templates are considered exhaustive enumerations

   If the user specified a list of formats/variants in a `respond_to`
   block, anything that is not explicitly included should result
   in an `UnknownFormat` error (which is then caught upstream to
   mean “406 Not Acceptable” by default). This is already how it
   works before this commit.

   Same goes for templates – if the user defined a set of templates
   (usually in the file system), that set is now considered exhaustive,
   which means that “missing” templates are considered `UnknownFormat`
   errors (406).

3. To keep API endpoints simple, the implicit render behavior for
   actions with no templates defined at all (regardless of formats,
   locales, variants, etc) are defaulted to “204 No Content”. This
   is a strictly narrower version of the feature landed in #19036 and
   #19377.

4. To avoid confusion when interacting in the browser, these actions
   will raise a `TemplateMissing` error for “interactive” requests
   instead. (The precise definition of “interactive” requests might
   change – the spirit here is to give helpful messages and avoid
   confusions.)

Closes #20666, #22854, #23062, #23077, #23564

[Godfrey Chan, Jon Moss, Mike Clark, Matthew Draper]
@chancancode chancancode mentioned this pull request Feb 23, 2016
2 of 2 tasks complete
kaspth added a commit to kaspth/rails that referenced this pull request Feb 23, 2016
1. Conceptually revert rails#20276

   The feature was implemented for the `responders` gem. In the end,
   they did not need that feature, and have found a better fix (see
   heartcombo/responders#131).

   `ImplicitRender` is the place where Rails specifies our default
   policies for the case where the user did not explicitly tell us
   what to render, essentially describing a set of heuristics. If
   the gem (or the user) knows exactly what they want, they could
   just perform the correct `render` to avoid falling through to
   here, as `responders` did (the user called `respond_with`).

   Reverting the patch allows us to avoid exploding the complexity
   and defining “the fallback for a fallback” policies.

2. `respond_to` and templates are considered exhaustive enumerations

   If the user specified a list of formats/variants in a `respond_to`
   block, anything that is not explicitly included should result
   in an `UnknownFormat` error (which is then caught upstream to
   mean “406 Not Acceptable” by default). This is already how it
   works before this commit.

   Same goes for templates – if the user defined a set of templates
   (usually in the file system), that set is now considered exhaustive,
   which means that “missing” templates are considered `UnknownFormat`
   errors (406).

3. To keep API endpoints simple, the implicit render behavior for
   actions with no templates defined at all (regardless of formats,
   locales, variants, etc) are defaulted to “204 No Content”. This
   is a strictly narrower version of the feature landed in rails#19036 and
   rails#19377.

4. To avoid confusion when interacting in the browser, these actions
   will raise a `TemplateMissing` error for “interactive” requests
   instead. (The precise definition of “interactive” requests might
   change – the spirit here is to give helpful messages and avoid
   confusions.)

Closes rails#20666, rails#22854, rails#23062, rails#23077, rails#23564

[Godfrey Chan, Jon Moss, Kasper Timm Hansen, Mike Clark, Matthew Draper]
chancancode added a commit that referenced this pull request Feb 24, 2016
1. Conceptually revert #20276

   The feature was implemented for the `responders` gem. In the end,
   they did not need that feature, and have found a better fix (see
   heartcombo/responders#131).

   `ImplicitRender` is the place where Rails specifies our default
   policies for the case where the user did not explicitly tell us
   what to render, essentially describing a set of heuristics. If
   the gem (or the user) knows exactly what they want, they could
   just perform the correct `render` to avoid falling through to
   here, as `responders` did (the user called `respond_with`).

   Reverting the patch allows us to avoid exploding the complexity
   and defining “the fallback for a fallback” policies.

2. `respond_to` and templates are considered exhaustive enumerations

   If the user specified a list of formats/variants in a `respond_to`
   block, anything that is not explicitly included should result
   in an `UnknownFormat` error (which is then caught upstream to
   mean “406 Not Acceptable” by default). This is already how it
   works before this commit.

   Same goes for templates – if the user defined a set of templates
   (usually in the file system), that set is now considered exhaustive,
   which means that “missing” templates are considered `UnknownFormat`
   errors (406).

3. To keep API endpoints simple, the implicit render behavior for
   actions with no templates defined at all (regardless of formats,
   locales, variants, etc) are defaulted to “204 No Content”. This
   is a strictly narrower version of the feature landed in #19036 and
   #19377.

4. To avoid confusion when interacting in the browser, these actions
   will raise an `UnknownFormat` error for “interactive” requests
   instead. (The precise definition of “interactive” requests might
   change – the spirit here is to give helpful messages and avoid
   confusions.)

Closes #20666, #22854, #23062, #23077, #23564

[Godfrey Chan, Jon Moss, Kasper Timm Hansen, Mike Clark, Matthew Draper]
chancancode added a commit that referenced this pull request Feb 25, 2016
1. Conceptually revert #20276

   The feature was implemented for the `responders` gem. In the end,
   they did not need that feature, and have found a better fix (see
   heartcombo/responders#131).

   `ImplicitRender` is the place where Rails specifies our default
   policies for the case where the user did not explicitly tell us
   what to render, essentially describing a set of heuristics. If
   the gem (or the user) knows exactly what they want, they could
   just perform the correct `render` to avoid falling through to
   here, as `responders` did (the user called `respond_with`).

   Reverting the patch allows us to avoid exploding the complexity
   and defining “the fallback for a fallback” policies.

2. `respond_to` and templates are considered exhaustive enumerations

   If the user specified a list of formats/variants in a `respond_to`
   block, anything that is not explicitly included should result
   in an `UnknownFormat` error (which is then caught upstream to
   mean “406 Not Acceptable” by default). This is already how it
   works before this commit.

   Same goes for templates – if the user defined a set of templates
   (usually in the file system), that set is now considered exhaustive,
   which means that “missing” templates are considered `UnknownFormat`
   errors (406).

3. To keep API endpoints simple, the implicit render behavior for
   actions with no templates defined at all (regardless of formats,
   locales, variants, etc) are defaulted to “204 No Content”. This
   is a strictly narrower version of the feature landed in #19036 and
   #19377.

4. To avoid confusion when interacting in the browser, these actions
   will raise an `UnknownFormat` error for “interactive” requests
   instead. (The precise definition of “interactive” requests might
   change – the spirit here is to give helpful messages and avoid
   confusions.)

Closes #20666, #23062, #23077, #23564

[Godfrey Chan, Jon Moss, Kasper Timm Hansen, Mike Clark, Matthew Draper]
leenagupte added a commit to alphagov/whitehall that referenced this pull request Nov 5, 2019
A 204 No Content is returned if the lock request is successful.
[Rails 5](rails/rails#19377) does this by
default by automatically adding `:no_content` to the header if it can't
find a template to respond with.
leenagupte added a commit to alphagov/whitehall that referenced this pull request Nov 5, 2019
A 204 No Content is returned if the unlock request is successful.
[Rails 5](rails/rails#19377) does this by
default by automatically adding `:no_content` to the header if it can't
find a template to respond with.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.