gnodet-bot commented on code in PR #13542: URL: https://github.com/apache/maven/pull/13542#discussion_r4330407528
########## impl/maven-core/src/main/java/org/apache/maven/internal/build/DefaultBuildInvocation.java: ########## @@ -0,0 +1,186 @@ +/* + * 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.Locale; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; + +import org.apache.maven.api.BuildInvocation; +import org.apache.maven.api.MakeBehavior; +import org.apache.maven.api.ReactorFailureBehavior; +import org.apache.maven.execution.MavenSession; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Internal immutable implementation of {@link BuildInvocation}. + * + * @param goals requested goals/phases + * @param userProperties user properties with sensitive values redacted + * @param systemInfo curated system properties for reproducibility + * @param activeProfiles explicitly activated profiles + * @param selectedProjects projects selected via {@code -pl} + * @param excludedProjects projects excluded via {@code -pl !module} + * @param makeBehavior reactor make behavior ({@code -am}/{@code -amd}), or {@code null} + * @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 recursive whether recursive processing is enabled + * @param resume whether resume mode was active + * @param threads degree of concurrency + */ +public record DefaultBuildInvocation( + List<String> goals, + Map<String, String> userProperties, + Map<String, String> systemInfo, + List<String> activeProfiles, + List<String> selectedProjects, + List<String> excludedProjects, + MakeBehavior makeBehavior, + String resumeFrom, + ReactorFailureBehavior reactorFailureBehavior, + boolean offline, + boolean updateSnapshots, + boolean noTransferProgress, + boolean batchMode, + boolean recursive, + boolean resume, + int threads) + implements BuildInvocation { + + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultBuildInvocation.class); + + private static final Set<String> SENSITIVE_KEY_FRAGMENTS = + Set.of("password", "passwd", "secret", "token", "apikey", "api_key", "credential", "passphrase"); + + public DefaultBuildInvocation { + goals = List.copyOf(goals); + userProperties = Map.copyOf(userProperties); + systemInfo = Map.copyOf(systemInfo); + activeProfiles = List.copyOf(activeProfiles); + selectedProjects = List.copyOf(selectedProjects); + excludedProjects = List.copyOf(excludedProjects); + } + + /** + * Creates a {@link BuildInvocation} snapshot from the current session state. + * + * @param session the Maven session + * @return a new immutable build invocation instance + */ + public static BuildInvocation 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())); + + List<String> activeProfiles = + request.getActiveProfiles() != null ? List.copyOf(request.getActiveProfiles()) : List.of(); + + List<String> selectedProjects = + request.getSelectedProjects() != null ? List.copyOf(request.getSelectedProjects()) : List.of(); + Review Comment: ⚠️ **Silent exception swallowing masks misconfiguration** If `request.getReactorFailureBehavior()` returns an unrecognized string (e.g. a user typo like `"FAIL-FAST"` instead of `"FAIL_FAST"`), `fromString()` throws `IllegalArgumentException` which is silently discarded here. The build then proceeds with `FAIL_FAST` as if it were the correct behavior — with no indication to the user that their `-fn`/`-fae` flag was ignored. At minimum, log a debug/warn message so the misconfiguration is visible: ```suggestion } catch (IllegalArgumentException e) { // Unknown string — fall back to FAIL_FAST and warn org.slf4j.LoggerFactory.getLogger(DefaultBuildInvocation.class) .warn("Unknown reactor failure behavior '{}', defaulting to FAIL_FAST", request.getReactorFailureBehavior()); } ``` Or if a logger field is not practical here (static factory), consider propagating the exception and letting `DefaultSession` handle it. ########## impl/maven-core/src/test/java/org/apache/maven/internal/build/DefaultBuildInvocationTest.java: ########## @@ -0,0 +1,391 @@ +/* + * 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.io.File; +import java.util.List; +import java.util.Map; +import java.util.Properties; + +import org.apache.maven.api.BuildInvocation; +import org.apache.maven.api.MakeBehavior; +import org.apache.maven.api.ReactorFailureBehavior; +import org.apache.maven.execution.DefaultMavenExecutionRequest; +import org.apache.maven.execution.MavenExecutionRequest; +import org.apache.maven.execution.MavenSession; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class DefaultBuildInvocationTest { + + private static MavenExecutionRequest newRequest() { + MavenExecutionRequest request = new DefaultMavenExecutionRequest(); + request.setLocalRepositoryPath(new File("/tmp/test-repo")); + return request; + } + + @Test + void fromBasicRequest() { + MavenExecutionRequest request = new DefaultMavenExecutionRequest(); + request.setGoals(List.of("clean", "install")); + request.setOffline(true); + request.setUpdateSnapshots(true); + request.setInteractiveMode(false); + request.setReactorFailureBehavior(MavenExecutionRequest.REACTOR_FAIL_AT_END); + + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + BuildInvocation inv = DefaultBuildInvocation.from(session); + + assertEquals(List.of("clean", "install"), inv.goals()); + assertTrue(inv.offline()); + assertTrue(inv.updateSnapshots()); + assertTrue(inv.batchMode()); + assertEquals(ReactorFailureBehavior.FAIL_AT_END, inv.reactorFailureBehavior()); + assertTrue(inv.recursive()); + assertFalse(inv.resume()); + assertNull(inv.makeBehavior()); + assertTrue(inv.excludedProjects().isEmpty()); + } + + @Test + void fromNullGoals() { + MavenExecutionRequest request = newRequest(); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + BuildInvocation inv = DefaultBuildInvocation.from(session); + + assertNotNull(inv.goals()); + assertTrue(inv.goals().isEmpty()); + } + + @Test + void fromNullActiveProfiles() { + MavenExecutionRequest request = newRequest(); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + BuildInvocation inv = DefaultBuildInvocation.from(session); + + assertNotNull(inv.activeProfiles()); + assertTrue(inv.activeProfiles().isEmpty()); + } + + @Test + void fromNullSelectedProjects() { + MavenExecutionRequest request = newRequest(); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + BuildInvocation inv = DefaultBuildInvocation.from(session); + + assertNotNull(inv.selectedProjects()); + assertTrue(inv.selectedProjects().isEmpty()); + } + + @Test + void fromNullLocalRepositoryPathThrows() { + MavenExecutionRequest request = new DefaultMavenExecutionRequest(); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + assertThrows(IllegalStateException.class, () -> DefaultBuildInvocation.from(session)); + } + + @Test + void fromNullReactorFailureBehaviorDefaultsToFailFast() { + MavenExecutionRequest request = newRequest(); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + BuildInvocation inv = DefaultBuildInvocation.from(session); + + assertEquals(ReactorFailureBehavior.FAIL_FAST, inv.reactorFailureBehavior()); + } + + @Test + void resumeFrom() { + MavenExecutionRequest request = newRequest(); + request.setResumeFrom(":my-module"); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + BuildInvocation inv = DefaultBuildInvocation.from(session); + + assertEquals(":my-module", inv.resumeFrom()); + } + + @Test + void resumeFromNull() { + MavenExecutionRequest request = newRequest(); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + BuildInvocation inv = DefaultBuildInvocation.from(session); + + assertNull(inv.resumeFrom()); + } + + @Test + void noTransferProgress() { + MavenExecutionRequest request = newRequest(); + request.setNoTransferProgress(true); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + BuildInvocation inv = DefaultBuildInvocation.from(session); + + assertTrue(inv.noTransferProgress()); + } + + @Test + void sensitiveKeyRedaction() { + MavenExecutionRequest request = newRequest(); + Properties userProps = new Properties(); + userProps.setProperty("my.password", "secret123"); + userProps.setProperty("api_key", "key123"); + userProps.setProperty("auth.token", "tok123"); + userProps.setProperty("safe.property", "visible"); + request.setUserProperties(userProps); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + BuildInvocation inv = DefaultBuildInvocation.from(session); + + assertEquals("***", inv.userProperties().get("my.password")); + assertEquals("***", inv.userProperties().get("api_key")); + assertEquals("***", inv.userProperties().get("auth.token")); + assertEquals("visible", inv.userProperties().get("safe.property")); + } + + @ParameterizedTest + @ValueSource( + strings = { + "password", + "PASSWORD", + "my.passwd", + "db_secret", + "auth_token", + "APIKEY", + "my.api_key", + "credential", + "ssh.passphrase" + }) + void sensitiveKeyFragments(String key) { + assertTrue(DefaultBuildInvocation.isSensitiveKey(key)); + } + + @ParameterizedTest + @ValueSource(strings = {"safe.property", "skip.tests", "maven.compiler.source", "project.build.outputDirectory"}) + void nonSensitiveKeys(String key) { + assertFalse(DefaultBuildInvocation.isSensitiveKey(key)); + } + + @Test + void systemInfoCapturesExpectedKeys() { + MavenExecutionRequest request = newRequest(); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + BuildInvocation inv = DefaultBuildInvocation.from(session); + + Map<String, String> sysInfo = inv.systemInfo(); + assertNotNull(sysInfo.get("os.name")); + assertNotNull(sysInfo.get("os.arch")); + assertNotNull(sysInfo.get("java.vm.version")); + assertNotNull(sysInfo.get("available.processors")); + } + + @Test + void collectionsAreImmutable() { + DefaultBuildInvocation inv = new DefaultBuildInvocation( + List.of("compile"), + Map.of("key", "val"), + Map.of("os.name", "Linux"), + List.of("profile1"), + List.of(":module1"), + List.of(":excluded"), + null, + null, + ReactorFailureBehavior.FAIL_FAST, + false, + false, + false, + false, + true, + false, + 1); + + assertThrows(UnsupportedOperationException.class, () -> inv.goals().add("test")); + assertThrows( + UnsupportedOperationException.class, () -> inv.userProperties().put("new", "val")); + assertThrows( + UnsupportedOperationException.class, () -> inv.activeProfiles().add("p2")); + assertThrows( + UnsupportedOperationException.class, + () -> inv.selectedProjects().add(":m2")); + assertThrows( + UnsupportedOperationException.class, + () -> inv.excludedProjects().add(":m3")); + assertThrows(UnsupportedOperationException.class, () -> inv.systemInfo().put("k", "v")); + } + + @Test + void reactorFailureBehaviorValues() { + MavenExecutionRequest request = newRequest(); + MavenSession session = mock(MavenSession.class); + when(session.getRequest()).thenReturn(request); + + request.setReactorFailureBehavior(MavenExecutionRequest.REACTOR_FAIL_FAST); + assertEquals( + ReactorFailureBehavior.FAIL_FAST, + DefaultBuildInvocation.from(session).reactorFailureBehavior()); Review Comment: 💡 **Missing test cases for edge conditions** Three untested paths: 1. **Unrecognized `reactorFailureBehavior` string** — what happens when the request returns `"UNKNOWN_VALUE"`? Should either verify the silent `FAIL_FAST` fallback (if the current behavior is intentional) or the exception propagation (if it's changed per the comment above). 2. **`localRepository()` when `localRepositoryPath` is null** — `fromNullLocalRepositoryPath()` only asserts `assertNotNull(inv.localRepository())`. It should also assert `assertEquals(Path.of(""), inv.localRepository())` to document the contract explicitly. 3. **`threads()` default** — no test verifies the default `getDegreeOfConcurrency()` value (it's 0 from `DefaultMavenExecutionRequest` unless explicitly set, which may be surprising for callers expecting 1). ########## impl/maven-core/src/main/java/org/apache/maven/internal/build/DefaultBuildInvocation.java: ########## @@ -0,0 +1,186 @@ +/* + * 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.Locale; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; + +import org.apache.maven.api.BuildInvocation; +import org.apache.maven.api.MakeBehavior; +import org.apache.maven.api.ReactorFailureBehavior; +import org.apache.maven.execution.MavenSession; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Internal immutable implementation of {@link BuildInvocation}. + * + * @param goals requested goals/phases + * @param userProperties user properties with sensitive values redacted + * @param systemInfo curated system properties for reproducibility + * @param activeProfiles explicitly activated profiles + * @param selectedProjects projects selected via {@code -pl} + * @param excludedProjects projects excluded via {@code -pl !module} + * @param makeBehavior reactor make behavior ({@code -am}/{@code -amd}), or {@code null} + * @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 recursive whether recursive processing is enabled + * @param resume whether resume mode was active + * @param threads degree of concurrency + */ +public record DefaultBuildInvocation( + List<String> goals, + Map<String, String> userProperties, + Map<String, String> systemInfo, + List<String> activeProfiles, + List<String> selectedProjects, + List<String> excludedProjects, + MakeBehavior makeBehavior, + String resumeFrom, + ReactorFailureBehavior reactorFailureBehavior, + boolean offline, + boolean updateSnapshots, + boolean noTransferProgress, + boolean batchMode, + boolean recursive, + boolean resume, + int threads) + implements BuildInvocation { + + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultBuildInvocation.class); + + private static final Set<String> SENSITIVE_KEY_FRAGMENTS = + Set.of("password", "passwd", "secret", "token", "apikey", "api_key", "credential", "passphrase"); + + public DefaultBuildInvocation { + goals = List.copyOf(goals); + userProperties = Map.copyOf(userProperties); + systemInfo = Map.copyOf(systemInfo); + activeProfiles = List.copyOf(activeProfiles); + selectedProjects = List.copyOf(selectedProjects); + excludedProjects = List.copyOf(excludedProjects); + } + + /** + * Creates a {@link BuildInvocation} snapshot from the current session state. + * + * @param session the Maven session + * @return a new immutable build invocation instance + */ + public static BuildInvocation 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"); Review Comment: 💡 **`Path.of("")` as null fallback is misleading** `Path.of("").toAbsolutePath()` resolves to the JVM working directory — not a Maven local repository. If any downstream consumer calls `localRepository().toAbsolutePath()`, it silently gets CWD instead of the expected repo path. Maven always sets `localRepositoryPath` in practice, so this branch may never fire — but the empty-path sentinel is a trap. Two cleaner options: 1. Throw `IllegalStateException("localRepositoryPath is not set")` — hard contract, fails fast 2. Keep `Path.of("")` but add a comment explaining this is intentional for the "no repo configured" case If option 2, the test `fromNullLocalRepositoryPath()` should also assert `assertEquals(Path.of(""), inv.localRepository())` so the intent is documented in the test suite. -- 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]
