Fix NoMethodError when ActionController::Metal#response is an Array #420
Conversation
delner
changed the title
| when ActionDispatch::Response | ||
| response.status | ||
| when Array | ||
| status = response.first |
There was a problem hiding this comment.
the first element always contain the status? If yes, can you add a comment explaining why the first and if we have to change this behavior in the future?
There was a problem hiding this comment.
Yes, it's always the first because that's the Rack response standard: status code, headers, body.
delner
force-pushed
the
bugfix/devise_failure_app_mishandled_response
branch
from
8273d54
to
f579dfb
Compare
|
Pull request for Devise opened here: heartcombo/devise#4871 |
| @@ -231,6 +232,20 @@ def process_action(*args) | |||
| ensure | |||
| Datadog::Contrib::Rails::ActionController.finish_processing(payload) | |||
| end | |||
|
|
|||
| # rubocop:disable Style/EmptyElse | |||
There was a problem hiding this comment.
What are the benefits of using empty else here ?
Also is # rubocop:disable Style/EmptyElse reenabled after the scope of this method ?
There was a problem hiding this comment.
Okay, I thought that a case block would return the object passed into the case statement if none of the branches are hit. Since I wanted it to return nil, I thought I needed to be explicit. Turns out its just another one of those weird language things where it does in fact return nil if no branches are met. Probably should remove it then.
delner
force-pushed
the
bugfix/devise_failure_app_mishandled_response
branch
from
f579dfb
to
f9a9635
Compare
|
@pawelchcki Feedback addressed. |
|
Thanks @delner, looks good. |
When
ActionController::Metal#responsewas anArrayinstead of aActionDispatch::Response, tracing raised an error. This manifested in Devise'sFailureAppwhich is overriding the value ofresponsewith an Array, in an unexpected way. You can reproduce this by attempting to login with bad credentials.Only affects users who use Devise and ddtrace 0.12.0.
This pull request attempts to handle both
ActionDispatch::ResponseandArrayresponses, and if it doesn't receive either, it doesn't set a status.Addresses #419