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

Utoipa users poll: How do you use responses and how would you like them to work? Answer would help. #1068

Open
juhaku opened this issue Sep 27, 2024 · 7 comments
Labels
question Further information is requested

Comments

@juhaku
Copy link
Owner

juhaku commented Sep 27, 2024

Responses poll

Currently while I am working on refactoring multiple aspects of the utoipa lib for the coming release 5.0.0 this is good place to introduce some improvements to current workflow within boundaries set by its users. So here comes the question.

How do you currently use the responses with utoipa lib?

  1. Only tuples
  2. Tuples with derived ToResponse
  3. Derived IntoResponses
  4. Any other way, please describe.

Second question is how would you like the responses work, or is there any aspect you would like to change in order to them to be more user friendly?

@juhaku juhaku added the question Further information is requested label Sep 27, 2024
@juhaku juhaku pinned this issue Sep 27, 2024
@juhaku juhaku changed the title Utoipa users poll: How do you use the responses and how would you like them to work? Answer would help. Utoipa users poll: How do you use responses and how would you like them to work? Answer would help. Sep 27, 2024
@michaelvlach
Copy link

Hi, first of all huge thank you for this crate!

To answer your questions. I use dedicated structs and deriving from ToSchema: https://github.com/agnesoft/agdb/blob/main/agdb_api/rust/src/api_types.rs

I find it quite nice and I would not change it. There are two aspects I think would be nice if improved but I am not sure if possible:

  1. I miss validation of the stuff in the derived against actual code (route signature / types). Obviously things like status codes cannot be validated but I hope types could be. Often I missed the change in return type in the derive annotation and nothing complained. Luckily I have comprehensive test suite so I caught it but a compile error would be nice if there was a mismatch. Probably not possible though.

  2. More comprehensive documentation on the derive macro. I use a lots of stuff from utoipa and sometimes it was difficult to find out how to achieve what I wanted. E.g. https://github.com/agnesoft/agdb/blob/main/agdb_server/src/routes/db.rs#L82 Since I generate apis for different languages and they require things like tags, operation_id etc. That is a common struggle, no such thing as enough docs. :-)

@juhaku
Copy link
Owner Author

juhaku commented Oct 1, 2024

Great, thanks for a reply.

  1. There is improvement for this actually coming in the 5.0.0 release of utoipa. However if you completely forget the add the response(...) declaration then the improvement will not help there yet since the return types are not inferred. But could be with certain limitations. In the 5.0.0 release the request_body and response body schemas will be correctly spanned thus it will give compile error if an item does not exists and will also get renamed if you rename the type as well.
  2. True, more docs, that is forever target of improvement. 🙂

@irevoire
Copy link

irevoire commented Oct 3, 2024

Hey, not really an answer on how I uses it, but I would really like to be able to share responses between routes.

For example, in Meilisearch, most routes will return something we call a SummarizedTaskView. And from what I understand, currently I have to write a json!({ ... }).
I would really like to be able to reference it without rewriting the example entirely.

Going from:

/// Get a task
///
/// Get a [task](https://www.meilisearch.com/docs/learn/async/asynchronous_operations)
#[utoipa::path(
    post,
    path = "/{taskUid}",
    tag = "Tasks",
    params(("taskUid", format = UInt32, example = 0, description = "The task identifier", nullable = false)),
    responses(
        (status = 200, description = "Task successfully enqueued", body = SummarizedTaskView, content_type = "application/json", example = json!(
            {
              "uid": 1,
              "indexUid": "movies",
              "status": "succeeded",
              "type": "documentAdditionOrUpdate",
              "canceledBy": null,
              "details": {
                "receivedDocuments": 79000,
                "indexedDocuments": 79000
              },
              "error": null,
              "duration": "PT1S",
              "enqueuedAt": "2021-01-01T09:39:00.000000Z",
              "startedAt": "2021-01-01T09:39:01.000000Z",
              "finishedAt": "2021-01-01T09:39:02.000000Z"
            }
        ))
    )
)]
async fn get_task(
...

To:

/// Get a task
///
/// Get a [task](https://www.meilisearch.com/docs/learn/async/asynchronous_operations)
#[utoipa::path(
    post,
    path = "/{taskUid}",
    tag = "Tasks",
    params(("taskUid", format = UInt32, example = 0, description = "The task identifier", nullable = false)),
    responses(
        (status = 200, description = "Task successfully enqueued", body = SummarizedTaskView, content_type = "application/json", example = task_succeeded())
    )
)]
async fn get_task(
...

I’m using the latest beta version, let me know if I missed something.

@juhaku
Copy link
Owner Author

juhaku commented Oct 3, 2024

Hey, not really an answer on how I uses it, but I would really like to be able to share responses between routes.

Responses can be shared if used with ToResponse. Types that implement ToResponse can be added to the responses section of OpenApi. and then they can be references with response = MyResponse.

struct MyResponse; 

impl<'s> ToResponse<'s> for MyResponse {
  fn response() -> (&'s str, Response) -> {
    // add all the examples here programmatically
    ("MyResponse", Response::builder().build())
  }
}

#[utoipa::path(
    post,
    path = "/{taskUid}",
    tag = "Tasks",
    params(("taskUid", format = UInt32, example = 0, description = "The task identifier", nullable = false)),
    responses(
        (status = 200, response = inline(MyResponse))
    )
)]
async fn get_task(} {}

You could also derive the ToResponse but I am most likely going to change the ToResponse implementation. And I am not sure about the current behavior of the derive. I think it lacking something but I am not sure what, and perhaps I am going to remove that completely if I find some better way to do shareable responses.

example and examples attributes could be allowed to use method references though nonetheless but hasn't been done so far.

@irevoire
Copy link

irevoire commented Oct 9, 2024

Humm, yes, I tried to use it, but it didn’t work, and I don’t really have the time to understand why, It's probably an issue on my side.

But in terms of API, I find it strange that the ResponseBuilder doesn’t contain the HTTP status code and the schema 🤔
But it’s pretty nice to be able to define multiple examples for one type.
Defining and using the examples through a structure is also pretty good, IMO

Ideally, in our case, almost every route can have three different errors that share the same error code: If you forget to specify an API key, we don’t understand the format of your API key, or your API key doesn’t give you the right to access this route.
Iirc the first two returns an HTTP 400 while the last one return a 401 or something.
Then, they all share the same schema.
It would be really nice to be able to define all that on a single struct and then use it easily in most of my routes:

#[utoipa::path(
    post,
    path = "/{taskUid}",
    tag = "Tasks",
    params(("taskUid", format = UInt32, example = 0, description = "The task identifier", nullable = false)),
    responses(
        inline(ApiKeyError), // here I can define multiple examples with multiple HTTP status code
        inline(SummarizedTaskView), // here I only have one example
        (status = BAD_REQUEST, schema = ResponseError, example = json!(...)),
    )
)]
async fn get_task(} {}

I hope that can help you in your decisions!

@juhaku
Copy link
Owner Author

juhaku commented Nov 14, 2024

@irevoire Thanks for the response. Yeah that is worth thinking about. The current ResponseBuilder and IntoResponses schema thing is a bit convoluted and is really in a need of reassessment of how it actually should behave.

ResponseBuilder accpets content and then you need to use the ContentBuilder to provide the schema. However it does not contain the status code and that is in ResponsesBuilder. This is all because the implementation mimics the OpenAPI specification on the types and its definitions.

@Aderinom
Copy link

Aderinom commented Nov 24, 2024

Spend way too much time wrtiting this 😆
What I'd care deeply about is the Good Path Response being as easy to create as possible.

My reasoning/Viewpoint For us usually following is true:
  • Unless an error has technical implications and should
    cause the caller to do anything but log, we usually don't bother documenting these.
  • This e.g. on Bad Requests, Bad Auth, Internal Errors, Not Found etc.

Why:

  1. Because IMO on most apis these things can and will not be Programatically handled by the caller,
    just logged. As such spending time on perfect docs can be spend on something more effective.
  2. Because each documented error asks the caller to think about how to handle this case specifically
    so it adds mental fatigue and makes apis look much harder to implement than they actually are.

However: There are some rare cases where I want percise control over the errors, I would be fine having a a litte more clunky api for these cases.


At the moment, we usually do the tuple based approach with impl Responder as the return - so e.g.

    #[derive(Debug, serde::Serialize, serde::Deserialize, ToSchema)]
    pub struct Task {
        ...
    }

    /// Retrieves a certain task by id
    #[utoipa::path(
        responses(
            (status = 200, body = dto::Task),
        )
    )]
    #[get("/tasks/{id}")]
    pub async fn get_task_by_id(id : web::Path<i32>) -> impl Responder {
        println!("Should now get a task with id {}", id.into_inner());
        Ok(Json(Task:rand()))
    }
    

My dream would be:

    #[derive(Debug, serde::Serialize, serde::Deserialize, ToSchema, IntoResponses)]
    #[response(status = default_ok)] // Maybe allow this to be optional ? No defined = default ok - So Get 200, Post 201...
    pub struct Task {
        ...
    }

    /// Retrieves a certain task by id
    #[utoipa::path]
    #[get("/tasks/{id}")]
    pub async fn get_task_by_id(id : web::Path<i32>) ->  Result<Json<dto::Task>> {
        println!("Should now get a task with id {}", id.into_inner());
        Ok(Json(Task:rand()))
    }

What I would love to see:

  1. A "default_ok" status code which could be used with the IntoResponse derive.
    Why? We often use the same Response dto for all C/R/U/D ops.
    Thus mapping the status code 1:1 to my dto doesn't make sense for how I work.

  2. Ideally there would be a blanket

    1. impl<T:ToSchema/ToResponse> IntoResponses for Result<T, E> or
    2. impl<T:ToSchema/ToResponse, E:ToSchema/ToResponse> IntoResponses for Result<T, E>
      The 2nd would ofc be much more correct than the first. However would require that every error we return implements ToResponse.
      This would be annoying as for us at least, most errors are from upstream crates, thus making it impossible for us to implement the ToResponse directly.
      This would be required as the user of utoipa can not make these implementations themselves.
  3. For actix_extras there should e.g. be a impl IntoResponses/ToResponse for Json<T> as this can not be implemented by the user.

  4. IDK if this is possible - but it would also be nice if everything which implements ToResponse
    could be used with the responses attribute.
    They would then use the default_ok status code for the operation.

  5. Until I started looking into the topic specifically I did not understand the usage of the ToResponse and that
    with actix_extras I can automatically infer the response

    • This Could be me being slow, or maybe the Usage docs being hard to follow.
How I might start using this now

I might implement my own Result type like:

pub enum GetResult<Ok> {
    Ok(actix_web::web::Json<Ok>),
    Err(actix_web::Error),
} 


impl<Ok:Serialize> Responder for GetResult<Ok> {
    type Body =  EitherBody<<actix_web::web::Json<Ok> as Responder>::Body>;

    fn respond_to(self, req: &actix_web::HttpRequest) -> actix_web::HttpResponse<Self::Body> {
        match self {
            GetResult::Ok(t) => t.respond_to(req).map_into_left_body(),
            GetResult::Err(e) => e.error_response().map_into_right_body(),
        }
    }
}

impl<Ok: for <'a> ToResponse<'a>> IntoResponses for GetResult<Ok> {
    fn responses() -> std::collections::BTreeMap<String, utoipa::openapi::RefOr<utoipa::openapi::response::Response>> {
        let mut map = std::collections::BTreeMap::new();
        map.insert("200".to_string(), Ok::response().1);
        map
    }
}

/// ... PostResult

Don't love it as I'll have to into() all my responses this way, but not sure if there is a different idea.

Or possibly might add some post processing to the generated OpenAPI
which maps all the OK responses to the correct counterpart, and only require a single custom Result

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants