[Runtime] Choose the serialization method based on content type #12
Conversation
This file contains hidden or 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
czechboy0
commented
May 31, 2023
czechboy0
commented
May 31, 2023
|
@simonjbeaumont Okay, based on our discussion, I split the |
simonjbeaumont
requested changes
Jun 3, 2023
czechboy0
added a commit
that referenced
this pull request
Jun 6, 2023
Rename _StringParameterConvertible to _StringConvertible ### Motivation As part of reworking #12, we'll be using the protocol `_StringParameterConvertible` not just for parameters, but also for bodies, so renaming it accordingly to reflect that. ### Modifications Renamed `_StringParameterConvertible` to `_StringConvertible`. ### Result The naming is more accurate now. Compatibility-wise, this isn't affecting the generator, as this protocol is not used directly by generated code, only indirectly through `_AutoLosslessStringConvertible`, so we don't even need to stage this change. ### Test Plan All tests pass. Reviewed by: simonjbeaumont Builds: ✔︎ pull request validation (5.8) - Build finished. ✔︎ pull request validation (5.9) - Build finished. ✔︎ pull request validation (nightly) - Build finished. ✔︎ pull request validation (soundness) - Build finished. #13
|
@simonjbeaumont This is now ready. It's exactly what we discussed and already codified requirements for in apple/swift-openapi-generator#50 Now, this is a large PR and is by no means perfect, but it's a solid move in the right direction. There are things I'd like to improve in the future, but I wouldn't want to hold this PR open for it. It's very repetitive, as we're doing ~50 variants of similar logic, but I took special care, and all the methods are unit tested and used in our integration test in the generator repo, so I'm confident that this logic is at least as good as what we have in 0.1.0. |
simonjbeaumont
approved these changes
Jun 8, 2023
czechboy0
added a commit
to apple/swift-openapi-generator
that referenced
this pull request
Jun 8, 2023
[Generator] Choose the serialization method based on content type ### Motivation Fixes #43. Depends on apple/swift-openapi-runtime#12 landing first and tagging a release. ### Modifications Builds on top of the changes to the runtime library. ### Result We now always specify a coding strategy with a Swift type, leading to deterministic and understandable conversion logic. ### Test Plan Updated unit and integration tests, added a `500` case to one of the operations to explicitly test the missing plain text response body. Reviewed by: simonjbeaumont Builds: ✔︎ pull request validation (5.8) - Build finished. ✔︎ pull request validation (5.9) - Build finished. ✔︎ pull request validation (nightly) - Build finished. ✔︎ pull request validation (soundness) - Build finished. #48
czechboy0
added a commit
that referenced
this pull request
Jul 31, 2023
[Runtime] Design away EncodableBodyContent ### Motivation Until #12 (Choose the serialization method based on content type), we were still looking for the right way to spell the various encoding/decoding methods that take into account both the type of the underlying schema, and the desired content type. In #12, we found something decent, but we could have simplified the spelling even further, getting rid of a whole closure invocation and a specialized type. ### Modifications In this PR (in preparation for multiple content type support), we do the simplification by completely removing `EncodableBodyContent` and the associated closure trampoline that we used to propoagate the content type string. We now pass the string directly, and simplified all the helper functions that have to do with encoding body content. Now, this is done in a backwards compatible way, so we really just deprecated all the existing methods, and the `EncodableBodyContent` type, but it'll all be cleaned up when we prepare for tagging the next breaking version. ### Result The helper functions have a simpler spelling, which simplifies the generator and reduces the lines of code we have to generate. (The associated generator PR will be coming in shortly.) ### Test Plan I preserved all the tests for the deprecated variants, and added new unit tests for the new variants. The deprecated tests will also be cleaned up in the next breaking version. Reviewed by: simonjbeaumont Builds: ✔︎ pull request validation (5.8) - Build finished. ✔︎ pull request validation (5.9) - Build finished. ✔︎ pull request validation (api breakage) - Build finished. ✔︎ pull request validation (docc test) - Build finished. ✔︎ pull request validation (integration test) - Build finished. ✔︎ pull request validation (nightly) - Build finished. ✔︎ pull request validation (soundness) - Build finished. #30
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
Resolves apple/swift-openapi-generator#43. This PR also resolves apple/swift-openapi-generator#42. It's paired with apple/swift-openapi-generator#48.
This PR introduces the initial content-type-aware coding strategy, which is likely to get expanded in the future.
Modifications
This PR covers the scope described in issue 43 - handling a
type: stringappropriately, based on whether it's wrapped inapplication/jsonortext/plancontent.It introduces new variants of the SPI helper functions, which now has variants per "coding strategy". It's best to read https://github.com/apple/swift-openapi-generator/pull/48/files?short_path=aa92e58#diff-aa92e58fb5a311512cfcd285827a4aa2e6634cae7fdfe0090bd2cfc7b0986140 for details.
Deprecated all of the SPI methods that didn't contain the coding strategy parameter, to be removed in the next breaking version.
Result
We now have explicit control over the coding strategy for ambiguous types, such as
String, which conform to bothCodableandLosslessStringConvertible, to be taken advantage of by the generator (see the paired PR).Test Plan
Verified that the default behavior now matches version 0.1.0, and when paired with the updated generator, fully resolves issue 43.
Used code coverage to ensure that
Converter+{Common,Client,Server}.swiftare now 100% unit tested.