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] Add headerFields and body to UndocumentedPayload #488

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

czechboy0
Copy link
Collaborator

@czechboy0 czechboy0 commented Dec 14, 2023

Motivation

Fixes #299.

Depends on apple/swift-openapi-runtime#90 landing first and getting released, and the version dependency being bumped here.

Runtime dependency bumped to 1.1.0.

Modifications

Adapted to the runtime changes, added headerFields and body properties to UndocumentedPayload.

Now, the generated code actually also forwards the values to it.

Result

Easier access to the raw response data if an undocumented response is returned.

Test Plan

Adapted snippet and file-based reference tests, and also the unit tests of the generated code (PetstoreConsumerTests).

@groue
Copy link

groue commented Dec 14, 2023

Thank you @czechboy0, this is a much welcome addition :-)

I guess it is the change to Translator/ClientTranslator/translateClientMethod.swift that fills the undocumented payload with the response headers and body, although I don't quite understand how it works 😅

I'm not sure I understand what's in the response body of the undocumented payload, though. But that's only because I don't understand exactly when .undocumented is returned.

I assume that the response body was not fully buffered, since this would create a risk of high memory consumption.

I thus assume that not a single byte of response body was consumed, otherwise it would throw TooManyIterationsError when touched, and the undocumented payload would be unusable.

Hence the undocumented payload is only returned based on the headers of the response, and this will never change in the future. All unexpected data in the response body itself will never return .undocumented, but some kind of decoding error, and this is a strong contract of the library.

Do I understand well? If so, I'm very happy with this PR!

EDIT. Yes, that's exactly how UndocumentedPayload is documented. 👍

@czechboy0
Copy link
Collaborator Author

czechboy0 commented Dec 14, 2023

I don't understand exactly when .undocumented is returned.

For example, if you OpenAPI document defines responses for 200, 400, and 500, and you receive 401, you'll get an .undocumented case, with the associated status value saying 401. And now, the payload (the second associated value) will contain the header fields and the body stream.

I assume that the response body was not fully buffered, since this would create a risk of high memory consumption.

Correct, HTTPBody is an async sequence of byte chunks, and it's up to the ultimate consumer of this data to start pulling from there. So this change still doesn't lead to any buffering.

Hence the undocumented payload is only returned based on the headers of the response, and this will never change in the future. All unexpected data in the response body itself will never return .undocumented, but some kind of decoding error, and this is a strong contract of the library.

Yes, exactly. The response HTTP status code is how we decide the handling. If the status code is undocumented, you get the raw data. If it's documented, we unwrap the way that the OpenAPI document tells us. If you receive a documented response, but with body data that don't fit the documented structure, an error will be thrown.

Do I understand well? If so, I'm very happy with this PR!

I believe so - please just confirm again that the above fits well with your understanding 🙂

@groue
Copy link

groue commented Dec 14, 2023

Perfect! This was a quick win, enabled by a good initial design (the smoothest butter developers can dream of) 🙂👍

@czechboy0 czechboy0 marked this pull request as ready for review December 15, 2023 13:03
@czechboy0 czechboy0 enabled auto-merge (squash) December 15, 2023 13:11
@czechboy0 czechboy0 added the semver/patch No public API change. label Dec 15, 2023
@czechboy0 czechboy0 merged commit cd88cbe into apple:main Dec 15, 2023
9 checks passed
@czechboy0 czechboy0 deleted the hd-undocumented-payload-properties branch December 15, 2023 13:34
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.

Improve access to undocumented HTTP data
3 participants