calcite-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From els...@apache.org
Subject calcite git commit: [CALCITE-1185] Send ErrorResponse on request parsing error
Date Mon, 04 Apr 2016 21:45:06 GMT
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 <elserj@apache.org>
Authored: Mon Apr 4 17:42:31 2016 -0400
Committer: Josh Elser <elserj@apache.org>
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 {
 


Mime
View raw message