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
[RESTEASY-2882] Changed the processing of ResponseExceptionMapper to … #2850
[RESTEASY-2882] Changed the processing of ResponseExceptionMapper to … #2850
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test doesn't look quite right to me. I made some comments on it, but it also needs some kind of resource to invoke too. Something like:
@Path("/")
public class HealthResource {
@GET
@Path("/health")
@Produces(MediaType.APPLICATION_JSON)
public Response status() {
return Response.status(Response.Status.SERVICE_UNAVAILABLE).entity(new HealthCheckData("Down")).build();
}
}
.../test/java/org/jboss/resteasy/test/microprofile/restclient/Ignore503ExceptionMapperTest.java
Outdated
Show resolved
Hide resolved
.../test/java/org/jboss/resteasy/test/microprofile/restclient/Ignore503ExceptionMapperTest.java
Outdated
Show resolved
Hide resolved
.../test/java/org/jboss/resteasy/test/microprofile/restclient/Ignore503ExceptionMapperTest.java
Outdated
Show resolved
Hide resolved
.../test/java/org/jboss/resteasy/test/microprofile/restclient/Ignore503ExceptionMapperTest.java
Outdated
Show resolved
Hide resolved
.../test/java/org/jboss/resteasy/test/microprofile/restclient/Ignore503ExceptionMapperTest.java
Outdated
Show resolved
Hide resolved
.../test/java/org/jboss/resteasy/test/microprofile/restclient/Ignore503ExceptionMapperTest.java
Outdated
Show resolved
Hide resolved
...rofile-base/src/main/java/org/jboss/resteasy/microprofile/client/ProxyInvocationHandler.java
Show resolved
Hide resolved
...microprofile-base/src/main/java/org/jboss/resteasy/microprofile/client/ExceptionMapping.java
Show resolved
Hide resolved
Actually maybe you meant the 404 to be thrown which is why there wasn't a resource. If that is the case we should probably change the name of the test and the exception mapper to |
4064a05
to
7d51459
Compare
Changes complete |
...est/java/org/jboss/resteasy/test/microprofile/restclient/ExceptionMapperReturnsNullTest.java
Show resolved
Hide resolved
7d51459
to
bb72f69
Compare
...profile-base/src/main/java/org/jboss/resteasy/microprofile/client/RestClientBuilderImpl.java
Show resolved
Hide resolved
.../java/org/jboss/resteasy/test/microprofile/restclient/resource/Ignore503ExceptionMapper.java
Outdated
Show resolved
Hide resolved
…meet spec requirements
bb72f69
to
b910728
Compare
// falling through to here means no applicable exception mapper found | ||
// or applicable mapper returned null | ||
LOGGER.warnf("No default ResponseExceptionMapper found or user's ResponseExceptionMapper returned null." | ||
+ " Response status: %s messge: %s", handled.getStatus(), handled.getReasonPhrase()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo in "%s message: %s"
@@ -151,6 +151,8 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl | |||
} | |||
if (cause instanceof ExceptionMapping.HandlerException) { | |||
((ExceptionMapping.HandlerException)cause).mapException(method); | |||
// no applicable exception mapper found or applicable mapper returned null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't this be a debug log message too?
.getHealthData(); | ||
Assert.fail("Exception should have been returned"); | ||
} catch (Exception e) { | ||
// success exception was returned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert the expected exception class would be cool.
…meet spec requirements