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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raises exception when respond_to called multiple times in incompatible way #33446

Merged
merged 1 commit into from Jul 31, 2018

Conversation

Projects
None yet
4 participants
@ptoomey3
Copy link
Contributor

ptoomey3 commented Jul 26, 2018

Original PR was #27376, but it became stale.

Nesting respond_to calls can lead to unexpected behavior, so it should be
avoided. Currently, the first respond_to format match sets the content-type
for the resulting response. But, if a nested respond_to occurs, it is possible
to match on a different format. For example:

    respond_to do |outer_type|
      outer_type.js do
        respond_to do |inner_type|
          inner_type.html { render body: "HTML" }
        end
      end
    end

Browsers will often include / in their Accept headers. In the above example,
such a request would result in the outer_type.js match setting the content-
type of the response to text/javascript, while the inner_type.html match will
cause the actual response to return "HTML".

This change tries to minimize potential breakage by only raising an exception
if the nested respond_to calls are in conflict with each other. So, something
like the following example would not raise an exception:

    respond_to do |outer_type|
      outer_type.js do
        respond_to do |inner_type|
          inner_type.js { render body: "JS" }
        end
      end
    end

While the above is nested, it doesn't affect the content-type of the response. If people think it simpler/more obvious to just raise anytime a double respond_to occurs (ala a double render) that seems fine too. I'm conjecturing this could be a breaking change for some people, which is why I wanted to minimize breakage by only raising when we are in actual conflict.

/cc @tenderlove - Since we had chatted about "expected behavior" of nested respond_to calls and the general feeling was "raise YugeError" 馃槃.

/cc @gmcgibbon - Since you had provided feedback in the original PR.

@gmcgibbon
Copy link
Member

gmcgibbon left a comment

馃憤 Please add an entry to actionpack's changelog that describes this change.

actionpack/lib/abstract_controller/base.rb Outdated
# end
# end
# end
class RespondToMismatchError < StandardError

This comment has been minimized.

@gmcgibbon

gmcgibbon Jul 30, 2018

Member

Should this error class be moved to a different file? Maybe moving it to actionpack/lib/action_controller/metal/mime_responds.rb would better separate concerns.

This comment has been minimized.

@ptoomey3

ptoomey3 Jul 30, 2018

Author Contributor

I ended up searching for where other similar exceptions were defined (ex. ActionController::UnknownFormat) and stumbled upon actionpack/lib/action_controller/metal/exceptions.rb. So, I placed the exception there.

@@ -1,3 +1,18 @@
*. Raises `ActionController::RespondToMismatchError` with confliciting `respond_to` invocations.

`respond_to` can match multiple types and lead to undefined behavior when

This comment has been minimized.

@ptoomey3

ptoomey3 Jul 30, 2018

Author Contributor

Let me know if I'm not using language that isn't idiomatic with respond_to (or any other changes you would like).

@gmcgibbon
Copy link
Member

gmcgibbon left a comment

馃挴 One last thing, please squash your commits and then this should be good to go. Thanks for taking the time to fix this!

@schneems

This comment has been minimized.

Copy link
Member

schneems commented Jul 30, 2018

CI is failing

@ptoomey3

This comment has been minimized.

Copy link
Contributor Author

ptoomey3 commented Jul 30, 2018

CI is failing

I鈥檒l take a look and see what I must have mistyped when I moved/renames the exception.

Raises exception when respond_to called multiple times in incompatibl鈥
鈥 way

Nesting respond_to calls can lead to unexpected behavior, so it should be
avoided. Currently, the first respond_to format match sets the content-type
for the resulting response. But, if a nested respond_to occurs, it is possible
to match on a different format. For example:

    respond_to do |outer_type|
      outer_type.js do
        respond_to do |inner_type|
          inner_type.html { render body: "HTML" }
        end
      end
    end

Browsers will often include */* in their Accept headers. In the above example,
such a request would result in the outer_type.js match setting the content-
type of the response to text/javascript, while the inner_type.html match will
cause the actual response to return "HTML".

This change tries to minimize potential breakage by only raising an exception
if the nested respond_to calls are in conflict with each other. So, something
like the following example would not raise an exception:

    respond_to do |outer_type|
      outer_type.js do
        respond_to do |inner_type|
          inner_type.js { render body: "JS" }
        end
      end
    end

While the above is nested, it doesn't affect the content-type of the response.

@ptoomey3 ptoomey3 force-pushed the ptoomey3:nested-respond-to branch to 84e8b35 Jul 30, 2018

@ptoomey3

This comment has been minimized.

Copy link
Contributor Author

ptoomey3 commented Jul 30, 2018

I鈥檒l take a look and see what I must have mistyped when I moved/renames the exception.

A closer examination makes it look like the failures are not related to my changes. I'll squash the commits, push, and then see if CI passes.

@ptoomey3

This comment has been minimized.

Copy link
Contributor Author

ptoomey3 commented Jul 30, 2018

Yay..looks like a re-run of CI was all the was needed.

@schneems schneems merged commit 5deab3d into rails:master Jul 31, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.