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

Keep existing url config in utoipa-swagger-ui #274

Closed
wants to merge 1 commit into from

Conversation

komi1230
Copy link
Contributor

@komi1230 komi1230 commented Sep 8, 2022

Hello,

TL; DR

I found a bug in axum extension about url config.
So far if you set url config, the config could be overridden.
This PR resolves this by checking url and urls field beforehand.

Background

In my application, API is versioned and each endpoint has a prefix of /v1.
Axum has nest method to roll up endpoints, and I can achieve this API versioning.

About Swagger UI, swagger's endpoint also has to starts from /v1 and this is easily achieved by below

SwaggerUi::new("/swagger/*tail")
    .url("/openapi.json", openapi)

But swagger will read /openapi.json so I have to change this to /v1/openapi.json.

I have to set url config to make Swagger UI accessible in endpoint /v1/swagger and I wrote below code.

let config = Config::from("/v1/openapi.json");
let swagger = SwaggerUi::new("/swagger/*tail")
    .url("/openapi.json", openapi)
    .config(config);

But this has been overridden.

Caveats

I wrote in above, axum has nest method and can roll up routes.
To resolve my issue, I decided to attach url config to SwaggerUI but I feel it's also OK to add base_path field to Config and automatically apply base path to SwaggerUIBundle.
If you like the latter approach, I will resubmit PR.

Thank you for checking.

@juhaku
Copy link
Owner

juhaku commented Sep 8, 2022

It has been designed so that for SwaggerUi::new(...) and .url(...) and .urls(...) the path provided should be relative path from the root of the server.

So why just not like this?

SwaggerUi::new("/v1/swagger/*tail")
    .url("/v1/openapi.json", openapi);

Also in a same way the you could have only SwaggerUi have the path /swagger/*tail and url to have /v1/openapi.json

SwaggerUi::new("/swagger/*tail")
    .url("/v1/openapi.json", openapi);

It originally was not designed so that someone would define the paths separately with additional config thus it just overrides the config urls with the SwaggerUi urls. We could add this support though as well but this should be also added to the actix.rs and rocket.rs since they have the same behaviour as the axum.rs has.

@komi1230
Copy link
Contributor Author

komi1230 commented Sep 9, 2022

I think this issue is caused by axum's nest method.

My application is like below.

let swagger = SwaggerUi::new("/swagger/*tail")
        .url("/openapi", openapi);

let services = Router::new()
        .route("/", get(root))
        .route("/users", get(list_users))
        .merge(swagger);

let app = Router::new()
        .nest("/v1", services)  // This makes all endpoints start from "/v1" prefix
        .layer(Extension(db_connection_pool));

let addr = SocketAddr::from(([0, 0, 0, 0], 3000));
axum::Server::bind(&addr)
    .serve(app.into_make_service())
    .await
    .unwrap();

Because of nest, in the above code SwaggerUI is served in /v1/swagger and OpenAPI JSON is served /v1/openapi.
But SwaggerUIBundle reads /openapi to fetch JSON.

If you set SwaggerUi::new("/swagger/*tail").url("/v1/openapi", openapi) on SwaggerUI struct (this is your idea), then SwaggerUI is served in /v1/swagger and OpenAPI JSON is served in /v1/v1/openapi.
In this case SwaggerUIBundle reads /v1/openapi correctly.

Like this, I feel it's better to split url config between router and SwaggerUIBundle config.

Of course, I can achieve this design by merging swagger route with avoiding nest method, like this.

Router::new()
    .nest("/v1", services)
    .merge(swagger)

Maybe this issue can be resolved by describing this specification in docs without changing utoipa's codes.

@juhaku
Copy link
Owner

juhaku commented Sep 9, 2022

If you set SwaggerUi::new("/swagger/*tail").url("/v1/openapi", openapi) on SwaggerUI struct (this is your idea), then SwaggerUI is served in /v1/swagger and OpenAPI JSON is served in /v1/v1/openapi.
In this case SwaggerUIBundle reads /v1/openapi correctly.

Oh yeah, true it does not work since the SwaggerUi writes the given urls to the swagger-ui bundle as well which is not aware of the nest function.

Of course, I can achieve this design by merging swagger route with avoiding nest method, like this.

Router::new()
   .nest("/v1", services)
   .merge(swagger)

Maybe this issue can be resolved by describing this specification in docs without changing utoipa's codes.

This looks to me quite elegant solution, and if it works it could be mentioned in docs. There also could be an example in examples folder as well regarding this.

I'm not opposing the change, it's just that I feel like there is some room for an improvement on how users define the url and openapi mapping. If there are multiple definitions of single url it's hard to tell which one should be the authoritative one.

@juhaku
Copy link
Owner

juhaku commented Dec 22, 2022

@komi1230 I was thinking about this change and it is reasonable to make. So after next release you also split the configuration as you originally intented. 🙂

Like this, I feel it's better to split url config between router and SwaggerUIBundle config.

I agree, I continue on this branch to make this change for other frameworks as well.

@juhaku juhaku self-assigned this Dec 22, 2022
@juhaku juhaku mentioned this pull request Dec 22, 2022
@juhaku juhaku closed this in #418 Dec 22, 2022
@juhaku juhaku mentioned this pull request Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released
Development

Successfully merging this pull request may close these issues.

2 participants