fix exception swallowing in searchResponse callback

crate · Mar 9, 2015 · ab1f170 · ab1f170
1 parent a62ca1c
commit ab1f170
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 10 deletions.
diff --git a/sql/src/main/java/io/crate/executor/transport/task/elasticsearch/QueryThenFetchTask.java b/sql/src/main/java/io/crate/executor/transport/task/elasticsearch/QueryThenFetchTask.java
@@ -44,6 +44,7 @@
 import org.elasticsearch.search.SearchHit;
 import org.elasticsearch.search.internal.InternalSearchResponse;
 
+import javax.annotation.Nonnull;
 import java.io.IOException;
 import java.util.*;
 
@@ -102,23 +103,23 @@ public void onSuccess(@Nullable final QueryThenFetchOperation.QueryThenFetchCont
                     Futures.addCallback(context.createSearchResponse(), new FutureCallback<InternalSearchResponse>() {
                         @Override
                         public void onSuccess(@Nullable InternalSearchResponse searchResponse) {
-                            if (pageInfo.isPresent()) {
-                                ObjectArray<Object[]> pageSource = context.toPage(searchResponse.hits().hits(), extractors);
-                                context.cleanAfterFirstPage();
-                                result.set(new QueryThenFetchPageableTaskResult(operation, context, extractors, pageInfo.get(), pageSource, 0L));
-                            } else {
-                                Object[][] rows = context.toRows(searchResponse.hits().hits(), extractors);
-                                try {
+                            try {
+                                if (pageInfo.isPresent()) {
+                                    ObjectArray<Object[]> pageSource = context.toPage(searchResponse.hits().hits(), extractors);
+                                    context.cleanAfterFirstPage();
+                                    result.set(new QueryThenFetchPageableTaskResult(operation, context, extractors, pageInfo.get(), pageSource, 0L));
+                                } else {
+                                    Object[][] rows = context.toRows(searchResponse.hits().hits(), extractors);
                                     context.close();
                                     result.set(new QueryResult(rows));
-                                } catch (IOException e) {
-                                    onFailure(e);
                                 }
+                            } catch (Throwable t) {
+                                onFailure(t);
                             }
                         }
 
                         @Override
-                        public void onFailure(Throwable t) {
+                        public void onFailure(@Nonnull Throwable t) {
                             try {
                                 context.close();
                                 logger.error("error creating a QueryThenFetch response", t);

diff --git a/sql/src/test/java/io/crate/integrationtests/QueryThenFetchIntegrationTest.java b/sql/src/test/java/io/crate/integrationtests/QueryThenFetchIntegrationTest.java
@@ -21,14 +21,21 @@
 
 package io.crate.integrationtests;
 
+import io.crate.action.sql.SQLActionException;
 import io.crate.test.integration.CrateIntegrationTest;
+import org.hamcrest.Matchers;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import static org.hamcrest.Matchers.is;
 
 @CrateIntegrationTest.ClusterScope(scope = CrateIntegrationTest.Scope.GLOBAL)
 public class QueryThenFetchIntegrationTest extends SQLTransportIntegrationTest {
 
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
     @Test
     public void testCrateSearchServiceSupportsOrderByOnFunctionWithBooleanReturnType() throws Exception {
         execute("create table t (name string) with (number_of_replicas = 0)");
@@ -40,4 +47,21 @@ public void testCrateSearchServiceSupportsOrderByOnFunctionWithBooleanReturnType
         assertThat(((String) response.rows()[0][0]), is("Trillian"));
         assertThat(((String) response.rows()[1][0]), is("Marvin"));
     }
+
+    @Test
+    public void testThatErrorsInSearchResponseCallbackAreNotSwallowed() throws Exception {
+        expectedException.expect(SQLActionException.class);
+        expectedException.expectMessage(Matchers.anyOf(
+                is("value of argument to extract must be of type long (timestamp)"),
+                is("java.lang.Integer cannot be cast to java.lang.Long")
+        ));
+
+        execute("create table t (d timestamp) clustered into 1 shards with (number_of_replicas = 0)");
+        ensureYellow();
+
+         // timestamp is invalid.. will be an Integer and cause a classCastException in extract because long is required
+        execute("insert into t (d) values (1425910189)");
+        execute("refresh table t");
+        execute("select extract(day from d) from t");
+    }
 }