CP-1966 Handle request with no encoding and no charset #161
Conversation
| {Encoding fallbackEncoding}) { | ||
| {Encoding fallbackEncoding: UTF8}) { | ||
| if (fallbackEncoding == null) { | ||
| throw new ArgumentError.notNull('fallbackEncoding'); |
There was a problem hiding this comment.
Would it be okay to set it to UTF8 if fallbackEncoding is null here instead of throwing an error?
There was a problem hiding this comment.
Yeah that's better.
|
Sorry, hold off on this briefly, just thought of one improvement. |
evanweible-wf
force-pushed
the
default-request-encoding
branch
2 times, most recently
from
e3e58bf to
ef80a24
Compare
evanweible-wf
changed the title
|
@trentgrover-wf Ready to review now. |
Current coverage is 99.45%
|
|
+1 It looks like there's one branch that isn't covered... specifying the optional |
evanweible-wf
force-pushed
the
default-request-encoding
branch
from
ef80a24 to
d05d754
Compare
|
@maxwellpeterson-wf good catch, my new HttpBody tests were supposed to test both constructors but I copy pasted and didn't change |
|
+1 |
1 similar comment
|
+1 |
| {Encoding fallbackEncoding}) { | ||
| _encoding = http_utils.parseEncodingFromContentType(contentType, | ||
| fallback: fallbackEncoding); | ||
| {Encoding encoding, Encoding fallbackEncoding: UTF8}) { |
There was a problem hiding this comment.
there's some inconsistency in this fallbackEncoding code. why does this method signature have a default value but the other does not? if there's a default value, no need to the null check to set UTF8
There was a problem hiding this comment.
Good catch, I had it this way at first but switched to Max's suggestion of just populating fallbackEncoding to UTF8 if it's null instead of throwing an ArgumentError. I'll fix this
evanweible-wf
force-pushed
the
default-request-encoding
branch
from
d05d754 to
1d8a58a
Compare
- Throw ArgumentError if a request's encoding is set to null - Null check for encoding before reading name in ResponseFormatException - HttpBody constructors now accept encoding param and will use if given - HttpBody constructors only parse content-type for charset if encoding not given - HttpBody constructors now default fallbackEncoding to UTF8 - Only catch ArgumentException when attempting to encode a body - Only catch FormatException when attempting to decode body bytes
|
@trentgrover-wf @maxwellpeterson-wf @sebastianmalysa-wf Addressed Trent's comment. |
|
+1 |
|
+1 |
|
QA Resource Approval: +10
Merging into master. |
Fixes #159.
Issue
#159 reported an issue when setting a request's content-type manually. If that content-type has no
charsetparameter, and the request's encoding was not set, you will hit an RTE inHttpBodywhen trying to read the body and a conversion is required (bytes->string or vice versa). This is because theHttpBodyconstructors have an optionalfallbackEncodingparameter that has no default value. For responses, we pass in a fallback encoding ofLATIN1per RFC 2616, but we aren't passing in a fallback encoding at all for request bodies.This issue hadn't manifested itself before because most requests set the content-type automatically, including the charset, but #153 introduced the ability to manually set the content-type.
Solution
HttpBody.fromString()andHttpBody.fromBytes()to give the optionalfallbackEncodingparameter a default value ofUTF8.ArgumentErroriffallbackEncodinghappens to be nullResponseFormatException.ResponseFormatException, which is what happened in the stack trace from Handle request with no encoding and no charset #159.Testing
Code Review
@trentgrover-wf
@maxwellpeterson-wf
@dustinlessard-wf
@jayudey-wf
@sebastianmalysa-wf
@srinivasdhanwada-wf
fyi: @stevenosborne-wf