korlov42 commented on code in PR #2613:
URL: https://github.com/apache/ignite-3/pull/2613#discussion_r1350164871
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/JdbcQueryEventHandlerImpl.java:
##########
@@ -498,7 +498,7 @@ <T> CompletableFuture<T> doInSession(SessionAwareAction<T>
action) {
return action.perform(newSessionId);
}
- return
CompletableFuture.failedFuture(IgniteExceptionMapperUtil.mapToPublicException(error));
+ return
CompletableFuture.failedFuture(IgniteExceptionMapperUtil.mapToPublicSqlException(error));
Review Comment:
as far as I see, none of the tests are affected by this change. So, why have
you changed this?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItTablesApiTest.java:
##########
@@ -140,8 +142,10 @@ public void testTableAlreadyCreated() {
Table tbl = createTable(ignite0, TABLE_NAME);
- // TODO: IGNITE-20388 Fix it
- assertThrowsWithCause(() -> createTable(ignite0, TABLE_NAME),
TableExistsValidationException.class);
+ assertThrowsSqlException(
+ Catalog.VALIDATION_ERR,
Review Comment:
I'm wondering why does it throw `Catalog.VALIDATION_ERR` and not
`Sql.STMT_VALIDATION_ERR`?
The same question for rest of the changes in this class
##########
modules/core/src/main/java/org/apache/ignite/internal/lang/IgniteExceptionMapperUtil.java:
##########
@@ -83,36 +86,59 @@ static void registerMapping(
* @return Public exception.
*/
public static Throwable mapToPublicException(Throwable origin) {
- if (origin instanceof Error) {
- if (origin instanceof AssertionError) {
- return new IgniteException(INTERNAL_ERR, origin);
+ return mapToPublicExceptionInternal(origin, (e) -> {
+ if (e instanceof Error) {
+ return e;
}
- return origin;
- }
-
- IgniteExceptionMapper<? extends Exception, ? extends Exception> m =
EXCEPTION_CONVERTERS.get(origin.getClass());
- if (m != null) {
- Exception mapped = map(m, origin);
+ if (e instanceof IgniteException || e instanceof
IgniteCheckedException) {
+ return e;
+ }
- assert mapped instanceof IgniteException || mapped instanceof
IgniteCheckedException :
- "Unexpected mapping of internal exception to a public one
[origin=" + origin + ", mapped=" + mapped + ']';
+ // There are no exception mappings for the given exception. This
case should be considered as internal error.
+ return new IgniteException(INTERNAL_ERR, origin);
+ });
+ }
- return mapped;
- }
+ /**
+ * This method provides a mapping from internal exception to SQL public
ones.
+ *
+ * <p>The rules of mapping are the following:</p>
+ * <ul>
+ * <li>any instance of {@link Error} is returned as is, except {@link
AssertionError}
+ * that will always be mapped to {@link IgniteException} with the
{@link Common#INTERNAL_ERR} error code.</li>
+ * <li>any instance of {@link IgniteException} or {@link
IgniteCheckedException} is returned as is.</li>
+ * <li>any other instance of {@link TraceableException} is wrapped
into {@link SqlException}
+ * with the original {@link TraceableException#traceId() traceUd}
and {@link TraceableException#code() code}.</li>
+ * <li>if there are no any mappers that can do a mapping from the
given error to a public exception,
+ * then {@link SqlException} with the {@link Common#INTERNAL_ERR}
error code is returned.</li>
+ * </ul>
+ *
+ * @param origin Exception to be mapped.
+ * @return Public exception.
+ */
+ public static Throwable mapToPublicSqlException(Throwable origin) {
Review Comment:
I would say such method should not be a part of the core library. Only sql
should return SqlException, thus this mapping should be placed to sql-related
module
--
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]
