Jakarta - EE 10 - AWT test failures #27809
Comments
|
@Karm as mentioned in the description, I suspect a problem in RESTEasy but I think we need more details to understand what exactly is not working and what makes the tests fail. |
|
This is related to this change: resteasy/resteasy#3044. Basically, it will map all the unhandled exceptions to return 500 and collect all the inner exception messages. This test relies on the exception messages to be logged and this can be enabled using the logger |
|
When enabling the logging for the DefaultExceptionMapper by adding this build step in ResteasyBuiltinsProcessor: @BuildStep
void logging(BuildProducer<LogCategoryBuildItem> log) {
// Increase log level for default exception mapper: https://github.com/resteasy/resteasy/pull/3044
log.produce(new LogCategoryBuildItem("org.jboss.resteasy.core.providerfactory.DefaultExceptionMapper", Level.FINE));
}And running the test again, we got the exception message now: Still we would need to adjust the test exception because we're losing the path. The exception message before Resteasy 6.1 was: And I don't think this is the right direction as the exception is not going through our QuarkusErrorHandler handler. Therefore, these are the options I can think of:
|
|
The only option I can see here is have a way for RESTEasy to disable the default exception mapper with a configuration property. We have to include the default exception mapper per the spec. The only other option would be to look at the response rather than the logs. |
|
Maybe not ideal, but another option is to have Quarkus register it's own @Provider
public class QuarkusExceptionMapper implements ExceptionMapper<Throwable> {
@Override
public Response toResponse(final Throwable exception) {
throw exception instanceof RuntimeException ? (RuntimeException) exception : new RuntimeException(exception);
}
} |
I also tried that and spites it fixes the test, it opens a can of worms in other places (many other tests will fail in |
|
@jamezp what I fail to understand is how we can consider than not logging any errors on the server side is an improvement? I mean, from this version of RESTEasy, users have absolutely no idea what's failing anymore. How do you expect people to be able to fix issues? It looks like a detail but I think it's a serious usability regression. /cc @geoand |
|
So the issue is that RESTEasy now does not automatically log an exception? What kinds of exceptions are we talking about? |
|
Well, from what I can see, any exception causing a 500 is not logged anymore (and I suppose any exception really). At least I don't get any stacktrace in the logs when we have an error in a test. |
|
Okay, that does seem problematic. In RR we always log exceptions that were not handled by any |
|
@geoand yeah so the problem is that now the spec mandates a default |
|
In that case, I would log the error in that exception mapper |
|
To be precise, this happens to all unhandled exceptions. The previous behaviour was that all the unhandled exceptions were not mapped by Resteasy, so we could map it using our Quarkus error handler implementation. With Resteasy 6.x, all the unhandled exceptions are now mapped using a new Default exception mapper that won't log anything unless users configure the log level debug for this new exception mapper. Also, the Quarkus error handler won't do anything in this scenario any longer. |
|
@Sgitario do you remember where is the Quarkus exception mapper implementation? |
|
This handler was the logic that we were calling for unhandled exceptions: https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/QuarkusErrorHandler.java |
|
@Sgitario @geoand @jamezp so even if not spec compliant, I wonder if the best wouldn't be to have a property to not register the default RESTEasy exception mapper in Quarkus and get back to the previous behavior? Another option would be to override it and provide a behavior similar to I think we were pretty much happy with the existing behavior in Quarkus? @jamezp even if we do that, I'm still unsure the current behavior of not logging anything in RESTEasy is really something you want for WildFly and EAP. |
|
I think we should override it and get back the previous behavior |
|
Well I'm not sure it would be so easy to override it and get back to the previous behavior with a JAX-RS exception mapper. Thus why I was suggesting we shouldn't register the default exception mapper in the first place (and thus get back to the previous behavior). |
|
I'm definitely convinced on the logging errors now :) The only reason I went with debug logging to begin with a concern over being too verbose. However, I realize now that was not a good idea. I can look at a way to not register a default exception mapper. I feel like that idea came from the MP REST Client, but IMO it makes more sense on a client than in the container. |
|
I've filed https://issues.redhat.com/browse/RESTEASY-3225 and will work on that this morning. Will a simple property work fine? Something like |
|
Fine with me |
|
This resteasy/resteasy#3278 includes the ability to disable the default exception mapper. It also does allow the Quarkus test to pass with no changes to Quarkus. However, if you want it to be processed through the |
|
Hello, I am sorry I am coming late to the party. I read the comments and it's clearly unrelated to AWT (and not related to native either I think). It is fine to have the power to fine tune it, to mute it, to consume it etc., but the default should be rather gruff and simple IMHO.
Yes. This test tests the UX in a situation when user clearly attempts to use something that would require the AWT extension but it is not loaded, i.e. a friendly, instructive error must be logged. No point to run it in JVM mode. |
|
@Karm yeah thanks, we figured it out :). I'm trying an upgrade to RESTEasy 6.2.0.Final and will see how it goes. |
Since the upgrade to RESTEasy 6.1, AWT tests for
ImageDecodersTest#testComplexImagesare failing in the jakarta-rewrite branch (for more information, see https://github.com/quarkusio/quarkus/tree/main/jakarta#build-locally):Typically in integration-tests/awt which makes use of multipart to upload images (in JVM mode):
Also, the tests are failing in
integration-tests/no-awtbut only in native (maybe they are only run in native?):My guess is that it's related.
My guess is that a behavior has changed in RESTEasy related to multipart but I want us to get more details about what is exactly going on before involving James Perkins.
The text was updated successfully, but these errors were encountered: