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

v1.2.1 does not parse JSON if there are no response headers #293

Closed
WvanLelyveld opened this issue Sep 25, 2024 · 3 comments
Closed

v1.2.1 does not parse JSON if there are no response headers #293

WvanLelyveld opened this issue Sep 25, 2024 · 3 comments

Comments

@WvanLelyveld
Copy link

In v1.2.0, JSON would still be parsed if the response would not have response headers.
v1.2.1 only parses when the response headers say it's a JSON response.

See the comment here.

I've made a test repo with two branches.
You can see the only difference is the version of faraday_middleware, v1.2.0 and v.1.2.1
In the main branch, v1.2.1, rspec fails, where in the working-with-1.2.0 branch, the rspec test passes.

This should illustrate that this version breaks things in certain case.
I'm suspecting it has something to do with the absence of the response_headers in the mocked response.
The original FaradayMiddleware::ParseJson class used in faraday_middleware v1.2.0 parses this response, even in the absence of headers, where the Faraday::Response::Json class used in v1.2.1 does not parse when there are no headers present.

Originally posted by @WvanLelyveld in #288 (comment)

@iMacTia
Copy link
Member

iMacTia commented Sep 25, 2024

Hi @WvanLelyveld I suspect this is because v1.2.1 does not override the built-in JSON middleware in Faraday if it's present.
The built-in middleware might have a slight different implementation but it should be generally better.

You're indeed correct the built-in middleware does check the Content-Type header (you can see the code here), so if you're dealing with a server that does not set this header properly, you should be able to pass an empty array to the content_type option of the middleware (see docs).

This should illustrate that this version breaks things in certain case

I appreciate this is technically breaking the behaviour on a patch release of the gem, however the decision to not override the built-in middleware should be seen as an actual bug fix, which if not addressed would cause other, more annoying issues

I'm open to alternative suggestions, but I'd also like us all to keep in mind that the RFC clearly states that a JSON response should have the Content-Type header set accordingly.

@WvanLelyveld
Copy link
Author

Ok, it was just breaking our tests, so I had to investigate what was going on.
It's fine if you don't want to fix it, and see this as a bug fix itself.

I can simply add the headers to our tests, but since it broke quite a few of our tests, I'm sure it will happen to other people as well, and it's good to have this ticket for reference for anyone who runs into the same issue.

@iMacTia
Copy link
Member

iMacTia commented Sep 25, 2024

Absolutely! Thank you for opening this ticket, appreciate the communication and I'm sure others will find this useful in future ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants