papsign / Ktor-OpenAPI-Generator

Ktor OpenAPI/Swagger 3 Generator
Apache License 2.0
243 stars 42 forks source link

Can't catch errors of path params #28

Closed SerVB closed 4 years ago

SerVB commented 4 years ago

When I do http://localhost:8080/my/dassda , I want to see a bad request, but I get I received 0. Revision I use is be025c92. Is there a way to handle bad params?

@Path("{iii}")
data class MyParam(@PathParam("I'm int") val iii: Int)

route("my") {
    throws(
        status = HttpStatusCode.BadRequest,
        example = "bad request",
        exClass = Throwable::class
    ) {
        get<MyParam, String> { param ->
            respond("I received ${param.iii}")
        }
    }
}
Wicpar commented 4 years ago

Due to the lack of a proper way to handle errors generated by the library i preferred going the default value route of handling bad values. Currently you have to declare it nullable and check for null to throw the appropriate error.

I am still unsure what would be a good way to handle the exceptions, but just throwing the exceptions is unacceptable. Maybe a route module that has a function to generate the error, but then how do you differentiate the errors ? How do you make proper error codes ? How do you handle error payloads with the codes ? The goal is to make the default be the less damaging if overlooked. A bad value generating a default value is better than an unhandled error because every web service will handle a wrong value, but not an exception thrown by the library itself, which would be way harder to debug.

SerVB commented 4 years ago

So I should do something like the following?

@Path("{iii}")
data class MyParam(@PathParam("I'm int") val iii: Int?)

route("my") {
    throws(
        status = HttpStatusCode.BadRequest,
        example = "bad request",
        exClass = Throwable::class
    ) {
        get<MyParam, String> { param ->
            requireNotNull(param.iii)

            respond("I received ${param.iii}")
        }
    }
}

Okay...

Wicpar commented 4 years ago

More like:

val validParam = iii ?: error("iii: invalid value, must be an int")
...

On a small scale it may seem easier to just throw a bad request but on bigger codebases, with a more complex error ecosystem you can't get away with only one handler. You want to generate error codes and payloads with relevant data so you can handle them properly in the front end without massive lookup tables that can break easily.

For now the nullable approach guarantees that whatever we come up with won't break existing code when we do implement a proper handling system.

Wicpar commented 4 years ago

Redirecting to #29

SerVB commented 4 years ago

I've just understood that if I declare such a field as nullable, it will be shown as nullable in Swagger. So I need to add some extra text like "if you pass nullable or incorrect value, 400 will be returned". So it's not good working workaround...

Wicpar commented 4 years ago

You can replace the primitive default values with a ParsingError throw in https://github.com/papsign/Ktor-OpenAPI-Generator/blob/reworked-model/src/main/kotlin/com/papsign/ktor/openapigen/parameters/parsers/converters/primitive/PrimitiveConverter.kt I can't work on it today as i am on a consulting mission.

Wicpar commented 4 years ago

You could also create a processor like this to remove the nullable and throw your custom error: https://github.com/papsign/Ktor-OpenAPI-Generator/blob/reworked-model/src/main/kotlin/com/papsign/ktor/openapigen/annotations/type/number/integer/min/MinProcessor.kt