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

[Proposal] SOAR-0005: Adopting the Swift HTTP Types package #255

Merged
merged 7 commits into from
Sep 26, 2023

Conversation

czechboy0
Copy link
Contributor

@czechboy0 czechboy0 commented Sep 8, 2023

@gjcairo
Copy link
Contributor

gjcairo commented Sep 8, 2023

This proposal is now In Review, which will run until 15th September - feel free to leave your feedback either on this PR or on the corresponding Swift Forums post.

Thanks, and thanks @czechboy0 for the write-up!

@gjcairo
Copy link
Contributor

gjcairo commented Sep 12, 2023

Overall this looks good to me, and I believe it's the right direction for the ecosystem to move in 👍🏼

Just one question though: wouldn't it make sense to also make the HTTPBodys in ClientTransport and ClientMiddleware also optional?

@czechboy0
Copy link
Contributor Author

czechboy0 commented Sep 12, 2023

Just one question though: wouldn't it make sense to also make the HTTPBodys in ClientTransport and ClientMiddleware also optional?

Great question, I actually tried to make that change, unfortunately unlike with request bodies (which OpenAPI documents as optional, and semantically, a nil body is distinct from an empty body), response bodies are required, and if there's no documented response body, it's considered empty. There's no concept of a nil response body.

The reasoning seems to come from HTTP semantics, where GET and DELETE requests are not supposed to have request bodies, unlike other methods, where a request body is allowed. In contrast, response bodies are not discouraged per method by the HTTP spec, so there's no nullability to communicate.

Consider the client transport implementing this and trying to make a decision of whether to return an empty or nil body - how do you make that? You can't, as there's no way to know whether the server sent a "nil body" or an empty body. That, again, is in contrast to a server transport, which can reliably, for GET and DELETE requests, provide a nil body (no need to even instantiate an empty HTTPBody value) without looking at the wire, as even if there are bytes, servers are supposed to ignore them for these methods.

@gjcairo
Copy link
Contributor

gjcairo commented Sep 15, 2023

The review period is now over, and the proposal has been accepted. SOAR-0005 is now Ready for Implementation.

@czechboy0
Copy link
Contributor Author

Landing the proposals to avoid more conflicts, will update their status once the implementation lands.

@czechboy0 czechboy0 merged commit 82678a7 into apple:main Sep 26, 2023
@czechboy0 czechboy0 deleted the hd-soar-0005 branch September 26, 2023 07:43
czechboy0 added a commit to apple/swift-openapi-runtime that referenced this pull request Sep 27, 2023
[Runtime] Async bodies + swift-http-types adoption

### Motivation

Runtime changes of the approved proposals apple/swift-openapi-generator#255 and apple/swift-openapi-generator#254.

### Blockers of merging this
- [x] 1.0 of swift-http-types

### Modifications

⚠️ Contains breaking changes, this will land to main and then 0.3.0 will be tagged, so not backwards compatible with 0.2.0.

- add a dependency on https://github.com/apple/swift-http-types
- remove our currency types `Request`, `Response`, `HeaderField`
- replace them with the types provided by `HTTPTypes`
- remove `...AsString` and the whole string-based serialization strategy, which was only ever used by bodies, but with streaming, we can't safely stream string chunks, only byte chunks, so we instead provide utils on `HTTPBody` to create it from string, and to collect it into a string. This means that the underlying type for the `text/plain` content type is now `HTTPBody` (a sequence of byte chunks) as opposed to `String`
- adapted Converter extensions to work with the new types
- added some internal utils for working with the query section of a path, as `HTTPTypes` doesn't provide that, the `path` property contains both the path and the query components (in `setEscapedQueryItem`)
- adapted error types
- adapted printing of request/response types, now no bytes of the body are printed, as they cannot be assumed to be consumable more than once
- adjusted the transport and middleware protocols, as described in the proposal
- removed the `queryParameters` property from `ServerRequestMetadata`, as now we parse the full query ourselves, instead of relying on the server transport to do it for us
- removed `RouterPathComponent` as now we pass the full path string to the server transport in the `register` function, allowing transport with more flexible routers to handle mixed path components, e.g. `/foo/{bar}.zip`.
- introduced `HTTPBody`, as described by the proposal
- adjusted UniversalClient and UniversalServer
- moved from String to Substring in a few types, to allow more passthrough of parsed string data without copying

### Result

SOAR-0004 and SOAR-0005 implemented.

### Test Plan

Introduced and adjusted tests for all of the above.


Reviewed by: 

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 (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 
     ✖︎ pull request validation (api breakage) - Build finished. 
     ✖︎ pull request validation (integration test) - Build finished. 

#47
czechboy0 added a commit to swift-server/swift-openapi-async-http-client that referenced this pull request Sep 27, 2023
[AHC Transport] Async bodies + swift-http-types adoption

### Motivation

AHC transport changes of the approved proposals apple/swift-openapi-generator#255 and apple/swift-openapi-generator#254.

### Modifications

- Adapts to the runtime changes, depends on HTTPTypes now.
- Both request and response streaming works.

### Result

Transport works with the 0.3.0 runtime API of.

### Test Plan

Adapted tests.


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

#16
czechboy0 added a commit to apple/swift-openapi-urlsession that referenced this pull request Oct 2, 2023
[URLSession Transport] Async bodies + swift-http-types adoption

### Motivation

URLSession transport changes of the approved proposals apple/swift-openapi-generator#255 and apple/swift-openapi-generator#254.

### Modifications

- Adapts to the runtime changes, depends on HTTPTypes now.
- Doesn't do streaming yet, we'll addressed that separately, continues to buffer for now (apple/swift-openapi-generator#301)

### Result

Transport works with the 0.3.0 runtime API of.

### Test Plan

Adapted tests.


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. 

#15
czechboy0 added a commit that referenced this pull request Oct 2, 2023
[Generator] Async bodies + swift-http-types adoption

### Motivation

Generator changes of the approved proposals #255 and #254.

### Modifications

- Adapts to the runtime changes.
- Most changes are tests updating to the new generated structure.
- As usual, easiest to start with the diff to the file-based reference tests to understand the individual changes, and then review the rest of the PR.
- To see how to use the generated code, check out some streaming examples in https://github.com/apple/swift-openapi-generator/pull/245/files#diff-2be042f4d1d5896dc213e3a5e451b168bd1f0143e76753f4a5be466a455255eb

### Result

Generator works with the 0.3.0 runtime API of.

### Test Plan

Adapted tests.


Reviewed by: simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (compatibility test) - Build finished. 
     ✔︎ pull request validation (docc test) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 
     ✖︎ pull request validation (integration test) - Build finished. 

#245
@czechboy0 czechboy0 added the semver/none No version bump required. label Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants