lostisland / faraday

Simple, but flexible HTTP client library, with support for multiple backends.
https://lostisland.github.io/faraday
MIT License
5.71k stars 972 forks source link

`Faraday::Request::Json` doesn't encode the value `false` while it encodes `true` to `"true"` #1503

Closed yykamei closed 1 year ago

yykamei commented 1 year ago

Basic Info

Issue description

Faraday::Request::Json doesn't encode the value false while it encodes true to "true". I think false is a valid JSON value, so the Faraday middleware should encode false to "false".

The reason why false is not encoded is the code below returns false after evaluating (body = env[:body]), which is false because env[:body] is false.

https://github.com/lostisland/faraday/blob/111d354b7f232f6bc3292e4cde6d08d2b983e9ae/lib/faraday/request/json.rb#L43

Steps to reproduce

  1. Clone this repository
  2. Run ./bin/setup
  3. Run ./bin/console
  4. Type the following code
body = false
result = Faraday::Request::Json.new(->(e) { Faraday::Response.new(e) })
  .call(Faraday::Env.from({ body: body, request_headers: Faraday::Utils::Headers.new }))
result.env[:body] # => false, not "false"

Then, IRB gives you false, not "false".

By the way, you can get "true" with this code:

body = true
result = Faraday::Request::Json.new(->(e) { Faraday::Response.new(e) })
  .call(Faraday::Env.from({ body: body, request_headers: Faraday::Utils::Headers.new }))
result.env[:body] # => "true", not true
iMacTia commented 1 year ago

Great catch @yykamei, I think you spot a valid bug. I had a quick look and yes, according to the JSON specification true or false are valid JSON payloads like any other scalar value (like a single string or number).

So false should be encoded into "false" as you'd expect. There's also the extreme edge-case scenario where you might want to send a plain null value, which is interestingly also a valid JSON! But we can leave that for another day 😄

Would you have some time to open a PR to fix the false edge case and add some tests?

yykamei commented 1 year ago

Thank you for your reply! I'll do that 💪

There's also the extreme edge-case scenario where you might want to send a plain null value, which is interestingly also a valid JSON! But we can leave that for another day 😄

Yes, I agree with you. there could be null body. I guess there could be programs that make the payload by putting nil to env[:body], so I will keep it as is.