Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Generator] Multiple content types support #146

Merged
merged 11 commits into from
Aug 4, 2023

Conversation

czechboy0
Copy link
Contributor

@czechboy0 czechboy0 commented Jul 27, 2023

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

  • rename IfConditionPair to IfBranch
  • break out the first if from the subsequence else ifs in IfStatement, to 3 properties: first if, then an array of else ifs, then an optional else; to avoid creating invalid code
  • create an SPI type ContentType to avoid repeatedly parsing the received content type

czechboy0 added a commit that referenced this pull request Jul 31, 2023
[Generator] Design away EncodableBodyContent

### Motivation

See apple/swift-openapi-runtime#30

Depends on Runtime 0.1.6

### Modifications

Adapted the generator code for the runtime changes. Also, since this was factored out of the big PR (#146) adding multiple content types, you can see the code was prepared for handling multiple content types, even though still in this PR it only ever gets N=1.

### Result

The generated code uses the new simplified runtime functions, and prepares us for multiple content types.

### Test Plan

Adapted file-based reference tests to generate the new syntax.


Reviewed by: 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. 

#152
czechboy0 added a commit that referenced this pull request Aug 1, 2023
[NFC] Refactor PetstoreConsumerTests to allow multiple versions

### Motivation

In upcoming features that contain breaking changes, but we'll stage them in behind a feature flag, we'd like to have multiple variants of the `PetstoreConsumerTests` test target, allowing us to test both the existing behavior and the new behavior once a feature flag is enabled. These additional variants would be temporary, and would be deleted again (or rather, the main test suite would be updated) once the feature flag is enabled unconditionally. So the steady state number of `PetstoreConsumerTest` variants would continue to be 1, just for short periods of time, it might be higher.

### Modifications

This PR makes that possible by refactoring common functionality into a new internal target `PetstoreConsumerTestCore`. Note that all the variants of the test target share the same OpenAPI document, but generate different variants of the reference code.

Highlights:
- new `PetstoreConsumerTestCore` target, depended on by the existing `PetstoreConsumerTests`
- added an ability to _not_ fail the test when specific diagnostics are emitted (through the new `ignoredDiagnosticMessages: Set<String>` parameter), which allows us to test both existing and new behavior on the same OpenAPI document. Other, non-allowlisted diagnostics, still continue to fail the test, so new ones won't sneak through undetected.
- many test functions now optionally take `featureFlags` and `ignoredDiagnosticMessages`, again allowing us to test both existing and proposed behavior hidden behind a feature flag

### Result

It will be a lot easier to temporarily introduce other variants of the `PetstoreConsumerTests` test target to allow for thoroughly testing features even before they are enabled by default, giving us more confidence in the accuracy of proposals and their associated implementations.

### Test Plan

All tests continue to pass, this is an NFC, a pure refactoring, making the next PR much smaller. To see how this all fits together, check out the PR that has all the changes: #146. That said, these partial PRs are easier to review, as they're each focused on one task.


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. 

#157
.swift-format Show resolved Hide resolved
@czechboy0 czechboy0 changed the title [WIP] Multiple content types Multiple content types support Aug 1, 2023
@czechboy0 czechboy0 marked this pull request as ready for review August 1, 2023 14:12
@simonjbeaumont
Copy link
Collaborator

Before digging into the review proper, thought I'd pick up on this question:

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.

I think it's reasonable for us to throw an error if Content-Type is provided but doesn't match what we get. However, in the case the server omits Content-Type, shouldn't we be trying all of them, not just the first, similar to how we handle anyOf/oneOf? Failing if it doesn't match the first seems to be overly restrictive since it will reject a response from the server which is "in spec"? Or have I misunderstood?

@czechboy0
Copy link
Contributor Author

Trying all of them is another option, yeah. Although if the first content type is a binary type, we'll never fail to parse as it, and move to the next one. But if it's ordered from the most to least restrictive, this would work well. One concern about this would be performance, as trying to parse large (e.g. image) data as JSON might be slow, but if it still "works", the adopter might not know they should probably go and fix their server or OpenAPI doc.

So, to recap our options:

  • try all, until one that works
  • try the first one only
  • throw an error if no content type is provided, and more than 1 content type is documented

@czechboy0 czechboy0 changed the title Multiple content types support [Generator] Multiple content types support Aug 1, 2023
@czechboy0 czechboy0 force-pushed the hd-draft-multiple-content-types branch from def5585 to bcdbcd5 Compare August 2, 2023 06:40
Copy link
Collaborator

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to rebase this PR for easier review. It's looking really good. I've added a few questions, otherwise I think it's looking really good!

@gjcairo
Copy link
Contributor

gjcairo commented Aug 2, 2023

Q: Is choosing the first reasonable? What would be better?

Answering this question from the description: I think this is reasonable. The only alternative is to send supported ones, I guess, but I'm not sure what benefit this would have over sending just the first one.

czechboy0 added a commit to apple/swift-openapi-runtime that referenced this pull request Aug 4, 2023
[Runtime] Multiple content types support

Runtime changes for apple/swift-openapi-generator#146

### Motivation

See apple/swift-openapi-generator#146 for motivation.

### Modifications

Adds new methods for:
- `extractContentTypeIfPresent` - returns the `content-type` header
- `isValidContentType` - evaluates content type equivalence, including wildcards
- `makeUnexpectedContentTypeError` - returns an error that the generated code can throw

Deprecates 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 body

### Result

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.


Reviewed by: gjcairo, 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. 

#29
@czechboy0 czechboy0 merged commit cb41d40 into apple:main Aug 4, 2023
@czechboy0 czechboy0 deleted the hd-draft-multiple-content-types branch August 4, 2023 07:52
@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants