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

Ensure the request body is not emptied in Faraday v1.0.0 #43

Merged
merged 2 commits into from Feb 7, 2020

Conversation

Domon
Copy link
Contributor

@Domon Domon commented Feb 6, 2020

Context

JWT Signed Request's Faraday middleware does not work as expected when the request has a body (e.g. a POST request) and Faraway version is v1.0.0.

It empties the request body no matter the request body has been set or not.

Change

Replace the problematic #fetch method with #[] and #[]= to ensure JWT Signed Request works as expected in Faraday v0.17.3 or v1.0.0.

Considerations

The Faraday::Env#fetch method (inherited from Faraday#Options) works in a way different from the Hash#fetch method. It not only returns the value when the key exists but also sets the value of the key (❗️) if a second argument is provided.

https://github.com/lostisland/faraday/blob/099dd45f63ff99bbb343eebf7504a3cf0b10bc63/lib/faraday/options.rb#L76-L88

JWT Signed Request uses this method to obtain the request body or set it to an empty string if it was nil.

In Faraday v1.0.0, the :body option has been replaced with the new :request_body and :response_body options. Smarter getter (#[]) and setter (#[]=) methods are provided. When the argument is :body, they access :request_body or :response_body depending on if :status has been set.

lostisland/faraday#847

However, the #fetch method remains the same.

A request with a body will have the :request_body option set but not the :body option. That results in JWT Signed Request think the request body was not provided and sets :request_body to an empty string.

Instead of the #fetch method, I propose we use the #[] and #[]= method instead.

Copy link
Member

@orien orien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice investigation @Domon!

LGTM

Copy link
Contributor

@delfinag delfinag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed #40 addresses this too. I'll close that one off favour of this PR which includes tests.

@@ -16,7 +16,7 @@ def call(env)
method: env[:method],
path: env[:url].request_uri,
headers: env[:request_headers],
body: env.fetch(:body, ::JWTSignedRequest::EMPTY_BODY),
body: env[:body] || (env[:body] = ::JWTSignedRequest::EMPTY_BODY),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be less surprising (more readable) to set this beforehand? eg:

env[:body] ||= ::JWTSignedRequest::EMPTY_BODY
@jwt_token = ::JWTSignedRequest.sign(

So this would become simply:

Suggested change
body: env[:body] || (env[:body] = ::JWTSignedRequest::EMPTY_BODY),
body: env[:body],

Great catch btw!

Copy link
Contributor

@zubin zubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM! 👍

@Domon
Copy link
Contributor Author

Domon commented Feb 7, 2020

Thanks everyone for reviewing 🙏

@Domon Domon merged commit 0b40aec into master Feb 7, 2020
@Domon Domon deleted the fix-incompability-with-faraday-1-0-0 branch February 7, 2020 01:04
@Domon Domon mentioned this pull request Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants