gnodet-bot commented on code in PR #13197:
URL: https://github.com/apache/maven/pull/13197#discussion_r4053415991
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeStrategy.java:
##########
@@ -94,10 +143,86 @@ public final UpgradeResult apply(UpgradeContext context,
Map<Path, Document> pom
context.failure("Strategy execution failed: " + e.getMessage());
return UpgradeResult.failure(pomMap.keySet(), Set.of());
} finally {
+ effectiveModelCache = null;
+ sharedModelBuilderSession = null;
context.unindent();
}
}
+ /**
+ * Pre-builds effective models for every POM in the reactor with a single
+ * {@code BUILD_PROJECT} pass and stores them in {@link
#effectiveModelCache}.
+ *
+ * <p>{@code BUILD_PROJECT} is used because it:
+ * <ul>
+ * <li>calls {@code loadFromRoot}, which populates {@code mappedSources}
for the
+ * whole reactor before any single effective model is assembled —
this is what
+ * makes Maven 4 coordinate inference work across modules;</li>
+ * <li>applies full profile activation (file, property, condition)
rather than
+ * the reduced set used by {@code BUILD_CONSUMER};</li>
+ * <li>produces child results (via {@code getChildren()}) that map
1-to-1 to the
Review Comment:
⚠️ **Inaccurate Javadoc (carried over from previous review — not addressed)**
This bullet claims `getChildren()` maps 1-to-1 to reactor modules. It does
not: `getChildren().add(cr)` in `DefaultModelBuilder.loadFilePom` is gated on
`request.isRecursive()`, which defaults to `false`. With the current request
(no `.recursive(true)` call), `getChildren()` always returns an empty list.
`allResults()` sees only the root result and the cache ends up with exactly one
entry for any multi-module project.
Either add `.recursive(true)` to the request, or correct this bullet to
reflect reality:
```suggestion
* <li>populates {@code mappedSources} for the whole reactor so that
subsequent
* {@code BUILD_EFFECTIVE} calls on the shared session can resolve
sibling coordinates;</li>
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeStrategy.java:
##########
@@ -94,10 +143,86 @@ public final UpgradeResult apply(UpgradeContext context,
Map<Path, Document> pom
context.failure("Strategy execution failed: " + e.getMessage());
return UpgradeResult.failure(pomMap.keySet(), Set.of());
} finally {
+ effectiveModelCache = null;
+ sharedModelBuilderSession = null;
context.unindent();
}
}
+ /**
+ * Pre-builds effective models for every POM in the reactor with a single
+ * {@code BUILD_PROJECT} pass and stores them in {@link
#effectiveModelCache}.
+ *
+ * <p>{@code BUILD_PROJECT} is used because it:
+ * <ul>
+ * <li>calls {@code loadFromRoot}, which populates {@code mappedSources}
for the
+ * whole reactor before any single effective model is assembled —
this is what
+ * makes Maven 4 coordinate inference work across modules;</li>
+ * <li>applies full profile activation (file, property, condition)
rather than
+ * the reduced set used by {@code BUILD_CONSUMER};</li>
+ * <li>produces child results (via {@code getChildren()}) that map
1-to-1 to the
+ * reactor modules, so we can populate the cache in a single
pass.</li>
+ * </ul>
+ *
+ * <p>The root POM is identified as the shallowest (fewest name parts)
path in
+ * {@code pomPaths}.</p>
+ *
+ * <p>If the build fails (e.g. the project has no network access and an
external
+ * parent cannot be resolved), the cache is left empty and
+ * {@link #buildEffectiveModel} falls back to individual {@code
BUILD_EFFECTIVE}
+ * calls.</p>
+ *
+ * @param context the upgrade context (used for debug/warning logging)
+ * @param pomPaths the set of POM paths that make up the reactor
+ */
+ protected void prebuildReactorModels(UpgradeContext context, Set<Path>
pomPaths) {
Review Comment:
**Nit (carried over — not addressed): should be `private`**
No subclass overrides this method; it is purely an implementation detail of
`apply()`. The `protected` visibility exposes an internal lifecycle step that
subclasses should not call directly (calling it again would corrupt
`effectiveModelCache` and `sharedModelBuilderSession` mid-flight).
```suggestion
private void prebuildReactorModels(UpgradeContext context, Set<Path>
pomPaths) {
```
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java:
##########
@@ -1164,49 +1168,61 @@ void
shouldOverrideRemoteParentPluginViaPluginManagementWithComment() throws Exc
</project>
""";
- Document document = Document.of(pomXml);
- Path pomPath = Paths.get("/project/pom.xml").toAbsolutePath();
- Map<Path, Document> pomMap = Map.of(pomPath, document);
+ Path tempDir = Files.createTempDirectory("mvnup-test-");
+ try {
+ Files.createDirectories(tempDir.resolve(".mvn"));
+ Path pomPath = tempDir.resolve("pom.xml");
+ Files.writeString(pomPath, pomXml);
+ Document document = Document.of(pomXml);
+ Map<Path, Document> pomMap = Map.of(pomPath, document);
- UpgradeContext context = createMockContext();
- UpgradeResult result = strategy.doApply(context, pomMap);
+ UpgradeContext context = createMockContext(tempDir);
+ UpgradeResult result = strategy.doApply(context, pomMap);
Review Comment:
⚠️ **Still calls `doApply` — bypasses the fix (fifth review)**
`doApply` skips `prebuildReactorModels`. The `apply()` →
`prebuildReactorModels` → `mappedSources` populated → inference path is
untested. This test exercises the removal of the temp-dir bridge, not the BOM
inference fix.
A dedicated test should call `strategy.apply(context, pomMap)` on a
two-module reactor where a child omits `<version>` for a BOM-managed dependency
and assert no `ModelBuilderException` with "version is missing".
--
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]
