paperclip-rs / paperclip

WIP OpenAPI tooling for Rust.
Apache License 2.0
897 stars 117 forks source link

Use different response types #306

Closed tiagolobocastro closed 3 years ago

tiagolobocastro commented 3 years ago

So, looks like openapi supports using a schema for error types, including different schemas for each error code.

See: https://swagger.io/docs/specification/2-0/describing-responses/

I can see we can add error descriptions as per: https://paperclip.waffles.space/actix-status-codes.html

Looking at the code doesn't look like we can do it but I'll ask anyway in case I missed it: Is it possible to add a type to the error? Can we specify a type for each status code?

tiagolobocastro commented 3 years ago

@dunnock would you happen to know?

tiagolobocastro commented 3 years ago

Ok so after having a better look, looks like it really doesn't exist. Missing things:

  1. ability to add error codes not just to an error type but to a handler Why? If we have a generic error type, will all the possible error codes, then all handlers will report an error response which may not make sense for a given handler.
  2. ability to associate an error type with the status code/description pair which gets added to the response, allowing us to send back errors with additional information in the body

Any thoughts?

EDIT: hmm the "response" comes from the returned type, eg: Result<T, E>, which means we may not be able to retrieve the errors codes from the handler attributes. But that's probably ok, as the operation() is implemented for the handler anyway so we should be able to get extra error info from there.

tiagolobocastro commented 3 years ago

I got a noddy example going with adding response codes to the handler. I wonder if that work in isolation, as in, not related to the api_v2_errors, or if that should work as a "filter".

Eg, if no responses are specified in the handler then you get all the error codes from your error type; if response codes are specified in the handler then those codes are the only ones from the error type that actually make it into the spec? (inheriting the description and whatever schema the error type has)

tiagolobocastro commented 3 years ago

I've raised #307 to experiment. Here's how the error type could work:

#[api_v2_errors(
    code = 400,
    code = 401,
    description = "Unauthorized: Can't read session from header",
    code = 500,
    scheme = "ErrorFormat",
    code = 404,
   // default_scheme = "ErrorFormat
)]
pub struct Error { ... }

And of course Error format must impl the schema:

#[derive(Apiv2Schema)]
pub struct ErrorFormat { ... }

The resulting response:

"responses": {"200":{"description":"OK","schema":{"type":"array","items":{"$ref":"#/definitions/Nexus"}}},"201":{},"400":{"description":"Bad Request"},"401":{"description":"Unauthorized: Can't read session from header"},"404":{"description":"Not Found"},"500":{"schema":{"$ref":"#/definitions/ErrorFormat"}}
cydergoth commented 3 years ago

👍

cydergoth commented 3 years ago

Would it be possible to do something like the overloads for Json like JsonCreated used in response part?

tiagolobocastro commented 3 years ago

I haven't really used those Json types, but do you mean having an Error wrapper type for the actual error, eg: Response<Json<Pet>, **Error**<PetError>> ?

cydergoth commented 3 years ago

Yes, something like that. If I've understood properly the Json and CreatedJson etc classes are basically type wrappers which provide more type information hints to the introspection engine.

cydergoth commented 3 years ago

In the error case it may need to be a union object of some type because of the possibility of returning one of several different codes

tiagolobocastro commented 3 years ago

It should be possible to provide a macro that receives the codes/descriptions/schemes and creates a type similar to those you mentioned, eg:

    #[api_v2_error_overlay(
        400, 401, 403
    )]
    #[derive(Debug)]
    struct MyErrorOverlay(pub PetError)

    #[api_v2_operation]
    async fn echo_pet_with_errors(body: web::Json<Pet>) -> Result<web::Json<Pet>, MyErrorOverlay> { }

The inner type must implement Serialize, ErrorResponse and perhaps even Apiv2Error.

tiagolobocastro commented 3 years ago

Here's my wip: https://github.com/wafflespeanut/paperclip/pull/307/commits/097de616bf7aa379812a369e39f7da8d916d8c8d. It works, now we I just need to filter out the error codes. We may not even need a proc-macro for this, and just make it similar to those macros you mentioned, by passing in a name and a list of the error codes.

tiagolobocastro commented 3 years ago

The alternative macros: One for a specific type and another for a generic

macro_rules! api_v2_error_overlay_filter_typed {
        ($error:ident - $($status:literal)+ => $name:ident) => {
            #[derive(Debug)]
            struct $name(pub $error);

            impl std::fmt::Display for $name {
                fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
                    std::fmt::Display::fmt(&self.0, f)
                }
            }

            impl actix_web::error::ResponseError for $name {
                fn status_code(&self) -> actix_web::http::StatusCode {
                    self.0.status_code()
                }
                fn error_response(&self) -> actix_web::HttpResponse {
                    self.0.error_response()
                }
            }

            impl paperclip::v2::schema::Apiv2Errors for $name {
                const ERROR_MAP: &'static [(u16, &'static str)] = &[];
                fn update_error_definitions(op: &mut paperclip::v2::models::DefaultOperationRaw) {
                    $error::update_error_definitions(op);
                    for code in &[ $($status,)+ ] {
                        op.responses.remove(&code.to_string());
                    }
                }
                fn update_definitions(
                    map: &mut std::collections::BTreeMap<
                        String,
                        paperclip::v2::models::DefaultSchemaRaw,
                    >,
                ) {
                    $error::update_definitions(map);
                }
            }
        };
    }
    macro_rules! api_v2_error_overlay_filter {
        (- $($status:literal)+ => $name:ident) => {
            #[derive(Debug)]
            struct $name<T: actix_web::error::ResponseError + paperclip::v2::schema::Apiv2Errors>(pub T);

            impl<T: actix_web::error::ResponseError + paperclip::v2::schema::Apiv2Errors> std::fmt::Display for $name<T> {
                fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
                    std::fmt::Display::fmt(&self.0, f)
                }
            }

            impl<T: actix_web::error::ResponseError + paperclip::v2::schema::Apiv2Errors> actix_web::error::ResponseError for $name<T> {
                fn status_code(&self) -> actix_web::http::StatusCode {
                    self.0.status_code()
                }
                fn error_response(&self) -> actix_web::HttpResponse {
                    self.0.error_response()
                }
            }

            impl<T: actix_web::error::ResponseError + paperclip::v2::schema::Apiv2Errors> paperclip::v2::schema::Apiv2Errors for $name<T> {
                const ERROR_MAP: &'static [(u16, &'static str)] = &[];
                fn update_error_definitions(op: &mut paperclip::v2::models::DefaultOperationRaw) {
                    T::update_error_definitions(op);
                    for code in &[ $($status,)+ ] {
                        op.responses.remove(&code.to_string());
                    }
                }
                fn update_definitions(
                    map: &mut std::collections::BTreeMap<
                        String,
                        paperclip::v2::models::DefaultSchemaRaw,
                    >,
                ) {
                    T::update_definitions(map);
                }
            }
        };
    }

    api_v2_error_overlay_filter_typed!(PetError2 - 600 700 => PetErrorOverlay2);
    api_v2_error_overlay_filter!(-600 700 => PetErrorOverlay3);
tiagolobocastro commented 3 years ago

@dunnock any input on whether this should be a proc-macro(https://github.com/wafflespeanut/paperclip/commit/097de616bf7aa379812a369e39f7da8d916d8c8d) or macro_rules macro (last comment)? And should we use a specific type or generic, or both.