diff --git a/src/integrationTest/java/integration/tests/StatementTest.java b/src/integrationTest/java/integration/tests/StatementTest.java index 05331d865..2ed7ccc51 100644 --- a/src/integrationTest/java/integration/tests/StatementTest.java +++ b/src/integrationTest/java/integration/tests/StatementTest.java @@ -3,6 +3,7 @@ import com.firebolt.jdbc.connection.FireboltConnection; import com.firebolt.jdbc.exception.FireboltException; import integration.ConnectionInfo; +import integration.EnvironmentCondition; import integration.IntegrationTest; import kotlin.collections.ArrayDeque; import org.hamcrest.Matchers; @@ -99,6 +100,25 @@ void shouldThrowExceptionWhenTryingToReuseStatementClosedOnCompletion() throws S } } + @Test + void shouldThrowExceptionWhenExecutingWrongQuery() throws SQLException { + try (Connection connection = createConnection(); Statement statement = connection.createStatement()) { + String errorMessage = assertThrows(FireboltException.class, () -> statement.executeQuery("select wrong query")).getMessage(); + assertTrue(errorMessage.contains("Column 'wrong' does not exist.")); + } + } + + @Test + @EnvironmentCondition(value = "4.2.0", comparison = EnvironmentCondition.Comparison.GE) + void shouldThrowExceptionWhenExecutingWrongQueryWithJsonError() throws SQLException { + try (Connection connection = createConnection(); Statement statement = connection.createStatement()) { + statement.execute("set advanced_mode=1"); + statement.execute("set enable_json_error_output_format=true"); + String errorMessage = assertThrows(FireboltException.class, () -> statement.executeQuery("select wrong query")).getMessage(); + assertTrue(errorMessage.contains("Column 'wrong' does not exist.")); + } + } + @Test void shouldReturnTrueWhenExecutingAStatementThatReturnsAResultSet() throws SQLException { try (Connection connection = createConnection(); Statement statement = connection.createStatement()) { diff --git a/src/integrationTest/java/integration/tests/SystemEngineTest.java b/src/integrationTest/java/integration/tests/SystemEngineTest.java index 6ba024310..7b40c7293 100644 --- a/src/integrationTest/java/integration/tests/SystemEngineTest.java +++ b/src/integrationTest/java/integration/tests/SystemEngineTest.java @@ -38,6 +38,7 @@ import java.util.stream.Stream; import static com.firebolt.jdbc.connection.FireboltConnectionUserPassword.SYSTEM_ENGINE_NAME; +import static integration.EnvironmentCondition.Attribute.fireboltVersion; import static java.lang.String.format; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Map.entry; @@ -91,6 +92,25 @@ void shouldSelect1() throws SQLException { } } + @Test + void shouldThrowExceptionWhenExecutingWrongQuery() throws SQLException { + try (Connection connection = createConnection(getSystemEngineName()); Statement statement = connection.createStatement()) { + String errorMessage = assertThrows(FireboltException.class, () -> statement.executeQuery("select wrong query")).getMessage(); + assertTrue(errorMessage.contains("Column 'wrong' does not exist.")); + } + } + + @Test + @EnvironmentCondition(value = "4.2.0", attribute = fireboltVersion, comparison = EnvironmentCondition.Comparison.GE) + void shouldThrowExceptionWhenExecutingWrongQueryWithJsonError() throws SQLException { + try (Connection connection = createConnection(getSystemEngineName()); Statement statement = connection.createStatement()) { + statement.execute("set advanced_mode=1"); + statement.execute("set enable_json_error_output_format=true"); + String errorMessage = assertThrows(FireboltException.class, () -> statement.executeQuery("select wrong query")).getMessage(); + assertTrue(errorMessage.contains("Column 'wrong' does not exist.")); + } + } + @Test void shouldFailToSelectFromCustomDbUsingSystemEngine() throws SQLException { ConnectionInfo current = integration.ConnectionInfo.getInstance(); @@ -103,7 +123,7 @@ void shouldFailToSelectFromCustomDbUsingSystemEngine() throws SQLException { Collection expectedErrorMessages = Set.of( "Queries against table dummy require a user engine", "The system engine doesn't support queries against table dummy. Run this query on a user engine.", - "Line 1, Column 22: relation \"dummy\" does not exist"); + "relation \"dummy\" does not exist"); try (Connection systemConnection = DriverManager.getConnection(systemEngineJdbcUrl, principal, secret); Connection customConnection = DriverManager.getConnection(customEngineJdbcUrl, principal, secret)) { diff --git a/src/main/java/com/firebolt/jdbc/client/FireboltClient.java b/src/main/java/com/firebolt/jdbc/client/FireboltClient.java index 29c9e93d0..f7b597846 100644 --- a/src/main/java/com/firebolt/jdbc/client/FireboltClient.java +++ b/src/main/java/com/firebolt/jdbc/client/FireboltClient.java @@ -3,6 +3,8 @@ import com.firebolt.jdbc.connection.CacheListener; import com.firebolt.jdbc.connection.FireboltConnection; import com.firebolt.jdbc.exception.FireboltException; +import com.firebolt.jdbc.exception.ServerError; +import com.firebolt.jdbc.exception.ServerError.Error.Location; import com.firebolt.jdbc.resultset.compress.LZ4InputStream; import com.firebolt.jdbc.util.CloseableUtil; import lombok.Getter; @@ -13,6 +15,7 @@ import okhttp3.Request; import okhttp3.RequestBody; import okhttp3.Response; +import org.json.JSONException; import org.json.JSONObject; import java.io.BufferedReader; @@ -24,13 +27,17 @@ import java.nio.charset.StandardCharsets; import java.sql.SQLException; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Objects; import java.util.Optional; import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.stream.Collectors; import static java.lang.String.format; @@ -46,6 +53,7 @@ public abstract class FireboltClient implements CacheListener { private static final String HEADER_USER_AGENT = "User-Agent"; private static final String HEADER_PROTOCOL_VERSION = "Firebolt-Protocol-Version"; private static final Logger log = Logger.getLogger(FireboltClient.class.getName()); + private static final Pattern plainErrorPattern = Pattern.compile("Line (\\d+), Column (\\d+): (.*)$", Pattern.MULTILINE); private final OkHttpClient httpClient; private final String headerUserAgentValue; protected final FireboltConnection connection; @@ -142,17 +150,19 @@ protected void validateResponse(String host, Response response, Boolean isCompre throw new FireboltException(format("Could not query Firebolt at %s. The engine is not running.", host), statusCode); } String errorMessageFromServer = extractErrorMessage(response, isCompress); - validateResponse(host, statusCode, errorMessageFromServer); + ServerError serverError = parseServerError(errorMessageFromServer); + String processedErrorMessage = serverError.getErrorMessage(); + validateResponse(host, statusCode, processedErrorMessage); String errorResponseMessage = format( "Server failed to execute query with the following error:%n%s%ninternal error:%n%s", - errorMessageFromServer, getInternalErrorWithHeadersText(response)); + processedErrorMessage, getInternalErrorWithHeadersText(response)); if (statusCode == HTTP_UNAUTHORIZED) { getConnection().removeExpiredTokens(); throw new FireboltException(format( "Could not query Firebolt at %s. The operation is not authorized or the token is expired and has been cleared from the cache.%n%s", - host, errorResponseMessage), statusCode, errorMessageFromServer); + host, errorResponseMessage), statusCode, processedErrorMessage); } - throw new FireboltException(errorResponseMessage, statusCode, errorMessageFromServer); + throw new FireboltException(errorResponseMessage, statusCode, processedErrorMessage); } } @@ -194,6 +204,50 @@ private String extractErrorMessage(Response response, boolean isCompress) throws return new String(entityBytes, StandardCharsets.UTF_8); } + private ServerError parseServerError(String responseText) { + try { + if (responseText == null) { + return new ServerError(null, null); + } + ServerError serverError = new ServerError(new JSONObject(responseText)); + ServerError.Error[] errors = serverError.getErrors() == null ? null : Arrays.stream(serverError.getErrors()).map(this::updateError).toArray(ServerError.Error[]::new); + return new ServerError(serverError.getQuery(), errors); + } catch (JSONException e) { + String message = responseText; + Location location = null; + Entry locationAndText = getLocationFromMessage(responseText); + if (locationAndText != null) { + location = locationAndText.getKey(); + message = locationAndText.getValue(); + } + return new ServerError(null, new ServerError.Error[] {new ServerError.Error(null, message, null, null, null, null, null, location)}); + } + } + + private ServerError.Error updateError(ServerError.Error error) { + if (error == null || error.getDescription() == null) { + return error; + } + Entry locationAndText = getLocationFromMessage(error.getDescription()); + if (locationAndText == null) { + return error; + } + Location location = Objects.requireNonNullElse(error.getLocation(), locationAndText.getKey()); + return new ServerError.Error(error.getCode(), error.getName(), error.getSeverity(), error.getSource(), + locationAndText.getValue(), error.getResolution(), error.getHelpLink(), location); + } + + private Entry getLocationFromMessage(String responseText) { + Matcher m = plainErrorPattern.matcher(responseText); + if (m.find()) { + int line = Integer.parseInt(m.group(1)); + int column = Integer.parseInt(m.group(2)); + String message = m.group(3); + return Map.entry(new Location(line, column, column), message); + } + return null; + } + protected boolean isCallSuccessful(int statusCode) { return statusCode >= 200 && statusCode <= 299; // Call is considered successful when the status code is 2XX } diff --git a/src/main/java/com/firebolt/jdbc/exception/ServerError.java b/src/main/java/com/firebolt/jdbc/exception/ServerError.java new file mode 100644 index 000000000..80e756984 --- /dev/null +++ b/src/main/java/com/firebolt/jdbc/exception/ServerError.java @@ -0,0 +1,210 @@ +package com.firebolt.jdbc.exception; + +import lombok.Getter; +import lombok.ToString; +import org.json.JSONArray; +import org.json.JSONObject; + +import java.util.Arrays; +import java.util.Map; +import java.util.Objects; +import java.util.function.Function; +import java.util.function.IntFunction; +import java.util.stream.IntStream; +import java.util.stream.Stream; + +import static java.util.Optional.ofNullable; +import static java.util.stream.Collectors.joining; +import static java.util.stream.Collectors.toUnmodifiableMap; + +@Getter +@ToString +public class ServerError { + private final Query query; + private final Error[] errors; + + public ServerError(Query query, Error[] errors) { + this.query = query; + this.errors = errors; + } + + public ServerError(JSONObject json) { + this(fromJson(json.optJSONObject("query"), Query::new), fromJson(json.optJSONArray("errors"), Error::new, Error[]::new)); + } + + private static T[] fromJson(JSONArray jsonArray, Function factory, IntFunction arrayFactory) { + return jsonArray == null ? null : IntStream.range(0, jsonArray.length()).boxed().map(jsonArray::getJSONObject).map(factory).toArray(arrayFactory); + } + + private static T fromJson(JSONObject json, Function factory) { + return ofNullable(json).map(factory).orElse(null); + } + + public String getErrorMessage() { + return errors == null ? + null + : + Arrays.stream(errors) + .filter(Objects::nonNull) + .map(e -> Stream.of(e.severity, e.source, e.code, e.name, e.description).filter(Objects::nonNull).map(Object::toString).collect(joining(" "))) + .collect(joining("; ")); + } + + public Query getQuery() { + return query; + } + + public Error[] getErrors() { + return errors; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + ServerError that = (ServerError) o; + return Objects.equals(query, that.query) && Objects.deepEquals(errors, that.errors); + } + + @Override + public int hashCode() { + return Objects.hash(query, Arrays.hashCode(errors)); + } + + @Getter + @ToString + public static class Query { + private final String queryId; + private final String requestId; + private final String queryLabel; + + public Query(String queryId, String requestId, String queryLabel) { + this.queryId = queryId; + this.requestId = requestId; + this.queryLabel = queryLabel; + } + + Query(JSONObject json) { + this(json.optString("query_id", null), json.optString("request_id", null), json.optString("query_label", null)); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Query query = (Query) o; + return Objects.equals(queryId, query.queryId) && Objects.equals(requestId, query.requestId) && Objects.equals(queryLabel, query.queryLabel); + } + + @Override + public int hashCode() { + return Objects.hash(queryId, requestId, queryLabel); + } + } + + @Getter + @ToString + public static class Error { + private final String code; + private final String name; + private final Severity severity; + private final Source source; + private final String description; + private final String resolution; + private final String helpLink; + private final Location location; + + @SuppressWarnings("java:S107") // the price of the immutability + public Error(String code, String name, Severity severity, Source source, String description, String resolution, String helpLink, Location location) { + this.code = code; + this.name = name; + this.severity = severity; + this.source = source; + this.description = description; + this.resolution = resolution; + this.helpLink = helpLink; + this.location = location; + } + + Error(JSONObject json) { + this(json.optString("code", null), json.optString("name", null), + json.optEnum(Severity.class, "severity"), + ofNullable(json.optString("source", null)).map(Source::fromText).orElse(Source.UNKNOWN), + json.optString("description", null), json.optString("resolution", null), json.optString("helpLink", null), + ofNullable(json.optJSONObject("location", null)).map(Location::new).orElse(null)); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Error error = (Error) o; + return Objects.equals(code, error.code) && Objects.equals(name, error.name) && severity == error.severity && source == error.source && Objects.equals(description, error.description) && Objects.equals(resolution, error.resolution) && Objects.equals(helpLink, error.helpLink) && Objects.equals(location, error.location); + } + + @Override + public int hashCode() { + return Objects.hash(code, name, severity, source, description, resolution, helpLink, location); + } + + public enum Severity { + ERROR, WARNING, + } + + public enum Source { + SYSTEM_ERROR("System Error"), + USER_ERROR("User Error"), + UNKNOWN("Unknown"), + USER_WARNING("User Warning"), + SYSTEM_WARNING("System Warning"), + SYSTEM_SEVIER_WARNING("System Sevier Warning"), + ; + private final String text; + private static final Map textToSource = Arrays.stream(values()).collect(toUnmodifiableMap(e -> e.text, e -> e)); + + Source(String text) { + this.text = text; + } + + public static Source fromText(String text) { + return textToSource.get(text); + } + + @Override + public String toString() { + return text; + } + } + + @Getter + @ToString + public static class Location { + private final int failingLine; + private final int startOffset; + private final int endOffset; + + public Location(int failingLine, int startOffset, int endOffset) { + this.failingLine = failingLine; + this.startOffset = startOffset; + this.endOffset = endOffset; + } + + Location(JSONObject json) { + this(json.optInt("failingLine"), json.optInt("startOffset"), json.optInt("endOffset")); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Location location = (Location) o; + return failingLine == location.failingLine && startOffset == location.startOffset && endOffset == location.endOffset; + } + + @Override + public int hashCode() { + return Objects.hash(failingLine, startOffset, endOffset); + } + } + } +} diff --git a/src/test/java/com/firebolt/jdbc/client/FireboltClientTest.java b/src/test/java/com/firebolt/jdbc/client/FireboltClientTest.java index b5ff5cc9d..fa0212048 100644 --- a/src/test/java/com/firebolt/jdbc/client/FireboltClientTest.java +++ b/src/test/java/com/firebolt/jdbc/client/FireboltClientTest.java @@ -13,6 +13,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Mockito; @@ -104,8 +105,18 @@ void cannotExtractCompressedErrorMessage() throws IOException { } } - @Test - void canExtractErrorMessage() throws IOException { + // FIR-33934: This test does not validate the fields of ServerError except error message including Location because this information is not exposed to FireboltException + @ParameterizedTest + @CsvSource(value = { + "Error happened; Error happened", + "Error happened on server: Line 16, Column 64: Something bad happened; Something bad happened", + "{}; null", + "{\"errors:\": [null]}; null", + "{errors: [{\"name\": \"Something wrong happened\"}]}; Something wrong happened", + "{errors: [{\"description\": \"Error happened on server: Line 16, Column 64: Something bad happened\"}]}; Something bad happened", + "{errors: [{\"description\": \"Error happened on server: Line 16, Column 64: Something bad happened\", \"location\": {\"failingLine\": 20, \"startOffset\": 30, \"endOffset\": 40}}]}; Something bad happened" + }, delimiter = ';') + void canExtractErrorMessage(String rawMessage, String expectedMessage) throws IOException { try (Response response = mock(Response.class)) { when(response.code()).thenReturn(HTTP_NOT_FOUND); ResponseBody responseBody = mock(ResponseBody.class); @@ -113,7 +124,7 @@ void canExtractErrorMessage() throws IOException { ByteArrayOutputStream baos = new ByteArrayOutputStream(); OutputStream compressedStream = new LZ4OutputStream(baos, 100); - compressedStream.write("Error happened".getBytes()); + compressedStream.write(rawMessage.getBytes()); compressedStream.flush(); compressedStream.close(); when(responseBody.bytes()).thenReturn(baos.toByteArray()); // compressed error message @@ -121,7 +132,7 @@ void canExtractErrorMessage() throws IOException { FireboltClient client = Mockito.mock(FireboltClient.class, Mockito.CALLS_REAL_METHODS); FireboltException e = assertThrows(FireboltException.class, () -> client.validateResponse("the_host", response, true)); assertEquals(ExceptionType.RESOURCE_NOT_FOUND, e.getType()); - assertTrue(e.getMessage().contains("Error happened")); // compressed error message is used as-is + assertTrue(e.getMessage().contains(expectedMessage)); // compressed error message is used as-is } } diff --git a/src/test/java/com/firebolt/jdbc/exception/ServerErrorTest.java b/src/test/java/com/firebolt/jdbc/exception/ServerErrorTest.java new file mode 100644 index 000000000..dc01ab015 --- /dev/null +++ b/src/test/java/com/firebolt/jdbc/exception/ServerErrorTest.java @@ -0,0 +1,34 @@ +package com.firebolt.jdbc.exception; + +import org.json.JSONObject; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import java.util.stream.Stream; + +import static com.firebolt.jdbc.exception.ServerError.Error; +import static com.firebolt.jdbc.exception.ServerError.Error.Location; +import static com.firebolt.jdbc.exception.ServerError.Error.Severity; +import static com.firebolt.jdbc.exception.ServerError.Error.Source; +import static com.firebolt.jdbc.exception.ServerError.Query; +import static org.junit.jupiter.api.Assertions.assertEquals; + +class ServerErrorTest { + protected static Stream parse() { + return Stream.of( + Arguments.of("{}", new ServerError(null, null)), + Arguments.of("{\"query\": {}, \"errors\": []}", new ServerError(new Query(null, null, null), new Error[0])), + Arguments.of("{\"query\": {\"query_id\": \"qid\", \"request_id\": \"rid\", \"query_label\": \"ql\"}, \"errors\": []}", new ServerError(new Query("qid", "rid", "ql"), new Error[0])), + Arguments.of("{\"errors\": [{}]}", new ServerError(null, new Error[] {new Error(null, null, null, Source.UNKNOWN, null, null, null, null)})), + Arguments.of("{\"errors\": [{\"code\": \"c1\", \"name\": \"name1\", \"severity\": \"ERROR\", \"source\": \"System Error\", \"description\": \"description1\", \"resolution\": \"resolution1\", \"helpLink\": \"http://help1.com\", \"location\": {\"failingLine\": 1, \"startOffset\": 10, \"endOffset\": 100}}]}", + new ServerError(null, new Error[] {new Error("c1", "name1", Severity.ERROR, Source.SYSTEM_ERROR, "description1", "resolution1", "http://help1.com", new Location(1, 10, 100))})) + ); + } + + @ParameterizedTest + @MethodSource("parse") + void parse(String json, ServerError expected) { + assertEquals(expected, new ServerError(new JSONObject(json))); + } +}