search

jakartaee / rest

Jakarta RESTful Web Services
Other
361 stars 117 forks source link

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 closed 1 year ago

brideck commented 2 years ago

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 https://github.com/resteasy/resteasy/pull/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 commented 2 years ago

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 commented 2 years ago

Seems related to #1097.

jamezp commented 2 years ago

Seems related to #1097.

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

jim-krueger commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

brideck commented 2 years ago

Was there an updated version of the TCK coming that would exclude (or fix) tests for both this challenge and https://github.com/jakartaee/rest/issues/1123?

spericas commented 2 years ago

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

brideck commented 2 years ago

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 https://github.com/jakartaee/rest/issues/1123.

Emily-Jiang commented 2 years ago

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 commented 2 years ago

Hi, 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?

Thanks

On Sep 16, 2022, at 3:39 AM, Emily Jiang @.***> wrote:

The ballot for granting an exception for updating TCKs instead of removing the TCKs has been approved (see here https://www.eclipse.org/lists/jakarta.ee-spec/msg02760.html). The PR can be merged and a service release can be performed asap.

— Reply to this email directly, view it on GitHub https://github.com/jakartaee/rest/issues/1126#issuecomment-1249087989, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHSLZQE7ICCX4CFXHQ6H5TTV6QW5ZANCNFSM56M7NRUQ. You are receiving this because you were mentioned.

alwin-joseph commented 2 years ago

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 https://github.com/jakartaee/rest/pull/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 https://github.com/jakartaee/rest/pull/1131 also need to be part of the new service release ?

alwin-joseph commented 2 years ago

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

Created https://github.com/jakartaee/rest/pull/1132 for version changes.

alwin-joseph commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@brideck Is currently running these tests and will verify.

jim-krueger commented 2 years ago

@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 commented 2 years ago

@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 https://github.com/jakartaee/rest/pull/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 commented 2 years ago

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 commented 1 year ago

This service release needs to include https://github.com/jakartaee/rest/pull/1131 https://github.com/jakartaee/rest/pull/1131 as well, which as been approved by not yet merged.

On Sep 16, 2022, at 8:01 AM, Jim Krueger @.***> wrote:

Hi, 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?

Thanks

On Sep 16, 2022, at 3:39 AM, Emily Jiang @. @.>> wrote:

The ballot for granting an exception for updating TCKs instead of removing the TCKs has been approved (see here https://www.eclipse.org/lists/jakarta.ee-spec/msg02760.html). The PR can be merged and a service release can be performed asap.

— Reply to this email directly, view it on GitHub https://github.com/jakartaee/rest/issues/1126#issuecomment-1249087989, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHSLZQE7ICCX4CFXHQ6H5TTV6QW5ZANCNFSM56M7NRUQ. You are receiving this because you were mentioned.

alwin-joseph commented 1 year ago

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 commented 1 year ago

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