gnodet-bot commented on code in PR #12695:
URL: https://github.com/apache/maven/pull/12695#discussion_r4046547659
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Session.java:
##########
@@ -46,6 +46,20 @@
@ThreadSafe
public interface Session extends ProtoSession {
+ /**
+ * Returns the environment context of this build: the flags, properties,
and
+ * platform settings that were active when the session started.
+ *
+ * <p>The returned instance is immutable and captures a snapshot of the
invocation
+ * context (offline mode, user properties, selected projects, etc.). It is
the same
Review Comment:
**[Medium] Javadoc contract: "same object" claim is false.**
The Javadoc states: _"It is the same object recorded in the structured build
report."_
It is not. `DefaultSession.buildEnvironment()` calls
`BuildReportCollector.buildEnvironment(getMavenSession())` — a static factory
that constructs a fresh `DefaultBuildEnvironment` on every call, reading system
properties and allocating new collections each time. The `BuildReportCollector`
stores the pre-captured instance in a `private volatile BuildEnvironment
buildEnvironment` field (set at `SessionStarted`), but `DefaultSession` has no
access to it — it always re-computes.
Result: `session.buildEnvironment()` returns a different object than
`buildReport.environment()`, and calling `session.buildEnvironment()` twice
returns two distinct instances.
Fix: either (a) remove the "same object" sentence and replace with
"equivalent immutable snapshot", or (b) wire `DefaultSession` to use the
pre-captured field from `BuildReportCollector` (requires injecting the
collector into `DefaultSession`, which may be cleaner than it sounds since
`BuildReportCollector` is already a `@Named @Singleton`).
```suggestion
* <p>The returned instance is immutable and captures a snapshot of the
invocation
* context (offline mode, user properties, selected projects, etc.). The
snapshot
* is equivalent to the one recorded in the structured build report.
```
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultSession.java:
##########
@@ -172,6 +172,12 @@ public int getDegreeOfConcurrency() {
return getMavenSession().getRequest().getDegreeOfConcurrency();
}
+ @Nonnull
+ @Override
+ public org.apache.maven.api.BuildEnvironment buildEnvironment() {
+ return
org.apache.maven.internal.build.BuildReportCollector.buildEnvironment(getMavenSession());
Review Comment:
**[Medium] `buildEnvironment()` re-allocates on every call — no caching.**
Each call to `Session.buildEnvironment()` goes through:
1. `BuildReportCollector.buildEnvironment(getMavenSession())` — static method
2. Reads all system properties (`os.name`, `java.vendor`, etc.) from
`System.getProperty()`
3. Iterates over `request.getUserProperties()` to redact sensitive keys
4. Constructs a new `DefaultBuildEnvironment` record with new
`LinkedHashMap` + `TreeMap`
The `@ThreadSafe` contract on `Session` means plugins may call this
concurrently. This is safe (no mutation) but wasteful — a plugin calling
`session.buildEnvironment()` on each mojo execution pays full allocation per
call.
The `BuildReportCollector` already pre-computes and caches the result in a
`private volatile BuildEnvironment buildEnvironment` field. `DefaultSession`
should use that cached instance instead of re-calling the static factory.
Simplest fix: inject `BuildReportCollector` into `DefaultSession` and
delegate:
```suggestion
return
org.apache.maven.internal.build.BuildReportCollector.buildEnvironment(getMavenSession());
```
(Or cache the result locally in `DefaultSession` with a `volatile` field
populated on first call.)
##########
impl/maven-core/src/main/java/org/apache/maven/internal/build/BuildReportCollector.java:
##########
@@ -0,0 +1,662 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.internal.build;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.nio.file.AtomicMoveNotSupportedException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
+import java.time.Duration;
+import java.time.Instant;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.maven.api.BuildEnvironment;
+import org.apache.maven.api.MonotonicClock;
+import org.apache.maven.api.build.report.BuildReport;
+import org.apache.maven.api.build.report.BuildStatus;
+import org.apache.maven.api.build.report.FailureReport;
+import org.apache.maven.api.build.report.LogEvent;
+import org.apache.maven.api.build.report.ModuleReport;
+import org.apache.maven.api.build.report.MojoReport;
+import org.apache.maven.eventspy.AbstractEventSpy;
+import org.apache.maven.execution.BuildFailure;
+import org.apache.maven.execution.BuildSuccess;
+import org.apache.maven.execution.BuildSummary;
+import org.apache.maven.execution.ExecutionEvent;
+import org.apache.maven.execution.MavenExecutionResult;
+import org.apache.maven.execution.MavenSession;
+import org.apache.maven.logging.ProjectBuildLogAppender;
+import org.apache.maven.plugin.MojoExecution;
+import org.apache.maven.project.MavenProject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Collects build lifecycle events and produces a structured {@link
BuildReport}
+ * at the end of the session.
+ * <p>
+ * Registered as an {@link org.apache.maven.eventspy.EventSpy} via {@code
@Named}/{@code @Singleton},
+ * following the same pattern as {@code DefaultPluginValidationManager}.
+ * <p>
+ * Thread-safe: concurrent module builds (with {@code -T}) each write to their
+ * own entry in a {@link ConcurrentHashMap}.
+ * <p>
+ * Log capture: registers a callback on {@link ProjectBuildLogAppender} to
+ * receive the already-formed {@link LogEvent} objects produced by the main
+ * logging pipeline. Uses thread-based tracking to associate events with
+ * the currently-executing mojo or module.
+ *
+ * @since 4.1.0
+ */
+@Singleton
+@Named
+public final class BuildReportCollector extends AbstractEventSpy {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(BuildReportCollector.class);
+
+ static final String REPORT_DIR = "build-reports";
+ static final String REPORT_LATEST = "build-report-latest.json";
+
+ private static final int MAX_STACKTRACE_LINES = 30;
+
+ /**
+ * Maximum number of log events captured per scope (mojo, module, or
build).
+ * Beyond this, events are dropped and a truncation notice is appended.
Review Comment:
**[Medium] Javadoc still lies about truncation notice — not fixed since
prior review.**
Line 95: _"Beyond this, events are dropped and a truncation notice is
appended."_
`captureLogEvent` (lines 282–307) still silently drops events when
`buffer.size() >= MAX_LOG_EVENTS_PER_SCOPE` with no `else` branch and no
sentinel event appended to any buffer. A consumer that reads a 500-event mojo
output has no way to know whether the output was complete.
This was raised in the previous review and not addressed by this commit.
Either implement the truncation notice or fix the Javadoc:
```suggestion
* Maximum number of log events captured per scope (mojo, module, or
build).
* Beyond this, events are dropped silently.
```
##########
impl/maven-core/src/main/java/org/apache/maven/internal/build/BuildReportJsonWriter.java:
##########
@@ -0,0 +1,437 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.internal.build;
+
+import org.apache.maven.api.BuildEnvironment;
+import org.apache.maven.api.build.report.BuildReport;
+import org.apache.maven.api.build.report.FailureReport;
+import org.apache.maven.api.build.report.LogEvent;
+import org.apache.maven.api.build.report.ModuleReport;
+import org.apache.maven.api.build.report.MojoReport;
+import org.apache.maven.api.services.BuilderProblem;
+
+/**
+ * Serializes a {@link BuildReport} to JSON without any external library
dependency.
+ * <p>
+ * The output is human-readable (indented with 2 spaces) and designed to be
+ * stable across Maven versions — field order is fixed, and new fields are
+ * appended at the end of each object.
+ */
+final class BuildReportJsonWriter {
+
+ private BuildReportJsonWriter() {}
+
+ /**
+ * Serialize the given report to a pretty-printed JSON string.
+ */
+ static String toJson(BuildReport report) {
+ StringBuilder sb = new StringBuilder(4096);
+ writeReport(sb, report, 0);
+ sb.append('\n');
+ return sb.toString();
+ }
+
+ private static void writeReport(StringBuilder sb, BuildReport report, int
indent) {
+ sb.append("{\n");
+ writeField(sb, indent + 1, "formatVersion", report.formatVersion());
+ writeField(sb, indent + 1, "status", report.status().name());
+ writeField(sb, indent + 1, "duration", report.duration().toString());
+ writeField(sb, indent + 1, "startTime", report.startTime().toString());
+ writeField(sb, indent + 1, "mavenVersion", report.mavenVersion());
+ writeField(sb, indent + 1, "javaVersion", report.javaVersion());
+ writeStringArray(sb, indent + 1, "goals", report.goals());
+ writeField(sb, indent + 1, "project", report.project());
+ writeField(sb, indent + 1, "multiModule", report.multiModule());
+ writeField(sb, indent + 1, "threads", report.threads());
+
+ // environment object
+ writeIndent(sb, indent + 1);
+ sb.append("\"environment\": ");
+ writeEnvironment(sb, report.environment(), indent + 1);
+ sb.append(",\n");
+
+ // modules array
+ writeIndent(sb, indent + 1);
+ sb.append("\"modules\": ");
+ if (report.modules().isEmpty()) {
+ sb.append("[]");
+ } else {
+ sb.append("[\n");
+ for (int i = 0; i < report.modules().size(); i++) {
+ writeIndent(sb, indent + 2);
+ writeModule(sb, report.modules().get(i), indent + 2);
+ if (i < report.modules().size() - 1) {
+ sb.append(',');
+ }
+ sb.append('\n');
+ }
+ writeIndent(sb, indent + 1);
+ sb.append(']');
+ }
+ sb.append(",\n");
+
+ // failures array
+ writeIndent(sb, indent + 1);
+ sb.append("\"failures\": ");
+ if (report.failures().isEmpty()) {
+ sb.append("[]");
+ } else {
+ sb.append("[\n");
+ for (int i = 0; i < report.failures().size(); i++) {
+ writeIndent(sb, indent + 2);
+ writeFailure(sb, report.failures().get(i), indent + 2);
+ if (i < report.failures().size() - 1) {
+ sb.append(',');
+ }
+ sb.append('\n');
+ }
+ writeIndent(sb, indent + 1);
+ sb.append(']');
+ }
+ sb.append(",\n");
+
+ // problems array
+ writeIndent(sb, indent + 1);
+ sb.append("\"problems\": ");
+ if (report.problems().isEmpty()) {
+ sb.append("[]");
+ } else {
+ sb.append("[\n");
+ for (int i = 0; i < report.problems().size(); i++) {
+ writeIndent(sb, indent + 2);
+ writeProblem(sb, report.problems().get(i), indent + 2);
+ if (i < report.problems().size() - 1) {
+ sb.append(',');
+ }
+ sb.append('\n');
+ }
+ writeIndent(sb, indent + 1);
+ sb.append(']');
+ }
+ sb.append(",\n");
+
+ // output array — build-level log lines (outside any module)
+ writeOutputArray(sb, indent + 1, report.output());
+ sb.append('\n');
+
+ writeIndent(sb, indent);
+ sb.append('}');
+ }
+
+ private static void writeEnvironment(StringBuilder sb, BuildEnvironment
env, int indent) {
+ sb.append("{\n");
+ writeStringArray(sb, indent + 1, "goals", env.goals());
+ writeStringMap(sb, indent + 1, "userProperties", env.userProperties());
+ writeStringMap(sb, indent + 1, "systemInfo", env.systemInfo());
+ writeField(sb, indent + 1, "localRepository", env.localRepository());
+ writeStringArray(sb, indent + 1, "activeProfiles",
env.activeProfiles());
+ writeStringArray(sb, indent + 1, "selectedProjects",
env.selectedProjects());
+ writeNullableField(sb, indent + 1, "resumeFrom", env.resumeFrom(),
true);
+ writeField(sb, indent + 1, "reactorFailureBehavior",
env.reactorFailureBehavior());
+ writeField(sb, indent + 1, "offline", env.offline());
+ writeField(sb, indent + 1, "updateSnapshots", env.updateSnapshots());
+ writeField(sb, indent + 1, "threads", env.threads());
+ removeTrailingComma(sb);
+ writeIndent(sb, indent);
+ sb.append('}');
+ }
+
+ private static void writeStringMap(StringBuilder sb, int indent, String
key, java.util.Map<String, String> map) {
+ writeIndent(sb, indent);
+ sb.append('"').append(key).append("\": ");
+ if (map.isEmpty()) {
+ sb.append("{}");
+ } else {
+ sb.append("{\n");
+ var entries = new java.util.ArrayList<>(map.entrySet());
+ for (int i = 0; i < entries.size(); i++) {
+ var entry = entries.get(i);
+ writeIndent(sb, indent + 1);
+ writeJsonString(sb, entry.getKey());
+ sb.append(": ");
+ writeJsonString(sb, entry.getValue());
+ if (i < entries.size() - 1) {
+ sb.append(',');
+ }
+ sb.append('\n');
+ }
+ writeIndent(sb, indent);
+ sb.append('}');
+ }
+ sb.append(",\n");
+ }
+
+ private static void writeProblem(StringBuilder sb, BuilderProblem problem,
int indent) {
+ sb.append("{\n");
+ writeField(sb, indent + 1, "severity", problem.getSeverity().name());
+ writeField(sb, indent + 1, "message", problem.getMessage());
+ String source = problem.getSource();
+ if (source != null && !source.isEmpty()) {
+ writeField(sb, indent + 1, "source", source);
+ }
+ if (problem.getLineNumber() > 0) {
+ writeField(sb, indent + 1, "line", problem.getLineNumber());
+ }
+ if (problem.getColumnNumber() > 0) {
+ writeField(sb, indent + 1, "column", problem.getColumnNumber());
+ }
+ // Remove the trailing comma from the last written field
+ int lastComma = sb.lastIndexOf(",\n");
+ if (lastComma > 0) {
+ sb.replace(lastComma, lastComma + 1, "");
+ }
+ writeIndent(sb, indent);
+ sb.append('}');
+ }
+
+ private static void writeModule(StringBuilder sb, ModuleReport module, int
indent) {
+ sb.append("{\n");
+ writeField(sb, indent + 1, "groupId", module.groupId());
+ writeField(sb, indent + 1, "artifactId", module.artifactId());
+ writeField(sb, indent + 1, "version", module.version());
+ writeField(sb, indent + 1, "status", module.status().name());
+ writeField(sb, indent + 1, "startTime", module.startTime().toString());
+ writeField(sb, indent + 1, "duration", module.duration().toString());
+
+ // mojos array
+ writeIndent(sb, indent + 1);
+ sb.append("\"mojos\": ");
+ if (module.mojos().isEmpty()) {
+ sb.append("[]");
+ } else {
+ sb.append("[\n");
+ for (int i = 0; i < module.mojos().size(); i++) {
+ writeIndent(sb, indent + 2);
+ writeMojo(sb, module.mojos().get(i), indent + 2);
+ if (i < module.mojos().size() - 1) {
+ sb.append(',');
+ }
+ sb.append('\n');
+ }
+ writeIndent(sb, indent + 1);
+ sb.append(']');
+ }
+ sb.append(",\n");
+
+ // output array — module-level log lines (between mojos)
+ writeOutputArray(sb, indent + 1, module.output());
+ sb.append('\n');
+
+ writeIndent(sb, indent);
+ sb.append('}');
+ }
+
+ private static void writeMojo(StringBuilder sb, MojoReport mojo, int
indent) {
+ sb.append("{\n");
+ writeField(sb, indent + 1, "groupId", mojo.groupId());
+ writeField(sb, indent + 1, "artifactId", mojo.artifactId());
+ writeField(sb, indent + 1, "version", mojo.version());
+ writeField(sb, indent + 1, "goal", mojo.goal());
+ writeNullableField(sb, indent + 1, "executionId", mojo.executionId(),
true);
+ writeNullableField(sb, indent + 1, "phase", mojo.phase(), true);
+ writeField(sb, indent + 1, "status", mojo.status().name());
+ writeField(sb, indent + 1, "startTime", mojo.startTime().toString());
+ writeField(sb, indent + 1, "duration", mojo.duration().toString());
+
+ // output array — captured log lines
+ writeOutputArray(sb, indent + 1, mojo.output());
+ sb.append('\n');
+
+ writeIndent(sb, indent);
+ sb.append('}');
+ }
+
+ private static void writeFailure(StringBuilder sb, FailureReport failure,
int indent) {
+ sb.append("{\n");
+ writeField(sb, indent + 1, "module", failure.module());
+ writeNullableField(sb, indent + 1, "mojo", failure.mojo(), true);
+ writeField(sb, indent + 1, "timestamp",
failure.timestamp().toString());
+ writeNullableField(sb, indent + 1, "exceptionType",
failure.exceptionType(), true);
+ if (failure.stackTrace() != null) {
+ writeField(sb, indent + 1, "message", failure.message());
+ writeLastField(sb, indent + 1, "stackTrace", failure.stackTrace());
+ } else {
+ writeLastField(sb, indent + 1, "message", failure.message());
+ }
+ writeIndent(sb, indent);
+ sb.append('}');
+ }
+
+ /**
+ * Writes an {@code "output": [...]} array of structured log events
+ * (used by report, module, and mojo).
+ * This is always the last field in its object, so no trailing comma.
+ */
+ private static void writeOutputArray(StringBuilder sb, int indent,
java.util.List<LogEvent> events) {
+ writeIndent(sb, indent);
+ sb.append("\"output\": ");
+ if (events.isEmpty()) {
+ sb.append("[]");
+ } else {
+ sb.append("[\n");
+ for (int i = 0; i < events.size(); i++) {
+ writeIndent(sb, indent + 1);
+ writeLogEvent(sb, events.get(i), indent + 1);
+ if (i < events.size() - 1) {
+ sb.append(',');
+ }
+ sb.append('\n');
+ }
+ writeIndent(sb, indent);
+ sb.append(']');
+ }
+ }
+
+ private static void writeLogEvent(StringBuilder sb, LogEvent event, int
indent) {
+ sb.append("{\n");
+ writeField(sb, indent + 1, "timestamp", event.timestamp().toString());
+ writeField(sb, indent + 1, "level", event.level().name());
+ if (event.loggerName() != null) {
+ writeField(sb, indent + 1, "loggerName", event.loggerName());
+ }
+ writeField(sb, indent + 1, "message", event.message());
+ if (event.stackTrace() != null) {
+ writeField(sb, indent + 1, "stackTrace", event.stackTrace());
+ }
+ // Source metadata — present for Log API and JUL events
+ if (event.sourceClassName() != null) {
+ writeField(sb, indent + 1, "sourceClassName",
event.sourceClassName());
+ }
+ if (event.sourceMethodName() != null) {
+ writeField(sb, indent + 1, "sourceMethodName",
event.sourceMethodName());
+ }
+ if (event.threadId() >= 0) {
+ writeField(sb, indent + 1, "threadId", event.threadId());
+ }
+ if (event.sequenceNumber() >= 0) {
+ writeField(sb, indent + 1, "sequenceNumber",
event.sequenceNumber());
+ }
+ removeTrailingComma(sb);
+ writeIndent(sb, indent);
+ sb.append('}');
+ }
+
+ // ---- Low-level JSON writing helpers ----
+
+ private static void writeField(StringBuilder sb, int indent, String key,
String value) {
+ writeIndent(sb, indent);
+ sb.append('"').append(key).append("\": ");
+ writeJsonString(sb, value);
+ sb.append(",\n");
+ }
+
+ private static void writeField(StringBuilder sb, int indent, String key,
int value) {
+ writeIndent(sb, indent);
+ sb.append('"').append(key).append("\": ").append(value).append(",\n");
+ }
+
+ private static void writeField(StringBuilder sb, int indent, String key,
long value) {
+ writeIndent(sb, indent);
+ sb.append('"').append(key).append("\": ").append(value).append(",\n");
+ }
+
+ private static void writeField(StringBuilder sb, int indent, String key,
boolean value) {
+ writeIndent(sb, indent);
+ sb.append('"').append(key).append("\": ").append(value).append(",\n");
+ }
+
+ /**
+ * Removes the trailing comma from the last field in a JSON object.
+ * Turns {@code "field": value,\n} into {@code "field": value\n}.
+ */
+ private static void removeTrailingComma(StringBuilder sb) {
+ int len = sb.length();
+ if (len >= 2 && sb.charAt(len - 2) == ',' && sb.charAt(len - 1) ==
'\n') {
+ sb.deleteCharAt(len - 2);
+ }
+ }
+
+ private static void writeLastField(StringBuilder sb, int indent, String
key, String value) {
+ writeIndent(sb, indent);
+ sb.append('"').append(key).append("\": ");
+ writeJsonString(sb, value);
+ sb.append('\n');
+ }
+
+ private static void writeNullableField(
+ StringBuilder sb, int indent, String key, String value,
@SuppressWarnings("unused") boolean hasMore) {
Review Comment:
**[Low] Dead `hasMore` parameter still present — not fixed since prior
review.**
`@SuppressWarnings("unused")` on a parameter you control means the API is
wrong. The parameter is never read — the method always appends a trailing comma
unconditionally (line ~381: `sb.append(",\n")`). All five call sites pass
`true` hardcoded.
This was raised in the previous review. Remove the parameter and update all
call sites (`resumeFrom` in `writeEnvironment`; `executionId`, `phase` in
`writeMojo`; `mojo`, `exceptionType` in `writeFailure`).
```suggestion
private static void writeNullableField(
StringBuilder sb, int indent, String key, String value) {
```
--
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]
