OpenLiberty / open-liberty

Open Liberty is a highly composable, fast to start, dynamic application server runtime environment
https://openliberty.io
Eclipse Public License 2.0
1.15k stars 592 forks source link

Get mpRestClient-3.0 TCK running without servlet feature #22280

Open Azquelt opened 2 years ago

Azquelt commented 2 years ago

With the servlet feature removed and the TCK switched to run with the rest arquillian test protocol, I get the following failures:

For FollowRedirectsTest and CDIFollowRedirectsTest, the following tests fail with both EE9 (where the servlet-5.0 feature is still loaded as a dependency) and EE10 features.

They all fail with the following error:

org.jboss.arquillian.test.spi.ArquillianProxyException: org.jboss.resteasy.client.exception.ResteasyWebApplicationException : Unknown error, status code 303 [Proxied because : Original exception caused: class java.io.NotSerializableException: org.jboss.resteasy.specimpl.BuiltResponse]
at org.jboss.resteasy.client.exception.WebApplicationExceptionWrapper.wrap(WebApplicationExceptionWrapper.java:107)
at org.jboss.resteasy.microprofile.client.DefaultResponseExceptionMapper.toThrowable(DefaultResponseExceptionMapper.java:21)
at org.jboss.resteasy.microprofile.client.ExceptionMapping$HandlerException.mapException(ExceptionMapping.java:41)
at org.jboss.resteasy.microprofile.client.ProxyInvocationHandler.invoke(ProxyInvocationHandler.java:153)
at org.jboss.resteasy.microprofile.client.ot.DefaultMethodInvocationHandler.invoke(DefaultMethodInvocationHandler.java:53)
at org.jboss.resteasy.microprofile.client.ot.LibertyProxyInvocationHandler.invoke(LibertyProxyInvocationHandler.java:46)
at com.sun.proxy.$Proxy1138.executeGet(Unknown Source)
at org.eclipse.microprofile.rest.client.tck.FollowRedirectsTest.execute(FollowRedirectsTest.java:140)
at org.eclipse.microprofile.rest.client.tck.FollowRedirectsTest.testDefault(FollowRedirectsTest.java:108)
at org.eclipse.microprofile.rest.client.tck.FollowRedirectsTest.test303Default(FollowRedirectsTest.java:80)
[...]

For EE9 only DefaultExceptionMapperTest.testPropagationOfResponseDetailsFromDefaultMapper fails with

jakarta.ws.rs.ProcessingException: java.lang.IllegalStateException: RESTEASY003291: Input stream was empty, there is no entity
at org.jboss.resteasy.specimpl.BuiltResponse.readFrom(BuiltResponse.java:199)
at org.jboss.resteasy.specimpl.BuiltResponse.readEntity(BuiltResponse.java:90)
at org.jboss.resteasy.specimpl.AbstractBuiltResponse.readEntity(AbstractBuiltResponse.java:262)
at org.eclipse.microprofile.rest.client.tck.DefaultExceptionMapperTest.testPropagationOfResponseDetailsFromDefaultMapper(DefaultExceptionMapperTest.java:92)
[...]
Caused by: java.lang.IllegalStateException: RESTEASY003291: Input stream was empty, there is no entity
at org.jboss.resteasy.specimpl.BuiltResponse.readFrom(BuiltResponse.java:147)

For EE10 only, HasConversationScopeTest and HasRequestScopeTest are skipped.

The skipped tests do seem to be caused by the removal of the servlet feature - they both assume that the scope is only present if the servlet APIs are present.

The failures do not look like they're caused by the removal of the servlet feature but may be caused by the change from the servlet protocol (where arquillian deploys a servlet in the test app to run tests on the server) to the rest protocol (where a rest application and rest resource is used instead).

Both failures appear to be problems with the behaviour of the default exception handler. Could this be different when the application contains rest resources?

Azquelt commented 2 years ago

7a7a5da8 has the changes needed to run the TCK without the servlet feature.

jim-krueger commented 2 years ago

[Jared Anderson] My speculation was that the test was returning the response object and trying to serialize it. With Servlet that is probably serializable, but with REST it is not.

Azquelt commented 2 years ago

The second problem I'm unsure about but I suspect the first issue is a bug.

It looks like when you have a rest client method which returns Response and doesn't follow redirects, if it receives a 303 response, it throws ResteasyWebApplicationException : Unknown error, status code 303 rather than returning the Response with the 303 status and Location header.

Although the test result does mention that a response couldn't be serialized, I think that doesn't happen until the test method has thrown the exception and arquillian gets hold of it and tries to send it back to the client side.

I think we need to find out why this happens when using the rest protocol but not the servlet protocol. The only difference should be that there's a rest application and resource class deployed in the app instead of a servlet class.

jim-krueger commented 2 years ago

Following investigation we'd discovered that this issue was caused by a change put into RESTEasy under PR 2632

In this PR the DefaultResponseExceptionMapper was changed to include cases where 300 status codes were returned while functioning on the server side and return a "sanitized" WebApplicationException instead of the Response object that had previously been returned.

jim-krueger commented 2 years ago

I posted a question about this on the resteasy developers mailing list and receive the following response:

HI Jim, It does seem odd that the DefaultResponseExceptionMapper is triggered on a 3xx status code. However, I also think it makes sense to only wrap exceptions if they are 400 or greater. I filed an issue for that [1] and will look at the RESTEasy core side too. ....

https://github.com/resteasy/resteasy-microprofile/issues/94

jim-krueger commented 2 years ago

For this issue I think it would be best to live with the servlet dependency in the TCK (or add the server.xml property to revert to the “original behavior”) in the near team and watch the issue that James posted. Assuming they confirm that the 300 series codes should not be handled in this way we can include that change in our overrides fork until we upgrade at some point in the future.

So this issue should remain open pending the outcome of the resteasy issue listed above.