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

FISH-161 OpenAPI Class Data Reading using HK2 Class Model API #4758

Merged
merged 10 commits into from
Jul 6, 2020
Merged

FISH-161 OpenAPI Class Data Reading using HK2 Class Model API #4758

merged 10 commits into from
Jul 6, 2020

Conversation

jGauravGupta
Copy link
Contributor

@jGauravGupta jGauravGupta commented Jul 2, 2020

FISH-161 OpenAPI Class Data Reading using HK2 Class Model API

Description

This is a feature that replaces OPEN API class metadata reading from reflection API to HK2 Class Model API (underlying processing with ASM Visitor API)

Important Info

Depends on PR

payara/patched-src-hk2#12
payara/patched-src-hk2#13
payara/patched-src-hk2#14
payara/Payara_PatchedProjects#316

Testing

Testing Performed

  • MicroProfile OpenAPI TCK
  • OpenAPI Impl unit tests
  • Manual testing with a sample application

Testing Environment

Windows 10, JDK 11.0

Notes for Reviewers

Exceptionally Classloading is needed to invoke the javax.ws.rs.core.Application#getClasses():
https://github.com/jGauravGupta/Payara/blob/f491727a90f87230faa0a45a256b9ebf979dcf82/appserver/payara-appserver-modules/microprofile/openapi/src/main/java/fish/payara/microprofile/openapi/impl/visitor/OpenApiWalker.java#L274-L275

Also Fixes

https://payara.atlassian.net/browse/FISH-70
https://payara.atlassian.net/browse/FISH-92
https://payara.atlassian.net/browse/FISH-50

@jGauravGupta jGauravGupta requested review from MattGill98 and jbee July 2, 2020 09:22
@MattGill98
Copy link
Contributor

jenkins test please

Copy link
Contributor

@smillidge smillidge left a comment

Choose a reason for hiding this comment

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

My only concern is deployment time for applications that don't want or need openapi support. Is it possible to check that openapi is enabled before building all the metadata?

I haven't reviewed the code to understand whether there is a performance impact for EJB jars and other applications that don't contain any REST endpoints.

Is it possible to do a "fast" check that the application has REST components before doing further processing.

@jGauravGupta
Copy link
Contributor Author

Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

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

I think keeping the AnnotationModel to MP model conversion in an extra method createInstance is a good way to keep things separated. I even would go as far as moving all the conversion outside of the MP model types into a utility class only does that.

It might even be a good idea to wrap the AnnotationModel API so that the conversion isn't directly based on these abstractions to decouple from the concrete types and to have a place (in the wrapper) to put some convenience methods to avoid repeating logic and get rid of parameters that are fixed in this context. I especially think about all the annotation.getValue calls. What about if expected values do not exist? Are we settings null or rather empty string (as usually default in annotations). Things like that.

I feel the code processing application types as Type could operate on Class but I might be wrong.

As far as I can tell the way the class processing is hooked into the deployment makes it eagerly evaluated which isn't good. I would think one can make this so that the metadata is computed on demand when OpenAPI actually needs it.

@jGauravGupta
Copy link
Contributor Author

What about if expected values do not exist? Are we settings null or rather empty string (as usually default in annotations).

if defaultValue not passed to the function AnnotationModel#getValue then the annotation type default value is returned.

<T> T getValue(String key, Class<T> type, Object defaultValue);
<T> T getValue(String key, Class<T> type);

See: https://github.com/payara/patched-src-hk2/blob/2.6.1.payara-maintenance/class-model/src/main/java/org/glassfish/hk2/classmodel/reflect/AnnotationModel.java#L47-L66

As far as I can tell the way the class processing is hooked into the deployment makes it eagerly evaluated which isn't good. I would think one can make this so that the metadata is computed on-demand when OpenAPI actually needs it.

Yes, HK2 Class Model processing is always hooked with application deployment and used by the annotation processor during application deployment.
Now, we are also storing the Class Model processed info and OpenAPI processing is performed on-demand on access to /openapi endpoint.

Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

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

Looks good. It would be nice if you could link or mention the issues this fixes that you mentioned.

@MarkWareham

This comment has been minimized.

@jGauravGupta

This comment has been minimized.

@jGauravGupta jGauravGupta merged commit 416ee1e into payara:master Jul 6, 2020
@jGauravGupta jGauravGupta added this to the 5.2020.3 milestone Jul 6, 2020
Cousjava pushed a commit to Cousjava/Payara that referenced this pull request Jan 27, 2021
FISH-161 OpenAPI Class Data Reading using HK2 Class Model API
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.

5 participants