[Runtime] Multiple content types support #29
Merged
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
Aug 1, 2023
czechboy0
commented
Aug 1, 2023
czechboy0
commented
Aug 1, 2023
3 tasks
czechboy0
changed the title
gjcairo
approved these changes
Aug 2, 2023
simonjbeaumont
approved these changes
Aug 3, 2023
czechboy0
added a commit
to apple/swift-openapi-generator
that referenced
this pull request
Aug 4, 2023
[Generator] Multiple content types support Depends on apple/swift-openapi-runtime#29 ### Motivation Up until now, when an OpenAPI document contained more than one content type in either a request or a response body, we would only generate code for now, and ignored the other ones. However, that was a temporary workaround until we could add proper support for multiple content types, which is what this PR is about. Addresses #6 and #7, except for the "Accept header" propagation, which will be addressed separately and have its own PR and even a proposal. That's one of the reasons this feature is landing disabled, hidden behind the `multipleContentTypes` feature flag. A tip for reviewing this change: first check out how the test OpenAPI document and the reference files changed, that shows the details of how we encode and decode multiple content types. I also added comments in the code for easier review. ### Modifications - Main: all supported content types found in a `content` map in request and response bodies are now generated in the `Body` enum. - Added a method `supportedTypedContents` that returns all supported content types, used now instead of the `bestTypedContent` method we used before. What is a "supported" content type? One that has a schema that doesn't return `false` to `isSchemaSupported`. (This is a pre-existing concept.) - Updated the logic for generating request and response bodies to use the new method, both on client and server. - Updated content type -> Swift enum case name logic, but that will go through a full proposal, so please hold feedback for the proposal, this is just a first pass based on some collected data of most commonly used content types. - ### Open Questions - What to do in an operation with multiple request/response content types when _no_ `content-type` header is provided? Previously, with up to 1 content type support, we still went ahead and tried to parse it. But here, we don't know which content to parse it as, so we just try it with the first in the list (we respect the order in the YAML dictionary, so what the user put first will be what we try to use when no `content-type` header is provided). If an invalid `content-type` value is provided, we throw an error (pre-existing behavior). Q: Is choosing the first reasonable? What would be better? Note that, unfortunately, many servers don't send `content-type` today. ### Result When an adopter provides multiple content types in their OpenAPI document, we generate one case in the Body enum for each! However, the new functionality is disabled by default, has to be explicitly requested by enabling the `multipleContentTypes` feature flag either on the CLI or in the config file. ### Test Plan Added `setStats` and `getStats` operations that employ multiple content types, one in a request body, the other in a response body. Added unit tests for it in `PetstoreConsumerTests` to verify it all works correctly at runtime. Deleted `Tests/OpenAPIGeneratorCoreTests/Translator/RequestBody/Test_translateRequestBody.swift` as snippet tests do this job better, and I didn't want to spend the time updating the test. Adds `PetstoreConsumerTests_FF_MultipleContentTypes`, a variant of `PetstoreConsumerTests` for when the `multipleContentTypes` feature flag is enabled, so we test both how the existing logic handles multiple content types (it picks one), and the new logic (it generates code for all supported ones it found). ## TODOs - [x] rename `IfConditionPair` to `IfBranch` - [x] break out the first `if` from the subsequence `else if`s in `IfStatement`, to 3 properties: first if, then an array of else ifs, then an optional else; to avoid creating invalid code - [x] create an SPI type `ContentType` to avoid repeatedly parsing the received content type Reviewed by: gjcairo, simonjbeaumont Builds: ✔︎ pull request validation (5.8) - Build finished. ✔︎ pull request validation (5.9) - 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. #146
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.
Runtime changes for apple/swift-openapi-generator#146
Motivation
See apple/swift-openapi-generator#146 for motivation.
Modifications
Adds new methods for:
extractContentTypeIfPresent- returns thecontent-typeheaderisValidContentType- evaluates content type equivalence, including wildcardsmakeUnexpectedContentTypeError- returns an error that the generated code can throwDeprecates this method, which will be removed in a future breaking version:
validateContentTypeIfPresent- only made sense when we supported only 1 content type value for a bodyResult
Enables generated code to support multiple content types.
Test Plan
Deprecated the tests for the deprecated method, and added unit tests for the new methods.