sk0x50 commented on code in PR #1024:
URL: https://github.com/apache/ignite-3/pull/1024#discussion_r966746894
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/Commons.java:
##########
@@ -803,7 +805,13 @@ public static SqlNodeList parse(String qry,
SqlParser.Config parserCfg) {
try {
return parse(new SourceStringReader(qry), parserCfg);
} catch (SqlParseException e) {
- throw new SqlException(QUERY_INVALID_ERR, "Failed to parse query",
e);
+ String message = "Failed to parse query";
Review Comment:
I would prefer the following:
```
throw withCauseAndCode(
SqlException::new,
QUERY_INVALID_ERR,
"Failed to parse query: " +
extractCauseMessage(e.getMessage()),
e);
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/cli/commands/sql/ItSqlCommandTest.java:
##########
@@ -63,8 +63,8 @@ void wrongJdbcUrl() {
assertAll(
() -> assertExitCodeIs(1),
this::assertOutputIsEmpty,
- // TODO: https://issues.apache.org/jira/browse/IGNITE-17090
- () -> assertErrOutputIs(CLIENT_CONNECTION_FAILED_MESSAGE +
System.lineSeparator())
+ () ->
assertErrOutputContains(CLIENT_CONNECTION_FAILED_MESSAGE),
+ () -> assertErrOutputContains("no-such-host.com: Name or
service not known")
Review Comment:
Well, this message will be "localized". For instance it can be `Этот хост
неизвестен (no-such-host.com)` instead of `no-such-host.com: Name or service
not known`. Perhaps, we just only need to check `() ->
assertErrOutputContains(CLIENT_CONNECTION_FAILED_MESSAGE)`. It is up to you.
Both variants are OK to me.
##########
modules/client/src/main/java/org/apache/ignite/internal/jdbc/JdbcStatement.java:
##########
@@ -734,16 +733,6 @@ public final void timeout(int timeout) throws SQLException
{
}
private static SQLException toSqlException(CompletionException e) {
- if (e.getCause() instanceof IgniteException) {
- IgniteException cause = (IgniteException) e.getCause();
- String message = cause.getMessage();
-
- if (message != null) {
- if (message.contains("Failed to parse query")) {
- return new SQLException("Sql query execution failed.",
SqlStateCode.PARSING_EXCEPTION, e);
- }
- }
- }
- return new SQLException("Internal server error.",
SqlStateCode.INTERNAL_ERROR, e);
+ return new SQLException(e.getCause());
Review Comment:
It does not seem correct. You should not lose original `CompletionException`
because it will lead to the fact that its stack frame will be lost as well.
Let's change this line to `return new SQLException(e));`
##########
modules/core/src/main/java/org/apache/ignite/lang/ErrorGroup.java:
##########
@@ -201,6 +201,13 @@ public static String errorMessage(UUID traceId, int code,
String message) {
* @return New error message with predefined prefix.
*/
public static String errorMessage(UUID traceId, String groupName, int
code, String message) {
+ if (message != null) {
Review Comment:
Nice catch! I think we don't need to create a new string and replace special
characters. The `DOTALL` option should do the trick.
```
/** Error message pattern. */
private static final Pattern EXCEPTION_MESSAGE_PATTERN =
Pattern.compile(..., DOTALL);
```
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/handler/SqlExceptionHandler.java:
##########
@@ -38,9 +47,46 @@ public class SqlExceptionHandler implements
ExceptionHandler<SQLException> {
public static final String CLIENT_CONNECTION_FAILED_MESSAGE = "Connection
failed";
public static final String CONNECTION_BROKE_MESSAGE = "Connection error";
+ public static final String UNRECOGNIZED_ERROR_MESSAGE = "Unrecognized
error while processing SQL query ";
+
+ private final Map<Integer, Function<IgniteException,
ErrorComponentBuilder>> sqlExceptionMappers = new HashMap<>();
+
+ /** Default constructor. */
+ public SqlExceptionHandler() {
+ sqlExceptionMappers.put(Client.CONNECTION_ERR,
this::connectionErrUiComponent);
+ sqlExceptionMappers.put(Sql.QUERY_INVALID_ERR,
this::invalidQueryErrUiComponent);
+ }
+
+ private ErrorComponentBuilder invalidQueryErrUiComponent(IgniteException
e) {
+ return fromExWithHeader(e, PARSING_ERROR_MESSAGE);
+ }
+
+ private ErrorComponentBuilder unrecognizedErrComponent(IgniteException e) {
+ return fromExWithHeader(e, UNRECOGNIZED_ERROR_MESSAGE);
+ }
+
+ private ErrorComponentBuilder connectionErrUiComponent(IgniteException e) {
+ if (e.getCause() instanceof IgniteClientConnectionException) {
+ return fromExWithHeader((IgniteClientConnectionException)
e.getCause(), CLIENT_CONNECTION_FAILED_MESSAGE);
+ }
+
+ return fromExWithHeader(e, CLIENT_CONNECTION_FAILED_MESSAGE);
+ }
+
+ private static ErrorComponentBuilder fromExWithHeader(IgniteException e,
String header) {
+ return ErrorUiComponent.builder()
+ .header(header)
+ .errorCode(e.codeAsString())
+ .traceId(e.traceId())
+ .details(ErrorGroup.extractCauseMessage(e.getMessage()));
+ }
@Override
public int handle(ExceptionWriter err, SQLException e) {
+ if (e.getCause() instanceof IgniteException) {
Review Comment:
In order to properly handle `ExecutionException` and `CompletionException` I
would propose:
```
Throwable unwrappedCause = ExceptionUtils.unwrapCause(e.getCause());
if (unwrappedCause instanceof IgniteException) {
return handleIgniteException(err, (IgniteException)
unwrappedCause);
}
```
By the way, do we need to take care of `IgniteCheckedException`'s?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
