gnodet-bot commented on code in PR #12695:
URL: https://github.com/apache/maven/pull/12695#discussion_r4050902975
##########
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, RERAISED] Javadoc contract violation — says "always non-negative"
but default returns `-1`.**
Line 161: `@return the sequence number, always non-negative`
Line 165: `return -1; // violates the stated contract`
This mismatch was raised in the prior review and remains unaddressed in the
squash. The default value of `-1` is used by `DefaultLogEvent`'s convenience
constructor (tests, programmatic construction) — so `-1` is a legitimate
sentinel meaning "not assigned".
Fix the Javadoc: change to `@return the sequence number, or {@code -1} if
not available`.
##########
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) {
Review Comment:
**[Medium, RERAISED] `mojoSkipped()` still does not call `setMojoId(null)` —
mojo ID leaked on skip.**
The base branch called `ProjectBuildLogAppender.setMojoId(null)` in
`mojoSkipped()` (mirroring `mojoSucceeded` and `mojoFailed`). This PR removed
that call.
With this gap: if a mojo is skipped, the MOJO_ID thread-local is never
cleared. Any subsequent log event on that thread (e.g. from a later mojo
startup or a project-level log) will be misrouted into the skipped mojo's scope
in the build report.
`mojoSucceeded` and `mojoFailed` both correctly call `setMojoId(null)`
(lines 132, 139). `mojoSkipped` must do the same:
```java
@Override
public void mojoSkipped(ExecutionEvent event) {
setMdc(event);
ProjectBuildLogAppender.setMojoId(null); // missing
delegate.mojoSkipped(event);
}
```
--
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]
