gnodet-bot commented on code in PR #12695:
URL: https://github.com/apache/maven/pull/12695#discussion_r4046264489
##########
api/maven-api-core/src/main/java/org/apache/maven/api/build/report/LogEvent.java:
##########
@@ -157,7 +158,8 @@ default long threadId() {
* providing a global ordering across all event sources (Log API,
* JUL, and direct SLF4J).
*
- * @return the sequence number, or {@code -1} if unavailable
+ * @return the sequence number, always non-negative
+ * @since 4.1.0
*/
default long sequenceNumber() {
return -1;
Review Comment:
**[Medium] Javadoc contract violation — says "always non-negative" but
default returns `-1`.**
The Javadoc was changed to `@return the sequence number, always
non-negative`, but the default implementation on the very next line still
returns `-1`:
```java
default long sequenceNumber() {
return -1; // violates the "always non-negative" contract
}
```
Downstream consumers that check `sequenceNumber() >= 0` to determine whether
a sequence number is available (as `writeLogEvent` in `BuildReportJsonWriter`
does) will produce correct output for concrete implementations — but any
`LogEvent` implementation that relies on the default will silently advertise
`-1` as a valid sequence number, breaking the stated contract.
Either:
- Revert the Javadoc to `@return the sequence number, or {@code -1} if
unavailable` (matching the actual default), OR
- Change the default to throw `UnsupportedOperationException` to force
implementors to provide a real value
##########
api/maven-api-core/src/main/java/org/apache/maven/api/plugin/Log.java:
##########
@@ -38,65 +38,48 @@
public interface Log {
/**
* {@return true if the <b>trace</b> error level is enabled}
- * <p>
- * The default implementation returns {@code false} for backward
- * compatibility with existing {@code Log} implementations.
+ * @since 4.1.0
*/
- default boolean isTraceEnabled() {
- return false;
- }
+ boolean isTraceEnabled();
Review Comment:
**[High] Binary-incompatible breaking change — `AbstractMethodError` at
runtime.**
`isTraceEnabled()`, `trace(CharSequence)`, `trace(CharSequence, Throwable)`,
`trace(Throwable)`, `trace(Supplier<String>)`, and `trace(Supplier<String>,
Throwable)` were all `default` methods in the parent PR (#12694). This PR
removes the `default` keyword, making them abstract again.
Any third-party plugin that:
1. Implemented `Log` without overriding `trace` methods (which was safe with
the defaults), OR
2. Tested their plugin against Maven 4.1-SNAPSHOT before this PR landed
...will now get `AbstractMethodError` at runtime. This is a
binary-incompatible change to a `@Provider`-annotated SPI interface.
The deleted `DefaultLogTest.defaultTraceIsDisabled()` test was the
regression guard for exactly this scenario.
If trace support is now mandatory for all `Log` implementors (because Maven
itself calls these methods), the change needs a migration path: keep the
`default` implementations as delegating stubs that throw
`UnsupportedOperationException` with a clear message, or at minimum document
the break in a migration guide and hold it for a major version bump.
##########
impl/maven-core/src/main/java/org/apache/maven/logging/LoggingExecutionListener.java:
##########
@@ -129,45 +129,40 @@ public void mojoStarted(ExecutionEvent event) {
@Override
public void mojoSucceeded(ExecutionEvent event) {
setMdc(event);
- delegate.mojoSucceeded(event);
ProjectBuildLogAppender.setMojoId(null);
+ delegate.mojoSucceeded(event);
}
@Override
public void mojoFailed(ExecutionEvent event) {
setMdc(event);
- delegate.mojoFailed(event);
ProjectBuildLogAppender.setMojoId(null);
+ delegate.mojoFailed(event);
}
@Override
public void mojoSkipped(ExecutionEvent event) {
setMdc(event);
delegate.mojoSkipped(event);
Review Comment:
**[Medium] Mojo ID leak on skipped mojos — log events misrouted.**
The base branch (`feature/logging-foundation`) called
`ProjectBuildLogAppender.setMojoId(null)` in `mojoSkipped()` after the delegate
call. This PR removed that call.
With this change, if a mojo is skipped (e.g. `MojoSkipped` event), the
`MOJO_ID` thread-local is never cleared on that thread. Any subsequent log
events on the same thread — module-level infrastructure messages, the next
mojo's startup — will be incorrectly attributed to the skipped mojo's ID in the
MDC.
In parallel builds (`-T`), where the thread is reused across module
executions, this could misroute log events from a later mojo on the same thread
into the skipped mojo's buffer in `BuildReportCollector`.
```suggestion
public void mojoSkipped(ExecutionEvent event) {
setMdc(event);
delegate.mojoSkipped(event);
ProjectBuildLogAppender.setMojoId(null);
}
```
##########
impl/maven-logging/src/main/java/org/apache/maven/slf4j/MavenSimpleLogger.java:
##########
@@ -120,6 +118,60 @@ protected void write(int level, String loggerName, String
cleanMessage, StringBu
}
}
+ /**
+ * Append a colorized throwable rendering to the given builder.
+ * Reuses the existing formatting logic for consistency with console
output.
+ */
+ private void appendFormattedThrowable(StringBuilder sb, Throwable t,
String prefix) {
+ MessageBuilder builder =
builder().a(prefix).failure(t.getClass().getName());
+ if (t.getMessage() != null) {
+ builder.a(": ").failure(t.getMessage());
+ }
+ sb.append(builder.toString()).append(System.lineSeparator());
+ appendStackTrace(sb, t, prefix);
+ }
+
+ private void appendStackTrace(StringBuilder sb, Throwable t, String
prefix) {
+ MessageBuilder builder = builder();
+ for (StackTraceElement e : t.getStackTrace()) {
+ builder.a(prefix);
+ builder.a(" ");
+ builder.strong("at");
+ builder.a(" ");
+ builder.a(e.getClassName());
+ builder.a(".");
+ builder.a(e.getMethodName());
+ builder.a("(");
+ builder.strong(getLocation(e));
+ builder.a(")");
+ sb.append(builder.toString()).append(System.lineSeparator());
+ builder.setLength(0);
+ }
+ for (Throwable se : t.getSuppressed()) {
+ builder.a(prefix)
+ .a(" ")
+ .strong("Suppressed")
+ .a(": ")
+ .a(se.getClass().getName());
+ if (se.getMessage() != null) {
+ builder.a(": ").failure(se.getMessage());
+ }
+ sb.append(builder.toString()).append(System.lineSeparator());
+ builder.setLength(0);
+ appendStackTrace(sb, se, prefix + " ");
+ }
+ Throwable cause = t.getCause();
+ if (cause != null && t != cause) {
+ builder.a(prefix).strong("Caused by").a(":
").a(cause.getClass().getName());
+ if (cause.getMessage() != null) {
+ builder.a(": ").failure(cause.getMessage());
+ }
+ sb.append(builder.toString()).append(System.lineSeparator());
+ builder.setLength(0);
+ appendStackTrace(sb, cause, prefix);
Review Comment:
**[Low] Recursive `appendStackTrace` has no depth guard —
`StackOverflowError` on deep exception chains.**
`appendStackTrace` calls itself recursively for suppressed exceptions (line
~161: `appendStackTrace(sb, se, prefix + " ")`) and for cause chains (here,
line ~171: `appendStackTrace(sb, cause, prefix)`). There is no maximum depth
limit.
A deeply nested exception chain (e.g. a plugin that wraps exceptions 50+
levels deep, or an exception with a large suppressed tree) can overflow the
call stack. The prefix concatenation for suppressed exceptions also grows the
string on each recursive level, adding minor allocation pressure.
Add a depth counter:
```java
private void appendStackTrace(StringBuilder sb, Throwable t, String prefix) {
appendStackTrace(sb, t, prefix, 0);
}
private void appendStackTrace(StringBuilder sb, Throwable t, String prefix,
int depth) {
if (depth > 20) {
sb.append(prefix).append(" [...depth limit
reached]").append(System.lineSeparator());
return;
}
// ... existing body ...
appendStackTrace(sb, se, prefix + " ", depth + 1);
// ...
appendStackTrace(sb, cause, prefix, depth + 1);
}
```
--
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]
