fix: malformed charset param #2263
Conversation
|
I'm actually thinking it might be better to omit it from the hash because actually we don't know whether it's a key, value, neither, or both... |
jeremyevans
left a comment
There was a problem hiding this comment.
Having it be an empty string is consistent with what we do for parameters themselves with nothing after the = ( e.g. ?key=), so that seems like the best way.
Could be useful to update CHANGELOG in this PR, but that can always be done later.
|
Could you please add documentation somewhere explaining the behaviour? |
a66d946 to
a4b3d29
Compare
|
Done 🙂 |
|
Thanks. Just to be clear, this isn't the same as query parsing: irb(main):002> Rack::Utils.parse_query("charset")
=> {"charset"=>nil}
irb(main):003> Rack::Utils.parse_query("charset=")
=> {"charset"=>""}I think @jeremyevans prefers to use empty string, but this isn't consistent with the current implementation (which uses WDYT? |
|
Huh. Okay. It might be slightly cheaper to cleave off the Does anyone in reality make the distinction between "present, but unspecified" and "present, but empty" for this kind of parameter? I...don't think the spec cares about it. |
|
I personally think it's good to be consistent with the query parser, but that's just me. @jeremyevans may have a different opinion. |
|
The description in #2250 is Note that using an empty string for both cases (with or without |
|
Yeah, apologies — the original issue's repro was wrong (I'll update that now), which is why it sat there for so long 😅 |
If a content type header is provided with a malformed part, there's no nice way of interpreting it since instead of being a key/value pair it's just a key. Instead, we mimic the behaviour of query parameters and opt to return an empty string in lieu of missing values—this way we can still identify their presence, but won't blow up. The behaviour isn't identical, but we're also not worried about backwards compatibility here (given the alternative was exploding) issue: rack#2250
a4b3d29 to
b682120
Compare
|
Alrighty, I've opted to go with empty strings for both, and I've updated the doc. |
|
I think it is hard to change the default behaviour of the query parser due to compatibility. It may be possible by using a feature flag and gracefully switching. Using a feature flag makes downstream testing harder (every library needs to test both ways = combinatoric explosion of testing configurations). |
|
Thanks for your contribution. |
|
@ioquatix can we get a release for this fix please? We're running into this on production 😄 Much appreciated and thanks for the hard work on this awesome gem! |
If a content type header is provided with a malformed part, there's no nice way of interpreting it since instead of being a key/value pair it's just a key. Instead, we mimic the behaviour of query parameters and opt to return an empty string in lieu of missing values—this way we can still identify their presence, but won't blow up. The behaviour isn't identical, but we're also not worried about backwards compatibility here (given the alternative was exploding)
If a content type header is provided with a malformed part, there's no nice way of interpreting it since instead of being a key/value pair it's just a key. Instead, we mimic the behaviour of query parameters and opt to return an empty string in lieu of missing values—this way we can still identify their presence, but won't blow up. The behaviour isn't identical, but we're also not worried about backwards compatibility here (given the alternative was exploding)
If a content type header is provided with a malformed part, there's no nice way of interpreting it since instead of being a key/value pair it's just a key. Instead, we mimic the behaviour of query parameters and opt to return an empty string in lieu of missing values—this way we can still identify their presence, but won't blow up. The behaviour isn't identical, but we're also not worried about backwards compatibility here (given the alternative was exploding) Co-authored-by: Ally <28497049+AllyMarthaJ@users.noreply.github.com>
|
@ioquatix thanks for releasing, much appreciated! 🙇 |
If a content type header is provided with a malformed part, there's no nice way of interpreting it since instead of being a key/value pair it's just a key.
I think the sensible option here is to assume, like in the case where a param is provided with an empty value, that the value is an empty string. I'm not super tied to this, though (for example, it could just as easily be omitted from the params entirely, or the value could simply be nil; each option are equally as valid).
issue: #2250