bkbnio / kompendium

Ktor OpenAPI Spec Generator
https://bkbn.gitbook.io/kompendium
MIT License
144 stars 22 forks source link

Multiple exceptions same status code #95

Closed adrG2 closed 2 years ago

adrG2 commented 2 years ago

I have a problem using notarizedExceptions for multiple exceptions with same status code, for example:

Endpoint can throw IllegalArgumentException and InvalidFormatException. Both exceptions should return 400 but the generated swagger document only shows the last notarizedException(within the statusPages module) with that status code.

I found this information about it. In Kompendium you are not allowed to pass a set of exceptions to a single notarized exception.

If your code can throw multiple exceptions, you would have to catch all of them and throw a generic exception. This generic exception would be the one that catches the notarizedException and inside its ResponseInfo is where you would put the different exception examples.

This solution I think is invasive and forces you to change your error handling.

Any other alternative?

Thanks a lot

brizzbuzz commented 2 years ago

Hmm.... this is a good point. I think I was too much of an idealist assuming there would be a one to one relation between exceptions and status codes :)

I will try to find some time in the next couple days to dig in to this and try to come up with a good solution, but feel free to take a stab at it yourself if you'd like!

brizzbuzz commented 2 years ago

At the moment I have this targeted for V2, because I think this is going to require some level of breaking changes with the way that exceptions are currently dealt with.

brizzbuzz commented 2 years ago

I am pretty stuck on this 😞 There are a couple things that make designing a clean API here a bit difficult.

If you look at the current implementation

install(StatusPages) {
      notarizedException<IllegalArgumentException, ExceptionResponse>(
        info = ResponseInfo(
          status = HttpStatusCode.BadRequest,
          description = "Bad Things Happened",
          examples = mapOf("example" to ExceptionResponse("hey bad things happened sorry"))
        )
      ) {
        call.respond(HttpStatusCode.BadRequest, ExceptionResponse("Why you do dis?"))
      }

      notarizedException<InvalidFormatException, ExceptionResponse>(
        info = ResponseInfo(
          status = HttpStatusCode.BadRequest,
          description = "Bad Things also Happened",
          examples = mapOf("example" to ExceptionResponse("hey bad things also happened sorry"))
        )
      ) {
        call.respond(HttpStatusCode.BadRequest, ExceptionResponse("Why you do dis dude?"))
      }
    }

each exception defines its own distinct ResponseInfo.

What you allude to in your issue is the schema which is nested several layers down in the ResponseInfo. Allowing two distinct responses to be merged together just to allow the schema to be of type anyOf is not really feasible.

The reason this works in things like polymorphic examples is that I am able to pull together all of the types under the hood and aggregate them underneath a single Response type.

Back to exceptions, solving this would likely require a complete rewrite of the notarized exception code.

Even accepting that, I'm not really sure how to get this to play nice with the existing Ktor status page system. Because Ktor expects a 1-to-1 relationship between exceptions and handlers, we need to have 1 notarized exception per handler, so then there is a weird situation where we have multiple "handlers" each defining their own schema, yet would need to define something else entirely to shape the overall response itself.

So all in all... there is nothing technical preventing this from being done, but in terms of doing so with a clean, intuitive API, I am having trouble coming up with a good solution.

brizzbuzz commented 2 years ago

Update: I ended up needing to completely rewrite exception notarization as part of my move to a plugin based architecture anyway, so this will be fixed in V2 :)

adrG2 commented 2 years ago

I am pretty stuck on this 😞 There are a couple things that make designing a clean API here a bit difficult.

If you look at the current implementation

install(StatusPages) {
      notarizedException<IllegalArgumentException, ExceptionResponse>(
        info = ResponseInfo(
          status = HttpStatusCode.BadRequest,
          description = "Bad Things Happened",
          examples = mapOf("example" to ExceptionResponse("hey bad things happened sorry"))
        )
      ) {
        call.respond(HttpStatusCode.BadRequest, ExceptionResponse("Why you do dis?"))
      }

      notarizedException<InvalidFormatException, ExceptionResponse>(
        info = ResponseInfo(
          status = HttpStatusCode.BadRequest,
          description = "Bad Things also Happened",
          examples = mapOf("example" to ExceptionResponse("hey bad things also happened sorry"))
        )
      ) {
        call.respond(HttpStatusCode.BadRequest, ExceptionResponse("Why you do dis dude?"))
      }
    }

each exception defines its own distinct ResponseInfo.

What you allude to in your issue is the schema which is nested several layers down in the ResponseInfo. Allowing two distinct responses to be merged together just to allow the schema to be of type anyOf is not really feasible.

The reason this works in things like polymorphic examples is that I am able to pull together all of the types under the hood and aggregate them underneath a single Response type.

Back to exceptions, solving this would likely require a complete rewrite of the notarized exception code.

Even accepting that, I'm not really sure how to get this to play nice with the existing Ktor status page system. Because Ktor expects a 1-to-1 relationship between exceptions and handlers, we need to have 1 notarized exception per handler, so then there is a weird situation where we have multiple "handlers" each defining their own schema, yet would need to define something else entirely to shape the overall response itself.

So all in all... there is nothing technical preventing this from being done, but in terms of doing so with a clean, intuitive API, I am having trouble coming up with a good solution.

Thank you for your time!

The temporary solution has been to use examples and exceptions only for documentation. Leaving the body of the notarized exception function empty.

notarizedException<UsersBadRequestException>(...) { }

In the examples map is where I place the types of exceptions such as: ParameterRequired or ErrorFormat.

Thanks a lot,

Greetings!

brizzbuzz commented 2 years ago

Ahh I'm glad you found a temporary work around :) will likely not get around to finishing up the V2 changes until the holidays are over. But I'm very excited for V2 🚀 lots of cool stuff coming

adrG2 commented 2 years ago

I'm quite hype about v2!

Feel free to contact me if you need a hand 😀

brizzbuzz commented 2 years ago

Would love your help and feedback if you have some time! Let me wrap up some stuff on the current v2 branch, I'll update all the documentation, and loop you in as we move it towards a beta state :) then we can see how it feels, what works, what doesn't and go from there!