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

Add response code to all endpoint when there are ExceptionMapper(s) defined #404

Closed
FreifeldRoyi opened this issue Jan 12, 2020 · 11 comments

Comments

@FreifeldRoyi
Copy link

FreifeldRoyi commented Jan 12, 2020

Hi
When adding annotations to all the endpoints I expose, it gets really cumbersome to copy response annotations that are defined in ExceptionMapper classes

I thought it would be better to annotate the ExceptionMapper definition and propagate it to all endpoints automatically

@EricWittmann
Copy link
Contributor

This is actually being worked on right now in SmallRye OpenAPI. Do you think a change to the spec is needed for this to be supported? Right now it's not something specifically called out by the spec, which leaves it up to each implementation. Or we could update the SDK to cover this.

Which implementation are you currently using?

@FreifeldRoyi
Copy link
Author

FreifeldRoyi commented Jan 13, 2020

I must say that I'm not too familiar with the spec so I'll just shoot whatever comes to mind.
I guess that @Schema (or something equivalent Say... @ErrorCode) needs to be added as an annotation. You may want to give it a name in order to add anignore argument (with a list of names) when describing your API, since not all REST calls will need the entire mapping sheet.
Or you can go with the less pessimistic flow and add errorCodeNames (instead of ignore) to incorporate the relevant mappings. The latter actually has a nice benefit since it's very clear when you read the code, instead of searching for ExceptionMapper(s) all around.

So that'll be a new annotation (or reuse an existing one) and add another field in ApiResponse to state the ignored or relevant error codes.

And I'm using SmallRye/Quarkus

@EricWittmann
Copy link
Contributor

Thoughts @MikeEdgar and @lamtrhieu - how does this compare to what's being worked on in SmallRye already?

@MikeEdgar
Copy link
Member

The main difference here is the linkage between the resource method and the ExceptionMapper. The changes in SmallRye use the method's throws to link the two rather than annotations. If the resource method declares the same responseCode that is present in the mapper, the resource response is the one that is used. We decided against doing anything with undeclared + unchecked exceptions for now.

@EricWittmann
Copy link
Contributor

Do you think there are any existing MP annotations that could be supported on Exception Mappers to improve support?

Note: I'm not against inventing new annotations in the spec to cover this use case if it's useful to do so. Which is why I marked this as a discussion topic for the next hangout.

@MikeEdgar
Copy link
Member

Nothing jumps out at me beyond the @APIResponse. There may be room to modify/add an annotation to explicitly provide for the linkage between resource methods and exception mappers.

@APIResponses(
    exceptionMappers = { My404Mapper.class, My500Mapper.class },
    value = { @APIResponse(responseStatus = "200", ...) })

I'm not sure it would be necessary in this case if the throws could be utilized, and both require access to code. It's also still a bit more verbose than desired.

It might also be interesting to allow an application to define their own annotation (similar to a CDI @Qualifier) that could be placed on both the response method and on the ExceptionMapper. The implementation's annotation scanner would include the response definitions from all mappers with the custom annotation matching the one(s) on the resource method.

@EricWittmann
Copy link
Contributor

Yeah that's interesting. Well, I think we can discuss this in more depth at the next hangout. Additionally, I'm hoping that @FreifeldRoyi will give the new SmallRye impl functionality a try. There is currently a PR here: smallrye/smallrye-open-api#227

Once merged, there will be some support for processing exception mappers and it would be great to get some feedback on it.

@FreifeldRoyi
Copy link
Author

I'll take a look
Thanks a lot!

@EricWittmann
Copy link
Contributor

We discussed this in the most recent spec hangout, and it was decided that we'll likely do the following two things:

  1. Update the javadoc for @Response and similar annotations (if any) to describe the behavior of the annotation when used on an ExceptionMapper
  2. Update the TCK to add tests for processing exception mappers appropriately

@EricWittmann
Copy link
Contributor

I'll be taking a swing at this work at some point. :)

@FreifeldRoyi
Copy link
Author

FreifeldRoyi commented Feb 25, 2020

I'm glad my proposal will be added to the spec 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants