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

fix(rust): add missing paths to the OpenAPI spec #1737

Merged
merged 11 commits into from
Nov 13, 2024
Merged

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Nov 8, 2024

The OpenAPI spec is not complete, as reported in #1700. This PR aims to add the missing pieces to the spec. As part of the work, I updated the utoipa dependency to version 5.2 which is much better when it comes to detect omissions and problems.

Tasks

  • Add missing /api prefix.
  • Add missing paths.
  • Add missing components.
  • Migrate to utoipa 5.
  • Add the issues spec (postponed).

@imobachgs imobachgs marked this pull request as ready for review November 11, 2024 09:59
Copy link
Contributor

@lslezak lslezak left a comment

Choose a reason for hiding this comment

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

I'm not a Rust or OpenAPI expert, but the change looks reasonable to me. 😄

@@ -737,6 +738,7 @@ pub struct InvalidMacAddress(String);

#[derive(Debug, Default, Clone, PartialEq, Serialize, utoipa::ToSchema)]
pub enum MacAddress {
#[schema(value_type = String, format = "MAC address in EUI-48 format")]
Copy link
Contributor

Choose a reason for hiding this comment

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

now I am confused. it should have descriptuon from that MacAddr6 schema and also its value type should be read from it, not?

Copy link
Contributor Author

@imobachgs imobachgs Nov 11, 2024

Choose a reason for hiding this comment

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

It should, but it does not work in that way. To make it work, I would need to convert the variant into a struct-based one:

pub enum MacAddress {
  MacAddress {
    address: macaddr::MacAddr6
  },
  Preserve,
  // etc.
}

Otherwise, the macro did not work (at least for me). I can change it, but I was unsure whether I should adapt the enum to the macro.

@mvidner
Copy link
Contributor

mvidner commented Nov 11, 2024

Thanks! But :)
I want to test its output, but agama-web-server docs no longer works, and this feature is poorly discoverable: no "openapi" mentioned in any *.md file (and there are too many hits in the remaining files)
Please add a mention to rust/xtask/README.md I guess

@mvidner
Copy link
Contributor

mvidner commented Nov 11, 2024

Yay, now I can see the paths that are wrongly missing the /api prefix:

cd rust
cargo xtask openapi
jq '.paths | keys' out/openapi/*json | grep -v /api | grep \"

gives me

  "/manager/logs/list",
  "/manager/logs/store"
  "/ping"
  "/network/connections/:id"

@imobachgs
Copy link
Contributor Author

Thanks! But :) I want to test its output, but agama-web-server docs no longer works, and this feature is poorly discoverable: no "openapi" mentioned in any *.md file (and there are too many hits in the remaining files) Please add a mention to rust/xtask/README.md I guess

The OpenAPI spec are now generated at build time and packaged as agama-openapi under /usr/share/agama/openapi.

@imobachgs
Copy link
Contributor Author

Yay, now I can see the paths that are wrongly missing the /api prefix:

cd rust
cargo xtask openapi
jq '.paths | keys' out/openapi/*json | grep -v /api | grep \"

gives me

  "/manager/logs/list",
  "/manager/logs/store"
  "/ping"
  "/network/connections/:id"

Good catch! I have fixed them.

@imobachgs
Copy link
Contributor Author

@mvidner And about use cases, code generation for our own clients could be pretty interesting.

@imobachgs imobachgs merged commit 9dc7bef into master Nov 13, 2024
6 checks passed
@imobachgs imobachgs deleted the fix-openapi-schema branch November 13, 2024 13:30
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.

4 participants