FISH-327 Implement MicroProfile Sniffers #4966
Conversation
Implement a new base module for each MicroProfile implementation to extend to provide their sniffers. Signed-off-by: Matthew Gill <[email protected]>
OpenAPI implementation now uses a sniffer to determine if the application is appropriate for OpenAPI scanning. Signed-off-by: Matthew Gill <[email protected]>
Metrics now uses a sniffer to determine if the servlet or CDI extensions are necessary to deploy. Signed-off-by: Matthew Gill <[email protected]>
Only run health CDI extension and servlet when the sniffer detects a MP compatible application. Move CDI bean registration from the ServletContextListener into its own CDI extension. Signed-off-by: Matthew Gill <[email protected]>
Added a sniffer to only apply the CDI extension when the fault tolerance annotations have been detected. Signed-off-by: Matthew Gill <[email protected]>
Move the RolesDeclaredInitializer and CDI Extension behind the sniffer logic, to improve startup performance. Signed-off-by: Matthew Gill <[email protected]>
Only enable CDI extension when the @ConfigProperty annotation is detected. Signed-off-by: Matthew Gill <[email protected]>
|
jenkins test please |
There was a problem hiding this comment.
First impressions feedback...
Classes extending MicroProfileSniffer have some duplication. Instead of implementing the getters in each one with constants the abstract class could have fields holding the values returned by the getters which are initialised in the constructor.
The classes extending MicroProfileApplicationContainer all implement the methods with return true while this is already how it is implemented in the abstract class - seems to be a case of unnecessary override.
Not sure why the CdiExtention did not get named differently for the different standards while almost all other classes have their standard prefix. I think for consistency and less confusion these should likewise be named e.g. JwtAuthCdiExtension and so forth (some actually have a prefix I found).
I am not sure if the idea of a lazy bootstrapping of MP is even compatible with MP standards as standards do require you to throw DeploymentExceptions in certain situations. Would this still work with the indirection added?
Maybe you can add a section to the description explaining how the general concept of sniffer, deployer, container and so forth do their magic? What would happen before, what is happing now in comparison.
...modules/microprofile/config/src/main/java/fish/payara/microprofile/config/ConfigSniffer.java
Outdated
Show resolved
Hide resolved
...rc/main/java/fish/payara/microprofile/faulttolerance/FaultToleranceApplicationContainer.java
Outdated
Show resolved
Hide resolved
Connectors were incorrectly activating for all module types. This change checks properly for a web activated module, and logs a proper message if this logic fails. Signed-off-by: Matthew Gill <[email protected]>
|
jenkins test please |
The updated connector module wasn't being used, and a ClassLoading issue occurred when trying to inject WarType. WarType has been moved into an exported OSGI module to be used by the MicroProfile sniffers, and the correct module has been referenced in the POM. Signed-off-by: Matthew Gill <[email protected]>
JWT-Auth and Config had generic "CdiExtension" classes, which aren't identifiable at runtime. This commit renames them appropriately. Signed-off-by: Matthew Gill <[email protected]>
Signed-off-by: Matthew Gill <[email protected]>
|
jenkins test please |
MicroProfile sniffers were handling all WAR archives, which is incorrect behaviour. Signed-off-by: Matthew Gill <[email protected]>
|
jenkins test please |
|
jenkins test please |
1 similar comment
|
jenkins test please |
Signed-off-by: Matthew Gill <[email protected]>
The password alias store failed to be found when the OpenAPI container didn't call ConfigProvider.getConfig(). This change calls ConfigProvider.getConfig() in the config sniffer regardless of whether the Config API annotations are detected or not. Signed-off-by: Matthew Gill <[email protected]>
Each MP container overrides methods unnecessarily, so they've been stubbed in the parent class to minimise boilerplate. Signed-off-by: Matthew Gill <[email protected]>
This reverts a part of commit d007311. The ServletContainerInitializer is returned, as the ServletContextListener caused the OpenAPI endpoint to use the app context root. The OpenAPI endpoint is now always deployed, but the sniffer determines if the app should be read. Signed-off-by: Matthew Gill <[email protected]>
This partially reverts commit 023866b. The ServletContainerInitializer is returned, as the ServletContextListener caused the metrics endpoint to use the app context root. Signed-off-by: Matthew Gill <[email protected]>
This partially reverts commit 372c734. The ServletContainerInitializer is returned, as the ServletContextListener cased the health endpoint to use the app context root. Signed-off-by: Matthew Gill <[email protected]>
Moved the sniffer code for each module into their own folder for better organisation. Signed-off-by: Matthew Gill <[email protected]>
|
jenkins test please |
Extensions now load only once, and sniffers now enable for EJB JARs. Signed-off-by: Matthew Gill <[email protected]>
Signed-off-by: Matthew Gill <[email protected]>
There was a problem hiding this comment.
It is a lot of ceremony needed here but my best guess is that all of it is required.
Not sure you have commented on one of my previous comments on DeploymentException required by MP standards. I assume we would detect usage of MP and bootstrap it which then would cause the exception to be thrown as required?
...icroprofile/openapi/src/main/java/fish/payara/microprofile/openapi/impl/OpenAPISupplier.java
Show resolved
Hide resolved
|
jenkins test please |
A race condition existed where the load() method of each MP sniffer depended on the Weld load() method. If they ran in the wrong order, the MP CDI extensions wouldn't load. This makes sure that the Weld sniffer creates this MetaData earlier in the deploy lifecycle. Signed-off-by: Matthew Gill <[email protected]>
The Static OpenAPI YAML file wasn't being scanned for by the sniffer. Signed-off-by: Matthew Gill <[email protected]>
The extension logged errors about the creational context, so the healthchecks are now registered later (once this can be created correctly). The extension also wasn't listening on all health check types, so the observer has been made more generic. Signed-off-by: Matthew Gill <[email protected]>
Config applications that injected the Config type weren't previously detected, but they now count as Config applications. Signed-off-by: Matthew Gill <[email protected]>
The metrics TCK caused ClassCastExceptions casting from ParameterImpl to Type. ParameterizedTypes such as ParameterImpl are now recognised and handled correctly, and unrecognised types are logged and ignored. Signed-off-by: Matthew Gill <[email protected]>
Allow Metrics sniffer to deploy Metrics if any Metric is injected, as well as the enabled annotations. Signed-off-by: Matthew Gill <[email protected]>
|
jenkins test please |
Signed-off-by: Matthew Gill <[email protected]>
|
The previous commit passed Jenkins and this was just a copyright update, so I'm going to merge it to not hold up CI |
Merge pull request payara#4966 from MattGill98/FISH-327 Merge pull request payara#5130 from MattGill98/FISH-760 Merge pull request payara#5025 from MattGill98/FISH-863 5080 Payara logback libs produce NPE / FISH-1007 (payara#5082)
Description
This is an improvement. MicroProfile servlets and CDI extensions are now hidden behind a sniffer engine. This should mean that they won't startup unnecessarily, and should be a decent performance improvement for applications not utilising any or all of the MP specs.
Important Info
Blockers
Needs testing against large non-MP app.
Testing Performed
Tested the test app for FISH-242.
Passes MP TCKs.
Notes for Reviewers
Should also fix FISH-431.
Make sure 1038060 is still relevant