Repository: calcite Updated Branches: refs/heads/master ea71f9295 -> 70829e53f
[CALCITE-1185] Send ErrorResponse on request parsing error Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/70829e53 Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/70829e53 Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/70829e53 Branch: refs/heads/master Commit: 70829e53f1ea036909f74400454ca6902fa818dc Parents: ea71f92 Author: Josh Elser <[email protected]> Authored: Mon Apr 4 17:42:31 2016 -0400 Committer: Josh Elser <[email protected]> Committed: Mon Apr 4 17:42:31 2016 -0400 ---------------------------------------------------------------------- .../calcite/avatica/remote/AbstractHandler.java | 44 +++++++++++--------- .../avatica/remote/AbstractHandlerTest.java | 12 +++--- .../calcite/avatica/remote/RemoteMetaTest.java | 43 +++++++++++++++++++ 3 files changed, 74 insertions(+), 25 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/70829e53/avatica/core/src/main/java/org/apache/calcite/avatica/remote/AbstractHandler.java ---------------------------------------------------------------------- diff --git a/avatica/core/src/main/java/org/apache/calcite/avatica/remote/AbstractHandler.java b/avatica/core/src/main/java/org/apache/calcite/avatica/remote/AbstractHandler.java index 1968fcd..0f8770b 100644 --- a/avatica/core/src/main/java/org/apache/calcite/avatica/remote/AbstractHandler.java +++ b/avatica/core/src/main/java/org/apache/calcite/avatica/remote/AbstractHandler.java @@ -90,33 +90,37 @@ public abstract class AbstractHandler<T> implements Handler<T> { * @return A {@link Response} with additional context about that response. */ public HandlerResponse<T> apply(T serializedRequest) { - final Service.Request request; - try { - request = decode(serializedRequest); - } catch (IOException e) { - // TODO provide a canned ErrorResponse. - throw new RuntimeException(e); - } - try { + final Service.Request request = decode(serializedRequest); final Service.Response response = request.accept(service); return new HandlerResponse<>(encode(response), HTTP_OK); } catch (Exception e) { - ErrorResponse errorResp = unwrapException(e); - - try { - return new HandlerResponse<>(encode(errorResp), HTTP_INTERNAL_SERVER_ERROR); - } catch (IOException e1) { - // TODO provide a canned ErrorResponse + return convertToErrorResponse(e); + } + } - // If we can't serialize error message to JSON, can't give a meaningful error to caller. - // Just try to not unnecessarily create more exceptions. - if (e instanceof RuntimeException) { - throw (RuntimeException) e; - } + /** + * Attempts to convert an Exception to an ErrorResponse. If there is an issue in serialization, + * a RuntimeException is thrown instead (wrapping the original exception if necessary). + * + * @param e The exception to convert. + * @return A HandlerResponse instance. + */ + private HandlerResponse<T> convertToErrorResponse(Exception e) { + ErrorResponse errorResp = unwrapException(e); - throw new RuntimeException(e); + try { + return new HandlerResponse<>(encode(errorResp), HTTP_INTERNAL_SERVER_ERROR); + } catch (IOException e1) { + // TODO provide a canned ErrorResponse + + // If we can't serialize the error message, we can't give a meaningful error to caller. + // Just try to not unnecessarily create more exceptions. + if (e instanceof RuntimeException) { + throw (RuntimeException) e; } + + throw new RuntimeException(e); } } http://git-wip-us.apache.org/repos/asf/calcite/blob/70829e53/avatica/core/src/test/java/org/apache/calcite/avatica/remote/AbstractHandlerTest.java ---------------------------------------------------------------------- diff --git a/avatica/core/src/test/java/org/apache/calcite/avatica/remote/AbstractHandlerTest.java b/avatica/core/src/test/java/org/apache/calcite/avatica/remote/AbstractHandlerTest.java index 012cccc..16402d8 100644 --- a/avatica/core/src/test/java/org/apache/calcite/avatica/remote/AbstractHandlerTest.java +++ b/avatica/core/src/test/java/org/apache/calcite/avatica/remote/AbstractHandlerTest.java @@ -148,17 +148,19 @@ public class AbstractHandlerTest { @SuppressWarnings("unchecked") final AbstractHandler<String> handler = Mockito.mock(AbstractHandler.class); final IOException exception = new IOException(); + final ErrorResponse errorResponse = new ErrorResponse(); + final String serializedErrorResponse = "Serialized ErrorResponse"; // Faked out // Accept a serialized request Mockito.when(handler.apply(Mockito.anyString())).thenCallRealMethod(); // Throw an Exception trying to convert it back into a POJO Mockito.when(handler.decode(Mockito.anyString())).thenThrow(exception); + Mockito.when(handler.unwrapException(exception)).thenReturn(errorResponse); + Mockito.when(handler.encode(errorResponse)).thenReturn(serializedErrorResponse); - try { - handler.apply("this is mocked out"); - } catch (RuntimeException e) { - assertEquals(exception, e.getCause()); - } + HandlerResponse<String> response = handler.apply("this is mocked out"); + assertEquals(serializedErrorResponse, response.getResponse()); + assertEquals(500, response.getStatusCode()); } } http://git-wip-us.apache.org/repos/asf/calcite/blob/70829e53/avatica/server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java ---------------------------------------------------------------------- diff --git a/avatica/server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java b/avatica/server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java index 43beba8..81fa369 100644 --- a/avatica/server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java +++ b/avatica/server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java @@ -46,8 +46,11 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; +import java.io.DataOutputStream; +import java.io.InputStream; import java.lang.reflect.Field; import java.lang.reflect.Method; +import java.net.HttpURLConnection; import java.net.InetAddress; import java.net.URL; import java.nio.charset.StandardCharsets; @@ -640,6 +643,46 @@ public class RemoteMetaTest { } } + @Test public void testMalformedRequest() throws Exception { + URL url = new URL("http://localhost:"; + this.port); + + HttpURLConnection conn = (HttpURLConnection) url.openConnection(); + conn.setRequestMethod("POST"); + conn.setDoInput(true); + conn.setDoOutput(true); + + try (DataOutputStream wr = new DataOutputStream(conn.getOutputStream())) { + // Write some garbage data + wr.write(new byte[] {0, 1, 2, 3, 4, 5, 6, 7}); + wr.flush(); + wr.close(); + } + final int responseCode = conn.getResponseCode(); + assertEquals(500, responseCode); + final InputStream inputStream = conn.getErrorStream(); + byte[] responseBytes = AvaticaUtils.readFullyToBytes(inputStream); + ErrorResponse response; + switch (this.serialization) { + case JSON: + response = JsonService.MAPPER.readValue(responseBytes, ErrorResponse.class); + assertTrue("Unexpected error message: " + response.errorMessage, + response.errorMessage.contains("Illegal character")); + break; + case PROTOBUF: + ProtobufTranslation pbTranslation = new ProtobufTranslationImpl(); + Response genericResp = pbTranslation.parseResponse(responseBytes); + assertTrue("Response was not an ErrorResponse, but was " + genericResp.getClass(), + genericResp instanceof ErrorResponse); + response = (ErrorResponse) genericResp; + assertTrue("Unexpected error message: " + response.errorMessage, + response.errorMessage.contains("contained an invalid tag")); + break; + default: + fail("Unhandled serialization " + this.serialization); + throw new RuntimeException(); + } + } + /** Factory that provides a {@link JdbcMeta}. */ public static class FullyRemoteJdbcMetaFactory implements Meta.Factory {
