From adae4019dd265c2db8b75c32553955d0cb232b67 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 15 Nov 2017 15:33:53 -0500 Subject: [PATCH] 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@00e542afc8910c77a1a05ce88330ae848c477ea1 --- .../sql/client/shared/RemoteFailure.java | 71 ++++++++++++++++--- .../sql/client/shared/RemoteFailureTests.java | 48 +++++++++++-- .../resources/remote_failure/invalid_json.txt | 1 + 3 files changed, 105 insertions(+), 15 deletions(-) create mode 100644 sql/shared-client/src/test/resources/remote_failure/invalid_json.txt diff --git a/sql/shared-client/src/main/java/org/elasticsearch/xpack/sql/client/shared/RemoteFailure.java b/sql/shared-client/src/main/java/org/elasticsearch/xpack/sql/client/shared/RemoteFailure.java index 8e561b0fcca..4ad7d55359c 100644 --- a/sql/shared-client/src/main/java/org/elasticsearch/xpack/sql/client/shared/RemoteFailure.java +++ b/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)) { - try { - return parseResponseTopLevel(parser); - } catch (IOException e) { - throw new IOException( - "Can't parse error from Elasticearch [" + e.getMessage() + "] at [line " - + parser.getTokenLocation().getLineNr() + " col " + parser.getTokenLocation().getColumnNr() + "]", - e); + // Mark so we can rewind to get the entire response in case we have to render an error. + stream = new BufferedInputStream(stream); + stream.mark(MAX_RAW_RESPONSE); + JsonParser parser = null; + try { + parser = JSON_FACTORY.createParser(stream); + return parseResponseTopLevel(parser); + } 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; + } } diff --git a/sql/shared-client/src/test/java/org/elasticsearch/xpack/sql/client/shared/RemoteFailureTests.java b/sql/shared-client/src/test/java/org/elasticsearch/xpack/sql/client/shared/RemoteFailureTests.java index 3674c5e46bb..fd5ad360afc 100644 --- a/sql/shared-client/src/test/java/org/elasticsearch/xpack/sql/client/shared/RemoteFailureTests.java +++ b/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( - "Can't parse error from Elasticearch [expected [stack_trace] cannot but didn't see it] at [line 5 col 3]", - e.getMessage()); + assertThat(e.getMessage(), + startsWith("Can't parse error from Elasticearch [expected [stack_trace] cannot but " + + "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); diff --git a/sql/shared-client/src/test/resources/remote_failure/invalid_json.txt b/sql/shared-client/src/test/resources/remote_failure/invalid_json.txt new file mode 100644 index 00000000000..75af77f2eac --- /dev/null +++ b/sql/shared-client/src/test/resources/remote_failure/invalid_json.txt @@ -0,0 +1 @@ +I'm not json at all