Copilot commented on code in PR #7854:
URL: https://github.com/apache/ignite-3/pull/7854#discussion_r3007410362
##########
modules/core/src/main/java/org/apache/ignite/internal/util/ViewUtils.java:
##########
@@ -48,36 +53,35 @@ public static <T> T sync(CompletableFuture<T> fut) {
Thread.currentThread().interrupt(); // Restore interrupt flag.
throw sneakyThrow(ensurePublicException(e));
- } catch (ExecutionException e) {
+ } catch (ExecutionException | CancellationException e) {
Throwable cause = unwrapCause(e);
throw sneakyThrow(ensurePublicException(cause));
}
}
/**
- * Wraps an exception in an IgniteException, extracting trace identifier
and error code when the specified exception or one of its
- * causes is an IgniteException itself.
+ * Ensures the provided exception complies with our public API.
+ * <ol>
+ * <li>Finds the root cause of the exception.</li>
+ * <li>Errors caused by {@link IgniteException} and {@link
IgniteCheckedException} are treated as server-side exceptions.
+ * Their stack trace is rebased to the current stack trace.</li>
+ * <li>Other errors are mapped using {@link
IgniteExceptionMapperUtil#mapToPublicException(Throwable, Function)},
+ * are treated as client-side errors, and their stack trace is not
rebased.</li>
+ * </ol>
Review Comment:
The Javadoc says this method “finds the root cause of the exception”, but
the implementation no longer unwraps (`unwrapCause`) and will treat wrapper
exceptions (e.g., `CompletionException`, `ExecutionException`) as client-side
errors. Either restore unwrapping at the start of `ensurePublicException` or
update the Javadoc to reflect the current behavior (and ensure callers always
unwrap before invoking it).
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItPkOnlyTableCrossApiTest.java:
##########
@@ -157,7 +158,13 @@ public void testKeyValueView(TestEnvironment env) {
rwTx -> {
IgniteException ex = assertThrows(IgniteException.class,
() -> tab.keyValueView(KeyObject.class,
Integer.class).put(rwTx, key, 1));
- assertThat(ex.getCause().getCause(),
is(instanceOf(MarshallerException.class)));
+
+ publicException(
+ MarshallerException.class,
Review Comment:
This block constructs a `publicException(...)` matcher but never asserts it
against the thrown exception. As written, the test doesn’t validate the
marshalling failure anymore (and `ex` is unused). Use `assertThat(ex, ...)` (or
similar) with this matcher, or otherwise assert on `ex` directly.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/KeyValueViewImpl.java:
##########
@@ -611,7 +611,11 @@ private KvMarshaller<K, V> marshaller(int schemaVersion) {
marsh = marshallerFactory.apply(registry.schema(schemaVersion));
this.marsh = marsh;
} catch (Exception ex) {
- throw new MarshallerException(ex.getMessage(), ex);
+ if (ex instanceof MarshallerException) {
+ throw ex;
Review Comment:
`throw ex;` here won’t compile because `ex` has the static type `Exception`
(a checked exception), while this method doesn’t declare `throws Exception`. If
the intent is to rethrow an existing `MarshallerException`, catch it separately
(`catch (MarshallerException e) { throw e; }`) or cast before throwing (`throw
(MarshallerException) ex`).
```suggestion
throw (MarshallerException) ex;
```
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/exception/handler/SqlExceptionHandler.java:
##########
@@ -63,14 +63,21 @@ private static ErrorUiComponent
connectionErrUiComponent(IgniteException e) {
return fromIgniteException("Client error", e);
}
- if (e.getCause() instanceof IgniteClientConnectionException) {
- IgniteClientConnectionException cause =
(IgniteClientConnectionException) e.getCause();
+ if (e instanceof IgniteClientConnectionException) {
+ IgniteClientConnectionException cause =
(IgniteClientConnectionException) e;
InvalidCredentialsException invalidCredentialsException =
findCause(cause, InvalidCredentialsException.class);
if (invalidCredentialsException != null) {
+ var msg = invalidCredentialsException.getMessage();
+ var details = msg.substring(0, msg.indexOf('\n'));
+ int traceInfoIdx = msg.indexOf(" TraceId:");
+ if (traceInfoIdx > 0) {
+ details = msg.substring(0, traceInfoIdx);
Review Comment:
`invalidCredentialsException.getMessage()` is not guaranteed to contain a
newline. If `msg.indexOf('\n')` returns `-1`, `substring(0, -1)` will throw
`StringIndexOutOfBoundsException`. Please guard for missing newline (and
consider null messages) before taking substrings, falling back to the full
message when the delimiter isn’t present.
```suggestion
String details = msg;
if (msg != null) {
int newlineIdx = msg.indexOf('\n');
if (newlineIdx >= 0) {
details = msg.substring(0, newlineIdx);
}
int traceInfoIdx = msg.indexOf(" TraceId:");
if (traceInfoIdx > 0) {
details = msg.substring(0, traceInfoIdx);
}
```
##########
modules/security/src/integrationTest/java/org/apache/ignite/internal/ssl/ItSslTest.java:
##########
@@ -239,11 +242,13 @@ void
clientCanNotConnectWithSslAndInvalidTrustStorePassword() {
}
});
- assertThat(ex.getMessage(), is("Client SSL configuration error:
keystore password was incorrect"));
- // Exceptions thrown from the synchronous build method are copied
to include the sync method
- assertThat(ex.getCause(),
isA(IgniteClientConnectionException.class));
- assertThat(ex.getCause().getCause(), isA(IOException.class));
- assertThat(ex.getCause().getCause().getMessage(), is("keystore
password was incorrect"));
+ IgniteExceptionTestUtils.publicException(
+ IgniteClientConnectionException.class,
+ CLIENT_SSL_CONFIGURATION_ERR,
+ "Client SSL configuration error: keystore password was
incorrect",
+ emptyList()
+ ).withCause(isA(IOException.class));
Review Comment:
`publicException(...).withCause(...)` creates a matcher but the result is
not used in an assertion, so this check is currently a no-op. Assert it against
`ex` (e.g., via `assertThat(ex, ...)`) or restore the previous explicit
assertions.
```suggestion
assertThat(ex, IgniteExceptionTestUtils.publicException(
IgniteClientConnectionException.class,
CLIENT_SSL_CONFIGURATION_ERR,
"Client SSL configuration error: keystore password was
incorrect",
emptyList()
).withCause(isA(IOException.class)));
```
##########
modules/client/src/integrationTest/java/org/apache/ignite/internal/client/ItThinClientComputeTest.java:
##########
@@ -886,14 +879,11 @@ <I, M, T> void
testExecuteMapReduceExceptionPropagation(Class<? extends MapReduc
TaskDescriptor<I, String> taskDescriptor =
TaskDescriptor.builder(taskClass).build();
IgniteException cause =
getExceptionInTaskExecutionAsync(client.compute().submitMapReduce(taskDescriptor,
null));
- assertThat(cause.getMessage(), containsString("Custom job error"));
- assertEquals(Jobs.TRACE_ID, cause.traceId());
- assertEquals(COLUMN_NOT_FOUND_ERR, cause.code());
- assertInstanceOf(Jobs.CustomException.class, cause);
- assertNotNull(cause.getCause());
- String hint = cause.getCause().getMessage();
-
- assertEquals("To see the full stack trace, set
clientConnector.sendServerExceptionStackTraceToClient:true on the server",
hint);
+ publicExceptionWithHint(
+ Jobs.CustomException.class,
+ COLUMN_NOT_FOUND_ERR,
+ "Custom job error"
+ ).withTraceId(is(Jobs.TRACE_ID));
Review Comment:
The created matcher is not asserted against `cause`, so this test currently
doesn’t verify the propagated exception (it’s another no-op). Wrap it in
`assertThat(cause, ...)` (and keep the traceId constraint via
`.withTraceId(...)`).
```suggestion
assertThat(
cause,
publicExceptionWithHint(
Jobs.CustomException.class,
COLUMN_NOT_FOUND_ERR,
"Custom job error"
).withTraceId(is(Jobs.TRACE_ID))
);
```
##########
modules/compute/src/integrationTest/java/org/apache/ignite/internal/compute/ItComputeBaseTest.java:
##########
@@ -972,23 +967,57 @@ private JobDescriptor<Collection<Tuple>,
Collection<Tuple>> tupleCollectionJob()
.units(units()).build();
}
- static Matcher<Exception> computeJobFailedException(String causeClass,
String causeMsgSubstring) {
- return traceableException(ComputeException.class)
- .withCode(is(COMPUTE_JOB_FAILED_ERR))
- .withMessage(both(containsString("Job execution failed:"))
- .and(containsString(causeClass)))
- .withCause(hasMessage(containsString(causeMsgSubstring)));
+ Matcher<Exception> computeJobFailedException(String causeClass, String
causeMsgSubstring) {
+ return computeJobFailedException(clientType(), causeClass,
causeMsgSubstring);
+ }
+
+ static Matcher<Exception> computeJobFailedException(ClientType clientType,
String causeClass, String causeMsgSubstring) {
+ var msgMatcher = both(containsString("Job execution
failed:")).and(containsString(causeClass));
+ switch (clientType) {
+ case JAVA:
+ return publicException(
+ ComputeException.class,
+ COMPUTE_JOB_FAILED_ERR,
+ "",
+ List.of(new Cause(causeClass, causeMsgSubstring))
+ )
+ .withMessage(msgMatcher);
+ case EMBEDDED:
Review Comment:
In the `JAVA` branch, `publicException(..., causes)` is followed by
`.withMessage(msgMatcher)`. Since `withMessage` overwrites the previously
configured message checks, this drops the intended assertion on
`causeMsgSubstring` (and any other message constraints from `publicException`).
Combine the matchers (single `withMessage(allOf(...))`) or adjust
`publicException(...)` / `TraceableExceptionMatcher` so message matchers are
additive.
##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/IgniteExceptionTestUtils.java:
##########
@@ -67,6 +70,58 @@ public static TraceableExceptionMatcher publicException(int
expectedCode, String
return traceableException(IgniteException.class, expectedCode,
containMessage);
}
+ /**
+ * Creates a matcher for public exceptions with stacktrace sent from the
server.
+ *
+ * @param expectedClass expected exception type.
+ * @param expectedCode expected code.
+ * @param containMessage message that exception should contain.
+ * @param causes Expected causes to be in the server sent stacktrace.
+ * @return message that exception should contain.
+ */
+ public static TraceableExceptionMatcher publicException(
+ Class<? extends TraceableException> expectedClass,
+ int expectedCode,
+ String containMessage,
+ List<Cause> causes
+ ) {
+ var ret = traceableException(expectedClass)
+ .withCode(is(expectedCode))
+ .withMessage(containsString(containMessage))
+ .withCause(nullValue(Throwable.class));
+
+ for (var cause : causes) {
+ if (cause.message() != null) {
+ ret = ret.withMessage(containsString(String.format("Caused by:
%s: %s", cause.className(), cause.message())));
+ } else {
+ ret = ret.withMessage(containsString(String.format("Caused by:
%s", cause.className())));
Review Comment:
`TraceableExceptionMatcher.withMessage(...)` stores only a single matcher,
so calling it repeatedly overwrites previous message checks. In this helper,
the loop replaces the initial `containMessage` matcher, and only the last
`Cause` (or only the hint in `publicExceptionWithHint`) ends up being asserted.
Consider combining all message requirements into a single matcher (e.g.,
`allOf(...)` / `both(...).and(...)`), or change `TraceableExceptionMatcher` to
accumulate multiple message matchers.
--
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]
