search

lostisland / faraday_middleware

Various Faraday middlewares for Faraday-based API wrappers
MIT License
557 stars 203 forks source link

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

Closed WvanLelyveld closed 1 month ago

WvanLelyveld commented 1 month ago

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 https://github.com/lostisland/faraday_middleware/issues/288#issuecomment-2374011124_

iMacTia commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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