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

Lock down new `ImplicitRender` behavior for 5.0 RC #23827

Merged
merged 1 commit into from Feb 25, 2016
Merged

Conversation

@chancancode
Copy link
Member

chancancode commented 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, #23062, #23077, #23564

[Godfrey Chan, Jon Moss, Mike Clark, Matthew Draper, Kasper Timm Hansen]


TODO:

  • Finish porting the new tests in the other PRs
  • Write helpful error messages for the two "no templates" scenario
@chancancode chancancode added this to the 5.0.0 milestone Feb 23, 2016
@kaspth kaspth self-assigned this Feb 23, 2016
@kaspth kaspth force-pushed the new_implicit_render branch from ec49054 to 868127c Feb 23, 2016
#
# Third and last, if the current request is a real "interactive" browser request,
# <tt>ActionView::MissingTemplate</tt> is raised to display a helpful error
# message.

This comment has been minimized.

@kaspth

kaspth Feb 23, 2016 Member

Some of the docs felt a bit hard to read to me. Took a stab at rephrasing them.

Something feels funny about assumed exhaustive too, but ¯_(ツ)_/¯.

Here's the originals:

# Handles the implicit behavior for a controller action when it did not
# explicitly call +render+ or otherwise indicate what the response  should be,
# such as by calling +respond_to+, +redirect+ and +head+.
#
# If this controller action has any associated templates, it tries to render
# the most appropiate template for the current action, taking into account of
# the action name, format, locales, variants etc.
#
# The list of available templates is assumed to be exhausative – if a suitable
# template cannot be found (for example, when the client requested the XML
# format when the only templates in HTML and JSON formats are available), an
# <tt>ActionController::UnknownFormat</tt> error will be raised.
#
# On the other hand, when there are no templates associated with the controller
# action at all, it tries to determine if the current request is "interactive"
# (i.e. came from a real browser). If so, it raises the <tt>ActionView::MissingTemplate</tt>
# error in order to display a helpful error message. Otherwise, it assumes the
# request is an API request and renders a "204 No Content" response.
#
# For API controllers, the implicit render is always "204 No Content" without
# performing any template lookups.
eow
elsif interactive_browser_request?
raise ActionView::MissingTemplate.new(<<-eow.strip_heredoc, action_name.to_s)
No template found for #{self.class.name}\##{action_name}.

This comment has been minimized.

@kaspth

kaspth Feb 23, 2016 Member

Couldn't figure out why we wouldn't want the existing failed lookup message, so I got a bit stumped here.

I could see the point in enriching the lookup message about why it failed.

elsif any_templates?(action_name.to_s, _prefixes)
raise ActionController::UnknownFormat, <<-eow.strip_heredoc
#{self.class.name}\##{action_name} did not have any templates for the
formats #{request.formats} or variant #{request.variant}.

This comment has been minimized.

@kaspth

kaspth Feb 23, 2016 Member

In writing the docs above, I liked the idea of writing a template means that makes them "known". I think we could play with that here. But didn't get around to that.

@@ -174,6 +179,32 @@ def detail_args_for(options)
[user_details, details_key]
end

def args_for_any(name, prefixes, partial)

This comment has been minimized.

@maclover7

maclover7 Feb 23, 2016 Member

Is this meant to be public API? 😬

This comment has been minimized.

@kaspth

kaspth Feb 24, 2016 Member

Let's nodoc it, yeah 👍

[name, prefixes, partial || false, details, details_key]
end

def detail_args_for_any

This comment has been minimized.

@maclover7

maclover7 Feb 23, 2016 Member

same as above

This comment has been minimized.

@kaspth

kaspth Feb 24, 2016 Member

Ditto.

@maclover7
Copy link
Member

maclover7 commented Feb 24, 2016

Taking the baton from @kaspth, and doing some additions to this.

r? @maclover7

@rails-bot rails-bot assigned maclover7 and unassigned kaspth Feb 24, 2016
@maclover7
Copy link
Member

maclover7 commented Feb 24, 2016

Pushed up a commit with some changes. Feel free to squash down -- only pushed up a separate commit so that way others can review.

Passing the baton back to @chancancode for the finale 😄

r? @chancancode

@rails-bot rails-bot assigned chancancode and unassigned maclover7 Feb 24, 2016
@maclover7 maclover7 force-pushed the new_implicit_render branch from 4e25739 Feb 24, 2016
@chancancode chancancode force-pushed the new_implicit_render branch to a30b7a9 Feb 24, 2016
@chancancode chancancode changed the title [WIP] Lock down new `ImplicitRender` behavior for 5.0 RC Lock down new `ImplicitRender` behavior for 5.0 RC Feb 24, 2016
"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 to due to other constraints, such as locales " \

This comment has been minimized.

@kaspth

kaspth Feb 24, 2016 Member

to due

"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 sumtting a form. " \

This comment has been minimized.

@kaspth

kaspth Feb 24, 2016 Member

sumtting

"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 appropiate response in the action using `render`, " \

This comment has been minimized.

@kaspth

kaspth Feb 24, 2016 Member

appropiate

This comment has been minimized.

@kaspth

kaspth Feb 24, 2016 Member

Two spaces before render

@chancancode chancancode force-pushed the new_implicit_render branch 2 times, most recently 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]
@chancancode chancancode force-pushed the new_implicit_render branch to 73b1efc Feb 25, 2016
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 " \

This comment has been minimized.

@kaspth

kaspth Feb 25, 2016 Member

repond

chancancode added a commit that referenced this pull request Feb 25, 2016
Lock down new `ImplicitRender` behavior for 5.0 RC
@chancancode chancancode merged commit 6b31761 into master Feb 25, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@chancancode chancancode deleted the new_implicit_render branch Feb 25, 2016
@chancancode
Copy link
Member Author

chancancode commented Feb 25, 2016

Merging to unblock the release! We have some ideas for improvements (shortening the error messages and move the details into the HTML page like routing errors, bring back the "searched in..." info, add an config to turn off the "interactive request warning", fine-tune the browser-detection, perhaps express defualt_render in terms of respond_to, etc), but the basic behavior is better than what we have on master. Feel free to open PRs to improve this further

@chancancode
Copy link
Member Author

chancancode commented Feb 25, 2016

Thanks everyone for the hard work!!!

@kaspth
Copy link
Member

kaspth commented Feb 25, 2016

❤️

@maclover7
Copy link
Member

maclover7 commented Feb 25, 2016

🎉

@claudiob
Copy link
Member

claudiob commented Feb 25, 2016

Should I try adding some new lines to the error message in the following page?

screen shot 2016-02-25 at 8 04 38 am

@claudiob
Copy link
Member

claudiob commented Feb 25, 2016

On a separate note… I think this message is very explicit regarding the edge cases, but it does not convey exactly what to do in the case that will occur 99% of the times.

The old message used to clearly state:

  1. you are missing a file
  2. the file should have one of these names and be in one of these folders

I think we can try to improve the current message and make it more accessible to newcomers, also given that this is part of the Rails guides:

template_is_missing_articles_new

The final part of this message tells us where Rails has looked for the templates. Templates within a basic Rails application like this are kept in a single location, but in more complex applications it could be many different paths.

The simplest template that would work in this case would be one located at app/views/articles/new.html.erb. The extension of this file name is important: the first extension is the format of the template, and the second extension is the handler that will be used. Rails is attempting to find a template called articles/new within app/views for the application. The format for this template can only be html and the handler must be one of erb, builder or coffee. Because you want to create a new HTML form, you will be using the ERB language which is designed to embed Ruby in HTML.

Therefore the file should be called articles/new.html.erb and needs to be located inside the app/views directory of the application.

@kaspth
Copy link
Member

kaspth commented Feb 25, 2016

@claudiob it's been rephrased because you aren't necessarily missing a file. If you expect a 204, then you don't need to do anything.

@claudiob
Copy link
Member

claudiob commented Feb 25, 2016

@kaspth I understand and I agree with this PR, don't get me wrong.

I just think that if you do miss a file (probably still the most common case), we are not telling you anymore where to put that file and which filenames are accepted.

@kaspth
Copy link
Member

kaspth commented Feb 25, 2016

Yeah, agreed. And it's one @chancancode's suggestions in #23827 (comment).

@chancancode
Copy link
Member Author

chancancode commented Feb 25, 2016

@claudiob please continue to improve it!

The case to keep in mind is that..

  1. def update; end is now a sanctioned/idiomatic way to write head :no_content
  2. If you are planning on relying on that behavior, everything might be fine until you decided to debug something in the browser – you might be thinking "this endpoint was working fine since always, was it the gem I just installed? should I restart the server??? what's happening????"

So I think somehow conveying the fact that "this might be okay, actually" is important. But I like @matthewd's suggestion of putting less in the exception message (and probably bring back the info we had before) and put more in the specialized HTML error page.

By the way, since the browser detection might not be perfect (an API explore extension in chrome or a BrokenKit.framework on iOS might send some browser-like headers), it is probably good to have a way to turn it off and mention that on the error page too. ("If you are expected the 204, everything is cool, don't worry. You can also turn this off via... if you like.")

@clarkware
Copy link

clarkware commented Feb 25, 2016

Thanks to @chancancode for all the work on this!

FWIW, I strongly agree with @claudiob that the message would be improved by addressing the most common case of missing the file. The current message puts a lot of emphasis on the least common case. In fact, given the interactive_browser_request? conditional, I'm not clear on why you'd ever get this message when building an API endpoint as the message suggests. Perhaps I'm missing some nuances.

@jeremy
Copy link
Member

jeremy commented Mar 3, 2016

Noticing that this generally applies to GET requests from the browser.

We have some internal API controllers that take multipart/form-data params or raw request bodies and don't return a response body. To use implicit responses, we'd need to force a fake request.format since it'd fall back to the HTML default, otherwise, and we'd get a missing template error.

Think we can get the best of both worlds by pruning the "is a developer viewing this in the browser and expecting a response?" check down to non-XHR HTML GETs. Non-XHR PUT/POST/DELETE are typically redirects to an HTML GET.

@matthewd
Copy link
Member

matthewd commented Mar 3, 2016

@jeremy I worry that'll mean "I forgot to do the redirect" becomes "my browser is pretending I'm not pressing the button" 😕

What Accept is being sent in that situation? No header whatsoever? It definitely seems safe to narrow "browser" down to an explicit request for at least one of text/html or */*.

If we really wanted to get creative, presence of cookies might be worth considering.

@jeremy
Copy link
Member

jeremy commented Mar 3, 2016

Agree. I wager that forgetting a redirect is fairly uncommon and, hopefully, easier to diagnose. The form will submit and you'll get a white page of death in response.

Checking for user-agent details gets tricky since more sophisticated API clients may appear browser-like in all those respects. Then a request could spookily seem to work with some clients but not others.

jeremy added a commit to jeremy/rails that referenced this pull request Mar 3, 2016
…urpose?" heuristics

Narrows the "are you in a browser, viewing the page?" check to exclude
non-GET requests. Allows content-less APIs to use implicit responses
without having to set a fake request format.

This will need further attention. If you forget to redirect from a POST
to a GET, you'll get a 204 No Content response that browsers will
typically treat as… do nothing. It'll seem like the form just didn't
work and knowing where to start debugging is non-obvious.

On the flip side, redirecting from POST and others is the default, done
everywhere, so it's less likely to be removed or otherwise missed.

Alternatives are to do more explicit browser sniffing.

Ref rails#23827.
y-yagi added a commit to y-yagi/rambulance that referenced this pull request Jun 9, 2016
In Rails 5 has changed the behavior of the render, to modify the expected value
in accordance with new behavior.
Ref: rails/rails#23827
y-yagi added a commit to y-yagi/rambulance that referenced this pull request Jun 9, 2016
In Rails 5 has changed the behavior of the render, to modify the expected value
in accordance with new behavior.
Ref: rails/rails#23827
y-yagi added a commit to y-yagi/rambulance that referenced this pull request Jun 9, 2016
In Rails 5 has changed the behavior of the render, to modify the expected value
in accordance with new behavior.
Ref: rails/rails#23827
y-yagi added a commit to y-yagi/rambulance that referenced this pull request Jun 9, 2016
In Rails 5 has changed the behavior of the render, to modify the expected value
in accordance with new behavior.
Ref: rails/rails#23827
@vlymar vlymar mentioned this pull request Sep 13, 2016
3 of 3 tasks complete
@bf4 bf4 mentioned this pull request Oct 24, 2017
4 of 4 tasks complete
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.

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