gnodet-bot commented on code in PR #13195: URL: https://github.com/apache/maven/pull/13195#discussion_r4052433745
########## its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh13192PomInlinerCiFriendlyPropertyTest.java: ########## @@ -0,0 +1,115 @@ +/* + * 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.it; + +import java.io.File; +import java.nio.file.Files; +import java.nio.file.Paths; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Verifies that {@code PomInlinerTransformer} correctly inlines CI-friendly version properties + * (e.g. {@code ${revision}}) that are defined in the project's own {@code <properties>} section + * rather than being passed via {@code -Drevision=...} on the command line. + * + * <p>Prior to the fix, Maven 4 would throw + * {@code IllegalArgumentException: Cannot inline property revision} in legacy mode + * when {@code revision} was present only in POM properties.</p> + * + * @see <a href="https://github.com/apache/maven/issues/13192";>GH-13192</a> + */ +class MavenITgh13192PomInlinerCiFriendlyPropertyTest extends AbstractMavenIntegrationTestCase { + + MavenITgh13192PomInlinerCiFriendlyPropertyTest() { + super("[4.0.0,)"); Review Comment: ⚠️ **Wrong version range.** The fix lands in rc-7 (milestone `4.0.0-rc-7`). The range `[4.0.0,)` excludes every pre-release artifact — including rc-7 and rc-8 — because Maven version ordering places qualifiers *before* the GA release (`4.0.0-rc-7 < 4.0.0`). The IT will never run against the release that ships the fix. Convention on `maven-4.0.x` for rc-7 features (e.g. `MavenITgh13004ConsumerPomProfileArtifactIdTest`): ```suggestion super("[4.0.0-rc-7,)"); ``` ########## impl/maven-core/src/main/java/org/apache/maven/internal/transformation/impl/PomInlinerTransformer.java: ########## @@ -113,6 +120,12 @@ private Set<String> needsInlining(RepositorySystemSession session) { PomInlinerTransformer.class.getName() + ".needsInlining", ConcurrentHashMap::newKeySet); } + @SuppressWarnings("unchecked") + private Map<String, String> pomProperties(RepositorySystemSession session) { + return (Map<String, String>) session.getData() + .computeIfAbsent(PomInlinerTransformer.class.getName() + ".pomProperties", ConcurrentHashMap::new); + } Review Comment: ⚠️ **NOT addressed — Session-scoped `pomProperties` map conflates values across reactor projects (carried from #13194 review).** The map is keyed by property name only (e.g. `"revision"`), shared across the entire `RepositorySystemSession`. `injectTransformedArtifacts` is called once per project; if two modules define the same CI-friendly property with different values the last writer wins, and `replacePom()` for an earlier project will use the wrong value. This doesn't bite the test case (all modules share `revision=1.0.0`), but it is a correctness trap for any reactor where modules carry different per-module CI-friendly versions. The fix is to key the map by project GAV, or — simpler — pass the resolved value directly through `needsInlining` as a `Map<String,String>` instead of a `Set<String>`, eliminating the need for the secondary map entirely: ```suggestion @SuppressWarnings("unchecked") private Map<String, String> pomProperties(RepositorySystemSession session) { // Key by property name — safe only because CI-friendly properties (revision, // sha1, changelist) are defined once in the root POM and inherited by all // modules; per-module overrides with different values are not supported by // the CI-friendly pattern and would require keying by project GAV. return (Map<String, String>) session.getData() .computeIfAbsent(PomInlinerTransformer.class.getName() + ".pomProperties", ConcurrentHashMap::new); } ``` If the current scope is intentionally "single root property only", at minimum add a `@throws` or inline comment documenting this assumption so it is not silently broken later. -- 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]
