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

Support for XML request body and response body #664

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ugocottin
Copy link

Motivation

See this issue for more.

Modifications

Based on modifications in the swift-openapi-runtime, I've added support for XML coding strategy and content-type category.

Result

Bodies with XML content-type can be encoded and decoded through xmlCoder in OpenAPIRuntime.Configuration.

Test Plan

I've updated test for ContentType parsing with application/xml content-type.
Encoding and decoding to XML are out of scope of the tests, because encoding and decoding logic must be provided by user through custom coder implementation.

Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

Looks great, thank you @ugocottin!

Added a few suggestions, and I wonder if you could also add an XML client and server example to the Examples directory? To show how to connect all the pieces to actually have it all working. You can add a dependency on an existing XML coder in those example packages.

@czechboy0 czechboy0 linked an issue Oct 31, 2024 that may be closed by this pull request
@czechboy0
Copy link
Contributor

@ugocottin could you also review this section of OpenAPI to see if we should be taking that info into account? https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#xml-object

@ugocottin
Copy link
Author

@czechboy0 Regarding XML object metadata, I think it is out of the scope of this PR, but should definitely be implemented later for better XML support.

@ugocottin ugocottin requested a review from czechboy0 January 9, 2025 20:09
Copy link
Contributor

@czechboy0 czechboy0 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 the updates @ugocottin, I added a few more minor comments.

Also two more questions for us to discuss:

  1. How does https://github.com/oai/openapi-specification/blob/main/versions/3.1.1.md#xml-object factor into the serialization? Or should we ignore that and the adopter-provided XML encoder/decoder is what should know about how to translate between the raw XML and a full Codable type?
  2. Could you add a new package to Examples that shows how to use this end to end, one for client and one for server, similar to the ones we already have? It'd be great for folks (including myself) to just clone the repo and run the example to see it work.

Thanks!

Comment on lines 167 to 168
/// 2. text
/// 3. binary
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// 2. text
/// 3. binary
/// 3. text
/// 4. binary

Comment on lines 236 to +237
static var applicationJSON: Self { try! .init(string: "application/json") }
/// The content type `application/xml`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static var applicationJSON: Self { try! .init(string: "application/json") }
/// The content type `application/xml`.
static var applicationJSON: Self { try! .init(string: "application/json") }
/// The content type `application/xml`.

- [ ] XML
- [X] XML
- when content type is `application/xml` or ends with `+xml`
- xmlCoder must be provided to `OpenAPIRuntime.Configuration`
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, please also add a note to the section XML Object to clarify the current state of support for XML.

@nihedr9
Copy link

nihedr9 commented Jan 12, 2025

Hello guys

Do you have an estimated date for merging this feature?

Thanks

@czechboy0
Copy link
Contributor

No date estimate. When it's ready, it'll get merged. We appreciate all contributions, on any timeline, as many folks invest their free time into these pull requests.

@nihedr9
Copy link

nihedr9 commented Jan 13, 2025

No date estimate. When it's ready, it'll get merged. We appreciate all contributions, on any timeline, as many folks invest their free time into these pull requests.

Is there anything I can do to help?

@czechboy0
Copy link
Contributor

Is there anything I can do to help?

I'll let @ugocottin speak to that as the author of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for XML request body and response body
3 participants