gnodet-bot commented on code in PR #13542:
URL: https://github.com/apache/maven/pull/13542#discussion_r4282324739
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Session.java:
##########
@@ -46,6 +46,16 @@
@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.
+ *
+ * @return the build environment, never {@code null}
+ * @since 4.1.0
+ */
+ @Nonnull
+ BuildEnvironment buildEnvironment();
Review Comment:
🔴 **Critical: `buildEnvironment()` is not a `default` method**
`SessionStub` in `impl/maven-testing` implements `Session` without
overriding this — this PR **breaks compilation**. Either add a `default` body
(e.g. throwing `UnsupportedOperationException`, consistent with other optional
Session methods), or add the override to `SessionStub`.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultSession.java:
##########
@@ -172,6 +175,19 @@ public int getDegreeOfConcurrency() {
return getMavenSession().getRequest().getDegreeOfConcurrency();
}
+ @Nonnull
+ @Override
+ public BuildEnvironment buildEnvironment() {
+ if (buildEnvironment == null) {
+ synchronized (this) {
Review Comment:
⚠️ **Concurrency: prefer a private lock object over `synchronized(this)`**
Any external code that also synchronizes on this `DefaultSession` instance
can deadlock this lazy initialization. Use a dedicated private lock:
```suggestion
private final Object buildEnvironmentLock = new Object();
```
Then `synchronized(buildEnvironmentLock)` here.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/build/DefaultBuildEnvironment.java:
##########
@@ -0,0 +1,170 @@
+/*
+ * 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 java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+
+import org.apache.maven.api.BuildEnvironment;
+import org.apache.maven.execution.MavenSession;
+
+/**
+ * Internal immutable implementation of {@link BuildEnvironment}.
+ *
+ * @param goals requested goals/phases
+ * @param userProperties user properties with sensitive values redacted
+ * @param systemInfo curated system properties for reproducibility
+ * @param localRepository local repository path
+ * @param activeProfiles explicitly activated profiles
+ * @param selectedProjects projects selected via {@code -pl}
+ * @param resumeFrom resume-from selector, or {@code null}
+ * @param reactorFailureBehavior reactor failure behavior
+ * @param offline whether offline mode was active
+ * @param updateSnapshots whether snapshot updates were forced
+ * @param noTransferProgress whether transfer progress was suppressed
+ * @param batchMode whether batch mode was active
+ * @param threads degree of concurrency
+ */
+public record DefaultBuildEnvironment(
+ List<String> goals,
+ Map<String, String> userProperties,
+ Map<String, String> systemInfo,
+ String localRepository,
+ List<String> activeProfiles,
+ List<String> selectedProjects,
+ String resumeFrom,
+ String reactorFailureBehavior,
+ boolean offline,
+ boolean updateSnapshots,
+ boolean noTransferProgress,
+ boolean batchMode,
+ int threads)
+ implements BuildEnvironment {
+
+ private static final Set<String> SENSITIVE_KEY_FRAGMENTS =
+ Set.of("password", "passwd", "secret", "token", "apikey",
"api_key", "credential", "passphrase");
+
+ @Override
+ public List<String> goals() {
+ return List.copyOf(goals);
+ }
+
+ @Override
+ public Map<String, String> userProperties() {
+ return Map.copyOf(userProperties);
+ }
+
+ @Override
+ public Map<String, String> systemInfo() {
+ return Map.copyOf(systemInfo);
+ }
+
+ @Override
+ public List<String> activeProfiles() {
+ return List.copyOf(activeProfiles);
+ }
+
+ @Override
+ public List<String> selectedProjects() {
+ return List.copyOf(selectedProjects);
+ }
+
+ /**
+ * Creates a {@link BuildEnvironment} snapshot from the current session
state.
+ *
+ * @param session the Maven session
+ * @return a new immutable build environment instance
+ */
+ public static BuildEnvironment from(MavenSession session) {
+ var request = session.getRequest();
+
+ List<String> goals = request.getGoals() != null ?
List.copyOf(request.getGoals()) : List.of();
+
+ Map<String, String> userProps = new LinkedHashMap<>();
+ if (request.getUserProperties() != null) {
+ request.getUserProperties().forEach((k, v) -> {
+ String key = String.valueOf(k);
+ String value = isSensitiveKey(key) ? "***" : String.valueOf(v);
+ userProps.put(key, value);
+ });
+ }
+
+ Map<String, String> sysInfo = new TreeMap<>();
+ addSystemProp(sysInfo, "os.name");
+ addSystemProp(sysInfo, "os.arch");
+ addSystemProp(sysInfo, "os.version");
+ addSystemProp(sysInfo, "java.vendor");
+ addSystemProp(sysInfo, "java.vm.name");
+ addSystemProp(sysInfo, "java.vm.version");
+ addSystemProp(sysInfo, "maven.home");
+ addSystemProp(sysInfo, "user.home");
+ addSystemProp(sysInfo, "user.name");
Review Comment:
💡 **PII: `user.home` and `user.name` should be documented**
These system properties are personally-identifiable (home directory path, OS
username). The `BuildEnvironment.systemInfo()` Javadoc should warn callers to
redact these values before persisting or transmitting build environment
snapshots.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/build/DefaultBuildEnvironment.java:
##########
@@ -0,0 +1,170 @@
+/*
+ * 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 java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeMap;
+
+import org.apache.maven.api.BuildEnvironment;
+import org.apache.maven.execution.MavenSession;
+
+/**
+ * Internal immutable implementation of {@link BuildEnvironment}.
+ *
+ * @param goals requested goals/phases
+ * @param userProperties user properties with sensitive values redacted
+ * @param systemInfo curated system properties for reproducibility
+ * @param localRepository local repository path
+ * @param activeProfiles explicitly activated profiles
+ * @param selectedProjects projects selected via {@code -pl}
+ * @param resumeFrom resume-from selector, or {@code null}
+ * @param reactorFailureBehavior reactor failure behavior
+ * @param offline whether offline mode was active
+ * @param updateSnapshots whether snapshot updates were forced
+ * @param noTransferProgress whether transfer progress was suppressed
+ * @param batchMode whether batch mode was active
+ * @param threads degree of concurrency
+ */
+public record DefaultBuildEnvironment(
+ List<String> goals,
+ Map<String, String> userProperties,
+ Map<String, String> systemInfo,
+ String localRepository,
+ List<String> activeProfiles,
+ List<String> selectedProjects,
+ String resumeFrom,
+ String reactorFailureBehavior,
+ boolean offline,
+ boolean updateSnapshots,
+ boolean noTransferProgress,
+ boolean batchMode,
+ int threads)
+ implements BuildEnvironment {
+
+ private static final Set<String> SENSITIVE_KEY_FRAGMENTS =
+ Set.of("password", "passwd", "secret", "token", "apikey",
"api_key", "credential", "passphrase");
+
+ @Override
+ public List<String> goals() {
+ return List.copyOf(goals);
+ }
+
+ @Override
+ public Map<String, String> userProperties() {
+ return Map.copyOf(userProperties);
+ }
+
+ @Override
+ public Map<String, String> systemInfo() {
+ return Map.copyOf(systemInfo);
+ }
+
+ @Override
+ public List<String> activeProfiles() {
+ return List.copyOf(activeProfiles);
+ }
+
+ @Override
+ public List<String> selectedProjects() {
+ return List.copyOf(selectedProjects);
Review Comment:
💡 **Redundant defensive copies in record accessors**
Each accessor calls `List.copyOf()`/`Map.copyOf()` on every invocation.
Since `from()` already constructs immutable collections before passing them to
the constructor, these copies are redundant on the common path. Consider
enforcing invariants via a compact constructor and removing the per-call copies
from the accessors.
##########
impl/maven-logging/src/main/java/org/apache/maven/slf4j/MavenJulHandler.java:
##########
@@ -0,0 +1,262 @@
+/*
+ * 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.slf4j;
+
+import java.text.MessageFormat;
+import java.util.MissingResourceException;
+import java.util.ResourceBundle;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.logging.Handler;
+import java.util.logging.Level;
+import java.util.logging.LogManager;
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+
+import org.slf4j.LoggerFactory;
+import org.slf4j.spi.LocationAwareLogger;
+
+/**
+ * A JUL {@link Handler} that routes {@code java.util.logging} events into
+ * Maven's structured logging pipeline, preserving the rich {@link LogRecord}
+ * metadata that the standard {@code SLF4JBridgeHandler} silently drops
+ * (source class name, source method name, thread ID).
+ * <p>
+ * When the underlying SLF4J logger is a {@link MavenSimpleLogger} (the
+ * normal case inside Maven), events are delivered via
+ * {@link MavenSimpleLogger#logDirect} — metadata flows as method
+ * parameters instead of through ThreadLocals. For non-Maven SLF4J
+ * backends the handler falls back to standard {@code logger.info()} etc.
+ * <p>
+ * Usage — replace the standard SLF4J bridge in {@code LookupInvoker}:
+ * <pre>
+ * MavenJulHandler.install();
+ * </pre>
+ *
+ * @since 4.1.0
+ * @see #install()
+ */
+public class MavenJulHandler extends Handler {
+
+ /**
+ * Private SLF4J logger cache using {@link ConcurrentMap#putIfAbsent}
+ * instead of {@link ConcurrentMap#computeIfAbsent}. This avoids the
+ * {@code ConcurrentHashMap.computeIfAbsent} reentrancy bug
+ * ({@code IllegalStateException("Recursive update")}) that occurs
+ * when a JUL event fires during SLF4J logger initialization: the
+ * handler's {@code publish()} calls {@code LoggerFactory.getLogger()},
+ * which internally uses {@code computeIfAbsent}, and if that triggers
+ * another JUL event whose logger name hashes to the same bucket,
+ * {@code ConcurrentHashMap} throws. {@code putIfAbsent} is safe
+ * against reentrancy — worst case, two threads create the same
+ * logger and one is discarded.
+ * <p>
+ * The cache is intentionally unbounded: its size is naturally bounded
+ * by the number of distinct JUL logger names observed across all builds
+ * in a JVM lifetime, which is small in practice (a few dozen at most).
+ */
+ private static final ConcurrentMap<String, org.slf4j.Logger> LOGGER_CACHE
= new ConcurrentHashMap<>();
+
+ /**
+ * Re-entrancy guard: set to {@code true} while {@link #publish} is routing
+ * a JUL event through SLF4J on this thread. Prevents recursive JUL events
+ * (e.g. JLine's {@code StyleResolver} calling {@code
java.util.logging.Logger}
+ * while inside {@link MavenSimpleLogger#renderLevel} lazy-initialisation,
+ * which in turn is triggered by a JUL event during terminal construction)
+ * from re-entering {@code publish} and crashing with
+ * {@code ConcurrentHashMap.computeIfAbsent
IllegalStateException("Recursive update")}.
+ */
+ private static final ThreadLocal<Boolean> IN_PUBLISH = new ThreadLocal<>();
+
+ /**
+ * Package-private test hook: sets or clears the re-entrancy flag on
+ * the current thread without reflection.
+ *
+ * @param inPublish {@code true} to simulate being inside {@link #publish},
+ * {@code false} to clear the flag
+ */
+ static void setInPublishForTest(boolean inPublish) {
+ if (inPublish) {
+ IN_PUBLISH.set(Boolean.TRUE);
+ } else {
+ IN_PUBLISH.remove();
+ }
+ }
+
+ /**
+ * Installs this handler on the JUL root logger, removing any
+ * previously installed handlers. This replaces the standard
+ * {@code SLF4JBridgeHandler.install()} call.
+ */
+ public static void install() {
Review Comment:
📌 **Note: `install()` is non-synchronized**
Two concurrent calls (possible in embedded scenarios) could register
duplicate handlers. The CLI bootstrap path is single-threaded so this is not a
production risk there, but worth a brief Javadoc note or a guard for embedded
callers.
--
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]
