[Rust-Axum] Fix uuid in header params causing compilation errors #18563

myz-dev · 2024-05-03T12:38:43Z

Routes with header parameters with a format of uuid in the openAPI specification used to cause a compilation error in the resulting Rust Axum code.
This commit fixes the issue by including the correct conversion trait implementation on the condition that at least one header parameter of format uuid is included in the specification.
Problem details: #18554

How to validate:

The faulty behavior can be reproduced like this:
On the current master branch generate a Rust axum library with the specification described in #18554 .
Then (when you have installed rustc and cargo, run cargo check in the rust workspace containing the generated library. You will get compile errors in the places where the uuid is extracted from the request headers.
To verify the fix do the same with the code changes contained in this pull request.
Now cargo check will not report any compilation errors. Note the change in the src/header.rs file. A few lines have been added to it.

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh ./bin/configs/*.yaml
./bin/utils/export_docs_generators.sh
```
(For Windows users, please run the script in Git BASH)
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.
File the PR against the correct branch: master (upcoming 7.1.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@frol @farcaller @richardwhiuk @paladinzh @jacob-pro

wing328 · 2024-05-03T13:13:14Z

thanks for the PR.

cc @linxGnu

linxGnu · 2024-05-04T04:14:32Z

...penapi-generator/src/main/java/org/openapitools/codegen/languages/RustAxumServerCodegen.java

@@ -552,6 +553,12 @@ public CodegenOperation fromOperation(String path, String httpMethod, Operation
            }
        }

+        // Include renderUuidConversionImpl exactly once in the vendorExtensions map when 
+        // at least one `uuid::Uuid` converted from a header value in the resulting Rust code. 
+        Boolean renderUuidConversionImpl = op.headerParams.stream().anyMatch(h -> h.getDataType().equals(uuidType));


Please change it to

final Boolean renderUuidConversionImpl

Using final whenever possible is a best/good practice

Thank you for the hint!

linxGnu · 2024-05-04T04:18:17Z

modules/openapi-generator/src/main/resources/rust-axum/header.mustache

@@ -178,3 +178,39 @@ impl TryFrom<IntoHeaderValue<DateTime<Utc>>> for HeaderValue {
        }
    }
 }
+
+{{#renderUuidConversionImpl}}


Could you please run integration test.

./bin/generate-samples.sh ./bin/configs/manual/*.yaml mvn integration-test -f samples/server/petstore/rust-axum/pom.xml

You might need to create new api with uuid in the header if those specs do not have

Okay, done that. Should have done it in the first place.

myz-dev · 2024-05-04T14:01:38Z

Thank you very much for having reviewed the PR so far. I have made the changes you described and hope to not have made any mistakes.

wing328 · 2024-05-04T14:52:29Z

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

Routes with header parameters with a `format` of `uuid` in the openAPI specification used to cause a compilation error in the resulting Rust Axum code. This commit fixes the issue by including the correct conversion trait implementation on the condition that at least one header parameter of `format` `uuid` is included in the specification.

The trait needs to be in scope for the TryFrom implementation: `TryFrom<HeaderValue> for IntoHeaderValue<uuid::Uuid> ` It will only be brought into scope when the implementation is rendered.

myz-dev · 2024-05-05T08:04:31Z

Sorry for causing so much work with this PR. Now I have rebased the history so that the e-mail aligns with my GitHub account.

I will change the integration test for this change because the same bug I have fixed for the rust-axum generator exists for the rust-server generator also. So I will create a new test that is only run during the integration test of the rust-axum generator. Then in an another PR, if I have the time, I will fix the same bug in the rust-server generator.

wing328 · 2024-05-05T08:07:19Z

No problem. We appreciate your contributions to the Rust generators.

This commit adds an integration test that tests the bug fix for OpenAPITools#18554. A header parameter of `format: uuid` is included in one route. This makes the example create a route handler that tries to extract a Rust `uuid::Uuid` type from the header. The integration test will check that the generated code compiles.

The generated samples are updated with: `./bin/generate-samples.sh ./bin/configs/manual/*.yaml` Most example projects have their version numbers bumped. Some changes show, that there are some other unrelated changes to the files, which indicates that some prior commit did not update the samples accordingly. The relevant integration test `mvn integration-test -f samples/server/petstore/rust-axum/pom.xml` passes.

myz-dev · 2024-05-05T12:35:22Z

I found time to add a clean integration test for the changes to the rust-axum generator. The relevant samples are updated and the relevant integration test passes.
Please inform me if there is anything I can/should do differently.

wing328 · 2024-05-05T15:04:48Z

https://github.com/OpenAPITools/openapi-generator/actions/runs/8958257509/job/24603984954?pr=18563

the build failure doesn't seem to be related to this PR, right?

myz-dev · 2024-05-05T15:31:32Z

Yes this is an unrelated error. I can try to tackle this together with #18572

myz-dev · 2024-05-05T20:08:23Z

https://github.com/OpenAPITools/openapi-generator/actions/runs/8958257509/job/24603984954?pr=18563

the build failure doesn't seem to be related to this PR, right?

I created a PR that should resolve the problem of the failing test.
I could not reproduce a failing build on my local system I do not know why, but I adjusted the code so that the problem you cited should be solved.

wing328 · 2024-05-06T04:25:33Z

merged #18575

@linxGnu when you've time, can you please review this PR? Thank you.

linxGnu

LGTM. Please also trying to use latest uuid crate.

linxGnu · 2024-05-07T09:34:23Z

@myz-dev both of your PR, please ensure that you pass Integration Test. It's critical to verify.

myz-dev · 2024-05-07T09:40:39Z

LGTM. Please also trying to use latest uuid crate.

Hi. The template includes (in Cargo.mustache):
uuid = { version = "1", features = ["serde"] }
This means the newest 1.x version of uuid is going to be pulled for the build.
If I look into samples/server/petstore/rust-axum/Cargo.lock I see this:

[[package]]
name = "uuid"
version = "1.8.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "a183cf7feeba97b4dd1c0d46788634f6221d87fa961b305bed08c851829efcc0"
dependencies = [
 "serde",
]

With 1.8.0 being the latest release: https://crates.io/crates/uuid

If I did not misunderstand your comment, the latest version of uuid is already used.

myz-dev · 2024-05-07T09:43:10Z

@myz-dev both of your PR, please ensure that you pass Integration Test. It's critical to verify.

Sorry can you please explain what test does not pass? When I run
mvn integration-test -f samples/server/petstore/rust-axum/pom.xml I get a passing test:

...
[INFO] --- exec:1.2.1:exec (clippy) @ RustServerTests ---
    Checking rust-axum-header-uui v0.1.9 (/home/myz/Workspace/Varta/openapi-generator/samples/server/petstore/rust-axum/output/rust-axum-header-uuid)
    Finished dev [unoptimized + debuginfo] target(s) in 0.80s
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  35.042 s
[INFO] Finished at: 2024-05-07T11:41:50+02:00

linxGnu · 2024-05-07T10:25:51Z

@myz-dev

uuid already latest -> nice, thank you for double checking
It looks good when tests mvn integration-test -f samples/server/petstore/rust-axum/pom.xml are passed

Then LGTM

…nAPITools#18563) * fix: Fix uuid in header params causing errors Routes with header parameters with a `format` of `uuid` in the openAPI specification used to cause a compilation error in the resulting Rust Axum code. This commit fixes the issue by including the correct conversion trait implementation on the condition that at least one header parameter of `format` `uuid` is included in the specification. * refactor: Add final to boolean * fix: Bring str::FromStr optionally into scope The trait needs to be in scope for the TryFrom implementation: `TryFrom<HeaderValue> for IntoHeaderValue<uuid::Uuid> ` It will only be brought into scope when the implementation is rendered. * test: Add integration test and its specification This commit adds an integration test that tests the bug fix for OpenAPITools#18554. A header parameter of `format: uuid` is included in one route. This makes the example create a route handler that tries to extract a Rust `uuid::Uuid` type from the header. The integration test will check that the generated code compiles. * test: Update examples and run integration test The generated samples are updated with: `./bin/generate-samples.sh ./bin/configs/manual/*.yaml` Most example projects have their version numbers bumped. Some changes show, that there are some other unrelated changes to the files, which indicates that some prior commit did not update the samples accordingly. The relevant integration test `mvn integration-test -f samples/server/petstore/rust-axum/pom.xml` passes.

wing328 added Issue: Bug Server: Rust labels May 3, 2024

wing328 added this to the 7.6.0 milestone May 3, 2024

linxGnu suggested changes May 4, 2024

View reviewed changes

myz-dev added 3 commits May 5, 2024 08:12

refactor: Add final to boolean

7a83cbb

fix: Bring str::FromStr optionally into scope

6bee82e

The trait needs to be in scope for the TryFrom implementation: `TryFrom<HeaderValue> for IntoHeaderValue<uuid::Uuid> ` It will only be brought into scope when the implementation is rendered.

myz-dev force-pushed the fix_#18554_rust_axum branch from b197ad1 to 71c367a Compare May 5, 2024 07:43

myz-dev added 2 commits May 5, 2024 14:26

myz-dev force-pushed the fix_#18554_rust_axum branch from 71c367a to a8e06cd Compare May 5, 2024 12:33

linxGnu reviewed May 7, 2024

View reviewed changes

wing328 merged commit 2d967cc into OpenAPITools:master May 7, 2024
17 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rust-Axum] Fix uuid in header params causing compilation errors #18563

[Rust-Axum] Fix uuid in header params causing compilation errors #18563

myz-dev commented May 3, 2024

wing328 commented May 3, 2024

linxGnu May 4, 2024

myz-dev May 4, 2024

linxGnu May 4, 2024

myz-dev May 4, 2024

myz-dev commented May 4, 2024

wing328 commented May 4, 2024

myz-dev commented May 5, 2024

wing328 commented May 5, 2024

myz-dev commented May 5, 2024

wing328 commented May 5, 2024

myz-dev commented May 5, 2024

myz-dev commented May 5, 2024

wing328 commented May 6, 2024

linxGnu left a comment

linxGnu commented May 7, 2024

myz-dev commented May 7, 2024

myz-dev commented May 7, 2024

linxGnu commented May 7, 2024

[Rust-Axum] Fix uuid in header params causing compilation errors #18563

[Rust-Axum] Fix uuid in header params causing compilation errors #18563

Conversation

myz-dev commented May 3, 2024

How to validate:

PR checklist

wing328 commented May 3, 2024

linxGnu May 4, 2024

Choose a reason for hiding this comment

myz-dev May 4, 2024

Choose a reason for hiding this comment

linxGnu May 4, 2024

Choose a reason for hiding this comment

myz-dev May 4, 2024

Choose a reason for hiding this comment

myz-dev commented May 4, 2024

wing328 commented May 4, 2024

myz-dev commented May 5, 2024

wing328 commented May 5, 2024

myz-dev commented May 5, 2024

wing328 commented May 5, 2024

myz-dev commented May 5, 2024

myz-dev commented May 5, 2024

wing328 commented May 6, 2024

linxGnu left a comment

Choose a reason for hiding this comment

linxGnu commented May 7, 2024

myz-dev commented May 7, 2024

myz-dev commented May 7, 2024

linxGnu commented May 7, 2024