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

Salvo does not take into account the schema name for generics #754

Open
rxdiscovery opened this issue Apr 17, 2024 · 11 comments
Open

Salvo does not take into account the schema name for generics #754

rxdiscovery opened this issue Apr 17, 2024 · 11 comments

Comments

@rxdiscovery
Copy link

rxdiscovery commented Apr 17, 2024

Hello,

I've noticed that when using generics, salvo doesn't take the schema name into account but gives the full path name of the struct.

let's take the example of these two structures :

use std::fmt::Debug;
use salvo::oapi::ToSchema;
use serde::{Deserialize, Serialize};

#[derive(Serialize, Deserialize, Clone, Debug, ToSchema)]
#[salvo(schema(symbol = "City"))]
pub struct CityDTO {
    #[salvo(schema(rename = "id"))]
    pub id: String,
    #[salvo(schema(rename = "name"))]
    pub name: String,
}

#[derive(Serialize, Deserialize, Debug, ToSchema)]
#[salvo(schema(symbol = "Response"))]
pub struct ApiResponse<T: Serialize + ToSchema + Send + Debug + 'static> {
    #[salvo(schema(rename = "status"))]
    /// status code
    pub status: String,
    #[salvo(schema(rename = "msg"))]
    /// Status msg
    pub message: String,
    #[salvo(schema(rename = "data"))]
    /// The data returned
    pub data: T,
}

and this is the simplified Rest controller :

#[endpoint(operation_id = "get_all_cities", tags("city"), status_codes(200, 400, 401, 403, 500))]
pub async fn get_all_cities() -> Result<Json<ApiResponse<Vec<CityDTO>>>, StatusError> {
    match services::city_service::get_all_cities().await {
        Ok(entities) => Ok(Json(ApiResponse::new(
            "200",
            "OK",
            CityMapper::entities_to_dtos(&entities),
        ))),
        Err(_) => Err(StatusError::internal_server_error()),
    }
}

here's the result swagger :

Screenshot from 2024-04-17 12-39-03

generic names are no good :

Response<alloc::string::String>
Response<alloc::vec::Vec<test_api::dtos::city_dto::CityDTO>>
salvo_core.http.errors.status_error.StatusError

normally it would be :

Response<String>
Response<[City]>
salvo_core.http.errors.status_error.StatusError // <!--------- it must not appear

thanks in advance

@zhangkai803
Copy link
Member

Hi, I don't really see this as an issue. Could you point out which framework allows you to handle the names of generic types as you wish? I also added a test case using your code as an example(#755 ), so that we can be notified in case the names of generic types need to be beautified in the future.

@rxdiscovery
Copy link
Author

Hello, (thank you for your reply)
Normally it must take into account the name defined here :
#[salvo(schema(symbol = "City"))]

and display City instead test_api::dtos::city_dto::CityDTO (This also exposes the internal structure of the code)

the main purpose of swagger is automatic code generation, and swagger UI must display a human friendly representation.

@zhangkai803
Copy link
Member

It makes sense.

the main purpose of swagger is automatic code generation, and swagger UI must display a human friendly representation.

However, in the OpenAPI documentation, it is explicitly stated that the value of the ref property should be a URI, which identifies the location of the value being referenced. So let's just keep it that way.

@rxdiscovery
Copy link
Author

rxdiscovery commented Apr 28, 2024

Why not add a “short_uri=true” option, for example, to allow the $ref to be renamed with :

"$ref": "#/components/schemas/{{symbol_rename}}
from
[salvo(schema(symbol = "XXXXXX"))]

in this case it will be XXXXXX
"$ref": "#/components/schemas/XXXXXX

@zhangkai803
Copy link
Member

Naming conflicts are likely to occur if $ref does not contain the full path of the struct. In practice, it is generally recommended to keep $ref unique to ensure consistency and avoid potential reference issues.

If we have to do this, my suggestion is to add another field to record the shorter name (Which is not within the OpenAPI specification).

@chrislearn Please take a look.

@zhangkai803
Copy link
Member

#619 Could be related.

@rxdiscovery
Copy link
Author

rxdiscovery commented Apr 30, 2024

@zhangkai803 I understand your point of view, and you're absolutely right, it's the safest way to go.

But, we generally use a short name to avoid problems with code generators (https://openapi-generator.tech/), and also to make the documentation readable.
To avoid the risk of name collisions, most OAPI spec libraries (Springdoc , Utoipa, Poem_oapi, etc..) detect redundancy and generate an exception during build and they only use the schema name in $ref .

even the official Swagger example doesn't use long paths :
https://petstore.swagger.io/

and the most widely used library in the Java/Spring universe (same thing it uses a name for the $ref) :
https://demos1.springdoc.org/demo-spring-boot-2-webmvc/swagger-ui/index.html

Thanks for your interest in my request.

@zhangkai803
Copy link
Member

zhangkai803 commented Apr 30, 2024

Thank you for your patient and examples. We can use title. It appears in Schema Object in both OAP2.0 and OAP3.0.

Here is a FastAPI example (OAP3.1.0):
image

@rxdiscovery
Copy link
Author

rxdiscovery commented Apr 30, 2024

title works well, it would be nice to integrate this option into Salvo, and also not to expose STD paths.

for example, add a mapping like this :

  • alloc::vec::Vec -> Array
  • alloc::string::String -> String
  • salvo_core.http.errors.status_error.StatusError -> StatusError

already ' :: ' is not standard in code generation, and for a hacker, it gives a lot of information about the technology used in the backend. (In this case, we can easily deduce that the backend is made in Rust and uses Salvo as a server.)

although I think the ideal would be :
[salvo(schema(symbol = "SYMBOL_1", group = "GROUP_1"))]

title = SYMBOL_1
$ref = GROUP_1.SYMBOL_1

with Group, you avoid name collisions and organize objects better.

@zhangkai803
Copy link
Member

#770

@rxdiscovery
Copy link
Author

rxdiscovery commented May 8, 2024

#770

@zhangkai803 Thank you for taking the time to work on this subject. 👍

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

No branches or pull requests

2 participants