jakartaee / jsonb-api

Jakarta JSON Binding
https://eclipse-ee4j.github.io/jsonb-api/
Other
79 stars 39 forks source link

Standalone TCK shouldn't require CDI SE #326

Closed brideck closed 2 years ago

brideck commented 2 years ago

Open Liberty would like to run this TCK against what we ship to ensure that we've pulled our JSON implementations into the product correctly. However, because of the TCK's dependency on an SE implementation of CDI, we can't currently do this.

Attempts to run in this fashion result in the following exception:

java.lang.NoClassDefFoundError: jakarta/enterprise/inject/se/SeContainer
        at java.base/java.lang.Class.getDeclaredFields0(Native Method)
        at java.base/java.lang.Class.privateGetDeclaredFields(Class.java:3297)
        at java.base/java.lang.Class.getDeclaredFields(Class.java:2371)
        at org.junit.platform.commons.util.ReflectionUtils.getDeclaredFields(ReflectionUtils.java:1392)
        at org.junit.platform.commons.util.ReflectionUtils.findAllFieldsInHierarchy(ReflectionUtils.java:1150)
        at org.junit.platform.commons.util.ReflectionUtils.findFields(ReflectionUtils.java:1138)
        at org.junit.platform.commons.util.AnnotationUtils.findAnnotatedFields(AnnotationUtils.java:371)
        at org.junit.platform.commons.util.AnnotationUtils.findAnnotatedFields(AnnotationUtils.java:348)
        at org.junit.jupiter.engine.descriptor.ExtensionUtils.registerExtensionsFromFields(ExtensionUtils.java:99)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.prepare(ClassBasedTestDescriptor.java:148)
        at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.prepare(ClassBasedTestDescriptor.java:78)
        ...

There are two tests in the TCK (both found in https://github.com/eclipse-ee4j/jsonb-api/tree/master/tck/src/main/java/ee/jakarta/tck/json/bind/cdi/customizedmapping) that define fields of this type. Should there be tests that are verifying integration with other Jakarta EE specs in a standalone TCK that can't run in an EE environment?

scottmarlow commented 2 years ago

FYI https://www.eclipse.org/lists/jakartaee-platform-dev/msg03362.html

gurunrao commented 2 years ago

@brideck - We have observed two failures with JSONB test with Glassfish 7 Web Profile, are the two failure related to above failure? Failure details:


java.lang.IllegalStateException: No valid CDI implementation found
    at jakarta.enterprise.inject.se.SeContainerInitializer.findSeContainerInitializer(SeContainerInitializer.java:108)
    at jakarta.enterprise.inject.se.SeContainerInitializer.newInstance(SeContainerInitializer.java:99)
    at ee.jakarta.tck.json.bind.cdi.customizedmapping.adapters.AdaptersCustomizationCDITest.startContainer(AdaptersCustomizationCDITest.java:59)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)

CI run https://ci.eclipse.org/jakartaee-tck/job/10/job/jakarta-jsonb-tck-glassfish-run/18/org.glassfish$glassfish.jsonb-tck/testReport/

gurunrao commented 2 years ago

Adding to above comment: JSONB TCK tests pass FULL profile GF bundle, due to jar weld-se-shaded.jar in classpath - https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/glassfish-runner/jsonb-tck/pom.xml#L90, same jar is missing from Web Profile GF 7 bundle.

brideck commented 2 years ago

Yes, this is exactly the sort of failure that I'm talking about. Whereas GF ships a weld-se implementation that it can add to the classpath, Open Liberty does not, and should not be required to -- EE is not a superset of SE.

scottmarlow commented 2 years ago

@lukasj this started as a question but implementations like GlassFish 7 Web Profile implementation do not include the weld-se-shaded.jar artifact which causes the TCK test to fail.

The same problem will happen for a Core Profile implementation that doesn't include CDI SE implementation (e.g. weld-se-shaded.jar). Please update this issue to make into a TCK challenge so we can start addressing it. Thank you!

lukasj commented 2 years ago

@scottmarlow I'm not a committer on this project, so there is really not much I can do.

@Verdent can you take a look?

lukasj commented 2 years ago

@scottmarlow btw I really do not think that maven is the best tool to run TCK tests against some ZIP binary. For that, I used gradle in parsson, where I can easily mix maven dependencies with local ones and alter all paths as necessary (see this if you're interested)

scottmarlow commented 2 years ago

@scottmarlow I'm not a committer on this project, so there is really not much I can do.

My bad, I should of looked at https://projects.eclipse.org/projects/ee4j.jsonb/who

@Verdent can you take a look?

David, could you please add the challenge label to make this an official TCK challenge. Thank you!

Verdent commented 2 years ago

@scottmarlow I do understand the problem, but JSONB spec also declares following Implementations must provide a CDI support in serializers/deserializers to allow injection of CDI managed beans into it. Because of this declaration, the TCK tests for CDI support should be in place from my point of view. CDI itself is optional for the impl run, but this should verify its support if it is present.

brideck commented 2 years ago

@scottmarlow I do understand the problem, but JSONB spec also declares following Implementations must provide a CDI support in serializers/deserializers to allow injection of CDI managed beans into it. Because of this declaration, the TCK tests for CDI support should be in place from my point of view. CDI itself is optional for the impl run, but this should verify its support if it is present.

This is what the jsonb/cdi tests that we left in the Platform TCK is designed to cover, too, right? If there's also a need to prove this function from an SE environment/perspective, then these tests could just be excluded from executions looking to supplement an EE certification effort. If CDI itself is optional, then there should also be a way to make the related tests in the TCK optional, no? Note: I fully understand that I could just pull in something like weld-se from Maven to make this all work, but should that be a requirement?

lukasj commented 2 years ago

If CDI itself is optional

let's get an answer to this question first. Does the spec say anywhere that CDI is optional or if CDI is available or similar thing to make it clear that CDI may not be available at runtime and when it is missing it is still fully compliant API/spec/impl combination?

Verdent commented 2 years ago

No, spec does not explicitly say, CDI is optional. Only that it supports CDI.

scottmarlow commented 2 years ago

From the Core Profile Specification https://github.com/eclipse-ee4j/jakartaee-platform/blob/master/specification/src/main/asciidoc/coreprofile/CoreProfileDefinition.adoc:

The Java SE section of the CDI 4.0 specification is not required for Core Profile implementations. Only Full CDI implementations are required to support the (CDI) Java SE API classes.

Also from the same Core Profile link, Jakarta JSON Binding 3.0 is a required component.

I think that Core Profile 10 implementations should have a way to pass the JSON-B 3.0 TCK without being required to implement jakarta.enterprise.inject.se.SeContainer.

As far as Jakarta EE 10 Full Platform implementations go though, I think they do need to provide the jakarta.enterprise.inject.se.SeContainer implementation as per guidance in https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html:

CDI implementations in Jakarta EE containers are required to support CDI Full.
brideck commented 2 years ago

As far as Jakarta EE 10 Full Platform implementations go though, I think they do need to provide the jakarta.enterprise.inject.se.SeContainer implementation as per guidance in https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html:

CDI implementations in Jakarta EE containers are required to support CDI Full.

The CDI specification also states:

CDI implementations that support the Java SE API are required to support CDI Full.

The wording here of "that support" implies that not every valid CDI implementation needs to support Java SE at all. I read all of the above to mean:

EE Core Profile = CDI Lite EE = CDI Full SE (if supported by implementation) = CDI Full

The statement in the draft Core Profile spec doc about Full CDI implementations being required to support SE seems like an overreach compared to what's in the CDI spec itself.

It would be useful if a CDI spec expert chimed in with what the intention was.

starksm64 commented 2 years ago

@brideck is correct in that support for SE requires support for Full, but support for Full does not require SE. The CDI TCK userguide examples for running against WildFly have the following group exclusions:

Full Java EE Platform, excluded groups = se Web Profile, excluded groups = javaee-full,se

So in both the Web Profile and Full Platform, Java SE related tests are excluded.

brideck commented 2 years ago

So in both the Web Profile and Full Platform, Java SE related tests are excluded.

I noticed that as well, but I also noticed that the 4.0 TCK user guide does not exclude the 'se' tests in section 4.5, showing how to run for Web Profile. Likewise, the user guide is completely silent on how to go about running for Full Platform (implying that you shouldn't need an exclude statement at all?).

It would seem that this is not a well-understood situation.

starksm64 commented 2 years ago

Ok, I see. I have created this CDI TCK issue to address that: https://github.com/jakartaee/cdi-tck/issues/378

As I said in that issue, it really does not make sense for Full Platform/Web Profile implementations to imply anything about the Java SE bootstrap behavior because that is a completely separate startup behavior that cannot be configured using the same Arquillian container.

starksm64 commented 2 years ago

The proposed userguide update PR is: https://github.com/jakartaee/cdi-tck/pull/379

Ladicek commented 2 years ago

Hi,

copying my reply to cdi-dev here:

the structure of the CDI specification is as follows:

  • CDI core (consists of CDI Lite and CDI Full, where Lite is a subset of Full)
  • CDI SE
  • CDI EE

Standalone implementations of CDI Lite indeed do not have to support CDI SE (and cannot, because of some methods in the SE API; we could define a subset of the CDI SE API that can be implemented under the Lite constraints, but didn't yet).

Implementations of CDI SE and CDI EE must support Full, as stated at the very beginning of the "CDI in Java SE" and "CDI in Jakarta EE" sections of the CDI specification.

Now, is it possible for standalone implementations of CDI Full to exist, without support of SE (or EE)? I don't think we ever considered that question, but I don't see anything preventing that. In any case, I don't think that matters in the JSON-B TCK issue. My understanding is that Jakarte Core Profile includes CDI Lite and not CDI Full, which implies that Jakarta Core Profile does not include CDI SE (and cannot, at the moment, as described above).

My recommendation for the JSON-B TCK would hence be: if you want tests to exercise some CDI SE or EE integration, those should be in separate groups that can be excluded. If you use the CDI SE API to bootstrap a CDI container in order to verify CDI Lite integration, that should most likely become a TCK SPI that integrators have to implement in any way they wish (because CDI Lite doesn't have a bootstrapping API).

scottmarlow commented 2 years ago

FYI the two tests being challenged are:

Are we ready to accept that these tests should be excluded in a https://download.eclipse.org/jakartaee/jsonb/3.0/jakarta-jsonb-tck-3.0.1.zip release? If yes, please mark the challenge as accepted.

Verdent commented 2 years ago

I think removal is not correct in this case. These test are needed there for certification reasons etc. What could be done, is to flag them in JUnit, so they can be easily excluded out of the run if needed. Or do something similar, but I do not think removing it is the way to go.

scottmarlow commented 2 years ago

So the tests would default to be run but a JSON-B implementation certification request would still be accepted if their test results are missing these two (challenged) CDI SE tests.

starksm64 commented 2 years ago

There needs to be a Junit CDI_SE group that can be excluded for certification of profiles/platform implementations that don't require the CDI Java SE support.

rmannibucau commented 2 years ago

While it is fine to run in standalone, it is a vendor subset of jsonb which requires cdi so test must pass with cdi but not use CDI SeContainer/Initializer since requirement is for its runtime not to bootstrap CDI.

Side note: CDI Lite is NOT required by JSON-B and must not be enforced for now.

scottmarlow commented 2 years ago

https://github.com/eclipse-ee4j/jakartaee-tck/pull/1062 shows how the (TCK) user can exclude the JSON-B CDI_SE tests via maven-surefire-plugin exclude. If The JSON-B team decides to accept this as a valid workaround for this challenge, then the challenge could be marked as accepted with the workaround being:

      <plugin>
        <artifactId>maven-surefire-plugin</artifactId>
        <version>3.0.0-M5</version>
        <configuration>
          <excludes>
             <exclude>ee.jakarta.tck.json.bind.cdi.customizedmapping.adapters.AdaptersCustomizationCDITest,ee.jakarta.tck.json.bind.cdi.customizedmapping.serializers.SerializersCustomizationCDITest</exclude>
          </excludes>
          </configuration>
      </plugin>
m0mus commented 2 years ago

Folks, I don't see it as a challenge. You are just trying to use JSONB TCKs a way they are not designed to be used. See my comment on Scott's PR: https://github.com/eclipse-ee4j/jakartaee-tck/pull/1062#issuecomment-1152200246

Short summary:

Don't run standalone tests on your platform implementation, run only the platform bundle. Standalone tests are designed to test JSONB implementation (not a PLATFORM implementation), they use JUnit and work on Java SE environment. You should certify your JSONB implementation separately. If you use Yasson, you don't need to do it because it's certified already.

m0mus commented 2 years ago

After doscussing this challenge with @scottmarlow and @brideck we agreed that everything works as expected. I am not accepting the challenge. Although everything works as expected it may not be a convenient for all use cases. I created an issue for CDI tests redesign which we will address later: https://github.com/eclipse-ee4j/jsonb-api/issues/331

scottmarlow commented 2 years ago

Just to clarify, the JSON-B Standalone TCK tests currently do need a SE_CDI implementation to pass, like Weld. :+1: for addressing this later!