We don't set any Content-Type in the client. It is more consistent with our other JSON-RPC use to set it to application/json.
Note that our server doesn't enforce content types, so it doesn't make a difference in practice. But it is fairly strange HTTP behavior to not set it at all for a POST request.
This came up in #18950.
We don't set any `Content-Type` in the client. It is more
consistent with our other JSON-RPC use to set it to `application/json`.
Note that our server doesn't enforce content types, so it doesn't make a
difference in practice. But it is fairly strange HTTP behavior to not set it.
This came up in #18950.
Note that our server doesn't enforce content types
Should?
Concept ACK
Should?
As it would be an interface change that affects any applications that mimicked previous -cli behavior, or followed the (strange) advice to set text/plain content type, I don't really think that's a good idea. It seems unnecessary.
ACK 7eab781a145a35d0373c4ab4d237a82b4919e88d: patch looks correct
This makes sense.
Tested ACK 7eab781a145a35d0373c4ab4d237a82b4919e88d
(Should the RPC helps content type be updated? In practice it doesn't seem to make much difference except what is seen in the server header: text/plain, application/json, or when no content type is specified, application/x-www-form-urlencoded)
Should the RPC helps content type be updated? I
Are we mentioning that anywhere?
ISTM all the help examples ATM say -H 'content-type: text/plain;' for the curl.
Isn't these just extra bytes transferred without any benefit? It's not that somebody will use bitcoin-cli against anything else than Bitcoin Core, so there isn't any need to care about "fairly strange HTTP behaviors" IMO.
Isn't these just extra bytes transferred without any benefit?
I think it makes sense to pass Content-Type consistently in all our code.
From the spec FWIW: "Any HTTP/1.1 message containing an entity-body SHOULD include a Content-Type header field defining the media type of that body. If and only if the media type is not given by a Content-Type field, the recipient MAY attempt to guess the media type via inspection of its content and/or the name extension(s) of the URI used to identify the resource. If the media type remains unknown, the recipient SHOULD treat it as type "application/octet-stream"."
It's not that somebody will use
bitcoin-cliagainst anything else than Bitcoin Core, so there isn't any need to care about "fairly strange HTTP behaviors" IMO.
I can think of testing scenarios where one would like to run bitcoin-cli against something other than Bitcoin Core (such as against a fuzzing HTTP server) :)
Isn't these just extra bytes transferred without any benefit? It's not that somebody will use bitcoin-cli against anything else than Bitcoin Core, so there isn't any need to care about "fairly strange HTTP behaviors" IMO.
Well if you're worried about a few extra bytes being transferred locally have it your way I guess, closign …
ISTM all the help examples ATM say -H 'content-type: text/plain;' for the curl.
That's a kind of bad suggestion, thanks for telling me where it comes from though. Not sure it's worth changing, it's okay, it doesn't really affect anything.
@laanwj I think you closed by accident, they should put more space between those buttons. @kristapsk this is clearly the right thing to do as a JSON-RPC over HTTP client.
Yeah, I also don't understand why this was closed.
Concept ACK
@laanwj I think you closed by accident, they should put more space between those buttons.
Because I don't really feel like having an argument about a one-line change that I thought was fairly obvious. I noticed this was off, and we can make this change or not, but if people think it's controversial I'm sure there's more important things.
Please consider re-opening.
Review comments should only be considered to they extent they have technical merit.
A single semi-negative comment does not necessarily make a change "controversial" :)
ACK
ACK 7eab781a145a35d0373c4ab4d237a82b4919e88d - Looks fine to me.
Post-merge utACK