gnodet-bot commented on code in PR #13194:
URL: https://github.com/apache/maven/pull/13194#discussion_r4050805913
##########
impl/maven-core/src/main/java/org/apache/maven/internal/transformation/impl/PomInlinerTransformer.java:
##########
@@ -113,6 +120,13 @@ 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:
⚠️ **Session-scoped `pomProperties` map conflates values across all projects
in the reactor.**
The map is keyed by property *name* only (e.g. `"revision"`), not by project
identity. In a parallel build, `injectTransformedArtifacts` may be called
concurrently for multiple modules. If two modules define `revision`
independently with different values, the last writer wins — and `replacePom()`
will use whatever value happens to be in the map at the time it runs, which may
belong to a different project.
This doesn't affect the common case (CI-friendly versions are typically
defined once in the root and inherited uniformly), but it's an underlying
design gap. Consider scoping the key by project GAV:
```java
pomProperties.put(project.getGroupId() + ":" + project.getArtifactId() + ":"
+ property, projectValue);
```
And correspondingly in `replacePom()`, since it has no project reference,
the key would need to embed the artifact's GAV — or alternatively, the map
could be nested: `Map<String /*artifactId*/, Map<String /*property*/,
String>>`. The cleanest solution may be to pass a snapshot of the project's
resolved properties directly alongside the artifact in a wrapper, avoiding
global state entirely.
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh13192PomInlinerCiFriendlyPropertyTest.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.nio.file.Files;
+import java.nio.file.Path;
+
+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 {
Review Comment:
⚠️ **Missing version-range constructor.** The integration test framework
uses the constructor to gate the test to the Maven versions where the feature
exists. Without it, this test will be executed against Maven 3 builds where
`PomInlinerTransformer` does not exist, causing spurious failures.
Convention from sibling tests (e.g.
`MavenITgh13004ConsumerPomProfileArtifactIdTest`,
`MavenITgh13068LegacyPluginDependenciesResolverTest`):
```suggestion
class MavenITgh13192PomInlinerCiFriendlyPropertyTest extends
AbstractMavenIntegrationTestCase {
MavenITgh13192PomInlinerCiFriendlyPropertyTest() {
super("[4.0.0-rc-7,)");
}
```
--
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]
