-
- Notifications
You must be signed in to change notification settings - Fork 7.2k
[rust-axum] Add support for multiple response types and streaming responses #22095
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
base: master
Are you sure you want to change the base?
[rust-axum] Add support for multiple response types and streaming responses #22095
Conversation
… stream and resposne enums Keep going, there are tests failing Remove the "single item" legacy support and wrap it in the multipart content umbrella Functional testing (with a test that can be uncommented to actually test building code if cargo is available) Fixing the box/pin for the stream to behave well in real code Cleanup unused extensions Clean up the operations Ensure the stream info propagates to other parts of the generator Cleanup tests
bcc335f to e95ced8 Compare | @jacob-mink-1996 just fyi, I will have a look this weekend |
| Sorry, I wasn’t able to review your PR last weekend due to some bug fixes for Rust Axum. I’ll focus on it this weekend and appreciate your patience. |
| In the meantime, could you please run integration test, using following command: mvn clean && ./bin/generate-samples.sh bin/configs/manual/rust-axum-* && mvn integration-test -f samples/server/petstore/rust-axum/pom.xml |
| Nice, thank you @linxGnu. I ran the integration tests and discovered some issues with responses that did not have a content-type. Updated the code to render these properly. My API spec output was unaffected by the change, but the integration test outputs had a bunch of updates, which I have committed (I think these are from the original set of changes, anyway). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will continue reviewing your PR. At the moment, I have one concern below
| @@ -1,49 +1,46 @@ | |||
| #[derive(Debug, PartialEq, Serialize, Deserialize)] | |||
| {{#vendorExtensions}}{{#x-has-event-stream-content}} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should remove:
#[derive(Debug, PartialEq, Serialize, Deserialize)]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting constraint @linxGnu - the problem is that by introducing Box<impl Stream…> to the enum, we lost the ability to derive these always. I even generated a custom Debug implementation of a stream type is present in the enum.
Additionally, all of them except for Debug are technically unused. The result enums are not serialized, it is the thing inside them that is (sometimes) serialized (which we already know implements Deserialize and Serialize because it was generated as such).
I’m not sure it is possible to leave them in like you suggest if Box<impl Stream…> is what we use. Plus, forcing the stream to be + Debug + Serialize + Deserialize will not work with many stream/async stream libraries as they typically return an opaque Stream type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacob-mink-1996
You should use conditional in Mustache. Something like this:
{{^stream}} #[derive(..., Serialize, Deserialize)] {{/stream}} {{#stream}} #[derive(Debug)] {{/stream}} There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the value in including them purely because it was done that way before? The Serializer, Deserialize, PartialEq, and Eq traits are not leveraged on these types anywhere in the generated source.
Anyway, I’ll fix that up tomorrow for you.
This change enables two related features in the rust-axum server generator
The core of the feature is to iterate over the content types rather than taking just the first, then build up a new list in the vendor extensions that includes the appropriate information. I took the approach of still keeping the templates simple and tried to compute types in the Java code as much as possible - we could of course go the other way and expand the type within the template, but it got pretty gnarly.
@linxGnu - I adopted a bit of a hybrid approach to the typing we discussed since this isn't just a return type change, it changes the enums. I went ahead and used a
(along with appropriate Sends/Syncs/'static annotations). This allowed me to shove it in an enum, and it appears to play nice with other methods that return an
impl Streamorimpl TryStream, both of which can be coerced into the pin-box using the.boxed()method fromStreamExt.Finally, the templates are updated to support the new code gen. I have validated this two ways:
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)fixes [REQ] [rust-axum] support text/event-stream #21967
@frol (2017/07) @farcaller (2017/08) @richardwhiuk (2019/07) @paladinzh (2020/05) @jacob-pro (2022/10) @dsteeley (2025/07)