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

TCK Challenge: Improper use of final keyword in ee.jakarta.tck.ws.rs.spec.contextprovider.JsonbContextProviderIT$EchoApplication and ee.jakarta.tck.ws.rs.spec.client.exceptions.ClientExceptionsIT$StatusApplication #1126

Closed
brideck opened this issue Aug 12, 2022 · 23 comments

Comments

@brideck
Copy link

brideck commented Aug 12, 2022

Challenged Tests:
ee.jakarta.tck.ws.rs.spec.contextprovider.JsonbContextProviderIT
ee.jakarta.tck.ws.rs.spec.client.exceptions.ClientExceptionsIT

TCK Version:
Jakarta RESTful Web Services 3.1.x

Tested Implementation:
Open Liberty -- containing RestEasy 6.1.0 & Weld 5.0.1

Description:
The Application subclasses of both of the challenged tests are annotated with @ApplicationPath, which RestEasy (and in turn Open Liberty) defines as a bean-defining annotation for CDI (see resteasy/resteasy#3119). CDI Full supports the notion of implicit bean archives, which in section 3.10.1 of the CDI 4.0 specification is defined in part as "any other archive which contains one or more bean classes with a bean defining annotation."

Section 11.2.3 of the RESTful WS 3.1 specification states that "In a product that supports CDI, implementations MUST support the use of CDI-style Beans as root resource classes, providers and Application subclasses. Providers and Application subclasses MUST be singletons or use application scope." In this case, the test subclasses are Application subclasses, so our implementation treats them as having application scope.

Application scope is a normal scope, so per section 2.5.3 of the CDI 4.0 specification "a client proxy is required." And this is where we run into trouble. The tests currently fail with:

org.jboss.weld.exceptions.UnproxyableResolutionException: WELD-001437: Bean type class ee.jakarta.tck.ws.rs.spec.contextprovider.JsonbContextProviderIT$EchoApplication is not proxyable because it is final - <unknownjakarta.enterprise.inject.spi.Bean instance>.
	at org.jboss.weld.util.Proxies.getUnproxyableClassException(Proxies.java:236)
	at org.jboss.weld.util.Proxies.getUnproxyableTypeException(Proxies.java:199)
	at org.jboss.weld.util.Proxies.getUnproxyableTypeException(Proxies.java:162)
	at org.jboss.weld.bean.proxy.ClientProxyProvider.getClientProxy(ClientProxyProvider.java:238)
	at org.jboss.weld.manager.BeanManagerImpl.getReference(BeanManagerImpl.java:669)
	at org.jboss.weld.manager.BeanManagerImpl.getReference(BeanManagerImpl.java:698)
	at org.jboss.resteasy.cdi.CdiConstructorInjector.construct(CdiConstructorInjector.java:69)
	at org.jboss.resteasy.core.providerfactory.Utils.createProviderInstance(Utils.java:102)
	at org.jboss.resteasy.core.providerfactory.ResteasyProviderFactoryImpl.createProviderInstance(ResteasyProviderFactoryImpl.java:1399)
	at org.jboss.resteasy.core.ResteasyDeploymentImpl.createApplication(ResteasyDeploymentImpl.java:424)
	at org.jboss.resteasy.core.ResteasyDeploymentImpl.initializeObjects(ResteasyDeploymentImpl.java:271)
	at org.jboss.resteasy.core.ResteasyDeploymentImpl.startInternal(ResteasyDeploymentImpl.java:143)
	at org.jboss.resteasy.core.ResteasyDeploymentImpl.start(ResteasyDeploymentImpl.java:127)
	at org.jboss.resteasy.plugins.server.servlet.ServletContainerDispatcher.init(ServletContainerDispatcher.java:140)
	at org.jboss.resteasy.plugins.server.servlet.HttpServletDispatcher.init(HttpServletDispatcher.java:42)
	at com.ibm.ws.webcontainer.servlet.ServletWrapper.init(ServletWrapper.java:299)
        ...

Per section 2.2.10 of the CDI 4.0 specification:

The container uses proxies to provide certain functionality. Certain legal bean types cannot be proxied by the container:
  - classes which don’t have a non-private constructor with no parameters,
  - classes which are declared final,
  - classes which have non-static, final methods with public, protected or default visibility,

In this case, the Application subclasses in both of the challenged tests are declared final and the EchoApplication subclass in the JsonbContextProviderIT class also contains two methods that are declared final.

An implementation that only supports CDI Lite would likely not see a problem in this space, since section 2.11.1 of the CDI 4.0 specification states that an implicit bean archive must contain a beans.xml file, which these test applications do not possess. But any implementor of CDI Full that discovers implicit bean archives in the absence of beans.xml could run into this error, rendering these tests impossible to execute.

@jamezp
Copy link
Contributor

jamezp commented Aug 15, 2022

This makes sense to me to make a change to not make these final. Having a requirement to treat resources, providers and applications as CDI beans means we must adhere to the CDI rules. Making a CDI bean final is not allowed.

@jansupol
Copy link
Contributor

Seems related to #1097.

@jamezp
Copy link
Contributor

jamezp commented Aug 15, 2022

Seems related to #1097.

I'd agree with that. Not sure how I missed the final there :).

@jim-krueger
Copy link
Contributor

Will this change be included in TCK 3.1.2? I'm assuming that the "finals" will simply be removed vs. excluding the testcases.

@spericas
Copy link
Contributor

@jim-krueger That would be much preferable to excluding them in my view, but I'm not sure this is allowed by the rules. Can anyone confirm this either way?

@arjantijms
Copy link
Contributor

@spericas at the very least an exception can be requested to the spec committee (cc @ivargrimstad). We recently did this for Jakarta Concurrency.

@brideck
Copy link
Author

brideck commented Sep 6, 2022

Was there an updated version of the TCK coming that would exclude (or fix) tests for both this challenge and #1123?

@spericas
Copy link
Contributor

spericas commented Sep 6, 2022

@ivargrimstad Could we make the change described here without excluding the test? That is, get an exception as @arjantijms suggested.

@brideck
Copy link
Author

brideck commented Sep 13, 2022

FWIW, I can verify that making the changes suggested here work for Open Liberty:

[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR]   JAXRSClientIT.traceWithGenericTypeStringTest:471->JAXRSClientIT.traceWithGenericTypeStringTest:1796->JAXRSClientIT.checkFutureString:1915 Future cannot be done, yet! ==> expected: <true> but was: <false>
[INFO]
[ERROR] Tests run: 2647, Failures: 1, Errors: 0, Skipped: 59

The only failure in this particular run (which includes a test correction) is due to #1123.

@Emily-Jiang
Copy link
Contributor

The ballot for granting an exception for updating TCKs instead of removing the TCKs has been approved (see here). The PR can be merged and a service release can be performed asap.

@jim-krueger
Copy link
Contributor

jim-krueger commented Sep 16, 2022 via email

@alwin-joseph
Copy link
Contributor

I need to request the creation of a 1.3.2 version of the TCK containing these changes. Can somebody direct me on how to go about this?

To create a new REST TCK release from this repository we have used the script jaxrs-tck-docs/tckbundle.sh in the past.

It would be better to make the TCK version changes for userguides as done in #1119.

I have created the job https://ci.eclipse.org/jakartaee-tck/job/10/job/jakarta-rest-tck-build-3.1.2 in the JakartaEE TCK CI that could create a 3.1.2 TCK bundle. Copying the relevant scripts here so it could be used from other Jenkins instances too.

mvn clean install


cd $WORKSPACE/jaxrs-tck-docs/

sh tckbundle.sh eftl 3.1.2

export BUNDLE_URL="http://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/"
export tck_bundle="jakarta-restful-ws-tck-3.1.2.zip"

cd $WORKSPACE/bundle
ls -ltr
ls $tck_bundle
scp $tck_bundle genie.jakartaee-tck@projects-storage.eclipse.org:/home/data/httpd/download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/

tckname=`ls "$tck_bundle" | rev | cut -c5- | rev`
rm -rf $WORKSPACE/$tckname-tckinfo.txt
touch $WORKSPACE/$tckname-tckinfo.txt
export NAME=$tck_bundle
echo '***********************************************************************************' >> $WORKSPACE/$tckname-tckinfo.txt
echo '***                        TCK bundle information                               ***' >> $WORKSPACE/$tckname-tckinfo.txt
echo "*** Name:       ${NAME}                                     ***" >> $WORKSPACE/$tckname-tckinfo.txt
echo "*** Bundle Copied to URL:    ${BUNDLE_URL}/${NAME} ***"  >> $WORKSPACE/$tckname-tckinfo.txt
echo '*** Date and size: '`stat -c "date: %y, size(b): %s" ${NAME}`'        ***'>> $WORKSPACE/$tckname-tckinfo.txt
echo "*** SHA256SUM: "`sha256sum ${NAME} | awk '{print $1}'`' ***' >> $WORKSPACE/$tckname-tckinfo.txt
echo '***                                                                             ***' >> $WORKSPACE/$tckname-tckinfo.txt
echo '***********************************************************************************' >> $WORKSPACE/$tckname-tckinfo.txt
scp $WORKSPACE/$tckname-tckinfo.txt genie.jakartaee-tck@projects-storage.eclipse.org:/home/data/httpd/download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/
#rm -rf $tck_bundle

Q: Will the PR #1131 also need to be part of the new service release ?

@alwin-joseph
Copy link
Contributor

It would be better to make the TCK version changes for userguides as done in #1119.

Created #1132 for version changes.

@alwin-joseph
Copy link
Contributor

I need to request the creation of a 1.3.2 version of the TCK containing these changes. Can somebody direct me on how to go about this?

I have generated the new service release TCK 3.1.2 at https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/jakarta-restful-ws-tck-3.1.2.zip .

@ivargrimstad
Copy link
Member

Submit a PR to https://github.com/jakartaee/specifications where you add a link to the new promoted TCK in https://github.com/jakartaee/specifications/tree/master/restful-ws/3.1/_index.md

Remember to add the link to the staged TCK in the PR description, so a spec committee member can promote it to the correct location.

@alwin-joseph
Copy link
Contributor

@jim-krueger Can you please help verify that the new service release TCK 3.1.2 https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/jakarta-restful-ws-tck-3.1.2.zip fixes the tests here and works as expected.

@jim-krueger
Copy link
Contributor

@brideck Is currently running these tests and will verify.

@jim-krueger
Copy link
Contributor

@alwin-joseph Brian's run has completed, however he mentions that #1123 is not included in this and should have been since the change was merged several days ago.

@alwin-joseph
Copy link
Contributor

@alwin-joseph Brian's run has completed, however he mentions that #1123 is not included in this and should have been since the change was merged several days ago.

The fix for #1123 was done in PR #1125 to exclude/disable the 68 tests. I can see this change included in the TCK build log and also those tests skipped when run with glassfish : https://ci.eclipse.org/jakartaee-tck/job/10/job/jakarta-rest-tck-glassfish-run/29/testReport/ .
Can you please confirm if the TCK bundle (3.1.2) used is correct.

@brideck @jim-krueger

@brideck
Copy link
Author

brideck commented Sep 21, 2022

Thanks ,@alwin-joseph. Yes, this looks good. Skipped tests don't show up on the initial report that I was looking at, so I was tricked by the test count staying the same. The 3.1.2 TCK is good from our perspective.

@jim-krueger
Copy link
Contributor

jim-krueger commented Oct 11, 2022 via email

@alwin-joseph
Copy link
Contributor

alwin-joseph commented Oct 11, 2022

This service release needs to include #1131 <#1131> as well, which as been approved by not yet merged.

@jim-krueger The PR #1131 was included in the TCK service release 3.1.2. To exclude the SE bootstrap tests -DexcludedGroups="se_bootstrap" can be used while running the tests.

Kindly make sure to use the TCK https://download.eclipse.org/jakartaee/restful-ws/3.1/jakarta-restful-ws-tck-3.1.2.zip

@brideck
Copy link
Author

brideck commented Feb 3, 2023

Closing this issue, given that the 3.1.2 TCK has long since been published.

@brideck brideck closed this as completed Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants