jakartaee / rest

Jakarta RESTful Web Services
Other
353 stars 114 forks source link

TCK tests using JAXB without xml_binding tag #1109

Closed starksm64 closed 4 months ago

starksm64 commented 2 years ago

In looking at running the TCK against a Core Profile implementation that does not include JAXB support, I found the following tests executing even when excludedGroups=xml_binding was specified:

ee.jakarta.tck.ws.rs.api.rs.core.linkjaxbadapter.JAXRSClientIT#marshallTest/unmarshallTest

brideck commented 2 years ago

I don't think that these tests are necessarily a problem. The only dependency on XML Binding in them is on the test client side, not in the server. So if you work really hard to excise JAXB artifacts from the dependencies in your pom, you'll get something like this:

[ERROR]   JAXRSClientIT.marshallTest:112 Fault jakarta.xml.bind.JAXBException: Implementation of Jakarta XML Binding-API has not been found on module path or classpath.
 - with linked exception: [java.lang.ClassNotFoundException: org.glassfish.jaxb.runtime.v2.ContextFactory]

We should handle all of the other tests that I've brought up in the Core Profile release review, though, so we don't set a precedent that the rest of the implementers will feel compelled to try to follow.

XML Binding tests:

ee.jakarta.tck.ws.rs.jaxrs21.ee.sse.sseeventsink.JAXRSClientIT.jaxbElementTest
ee.jakarta.tck.ws.rs.jaxrs21.ee.sse.sseeventsource.JAXRSClientIT.jaxbElementTest/xmlTest
ee.jakarta.tck.ws.rs.spec.provider.standardhaspriority.JAXRSClientIT.*

Security tests:

ee.jakarta.tck.ws.rs.ee.rs.container.requestcontext.security.JAXRSClientIT.*
ee.jakarta.tck.ws.rs.ee.rs.core.securitycontext.basic.JAXRSBasicClientIT.*

The above lists are easily provable by disabling/removing the indicated features from your server environment and running the TCK.

The first set produces obvious java.lang.NoClassDefFoundError: jakarta/xml/bind/JAXBElement type errors in the server.

The second set fails with unexpected server responses due to running with UNSECURED|NOROLE||NOSCHEME| instead of the users/contexts that the tests expect.

starksm64 commented 2 years ago

In the core profile TCK update I'm working on, these tests won't be executed. I have run this reduced set without the resteasy JAXB configuration in the test pom, and the security configuration on the WildFly side and these are running:

[INFO] Results:
[INFO] 
[WARNING] Tests run: 2700, Failures: 0, Errors: 0, Skipped: 59

I have not stripped the JAXB classes from the server side. I'll publish an updated TCK dist with an updated section in the user guide on the RESTful setup for your review later today.

brideck commented 2 years ago

I saw your update, but it looks like you still didn't remove ee.jakarta.tck.ws.rs.spec.provider.standardhaspriority.JAXRSClientIT.* from the list of what's run for Core Profile. From looking at the test class, I can see that an effort was made to mark a single test method as requiring JAXB, but there's too much usage of JAXBElement throughout the test application so it can't initialize, at least in OL.

brideck commented 2 years ago

I'm also not sure that the proposed/implemented solution is the best one for the xml_binding issue. That is not strictly a Core Profile problem, XML Binding is optional full stop, not just in certain profiles, and the correct set of tests really needs to be identified in the TCK itself.

starksm64 commented 2 years ago

So the ee.jakarta.tck.ws.rs.spec.provider.standardhaspriority.JAXRSClientIT.readWriteJaxbProviderTest test that uses JAXBElement is marked with @Tag("xml_binding"). I wonder if the JAXBElement reference in that test method was changed to a fully qualified class name and not in the imports if that would avoid attempting to load JAXBElement during initialization.

starksm64 commented 2 years ago

Looking at how the compiler handles imports, it seems that it makes no difference whether a class is explicitly imported or used as a fully qualified type. This means that xml_binding tests need to be factored into a separate artifacts.

brideck commented 2 years ago

Right, in this case it's not enough to just mark the individual test method. The application itself has further references to JAXBElement. In the case of OL, it's the reference in ee.jakarta.tck.ws.rs.spec.provider.standardhaspriority.Resource that causes the servlet to fail to initialize.

starksm64 commented 2 years ago

So another workaround that will work for any profile would be to simply attach the jaxb api jar to the test archive using an org.jboss.arquillian.container.test.spi.client.deployment.ApplicationArchiveProcessor implementation. There is an example of this in this test project:

https://github.com/jakartaredhat/jaxbtestjar

We can discuss tomorrow in the tck call.

scottmarlow commented 2 years ago

Could a committer please add the TCK Challenge to this issue.

scottmarlow commented 2 years ago

Note that the XML Binding use in REST in EE 9.1, had keyword.properties entries to allow XML Binding tests to be excluded.

brideck commented 2 years ago

Here's our proposed TCK update: https://github.com/jakartaee/rest/pull/1117

I've verified that this successfully excludes the remaining tests with XML Binding dependencies and we get the expected results with Open Liberty. It would be a good thing for another implementation to also verify that everything works as expected.

starksm64 commented 2 years ago

That PR works for WildFly.

brideck commented 2 years ago

Groovy. So if we can get the REST group to formally approve this challenge, we can go ahead and get that update merged.

spericas commented 2 years ago

I'd like @jansupol to comment on this issue, I believe he is back from vacation next week.

brideck commented 2 years ago

For reference, this is the set of tests that was previously marked as xml_binding when this was part of the Platform TCK.

com/sun/ts/tests/jaxrs/jaxrs21/ee/sse/sseeventsource = jaxrs javaee_optional jaxrs_web_profile javaee_web_profile_optional xml_binding
com/sun/ts/tests/jaxrs/jaxrs21/ee/sse/sseeventsink = jaxrs javaee_optional jaxrs_web_profile javaee_web_profile_optional xml_binding
com/sun/ts/tests/jaxrs/api/rs/core/linkjaxbadapter = jaxrs javaee_optional jaxrs_web_profile javaee_web_profile_optional xml_binding
com/sun/ts/tests/jaxrs/spec/filter/interceptor = jaxrs javaee_optional jaxrs_web_profile javaee_web_profile_optional xml_binding
com/sun/ts/tests/jaxrs/spec/provider/overridestandard = jaxrs javaee_optional jaxrs_web_profile javaee_web_profile_optional xml_binding
com/sun/ts/tests/jaxrs/spec/provider/standardhaspriority = jaxrs javaee_optional jaxrs_web_profile javaee_web_profile_optional  xml_binding
com/sun/ts/tests/jaxrs/spec/provider/standardnotnull = jaxrs javaee_optional jaxrs_web_profile javaee_web_profile_optional xml_binding
com/sun/ts/tests/jaxrs/spec/provider/standardwithxmlbinding = jaxrs javaee_optional jaxrs_web_profile javaee_web_profile_optional xml_binding
com/sun/ts/tests/jaxrs/spec/provider/jaxbcontext = jaxrs javaee_optional jaxrs_web_profile javaee_web_profile_optional xml_binding
com/sun/ts/tests/jaxrs/spec/client/typedentitieswithxmlbinding = jaxrs javaee_optional jaxrs_web_profile javaee_web_profile_optional xml_binding

To summarize, the proposed change here restores the tags that were not ported over from the Platform TCK for the two SSE test classes and the linkjaxbadapter test. It also expands the tagging for the standardhaspriority test that was previously excluded. The standardhaspriority test could be refactored in a future release so that only the test actually intending to use XML Binding need be excluded.

starksm64 commented 2 years ago

@spericas @jansupol We could use an update on this as it is blocking completing the core profile going to ballot.

spericas commented 2 years ago

@starksm64 I believe @jansupol should be back from vacation tomorrow. Will ping him.

Emily-Jiang commented 2 years ago

@jansupol can you please take a look at the corresponding PR . It is just to tag the tests to a group so that it can be excluded from core profile. Since the change is so minor, can any committer approve it and merge in as this blocks Core Profile ballot. @spericas thoughts?

Emily-Jiang commented 2 years ago

Addressed by #1117

jansupol commented 2 years ago

I have commented on the PR. One test class seems irrelevant to the missing JAX-B to me.

jansupol commented 2 years ago

One test class seems irrelevant to the missing JAX-B to me.

I was wrong, I approved the PR.

starksm64 commented 1 year ago

So at this point a new service release of the tck is needed and a PR to update the jakarta.ee page via the https://github.com/jakartaee/specifications/ repo.

alwin-joseph commented 1 year ago

@starksm64 @brideck A new service release of Jakarta REST TCK is available at https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/jakarta-restful-ws-tck-3.1.1.zip which includes the latest changes in the TCK. Please use this bundle for testing.

scottmarlow commented 1 year ago

If this TCK challenge is accepted, could one of the committers please add a accepted label to it. Thanks!

jim-krueger commented 4 months ago

@starksm64 Can this issue be closed?

starksm64 commented 4 months ago

Yes