From d52c2036224911d93fe3185f521768037e62a437 Mon Sep 17 00:00:00 2001 From: "yuzhao.cyz" Date: Thu, 9 May 2019 14:50:02 +0800 Subject: [PATCH] Revert "[CALCITE-2845] Avoid duplication of exception messages" This reverts commit 80ccd20 --- .../calcite/avatica/AvaticaConnection.java | 11 +-- .../calcite/avatica/AvaticaSqlException.java | 5 +- .../calcite/avatica/AvaticaStatement.java | 8 +- .../org/apache/calcite/avatica/Helper.java | 5 +- .../avatica/remote/AbstractHandler.java | 13 +--- .../apache/calcite/avatica/jdbc/JdbcMeta.java | 43 ++++++----- .../calcite/avatica/ExceptionUtils.java | 44 ----------- .../calcite/avatica/RemoteDriverTest.java | 13 ++-- .../calcite/avatica/jdbc/JdbcMetaTest.java | 6 +- .../avatica/remote/RemoteMetaTest.java | 11 ++- .../server/BasicAuthHttpServerTest.java | 6 +- .../server/DigestAuthHttpServerTest.java | 6 +- .../avatica/test/StringContainsOnce.java | 75 ------------------- 13 files changed, 58 insertions(+), 188 deletions(-) delete mode 100644 server/src/test/java/org/apache/calcite/avatica/ExceptionUtils.java delete mode 100644 server/src/test/java/org/apache/calcite/avatica/test/StringContainsOnce.java diff --git a/core/src/main/java/org/apache/calcite/avatica/AvaticaConnection.java b/core/src/main/java/org/apache/calcite/avatica/AvaticaConnection.java index e28fe461e6..b3552f84a3 100644 --- a/core/src/main/java/org/apache/calcite/avatica/AvaticaConnection.java +++ b/core/src/main/java/org/apache/calcite/avatica/AvaticaConnection.java @@ -552,11 +552,9 @@ protected ResultSet executeQueryInternal(AvaticaStatement statement, statement.updateCount = metaResultSet.updateCount; signature2 = executeResult.resultSets.get(0).signature; } - } catch (SQLException e) { - // We don't add meaningful info yet, so just rethrow the original exception - throw e; } catch (Exception e) { - throw HELPER.createException("Error while executing a prepared statement", e); + e.printStackTrace(); + throw HELPER.createException(e.getMessage(), e); } final TimeZone timeZone = getTimeZone(); @@ -575,12 +573,9 @@ protected ResultSet executeQueryInternal(AvaticaStatement statement, statement.openResultSet.execute(); isUpdateCapable(statement); } - } catch (SQLException e) { - // We don't add meaningful info yet, so just rethrow the original exception - throw e; } catch (Exception e) { throw HELPER.createException( - "Error while executing a resultset", e); + "exception while executing query: " + e.getMessage(), e); } return statement.openResultSet; } diff --git a/core/src/main/java/org/apache/calcite/avatica/AvaticaSqlException.java b/core/src/main/java/org/apache/calcite/avatica/AvaticaSqlException.java index af71381c0c..bb02f1f844 100644 --- a/core/src/main/java/org/apache/calcite/avatica/AvaticaSqlException.java +++ b/core/src/main/java/org/apache/calcite/avatica/AvaticaSqlException.java @@ -82,10 +82,7 @@ public String getRemoteServer() { } void printServerStackTrace(PrintStreamOrWriter streamOrWriter) { - List traces = this.stackTraces; - for (int i = 0; i < traces.size(); i++) { - String serverStackTrace = traces.get(i); - streamOrWriter.println("Remote driver error #" + i + ":"); + for (String serverStackTrace : this.stackTraces) { streamOrWriter.println(serverStackTrace); } } diff --git a/core/src/main/java/org/apache/calcite/avatica/AvaticaStatement.java b/core/src/main/java/org/apache/calcite/avatica/AvaticaStatement.java index ac76ce3ed2..2d3c75689a 100644 --- a/core/src/main/java/org/apache/calcite/avatica/AvaticaStatement.java +++ b/core/src/main/java/org/apache/calcite/avatica/AvaticaStatement.java @@ -160,8 +160,8 @@ protected void executeInternal(String sql) throws SQLException { } } } catch (RuntimeException e) { - throw AvaticaConnection.HELPER.createException( - "Error while executing SQL \"" + sql + "\"", e); + throw AvaticaConnection.HELPER.createException("Error while executing SQL \"" + sql + "\": " + + e.getMessage(), e); } throw new RuntimeException("Failed to successfully execute query after " @@ -231,8 +231,8 @@ public ResultSet executeQuery(String sql) throws SQLException { } return openResultSet; } catch (RuntimeException e) { - throw AvaticaConnection.HELPER.createException( - "Error while executing SQL \"" + sql + "\"", e); + throw AvaticaConnection.HELPER.createException("Error while executing SQL \"" + sql + "\": " + + e.getMessage(), e); } } diff --git a/core/src/main/java/org/apache/calcite/avatica/Helper.java b/core/src/main/java/org/apache/calcite/avatica/Helper.java index 215e2b9a6c..fe716db541 100644 --- a/core/src/main/java/org/apache/calcite/avatica/Helper.java +++ b/core/src/main/java/org/apache/calcite/avatica/Helper.java @@ -50,10 +50,7 @@ public SQLException createException(String message, String sql, Exception e) { if (null != rte.getRpcMetadata()) { serverAddress = rte.getRpcMetadata().serverAddress; } - // Note: we don't add "e" as a cause, so we add its message to the message of - // the newly created exception - return new AvaticaSqlException(message + ". " + e.getMessage(), - rte.getSqlState(), rte.getErrorCode(), + return new AvaticaSqlException(message, rte.getSqlState(), rte.getErrorCode(), rte.getServerExceptions(), serverAddress); } return new SQLException(message, e); diff --git a/core/src/main/java/org/apache/calcite/avatica/remote/AbstractHandler.java b/core/src/main/java/org/apache/calcite/avatica/remote/AbstractHandler.java index ab9c7c6213..b291d4c893 100644 --- a/core/src/main/java/org/apache/calcite/avatica/remote/AbstractHandler.java +++ b/core/src/main/java/org/apache/calcite/avatica/remote/AbstractHandler.java @@ -145,7 +145,7 @@ private HandlerResponse createErrorResponse(Exception e, int statusCode) { * @param e The Exception to summarize. * @return A summary message for the Exception. */ - private String getCausalChain(Throwable e) { + private String getCausalChain(Exception e) { StringBuilder sb = new StringBuilder(16); Throwable curr = e; // Could use Guava, but that would increase dependency set unnecessarily. @@ -156,17 +156,6 @@ private String getCausalChain(Throwable e) { String message = curr.getMessage(); sb.append(curr.getClass().getSimpleName()).append(": "); sb.append(null == message ? NULL_EXCEPTION_MESSAGE : message); - Throwable[] suppressed = curr.getSuppressed(); - if (suppressed.length > 0) { - sb.append(", suppressed: ["); - String sep = ""; - for (Throwable throwable : suppressed) { - sb.append(sep); - sb.append(getCausalChain(throwable)); - sep = ", "; - } - sb.append("]"); - } curr = curr.getCause(); } if (sb.length() == 0) { diff --git a/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java b/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java index 824e65e88b..7f19b11a3b 100644 --- a/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java +++ b/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java @@ -304,7 +304,7 @@ public Map getDatabaseProperties(ConnectionHandle ch) } return map; } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -315,7 +315,7 @@ private static Object addProperty(Map map, try { propertyValue = p.method.invoke(metaData); } catch (IllegalAccessException | InvocationTargetException e) { - throw propagate(e); + throw new RuntimeException(e); } } else { propertyValue = p.defaultValue; @@ -333,7 +333,7 @@ public MetaResultSet getTables(ConnectionHandle ch, String catalog, Pat schemaPa int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -359,7 +359,7 @@ public MetaResultSet getColumns(ConnectionHandle ch, String catalog, Pat schemaP int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -370,7 +370,7 @@ public MetaResultSet getSchemas(ConnectionHandle ch, String catalog, Pat schemaP int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -380,7 +380,7 @@ public MetaResultSet getCatalogs(ConnectionHandle ch) { int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -390,7 +390,7 @@ public MetaResultSet getTableTypes(ConnectionHandle ch) { int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -403,7 +403,7 @@ public MetaResultSet getProcedures(ConnectionHandle ch, String catalog, Pat sche int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -416,7 +416,7 @@ public MetaResultSet getProcedureColumns(ConnectionHandle ch, String catalog, Pa int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -429,7 +429,7 @@ public MetaResultSet getColumnPrivileges(ConnectionHandle ch, String catalog, St int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -442,7 +442,7 @@ public MetaResultSet getTablePrivileges(ConnectionHandle ch, String catalog, Pat int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -457,7 +457,7 @@ public MetaResultSet getBestRowIdentifier(ConnectionHandle ch, String catalog, S int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -470,7 +470,7 @@ public MetaResultSet getVersionColumns(ConnectionHandle ch, String catalog, Stri int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -483,7 +483,7 @@ public MetaResultSet getPrimaryKeys(ConnectionHandle ch, String catalog, String int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -509,7 +509,7 @@ public MetaResultSet getTypeInfo(ConnectionHandle ch) { int stmtId = registerMetaStatement(rs); return JdbcResultSet.create(ch.id, stmtId, rs); } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -630,7 +630,7 @@ public StatementHandle createStatement(ConnectionHandle ch) { throw new RuntimeException("Connection already exists: " + ch.id); } } catch (SQLException e) { - throw propagate(e); + throw new RuntimeException(e); } } @@ -691,9 +691,14 @@ protected void apply(Connection conn, ConnectionProperties connProps) } } - static RuntimeException propagate(Throwable e) throws E { - // We have nothing to add, so just throw the original exception - throw (E) e; + RuntimeException propagate(Throwable e) { + if (e instanceof RuntimeException) { + throw (RuntimeException) e; + } else if (e instanceof Error) { + throw (Error) e; + } else { + throw new RuntimeException(e); + } } public StatementHandle prepare(ConnectionHandle ch, String sql, diff --git a/server/src/test/java/org/apache/calcite/avatica/ExceptionUtils.java b/server/src/test/java/org/apache/calcite/avatica/ExceptionUtils.java deleted file mode 100644 index eff382ff9d..0000000000 --- a/server/src/test/java/org/apache/calcite/avatica/ExceptionUtils.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to you under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.calcite.avatica; - -import java.io.PrintWriter; -import java.io.StringWriter; -import java.util.Objects; - -/** Prints full stacktrace of {@link java.lang.Throwable} as a {@code String}. */ -public class ExceptionUtils { - private ExceptionUtils() { - // The constructor is extremely secret - } - - /** Prints full stacktrace of {@link java.lang.Throwable} as a {@code String}. - * @param e exception - * @return string representation of a given exception - */ - public static String toString(Exception e) { - //noinspection ThrowableResultOfMethodCallIgnored - Objects.requireNonNull(e); - StringWriter sw = new StringWriter(); - PrintWriter pw = new PrintWriter(sw); - e.printStackTrace(pw); - pw.flush(); - return sw.toString(); - } -} - -// End ExceptionUtils.java diff --git a/server/src/test/java/org/apache/calcite/avatica/RemoteDriverTest.java b/server/src/test/java/org/apache/calcite/avatica/RemoteDriverTest.java index 4d473ef093..66fcf8c940 100644 --- a/server/src/test/java/org/apache/calcite/avatica/RemoteDriverTest.java +++ b/server/src/test/java/org/apache/calcite/avatica/RemoteDriverTest.java @@ -67,12 +67,11 @@ import java.util.UUID; import java.util.concurrent.Callable; -import static org.apache.calcite.avatica.test.StringContainsOnce.containsStringOnce; - import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.core.StringContains.containsString; import static org.hamcrest.core.StringStartsWith.startsWith; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; @@ -915,9 +914,8 @@ private void checkPrepareBindExecuteFetch(Connection connection) fail("expected error, got " + resultSet); } catch (SQLException e) { LOG.info("Caught expected error", e); - String message = ExceptionUtils.toString(e); - assertThat(message, - containsStringOnce("unbound parameter")); + assertThat(e.getMessage(), + containsString("exception while executing query: unbound parameter")); } final ParameterMetaData parameterMetaData = ps.getParameterMetaData(); @@ -1844,9 +1842,8 @@ public LoggingLocalProtobufService(Service service, ProtobufTranslation translat @Override public Response _apply(Request request) { final RequestLogger logger = THREAD_LOG.get(); - String jsonRequest = null; try { - jsonRequest = JsonService.MAPPER.writeValueAsString(request); + String jsonRequest = JsonService.MAPPER.writeValueAsString(request); logger.requestStart(jsonRequest); Response response = super._apply(request); @@ -1856,7 +1853,7 @@ public LoggingLocalProtobufService(Service service, ProtobufTranslation translat return response; } catch (Exception e) { - throw new RuntimeException("Request " + jsonRequest + " failed", e); + throw new RuntimeException(e); } } } diff --git a/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java b/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java index 0c80f44ce3..f3ebc8a1d4 100644 --- a/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java +++ b/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java @@ -50,11 +50,13 @@ public class JdbcMetaTest { @Test public void testExceptionPropagation() throws SQLException { JdbcMeta meta = new JdbcMeta("url"); final Throwable e = new Exception(); + final RuntimeException rte; try { meta.propagate(e); fail("Expected an exception to be thrown"); - } catch (Throwable caughtException) { - assertThat(caughtException, is(e)); + } catch (RuntimeException caughtException) { + rte = caughtException; + assertThat(rte.getCause(), is(e)); } } diff --git a/server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java b/server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java index 452309c2cf..2a1fe8052a 100644 --- a/server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java +++ b/server/src/test/java/org/apache/calcite/avatica/remote/RemoteMetaTest.java @@ -329,8 +329,10 @@ private static String longString(String fragment, int length) { DriverManager.getConnection(url, "john", "doe"); fail("expected exception"); } catch (RuntimeException e) { - assertEquals("Remote driver error: " - + "SQLInvalidAuthorizationSpecException: invalid authorization specification - " + assertEquals("Remote driver error: RuntimeException: " + + "java.sql.SQLInvalidAuthorizationSpecException: invalid authorization specification" + + " - not found: john" + + " -> SQLInvalidAuthorizationSpecException: invalid authorization specification - " + "not found: john" + " -> HsqlException: invalid authorization specification - not found: john", e.getMessage()); @@ -355,8 +357,9 @@ private static String longString(String fragment, int length) { stmt2.executeQuery("select * from buffer"); fail("expected exception"); } catch (Exception e) { - assertEquals("Error -1 (00000) : Error while executing SQL \"select * from buffer\". " - + "Remote driver error: " + assertEquals("Error -1 (00000) : Error while executing SQL \"select * from buffer\": " + + "Remote driver error: RuntimeException: java.sql.SQLSyntaxErrorException: " + + "user lacks privilege or object not found: BUFFER -> " + "SQLSyntaxErrorException: user lacks privilege or object not found: BUFFER -> " + "HsqlException: user lacks privilege or object not found: BUFFER", e.getMessage()); diff --git a/server/src/test/java/org/apache/calcite/avatica/server/BasicAuthHttpServerTest.java b/server/src/test/java/org/apache/calcite/avatica/server/BasicAuthHttpServerTest.java index e11f1cbcaa..9f9914ad85 100644 --- a/server/src/test/java/org/apache/calcite/avatica/server/BasicAuthHttpServerTest.java +++ b/server/src/test/java/org/apache/calcite/avatica/server/BasicAuthHttpServerTest.java @@ -149,8 +149,10 @@ public class BasicAuthHttpServerTest extends HttpAuthBase { readWriteData(url, "DISALLOWED_DB_USER", props); fail("Expected an exception"); } catch (RuntimeException e) { - assertEquals("Remote driver error: " - + "SQLInvalidAuthorizationSpecException: invalid authorization specification - " + assertEquals("Remote driver error: RuntimeException: " + + "java.sql.SQLInvalidAuthorizationSpecException: invalid authorization specification" + + " - not found: USER1" + + " -> SQLInvalidAuthorizationSpecException: invalid authorization specification - " + "not found: USER1" + " -> HsqlException: invalid authorization specification - not found: USER1", e.getMessage()); diff --git a/server/src/test/java/org/apache/calcite/avatica/server/DigestAuthHttpServerTest.java b/server/src/test/java/org/apache/calcite/avatica/server/DigestAuthHttpServerTest.java index 73b5bb5967..a5d8e8667b 100644 --- a/server/src/test/java/org/apache/calcite/avatica/server/DigestAuthHttpServerTest.java +++ b/server/src/test/java/org/apache/calcite/avatica/server/DigestAuthHttpServerTest.java @@ -163,8 +163,10 @@ public class DigestAuthHttpServerTest extends HttpAuthBase { readWriteData(url, "DISALLOWED_HSQLDB_USER", props); fail("Expected a failure"); } catch (RuntimeException e) { - assertEquals("Remote driver error: " - + "SQLInvalidAuthorizationSpecException: invalid authorization specification - " + assertEquals("Remote driver error: RuntimeException: " + + "java.sql.SQLInvalidAuthorizationSpecException: invalid authorization specification" + + " - not found: USER1" + + " -> SQLInvalidAuthorizationSpecException: invalid authorization specification - " + "not found: USER1" + " -> HsqlException: invalid authorization specification - not found: USER1", e.getMessage()); diff --git a/server/src/test/java/org/apache/calcite/avatica/test/StringContainsOnce.java b/server/src/test/java/org/apache/calcite/avatica/test/StringContainsOnce.java deleted file mode 100644 index 3a55c1e89c..0000000000 --- a/server/src/test/java/org/apache/calcite/avatica/test/StringContainsOnce.java +++ /dev/null @@ -1,75 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to you under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.calcite.avatica.test; - -import org.hamcrest.Description; -import org.hamcrest.Factory; -import org.hamcrest.Matcher; -import org.hamcrest.core.SubstringMatcher; - -/** - * Tests if the argument is a string that contains a substring exactly once. - */ -public class StringContainsOnce extends SubstringMatcher { - public StringContainsOnce(String substring) { - super(substring); - } - - @Override public void describeMismatchSafely(String item, Description mismatchDescription) { - int cnt = countMatches(item); - if (cnt == 0) { - mismatchDescription.appendText("pattern is not found in \""); - } else if (cnt == 2) { - mismatchDescription.appendText("pattern is present more than once in \""); - } - mismatchDescription.appendText(item); - mismatchDescription.appendText("\""); - } - - @Override protected boolean evalSubstringOf(String s) { - return countMatches(s) == 1; - } - - private int countMatches(String s) { - int indexOf = s.indexOf(substring); - if (indexOf < 0) { - return 0; - } - // There should be just a single match - return s.indexOf(substring, indexOf + 1) == -1 ? 1 : 2; - } - - @Override protected String relationship() { - return "containing exactly once"; - } - - /** - * Creates a matcher that matches if the examined {@link String} contains the specified - * {@link String} anywhere exactly once. - * - *

For example: - * 

assertThat("myStringOfNote", containsStringOnce("ring"))
- * @param substring substring - * @return matcher - */ - @Factory - public static Matcher containsStringOnce(String substring) { - return new StringContainsOnce(substring); - } -} - -// End StringContainsOnce.java