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

MicroProfile Config 3.0 Support #23807

Merged
merged 17 commits into from Jun 11, 2022
Merged

MicroProfile Config 3.0 Support #23807

merged 17 commits into from Jun 11, 2022

Conversation

MattGill98
Copy link
Contributor

@MattGill98 MattGill98 commented Feb 15, 2022

This change supports the deployment of MicroProfile Config 3.0 compatible applications, and modifies GF full profile to pass the MP Config 3.0.1 TCK by using the Smallrye Config 3.0.0.RC1 implementation.

The Smallrye dependencies are currently inlined in the implementation module, with the org.glassfish.microprofile.config and io.smallrye.config packages exported. If further Smallrye dependencies are used in future, it may be helpful to export the common utilities to their own module to reduce bundle size. I've inlined the dependencies to improve performance rather than loading nested JARs, although I'm happy to change this if required.

A sniffer module has been created to enable the respective MP implementations when they are detected, although the module defers the specific deployment steps to Smallrye.

The TCK needed a few modifications which are detailed in the commit messages:

  • The TCK assumes that empty beans.xml files imply the all bean discovery mode. Since CDI 4.0 this is no longer the case, so all empty beans.xml files are replaced with a correct implementation by an Arquillian extension.
  • The TCK depends on some system properties, which an initial custom test is used to configure in a container neutral way.
  • The official Arquillian GlassFish connector throws all exceptions as GlassFishClientExceptions, which fails the deployment assertions. An Arquillian extension is used to cast these exceptions when the message is correct.
  • Hamcrest is required by some deployments, but not included. The Arquillian extension handles this.

@dmatej
Copy link
Contributor

dmatej commented Feb 15, 2022

Wow, that is a nice work! I will first fix my PR and then I will try to help you to fix them (if it would be needed) ;)

@arjantijms
Copy link
Contributor

Great work indeed @MattGill98!

Maybe though take into consideration alignment with Piranha Cloud:

OmniConfig packages MP Config from Helidon, while JWT is the one I coded at Payara, somewhat repackaged to be reusable (it works in Tomcat too etc when the right dependencies are present).

That said, regarding things like Tracing, SmallRye is already reusable out of the box. For Helidon some work may be needed for that. Piranha Cloud did use SmallRye for MP Config before.

@jansupol
Copy link
Contributor

Added CQ for Helidon Config 3.0.0-M1
Added CQ for Smallrye Config 3.0.0-RC1
Glassfish can piggybag.

@jansupol
Copy link
Contributor

Helidon Config requires JDK 17, Smallrye JDK 8.

@arjantijms
Copy link
Contributor

@jansupol

Glassfish can piggybag.

Thanks!

Btw, how do you keep track of which dependency has been CQ'ed and which one has not been? Or is it easier to just CQ everything and if it already has been CQ'ed ipzilla will let you know?

Is there a tool available that simply CQ's every dependency from a project, or do you manually create the bugs for each one of them?

@jansupol
Copy link
Contributor

They have a query https://dev.eclipse.org/ipzilla/query.cgi for the CQs.

@jansupol
Copy link
Contributor

I try to CQ everything I cannot find the CQ for. There is no tool I am aware of, some questions are complicated, the repository of the source code, all the licenses, description of the dependency, project home page, this information can be difficult to get automatically, I grab them from multiple sources. It is easier when a newer version is CQed, most of the information can be used from the previous CQ...

This change creates a MP Config TCK runner module to run the TCK
against a managed instance of Glassfish. There are existing dependency
issues, which means there are a decent proportion of tests that don't
run correctly. At this point it's fine to test that the basic deployment
of MP Config apps works as expected.

Each MP TCK module will have its own instance of Glassfish, but will be
passed a common arquillian.xml.

The TCK requires a Weld impl to run due to the Arquillian CDI enricher.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
This change implements Smallrye Config as a GF container.
Currently Smallrye config and common are packaged together.
If other Smallrye libraries are used elsewhere, the common
library should be separated out into its own bundle.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
The sniffer can now exit early without type scanning if the archive contains
a microprofile-config.properties in the correct location.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
Hamcrest isn't loaded with the applications by default, despite
the deployed applications requiring it. This change adds an
Arquillian extension to deploy a Hamcrest dependency with the
apps.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
The TCK wrongly assumes that, like in CDI versions <4, the 'all'
bean discovery mode is assumed. GlassFish is now CDI 4.0 compliant,
which means that the 'all' bean discovery mode now needs to be
manually specified. To fix this, the arquillian extension now replaces
the beans.xml files with custom versions. This will need modifying if
the TCK contains any meaningful beans.xml tests.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
When using managed and remote clients, the Arquillian GF connectors throw
each deployment exception as a GlassfishClientException, with the body of
the actual exception. For the TCK to expect a certain exception type, we
need an extension to interpret the response as the correct type. This
change adds an extension to convert the "CDI deployment failure"
responses to jakarta namespaced DeploymentExceptions, as the server produces.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
This change separated the Arquillian extension behaviours
into their own separate classes for readability.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
System properties should ideally be configured via the arquillian
container. This isn't possible via the official managed extension,
so a workaround is needed. The options were either to configure it
via a remote Arquillian extension (which doesn't currently work as
Smallrye initialises its config sources before these extensions
run), or to run a new test which sets it up. This change creates a
new test purely to setup the container programmatically.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
The MP TCK uses ShouldThrowException to check some deployment failures.
By default, Arquillian will not apply the extension services to those
deployments. The beans.xml processor is still required in some cases
though, to force the container to create bean deployments for the app.
This change applies the processer during the BeforeDeploy event when
the archive is non testable.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
A beans.xml file appears in the TCK deployments in WEB-INF
as well as META-INF. This change adds the WEB-INF directory
to the extension.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
The OSGI bundle plugin was importing the classes directly based
on package. This was a problem, as Smallrye has multi-release
JARs which wouldn't have their versioned classes imported. This
change adds the Multi-Release instruction to the manifest, as
well as imports the Smallrye versioned classes to allow JDK9
compatibility.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
MicroProfile annotations are allowed to be injected into method parameters,
but the sniffer code currently tries to cast the parameter to a member
(i.e. field or method), which causes a ClassCastException. This change
modifies the sniffer code to allow the sniffer to find the class containing
the parameter annotation.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
Helidon may be a better fit in the long term for GlassFish.
The OSGI includes have been trimmed as much as possible to reduce
bundle size, and unnecessary OSGI instructions have been removed.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
Smallrye used a custom file loading mechanism that doesn't have this bug.
Since Helidon config uses JarURLConnection to load the microprofile-config.properties
file, applications with the same name will return the same cached file contents.

Disabling it server-wide would be a big performance hit, so instead this fix disables
the cache during the config provider initialisation, and then returns it to the default
behaviour. This shouldn't cause performance problems as I expect the file to only be
read from the same archive once.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
The config converter to convert config properties to application classes was broken,
as the implementation was using the server classloader (for GF this is a bundle
classloader). Thankfully Helidon discovers converters using the ServiceLoader, so
this change overwrites the default with a class converter that uses the webapp
classloader

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
Helidon config causes some deployment errors to present as a
"CDI definition failure", rather than a "CDI deployment failure".

The Arquillian extension to transform exceptions has been made
more generic to allow this.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
@MattGill98
Copy link
Contributor Author

I've swapped out Smallrye Config for Helidon config - had to make a few small changes (mostly relating to classloading) but in general very little needed changing

What else needs completing for this change to make it in?

@dmatej
Copy link
Contributor

dmatej commented Jun 10, 2022

Wait for a while with the merge, my yesterday PR broke something - servlet TCK doesn't pass at this moment.

@dmatej dmatej merged commit c822239 into eclipse-ee4j:master Jun 11, 2022
@dmatej dmatej added this to the 7.0.0 milestone Dec 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.

None yet

4 participants