SQL: improve exception when error can't be parsed (elastic/x-pack-elasticsearch#3024)

When an error response wasn't parseable we would through fairly obscure
error messages about what is wrong with it. Great if you are fixing a
bug in Elasticsearch, no great if a proxy eats the response. This adds
the response to the error messages so you can see what was returned.

Original commit: elastic/x-pack-elasticsearch@00e542afc8

sql/shared-client/src/main/java/org/elasticsearch/xpack/sql/client/shared/RemoteFailure.java

@@ -7,10 +7,15 @@ package org.elasticsearch.xpack.sql.client.shared;
 
 import com.fasterxml.jackson.core.JsonFactory;
 import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParseException;
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.core.JsonToken;
+import java.io.BufferedInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.Reader;
+import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -20,6 +25,12 @@ import static java.util.Collections.emptyMap;
  * A failure that happened on the remote server.
  */
 public class RemoteFailure {
+    /**
+     * The maximum number of bytes before we no longer include the raw response if
+     * there is a catastrophic error parsing the remote failure.
+     */
+    private static final int MAX_RAW_RESPONSE = 50*1024;
+
     private static final JsonFactory JSON_FACTORY = new JsonFactory();
     static {
         // Set up the factory similarly to how XContent does
@@ -29,17 +40,30 @@ public class RemoteFailure {
         // Do not automatically close unclosed objects/arrays in com.fasterxml.jackson.core.json.UTF8JsonGenerator#close() method
         JSON_FACTORY.configure(JsonGenerator.Feature.AUTO_CLOSE_JSON_CONTENT, false);
         JSON_FACTORY.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, false);
-
+        // Don't close the stream because we might need to reset and reply it if there is an error. The caller closes the stream.
+        JSON_FACTORY.configure(JsonParser.Feature.AUTO_CLOSE_SOURCE, false);
     }
+
+    /**
+     * Parse a failure from the response. The stream is not closed when the parsing is complete.
+     * The caller must close it.
+     * @throws IOException if there is a catastrophic failure parsing the remote failure
+     */
     public static RemoteFailure parseFromResponse(InputStream stream) throws IOException {
-        try (JsonParser parser = JSON_FACTORY.createParser(stream)) {
+        // Mark so we can rewind to get the entire response in case we have to render an error.
-            try {
+        stream = new BufferedInputStream(stream);
-                return parseResponseTopLevel(parser);
+        stream.mark(MAX_RAW_RESPONSE);
-            } catch (IOException e) {
+        JsonParser parser = null;
-                throw new IOException(
+        try {
-                        "Can't parse error from Elasticearch [" + e.getMessage() + "] at [line "
+            parser = JSON_FACTORY.createParser(stream);
-                            + parser.getTokenLocation().getLineNr() + " col " + parser.getTokenLocation().getColumnNr() + "]",
+            return parseResponseTopLevel(parser);
-                        e);
+        } catch (JsonParseException e) {
+            throw new IOException(parseErrorMessage(e.getOriginalMessage(), stream, parser), e);
+        } catch (IOException e) {
+            throw new IOException(parseErrorMessage(e.getMessage(), stream, parser), e);
+        } finally {
+            if (parser != null) {
+                parser.close();
             }
         }
     }
@@ -216,4 +240,33 @@ public class RemoteFailure {
 
         return headers;
     }
+
+    /**
+     * Build an error message from a parse failure.
+     */
+    private static String parseErrorMessage(String message, InputStream stream, JsonParser parser) {
+        String responseMessage;
+        try {
+            stream.reset();
+            try (Reader reader = new InputStreamReader(stream, StandardCharsets.UTF_8)) {
+                StringBuilder builder = new StringBuilder();
+                builder.append("Response:\n");
+                char[] buf = new char[512];
+                int read;
+                while ((read = reader.read(buf)) != -1) {
+                    builder.append(buf, 0, read);
+                }
+                responseMessage = builder.toString();
+            }
+        } catch (IOException replayException) {
+            // NOCOMMIT check for failed reset and return different error
+            responseMessage = "Attempted to include response but failed because [" + replayException.getMessage() + "].";
+        }
+        String parserLocation = "";
+        if (parser != null) {
+            parserLocation = " at [line " + parser.getTokenLocation().getLineNr()
+                    + " col " + parser.getTokenLocation().getColumnNr() + "]";
+        }
+        return "Can't parse error from Elasticearch [" + message + "]" + parserLocation + ". "  + responseMessage;
+    }
 }

sql/shared-client/src/test/java/org/elasticsearch/xpack/sql/client/shared/RemoteFailureTests.java

@@ -9,11 +9,13 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.nio.file.Files;
 
+import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.test.ESTestCase;
 
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.startsWith;
 
 public class RemoteFailureTests extends ESTestCase {
     public void testParseBasic() throws IOException {
@@ -57,31 +59,65 @@ public class RemoteFailureTests extends ESTestCase {
     public void testNoError() throws IOException {
         IOException e = expectThrows(IOException.class, () -> parse("no_error.json"));
         assertEquals(
-            "Can't parse error from Elasticearch [Expected [error] but didn't see it.] at [line 2 col 1]",
+            "Can't parse error from Elasticearch [Expected [error] but didn't see it.] at [line 2 col 1]. Response:\n"
+                + "{\n}\n",
             e.getMessage());
     }
 
     public void testBogusError() throws IOException {
         IOException e = expectThrows(IOException.class, () -> parse("bogus_error.json"));
         assertEquals(
-            "Can't parse error from Elasticearch [Expected [error] to be an object but was [VALUE_STRING][bogus]] at [line 2 col 12]",
+            "Can't parse error from Elasticearch [Expected [error] to be an object but was [VALUE_STRING][bogus]] "
+                + "at [line 2 col 12]. Response:\n"
+                + "{\n  \"error\": \"bogus\"\n}",
             e.getMessage());
     }
 
     public void testNoStack() throws IOException {
         IOException e = expectThrows(IOException.class, () -> parse("no_stack.json"));
-        assertEquals(
+        assertThat(e.getMessage(),
-            "Can't parse error from Elasticearch [expected [stack_trace] cannot but didn't see it] at [line 5 col 3]",
+            startsWith("Can't parse error from Elasticearch [expected [stack_trace] cannot but "
-            e.getMessage());
+                + "didn't see it] at [line 5 col 3]. Response:\n{"));
     }
 
     public void testNoType() throws IOException {
         IOException e = expectThrows(IOException.class, () -> parse("no_type.json"));
+        assertThat(e.getMessage(),
+            startsWith("Can't parse error from Elasticearch [expected [type] but didn't see it] at [line 5 col 3]. Response:\n{"));
+    }
+
+    public void testInvalidJson() throws IOException {
+        IOException e = expectThrows(IOException.class, () -> parse("invalid_json.txt"));
         assertEquals(
-            "Can't parse error from Elasticearch [expected [type] but didn't see it] at [line 5 col 3]",
+            "Can't parse error from Elasticearch [Unrecognized token 'I': was expecting 'null', 'true', 'false' or NaN] "
+                + "at [line 1 col 1]. Response:\n"
+                + "I'm not json at all\n",
             e.getMessage());
     }
 
+    public void testExceptionBuildingParser() throws IOException {
+        IOException e = expectThrows(IOException.class, () -> RemoteFailure.parseFromResponse(new InputStream() {
+            @Override
+            public int read() throws IOException {
+                throw new IOException("Testing error");
+            }
+        }));
+        assertEquals(
+            "Can't parse error from Elasticearch [Testing error]. Attempted to include response but failed because [Testing error].",
+            e.getMessage());
+    }
+
+    public void testTotalGarbage() throws IOException {
+        IOException e = expectThrows(IOException.class, () ->
+            RemoteFailure.parseFromResponse(new BytesArray(new byte[] {
+                (byte) 0xEF, (byte) 0xBB, (byte) 0xBF, // The UTF-8 BOM
+                (byte) 0xFF // An invalid UTF-8 character
+            }).streamInput()));
+        assertThat(e.getMessage(),
+            startsWith("Can't parse error from Elasticearch [Unrecognized token 'ÿ': "
+                + "was expecting ('true', 'false' or 'null')] at [line 1 col 1]. Response:\n"));
+    }
+
     private RemoteFailure parse(String fileName) throws IOException {
         try (InputStream in = Files.newInputStream(getDataPath("/remote_failure/" + fileName))) {
             return RemoteFailure.parseFromResponse(in);

sql/shared-client/src/test/resources/remote_failure/invalid_json.txt

@@ -0,0 +1 @@
+I'm not json at all