Fix encoding of plain text bodies #9
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
simonjbeaumont
requested changes
May 26, 2023
simonjbeaumont
approved these changes
May 26, 2023
weissi
reviewed
May 27, 2023
| let decoded: T | ||
| if let myType = T.self as? _StringParameterConvertible.Type { | ||
| guard | ||
| let stringValue = String(data: data, encoding: .utf8), |
There was a problem hiding this comment.
Please avoid String(data:encoding:), especially in networked software as it has 2 failure modes (return nil or insert replacement characters for invalid UTF8 -- which one it chooses depends on what's broken).
The correct and faster way is String(decoding: data, as: UTF8.self) which also doesn't require Foundation and is not fallible
There was a problem hiding this comment.
Thanks, broke out into apple/swift-openapi-generator#42
weissi
reviewed
May 27, 2023
| @@ -128,6 +139,12 @@ extension Converter { | |||
| ) throws -> Data { | |||
| let body = transform(value) | |||
| headerFields.add(name: "content-type", value: body.contentType) | |||
| if let value = value as? _StringParameterConvertible { | |||
| guard let data = value.description.data(using: .utf8) else { | |||
There was a problem hiding this comment.
Same here, the correct and fast way is Data(value.utf8) which can't fail and is faster. You should audit your code to make sure String(data and data(using aren't present
There was a problem hiding this comment.
This was referenced May 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
When the content type
text/plainis used, we had a bug where we were encoding the text usingCodableinto a JSON fragment, i.e.hello->"hello"(note the added quotes), which is incorrect. We should be usingLosslessStringConvertiblefor primitive types, so thathellois sent ashello(without the quotes).Modifications
Fixed the body conversion methods to use the string conversion if possible, and only use the
Codableimplementation for complex types.Result
When content type is
text/plain(and other plain text variants),hellois sent ashello, not"hello".Test Plan
Updated unit tests and added new ones to cover these cases.