gnodet-bot commented on code in PR #13542: URL: https://github.com/apache/maven/pull/13542#discussion_r4286108227
########## 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); Review Comment: ⚠️ **Still open: redundant copy on every accessor call** This is the third review raising this. `List.copyOf(goals)` allocates a new list on every accessor call. The `from()` factory already constructs immutable collections (`List.copyOf()`, `Map.copyOf()`) before passing them to the constructor — these per-call defensive copies are purely wasteful. Fix: add a compact constructor that normalizes once, then remove these overriding accessors: ```suggestion public DefaultBuildEnvironment { goals = List.copyOf(goals); userProperties = Map.copyOf(userProperties); systemInfo = Map.copyOf(systemInfo); activeProfiles = List.copyOf(activeProfiles); selectedProjects = List.copyOf(selectedProjects); } @Override public List<String> goals() { ``` With the compact constructor, the generated `goals()` accessor returns the (now-immutable) field directly — no override needed. ########## 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: ⚠️ **Still open: `user.home` and `user.name` capture PII — add Javadoc warning** These system properties contain real usernames and home directory paths. The `BuildEnvironment.systemInfo()` Javadoc does not warn callers to scrub them before persisting or transmitting build environment data (e.g. in build logs, CI artifacts, or telemetry). This is still missing from the interface Javadoc. ########## 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"); + sysInfo.put("available.processors", String.valueOf(Runtime.getRuntime().availableProcessors())); + + String localRepo = request.getLocalRepositoryPath() != null + ? request.getLocalRepositoryPath().getAbsolutePath() Review Comment: ⚠️ **Still open: `localRepository()` returns `String` — use `Path` in a v4 API** A new v4 API type should expose `java.nio.file.Path`, not a stringified absolute path. The conversion `File → getAbsolutePath()` is the wrong direction. ```suggestion ? request.getLocalRepositoryPath().toPath() ``` And update `BuildEnvironment.localRepository()` to return `Path` instead of `String`. ########## 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: 🆕 **Naming inconsistency: `buildEnvironment()` vs `getX()` convention in `Session`** Every other accessor in `Session` uses the `getX()` JavaBeans convention: `getMavenVersion()`, `getSettings()`, `getLocalRepository()`, `getData()`, `getDegreeOfConcurrency()`. The new method `buildEnvironment()` uses noun-style naming, which breaks the pattern established throughout this interface. Note: noun-style is correct for `@Immutable` value types (like `BuildEnvironment` itself). But `Session` is a service interface (`@ThreadSafe`, not `@Immutable`) — its accessors follow `getX()`. Either rename to `getBuildEnvironment()` for consistency, or document the intentional style divergence. ########## 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: ⚠️ **Still open: `synchronized(this)` in DCL — prefer a private lock object** `DefaultSession` instances are passed around freely. Any external code that also `synchronized(session)` can deadlock this lazy initialization. Use a dedicated private lock field. Add near the `volatile` field declaration: ```java private final Object buildEnvironmentLock = new Object(); ``` Then change `synchronized (this)` to `synchronized (buildEnvironmentLock)`. -- 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]
