From b49dbd78a0cfb80197e6eed0cce57d6bebf2d699 Mon Sep 17 00:00:00 2001 From: Ewen Cheslack-Postava Date: Tue, 6 Jan 2015 18:22:31 -0800 Subject: [PATCH 1/2] Add RestException with additional error code and automatically convert to ErrorMessage. Add the RestException class to represent any errors that should be automatically converted to an ErrorMessage and set the specified HTTP status code. This is a subclass of WebApplicationExceptionMapper so it integrates well with the rest of Jersey. WebApplicationExceptionMapper is updated to handle this special case, using different values for the HTTP status and error code embedded in the response entity. This also adds a couple of subclasses for common HTTP statuses. Fixes #3. --- .../exceptions/DebuggableExceptionMapper.java | 6 +- .../exceptions/GenericExceptionMapper.java | 3 +- .../exceptions/NotAuthorizedException.java | 30 ++++ .../rest/exceptions/NotFoundException.java | 30 ++++ .../rest/exceptions/RestException.java | 49 +++++++ .../WebApplicationExceptionMapper.java | 4 +- .../confluent/rest/ExceptionHandlingTest.java | 129 ++++++++++++++++++ .../WebApplicationExceptionMapperTest.java | 62 +++++++++ 8 files changed, 308 insertions(+), 5 deletions(-) create mode 100644 core/src/main/java/io/confluent/rest/exceptions/NotAuthorizedException.java create mode 100644 core/src/main/java/io/confluent/rest/exceptions/NotFoundException.java create mode 100644 core/src/main/java/io/confluent/rest/exceptions/RestException.java create mode 100644 core/src/test/java/io/confluent/rest/ExceptionHandlingTest.java create mode 100644 core/src/test/java/io/confluent/rest/WebApplicationExceptionMapperTest.java diff --git a/core/src/main/java/io/confluent/rest/exceptions/DebuggableExceptionMapper.java b/core/src/main/java/io/confluent/rest/exceptions/DebuggableExceptionMapper.java index d08bc51ccb..600b77ea63 100644 --- a/core/src/main/java/io/confluent/rest/exceptions/DebuggableExceptionMapper.java +++ b/core/src/main/java/io/confluent/rest/exceptions/DebuggableExceptionMapper.java @@ -52,8 +52,8 @@ public DebuggableExceptionMapper(RestConfig restConfig) { * @param exc Throwable that triggered this ExceptionMapper * @param status HTTP response status */ - public Response.ResponseBuilder createResponse(Throwable exc, Response.Status status, - String msg) { + public Response.ResponseBuilder createResponse(Throwable exc, int errorCode, + Response.Status status, String msg) { String readableMessage = msg; if (restConfig != null && restConfig.getBoolean(RestConfig.DEBUG_CONFIG)) { readableMessage += " " + exc.getClass().getName() + ": " + exc.getMessage(); @@ -68,7 +68,7 @@ public Response.ResponseBuilder createResponse(Throwable exc, Response.Status st // Ignore } } - final ErrorMessage message = new ErrorMessage(status.getStatusCode(), readableMessage); + final ErrorMessage message = new ErrorMessage(errorCode, readableMessage); return Response.status(status) .entity(message); diff --git a/core/src/main/java/io/confluent/rest/exceptions/GenericExceptionMapper.java b/core/src/main/java/io/confluent/rest/exceptions/GenericExceptionMapper.java index d3e3924598..6113cfe57e 100644 --- a/core/src/main/java/io/confluent/rest/exceptions/GenericExceptionMapper.java +++ b/core/src/main/java/io/confluent/rest/exceptions/GenericExceptionMapper.java @@ -32,7 +32,8 @@ public GenericExceptionMapper(RestConfig restConfig) { public Response toResponse(Throwable exc) { // There's no more specific information about the exception that can be passed back to the user, // so we can only use the generic message. Debug mode will append the exception info. - return createResponse(exc, Response.Status.INTERNAL_SERVER_ERROR, + return createResponse(exc, Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), + Response.Status.INTERNAL_SERVER_ERROR, Response.Status.INTERNAL_SERVER_ERROR.getReasonPhrase()).build(); } } diff --git a/core/src/main/java/io/confluent/rest/exceptions/NotAuthorizedException.java b/core/src/main/java/io/confluent/rest/exceptions/NotAuthorizedException.java new file mode 100644 index 0000000000..23d5006fd7 --- /dev/null +++ b/core/src/main/java/io/confluent/rest/exceptions/NotAuthorizedException.java @@ -0,0 +1,30 @@ +/** + * Copyright 2015 Confluent Inc. + * + * 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 io.confluent.rest.exceptions; + +import javax.ws.rs.core.Response; + +public class NotAuthorizedException extends RestException { + + public NotAuthorizedException(String message, int errorCode) { + super(message, Response.Status.UNAUTHORIZED.getStatusCode(), errorCode); + } + + public NotAuthorizedException(String message, int errorCode, Throwable cause) { + super(message, Response.Status.UNAUTHORIZED.getStatusCode(), errorCode, cause); + } +} \ No newline at end of file diff --git a/core/src/main/java/io/confluent/rest/exceptions/NotFoundException.java b/core/src/main/java/io/confluent/rest/exceptions/NotFoundException.java new file mode 100644 index 0000000000..765c149630 --- /dev/null +++ b/core/src/main/java/io/confluent/rest/exceptions/NotFoundException.java @@ -0,0 +1,30 @@ +/** + * Copyright 2015 Confluent Inc. + * + * 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 io.confluent.rest.exceptions; + +import javax.ws.rs.core.Response; + +public class NotFoundException extends RestException { + + public NotFoundException(String message, int errorCode) { + super(message, Response.Status.NOT_FOUND.getStatusCode(), errorCode); + } + + public NotFoundException(String message, int errorCode, Throwable cause) { + super(message, Response.Status.NOT_FOUND.getStatusCode(), errorCode, cause); + } +} \ No newline at end of file diff --git a/core/src/main/java/io/confluent/rest/exceptions/RestException.java b/core/src/main/java/io/confluent/rest/exceptions/RestException.java new file mode 100644 index 0000000000..cd1b6d83ca --- /dev/null +++ b/core/src/main/java/io/confluent/rest/exceptions/RestException.java @@ -0,0 +1,49 @@ +/** + * Copyright 2015 Confluent Inc. + * + * 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 io.confluent.rest.exceptions; + +import javax.ws.rs.WebApplicationException; + +/** + * RestException is a subclass of WebApplicationException that always includes an error code and + * can be converted to the standard error format. It is automatically handled by the rest-utils + * exception mappers. For convenience, it provides subclasses for a few of the most common + * responses, e.g. NotFoundException. If you're using the standard error format, you + * should prefer these exceptions to the normal JAX-RS ones since they provide better error + * responses and force you to provide both the HTTP status code and a more specific error code. + */ +public class RestException extends WebApplicationException { + + private int errorCode; + + public RestException(final String message, final int status, final int errorCode) { + this(message, status, errorCode, (Throwable) null); + } + + public RestException(final String message, final int status, final int errorCode, + final Throwable cause) { + super(message, cause, status); + this.errorCode = errorCode; + } + + public int getStatus() { + return getResponse().getStatus(); + } + + public int getErrorCode() { + return errorCode; + } +} diff --git a/core/src/main/java/io/confluent/rest/exceptions/WebApplicationExceptionMapper.java b/core/src/main/java/io/confluent/rest/exceptions/WebApplicationExceptionMapper.java index 03d4387143..99c222690a 100644 --- a/core/src/main/java/io/confluent/rest/exceptions/WebApplicationExceptionMapper.java +++ b/core/src/main/java/io/confluent/rest/exceptions/WebApplicationExceptionMapper.java @@ -43,7 +43,9 @@ public Response toResponse(WebApplicationException exc) { // The human-readable message for these can use the exception message directly. Since // WebApplicationExceptions are expected to be passed back to users, it will either contain a // situation-specific message or the HTTP status message - Response.ResponseBuilder response = createResponse(exc, status, exc.getMessage()); + int errorCode = (exc instanceof RestException) ? ((RestException)exc).getErrorCode() + : status.getStatusCode(); + Response.ResponseBuilder response = createResponse(exc, errorCode, status, exc.getMessage()); // Apparently, 415 Unsupported Media Type errors disable content negotiation in Jersey, which // causes use to return data without a content type. Work around this by detecting that specific diff --git a/core/src/test/java/io/confluent/rest/ExceptionHandlingTest.java b/core/src/test/java/io/confluent/rest/ExceptionHandlingTest.java new file mode 100644 index 0000000000..5b5533df9d --- /dev/null +++ b/core/src/test/java/io/confluent/rest/ExceptionHandlingTest.java @@ -0,0 +1,129 @@ +/** + * Copyright 2015 Confluent Inc. + * + * 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 io.confluent.rest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.util.Properties; + +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.Produces; +import javax.ws.rs.client.ClientBuilder; +import javax.ws.rs.core.Configurable; +import javax.ws.rs.core.Response; + +import io.confluent.rest.entities.ErrorMessage; +import io.confluent.rest.exceptions.NotFoundException; + +import static org.junit.Assert.assertEquals; + +/** + * Tests that a demo app catches exceptions correctly and returns errors in the expected format. + */ +public class ExceptionHandlingTest { + + RestConfig config; + ExceptionApplication app; + + @Before + public void setUp() throws Exception { + Properties props = new Properties(); + props.setProperty("debug", "false"); + config = new RestConfig(props); + app = new ExceptionApplication(config); + app.start(); + } + + @After + public void tearDown() throws Exception { + app.stop(); + app.join(); + } + + private void testAppException(String path, int expectedStatus, int expectedErrorCode, + String expectedMessage) { + Response response = ClientBuilder.newClient() + .target("http://localhost:" + config.getInt(RestConfig.PORT_CONFIG)) + .path(path) + .request() + .get(); + assertEquals(expectedStatus, response.getStatus()); + + ErrorMessage msg = response.readEntity(ErrorMessage.class); + assertEquals(expectedErrorCode, msg.getErrorCode()); + assertEquals(expectedMessage, msg.getMessage()); + } + + @Test + public void testRestException() { + testAppException("/restnotfound", 404, 4040, "Rest Not Found"); + } + + @Test + public void testNonRestException() { + // These just duplicate the HTTP status code but should carry the custom message through + testAppException("/notfound", 404, 404, "Generic Not Found"); + } + + @Test + public void testUnexpectedException() { + // Under non-debug mode, this uses a completely generic message since unexpected errors + // is the one case we want to be certain we don't leak extra info + testAppException("/unexpected", 500, 500, + Response.Status.INTERNAL_SERVER_ERROR.getReasonPhrase()); + } + + // Test app just has endpoints that trigger different types of exceptions. + private static class ExceptionApplication extends Application { + + ExceptionApplication(RestConfig props) { + super(props); + } + + @Override + public void setupResources(Configurable config, RestConfig appConfig) { + config.register(ExceptionResource.class); + } + } + + @Produces("application/json") + @Path("/") + public static class ExceptionResource { + + @GET + @Path("/restnotfound") + public String restNotFound() { + throw new NotFoundException("Rest Not Found", 4040); + } + + @GET + @Path("/notfound") + public String notFound() { + throw new javax.ws.rs.NotFoundException("Generic Not Found"); + } + + @GET + @Path("/unexpected") + public String unexpected() { + throw new RuntimeException("Internal server error."); + } + } + +} diff --git a/core/src/test/java/io/confluent/rest/WebApplicationExceptionMapperTest.java b/core/src/test/java/io/confluent/rest/WebApplicationExceptionMapperTest.java new file mode 100644 index 0000000000..b028925dd4 --- /dev/null +++ b/core/src/test/java/io/confluent/rest/WebApplicationExceptionMapperTest.java @@ -0,0 +1,62 @@ +/** + * Copyright 2015 Confluent Inc. + * + * 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 io.confluent.rest; + +import org.junit.Before; +import org.junit.Test; + +import java.util.Properties; + +import javax.ws.rs.WebApplicationException; +import javax.ws.rs.core.Response; + +import io.confluent.rest.entities.ErrorMessage; +import io.confluent.rest.exceptions.RestException; +import io.confluent.rest.exceptions.WebApplicationExceptionMapper; + +import static org.junit.Assert.*; + +public class WebApplicationExceptionMapperTest { + + private WebApplicationExceptionMapper mapper; + + @Before + public void setUp() { + Properties props = new Properties(); + props.setProperty("debug", "false"); + RestConfig config = new RestConfig(props); + mapper = new WebApplicationExceptionMapper(config); + } + + @Test + public void testRestException() { + Response response = mapper.toResponse(new RestException("msg", 400, 1000)); + assertEquals(400, response.getStatus()); + ErrorMessage out = (ErrorMessage)response.getEntity(); + assertEquals("msg", out.getMessage()); + assertEquals(1000, out.getErrorCode()); + } + + @Test + public void testNonRestWebApplicationException() { + Response response = mapper.toResponse(new WebApplicationException("msg", 400)); + assertEquals(400, response.getStatus()); + ErrorMessage out = (ErrorMessage)response.getEntity(); + assertEquals("msg", out.getMessage()); + assertEquals(400, out.getErrorCode()); + } +} \ No newline at end of file From 1ac0aee58bf4a964d6ede72d043f255047019500 Mon Sep 17 00:00:00 2001 From: Ewen Cheslack-Postava Date: Fri, 9 Jan 2015 13:10:32 -0800 Subject: [PATCH 2/2] Prefix exceptions with Rest to distinguish them from the original WebApplicationException types. --- ...orizedException.java => RestNotAuthorizedException.java} | 6 +++--- .../{NotFoundException.java => RestNotFoundException.java} | 6 +++--- .../test/java/io/confluent/rest/ExceptionHandlingTest.java | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) rename core/src/main/java/io/confluent/rest/exceptions/{NotAuthorizedException.java => RestNotAuthorizedException.java} (79%) rename core/src/main/java/io/confluent/rest/exceptions/{NotFoundException.java => RestNotFoundException.java} (80%) diff --git a/core/src/main/java/io/confluent/rest/exceptions/NotAuthorizedException.java b/core/src/main/java/io/confluent/rest/exceptions/RestNotAuthorizedException.java similarity index 79% rename from core/src/main/java/io/confluent/rest/exceptions/NotAuthorizedException.java rename to core/src/main/java/io/confluent/rest/exceptions/RestNotAuthorizedException.java index 23d5006fd7..d7fb7657ca 100644 --- a/core/src/main/java/io/confluent/rest/exceptions/NotAuthorizedException.java +++ b/core/src/main/java/io/confluent/rest/exceptions/RestNotAuthorizedException.java @@ -18,13 +18,13 @@ import javax.ws.rs.core.Response; -public class NotAuthorizedException extends RestException { +public class RestNotAuthorizedException extends RestException { - public NotAuthorizedException(String message, int errorCode) { + public RestNotAuthorizedException(String message, int errorCode) { super(message, Response.Status.UNAUTHORIZED.getStatusCode(), errorCode); } - public NotAuthorizedException(String message, int errorCode, Throwable cause) { + public RestNotAuthorizedException(String message, int errorCode, Throwable cause) { super(message, Response.Status.UNAUTHORIZED.getStatusCode(), errorCode, cause); } } \ No newline at end of file diff --git a/core/src/main/java/io/confluent/rest/exceptions/NotFoundException.java b/core/src/main/java/io/confluent/rest/exceptions/RestNotFoundException.java similarity index 80% rename from core/src/main/java/io/confluent/rest/exceptions/NotFoundException.java rename to core/src/main/java/io/confluent/rest/exceptions/RestNotFoundException.java index 765c149630..0362e6de6f 100644 --- a/core/src/main/java/io/confluent/rest/exceptions/NotFoundException.java +++ b/core/src/main/java/io/confluent/rest/exceptions/RestNotFoundException.java @@ -18,13 +18,13 @@ import javax.ws.rs.core.Response; -public class NotFoundException extends RestException { +public class RestNotFoundException extends RestException { - public NotFoundException(String message, int errorCode) { + public RestNotFoundException(String message, int errorCode) { super(message, Response.Status.NOT_FOUND.getStatusCode(), errorCode); } - public NotFoundException(String message, int errorCode, Throwable cause) { + public RestNotFoundException(String message, int errorCode, Throwable cause) { super(message, Response.Status.NOT_FOUND.getStatusCode(), errorCode, cause); } } \ No newline at end of file diff --git a/core/src/test/java/io/confluent/rest/ExceptionHandlingTest.java b/core/src/test/java/io/confluent/rest/ExceptionHandlingTest.java index 5b5533df9d..1ce139eef0 100644 --- a/core/src/test/java/io/confluent/rest/ExceptionHandlingTest.java +++ b/core/src/test/java/io/confluent/rest/ExceptionHandlingTest.java @@ -30,7 +30,7 @@ import javax.ws.rs.core.Response; import io.confluent.rest.entities.ErrorMessage; -import io.confluent.rest.exceptions.NotFoundException; +import io.confluent.rest.exceptions.RestNotFoundException; import static org.junit.Assert.assertEquals; @@ -110,7 +110,7 @@ public static class ExceptionResource { @GET @Path("/restnotfound") public String restNotFound() { - throw new NotFoundException("Rest Not Found", 4040); + throw new RestNotFoundException("Rest Not Found", 4040); } @GET