Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
AssertionErrors are converted into service exceptions with type internal
At the moment, AssertionErrors are not handled by remoting, even though
they're used fairly widely within services and libraries we have
internally. This means that the body ends up being empty if they
ever get hit, which leads to a fairly cryptic exception being
thrown on the client.

Bugs are still debuggable, it's just harder. This PR makes the claim
that AssertionErrors should be handled specially.

Could also argue that all errors should be handled the same as e.g.
a random RuntimeException (specifically thinking NoSuchMethodError).
  • Loading branch information
j-baker committed Jun 8, 2018
commit 78fee75f5f5bdd38e33b6d091bf2b9398cec0c94
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright 2018 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.remoting3.servers.jersey;

import com.palantir.remoting.api.errors.ErrorType;
import javax.ws.rs.ext.Provider;

@Provider
final class AssertionErrorExceptionMapper extends JsonExceptionMapper<AssertionError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem pretty obscure that people are throwing AssertionErrors - I've generally only seen them in private constructors (throw new AssertionError("Not instantiable");). However, I think it does makes sense that in the strange case that this happens, we might as well give clients something helpful.

Can we expand it to all Errors? This would get us NoSuchFieldError, NoSuchMethodError and NoClassDefFoundError?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanding to Error or Throwable would also cover the occasional OutOfMemoryError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that was my reticence at the start. I guess there's the thing where we hope that OOMs get handled by the JVM itself?


@Override
ErrorType getErrorType(AssertionError exception) {
return ErrorType.INTERNAL;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public boolean configure(FeatureContext context) {
context.register(new RemoteExceptionMapper());
context.register(new ServiceExceptionMapper());
context.register(new QosExceptionMapper());
context.register(new AssertionErrorExceptionMapper());

// Cbor handling
context.register(new JacksonCBORProvider(ObjectMappers.newCborServerObjectMapper()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,10 @@
* When code in the server throws an {@link Exception} that reaches Jersey, this {@link ExceptionMapper} converts that
* exception into an HTTP {@link Response} for return to the caller/browser.
*/
abstract class JsonExceptionMapper<T extends Exception> implements ExceptionMapper<T> {
abstract class JsonExceptionMapper<T extends Throwable> implements ExceptionMapper<T> {

private static final Logger log = LoggerFactory.getLogger(JsonExceptionMapper.class);

static final ObjectMapper MAPPER = ObjectMappers.newClientObjectMapper().enable(SerializationFeature.INDENT_OUTPUT);

/** Returns the {@link ErrorType} that this exception corresponds to. */
abstract ErrorType getErrorType(T exception);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,31 +83,31 @@ public void before() {
* WebApplicationExceptionMapper} rather than the {@link RuntimeExceptionMapper}
*/
@Test
public void testForbiddenException() throws NoSuchMethodException, SecurityException {
public void testForbiddenException() {
Response response = target.path("throw-forbidden-exception").request().get();
assertThat(response.getStatus(), is(Status.FORBIDDEN.getStatusCode()));
}

@Test
public void testNotFoundException() throws NoSuchMethodException, SecurityException {
public void testNotFoundException() {
Response response = target.path("throw-not-found-exception").request().get();
assertThat(response.getStatus(), is(Status.NOT_FOUND.getStatusCode()));
}

@Test
public void testServerErrorException() throws NoSuchMethodException, SecurityException {
public void testServerErrorException() {
Response response = target.path("throw-server-error-exception").request().get();
assertThat(response.getStatus(), is(SERVER_EXCEPTION_STATUS.getStatusCode()));
}

@Test
public void testWebApplicationException() throws NoSuchMethodException, SecurityException {
public void testWebApplicationException() {
Response response = target.path("throw-web-application-exception").request().get();
assertThat(response.getStatus(), is(WEB_EXCEPTION_STATUS.getStatusCode()));
}

@Test
public void testRemoteException() throws NoSuchMethodException, SecurityException, IOException {
public void testRemoteException() throws IOException {
Response response = target.path("throw-remote-exception").request().get();
assertThat(response.getStatus(), is(REMOTE_EXCEPTION_STATUS_CODE));
String body =
Expand All @@ -124,7 +124,7 @@ public void testRemoteException() throws NoSuchMethodException, SecurityExceptio
}

@Test
public void testServiceException() throws NoSuchMethodException, SecurityException, IOException {
public void testServiceException() throws IOException {
Response response = target.path("throw-service-exception").request().get();
assertThat(response.getStatus(), is(REMOTE_EXCEPTION_STATUS_CODE));
String body =
Expand All @@ -145,13 +145,25 @@ public void testServiceException() throws NoSuchMethodException, SecurityExcepti
}

@Test
public void testQosException() throws Exception {
public void testQosException() {
Response response = target.path("throw-qos-retry-foo-exception").request().get();

assertThat(response.getStatus(), is(308));
assertThat(response.getHeaderString("Location"), is("http://foo"));
}

@Test
public void testAssertionErrorIsJsonException() throws IOException {
Response response = target.path("throw-assertion-error").request().get();
assertThat(response.getStatus(), is(SERVER_EXCEPTION_STATUS.getStatusCode()));
String body =
new String(ByteStreams.toByteArray(response.readEntity(InputStream.class)), StandardCharsets.UTF_8);

SerializableError error = ObjectMappers.newClientObjectMapper().readValue(body, SerializableError.class);
assertThat(error.errorCode(), is(ErrorType.INTERNAL.code().toString()));
assertThat(error.errorName(), is(ErrorType.INTERNAL.name()));
}

public static class ExceptionMappersTestServer extends Application<Configuration> {
@Override
public final void run(Configuration config, final Environment env) throws Exception {
Expand Down Expand Up @@ -203,6 +215,11 @@ public String throwQosRetryFooException() {
throw new RuntimeException(e);
}
}

@Override
public String throwAssertionError() {
throw new AssertionError();
}
}

@Path("/")
Expand Down Expand Up @@ -236,5 +253,9 @@ public interface ExceptionTestService {
@GET
@Path("/throw-qos-retry-foo-exception")
String throwQosRetryFooException();

@GET
@Path("/throw-assertion-error")
String throwAssertionError();
}
}