ruby
Do not supply a default content type.
Hello.
I would like to know if you would welcome a change to remove suppy_default_content_type.
Checking RFC for Content-Type: https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.5
A sender that generates a message containing a payload body SHOULD
generate a Content-Type header field in that message unless the
intended media type of the enclosed representation is unknown to the
sender. If a Content-Type header field is not present, the recipient
MAY either assume a media type of "application/octet-stream"
([RFC2046], Section 4.5.1) or examine the data to determine its type.
Based on the RFC, I think Net::HTTP behavior is incorrect - assuming a default content type is not correct because the media type is not known unless Net::HTTP reads/inspects the body. The receiver may assume octet-stream unless provided.
This has caused issues with AWS services, where content type may be a modeled API parameter in REST services, like with S3 where you can specify the content type of an object. Currently we work around this with a patch:
Thread.current[:net_http_skip_default_content_type] = true
def self.apply!
Net::HTTPGenericRequest.prepend(PatchDefaultContentType)
end
module PatchDefaultContentType
def supply_default_content_type
return if Thread.current[:net_http_skip_default_content_type]
super
end
end
We try to remove default content type for next release. If we faced large issue with this change, we may revert this or apply another approach.
Is there a place you are collecting reports of this causing problems for people? If so, what information would be useful to report?
I think this is an improvement, but many users are impacted where they depended on this behavior. Our organization has seen problems from this change.
Is there a place you are collecting reports of this causing problems for people? If so, what information would be useful to report?
I think this is an improvement, but many users are impacted where they depended on this behavior. Our organization has seen problems from this change.
This was a significant breaking change for us, and a VERY subtle bug to track down. I realise that the repository doesn't use semantic versioning, but it's quite a breaking change. I would have preferred adding warnings that this feature was going to change before it was made
You don’t know we already warn that https://github.com/ruby/net-http/pull/207/files#diff-1c8f56dc07513bad783e1dcf9353debd14fd86fbd79ad12fcc12ffaef9426573L378
You don’t know we already warn that https://github.com/ruby/net-http/pull/207/files#diff-1c8f56dc07513bad783e1dcf9353debd14fd86fbd79ad12fcc12ffaef9426573L378
Is the $VERBOSE flag the correct way to warn about a breaking change? If I start my Rails application with RUBYOPT=--verbose rails console I get the warning, and hundreds more on startup. It doesn't seem feasible to check for breaking changes on every update with --verbose.
Perhaps such warnings should be emitted by default, perhaps silencing itself after the first warning? I know an AWS gem is doing this currently to warn of a deprecated method. Or is my methodology wrong?
EDIT: After reading further on Ruby's warning philosophy, perhaps this should have been emitted when Warning[:deprecated] == true?
This is a very huge breaking change. There are breaking changes that are easy to catch in tests, but not this. This breaking change breaks the integration between lots of platforms.
What’s the reason this library is still in v0? For such an important part of the ruby ecosystem, making this breaking change in a minor version seems quite problematic.
What’s the reason this library is still in v0? For such an important part of the ruby ecosystem, making this breaking change in a minor version seems quite problematic.
In fairness, SemVer states that
Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.
I haven't read through SemVer in a while, and interestingly it defines major versions as API-incompatible changes rather than behavior-incompatible changes. Make of that what you will.
Major version X (X.y.z | X > 0) MUST be incremented if any backward incompatible changes are introduced to the public API.
Ruby stdlib gem versions are all over the place https://stdgems.org/ (outdated, but accurate-enough for an overview). I don't think there's any policy or adherence to SemVer in stdlib as a whole.
To expand on earlier comments, as this is an HTTP library it will tend to sit at the boundary between applications/services. Many of the places affected by this change are likely to be missed by test suites through a mixture of stubbing external calls and/or only testing key elements (destination, payload) of requests. I had to pin the version to 0.6.0 because it's very hard to be confident that our none of our code [+ the gems we use + the external service endpoints we use] relies on the old behaviour.
I can also say with certainty that I've never seen any of the warnings that were supposed to warn of this change, and I can't personally recall any other ruby or gem deprecation messages requiring running in $VERBOSE mode either. Downstream example of this change causing issues: https://github.com/jnunemaker/httparty/issues/826
The change is out in the wild now so it's hard to undo, but releasing a new version that restores previous behaviour alongside improved warnings seems like it would be a good idea. Whether such a version should be 0.6.1 or 0.9.0 can be debated.
Reflecting on the capitalised SHOULD in the quoted section of the RFC, I'd expect in many (most?) cases the developer using this gem does know what the content type will be, so it might be more developer friendly to:
- Warn whenever "Content-Type" is not specified ("Warning: no "Content-Type" header specified. Recipient may handle content as an octet stream.")
- Provide a way to explicitly specify that no Content-Type header is wanted, which then mutes the warning.
