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

Support APIReponse on resource class #1122

Merged
merged 3 commits into from
May 8, 2022

Conversation

Azquelt
Copy link
Contributor

@Azquelt Azquelt commented Apr 12, 2022

Scan for APIResponse annotations on resource classes and merge any
responses with those on the method.

APIResponse annotations on the method override APIResponse annotations
on the resource class with the same response code.

Implementation for eclipse/microprofile-open-api#524 I've opened this as a draft against the jakarta branch because it requires API updates planned for MP OpenAPI 3.1.

phillip-kruger
phillip-kruger previously approved these changes Apr 12, 2022
Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @MikeEdgar please also have a look :)

@MikeEdgar
Copy link
Member

I think this looks good also. This can target main once it's repurposed for the MP 3.1 milestone.

@Azquelt
Copy link
Contributor Author

Azquelt commented Apr 19, 2022

I've pushed another commit to support @APIResponse on exception mapper classes, as that requirement was added to the spec PR.

I had a question while writing this: what's your approach for handling when the user does something invalid or ambiguous? In this case it's possible to put @APIResponse on both the class and the method, or even to add the annotation multiple times in one place since it's repeatable. Should there be an error or warning if the user does this? How is this handled in the rest of the project?

@MikeEdgar
Copy link
Member

I had a question while writing this: what's your approach for handling when the user does something invalid or ambiguous? In this case it's possible to put @APIResponse on both the class and the method, or even to add the annotation multiple times in one place since it's repeatable. Should there be an error or warning if the user does this? How is this handled in the rest of the project?

Typically in situations like that, one of the options is the one that is more "specific". Although in this case putting the annotation on the class is more in alignment with ExceptionMappers, I suppose the method is still "closer" to the result. Even in this situation, I think it's still valid for the mapper to return multiple status codes depending on the internal logic of toResponse.

@Azquelt
Copy link
Contributor Author

Azquelt commented Apr 21, 2022

I think it's still valid for the mapper to return multiple status codes depending on the internal logic of toResponse.

I hadn't considered that but I guess there's no reason an ExceptionMapper couldn't pick the response based on data from the exception. I'll try reworking this to process all annotations present on the class and method rather than just the first.

@Azquelt Azquelt force-pushed the api-response-on-class branch from df2615a to 10f10b6 Compare April 22, 2022 17:36
@Azquelt Azquelt changed the base branch from jakarta to main April 22, 2022 17:36
@Azquelt Azquelt dismissed phillip-kruger’s stale review April 22, 2022 17:36

The base branch was changed.

@Azquelt
Copy link
Contributor Author

Azquelt commented Apr 22, 2022

I've rebased this on main and added a further commit to support multiple @APIResponse annotations on an exception mapper.

@Azquelt Azquelt marked this pull request as ready for review April 22, 2022 17:37
@MikeEdgar
Copy link
Member

Thanks @Azquelt . Once the MP 3.1 snapshot includes this, we can double-check here that they pass before merging.

@MikeEdgar MikeEdgar added this to the 3.1.0 milestone Apr 22, 2022
@MikeEdgar MikeEdgar force-pushed the api-response-on-class branch from 10f10b6 to b10a7e0 Compare April 26, 2022 23:41
Comment on lines +382 to +383
if (!this.responseCodeExistInMethodAnnotations(context, exMapperApiResponseAnnotation,
methodApiResponseAnnotations)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we would allow an exception mapper to override class-level responses, but not method responses. This makes sense I think 👍

Comment on lines +265 to +271
// public static AnnotationInstance getResponseAnnotation(final ClassInfo classInfo) {
// return classInfo.classAnnotation(ResponseConstant.DOTNAME_API_RESPONSE);
// }
//
// public static AnnotationInstance getResponseAnnotation(final MethodInfo method) {
// return method.annotation(ResponseConstant.DOTNAME_API_RESPONSE);
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove entirely?

Azquelt added 3 commits April 26, 2022 20:06
Scan for APIResponse annotations on resource classes and merge any
responses with those on the method.

APIResponse annotations on the method override APIResponse annotations
on the resource class with the same response code.
Read APIResponse annotations from ExceptionMapper classes as well as
from their toResponse methods.
If the APIResponse annotation is used more than once on an exception
mapper, use all annotations to generate responses rather than just the
first one.
@MikeEdgar MikeEdgar force-pushed the api-response-on-class branch from b10a7e0 to 8d60342 Compare April 27, 2022 00:06
@MikeEdgar
Copy link
Member

New APIResponse TCK tests are passing. @Azquelt let me know when you're ready for this to merge. Just one thing to possibly look at with the commented code please. Thank you!

@MikeEdgar
Copy link
Member

@Azquelt I think this may be good to merge as-is. Let me know if you're planning any other updates on this one.

@MikeEdgar MikeEdgar removed this from the 3.1.0 milestone May 7, 2022
@MikeEdgar MikeEdgar merged commit 8d60342 into smallrye:main May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants