gnodet-bot commented on code in PR #13197:
URL: https://github.com/apache/maven/pull/13197#discussion_r4053177903
##########
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:
**`protected` with no subclass override — should be `private`.**
No subclass overrides this method. Keeping it `protected` widens the
contract unnecessarily: any future subclass could accidentally shadow it,
bypassing the cache setup and causing `effectiveModelCache` to remain `null`
when `buildEffectiveModel` is called (cache lookup is skipped when `cache ==
null` — silent fallback to per-POM BUILD_EFFECTIVE, defeating the fix).
```suggestion
private void prebuildReactorModels(UpgradeContext context, Set<Path>
pomPaths) {
```
##########
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) {
+ if (pomPaths.isEmpty()) {
+ return;
+ }
+ Session s = getSession();
+ ModelBuilder modelBuilder = s.getService(ModelBuilder.class);
+ sharedModelBuilderSession = modelBuilder.newSession();
+
+ // Use the shallowest path as the root (fewest name elements).
+ Path rootPom = pomPaths.stream()
+ .min(java.util.Comparator.comparingInt(Path::getNameCount))
+ .orElseThrow();
+
+ context.debug("Pre-building reactor effective models from root: " +
rootPom);
+
+ try {
+ ModelBuilderRequest request = ModelBuilderRequest.builder()
+ .session(s)
+ .source(Sources.buildSource(rootPom))
+ .requestType(ModelBuilderRequest.RequestType.BUILD_PROJECT)
+ .build();
+
+ ModelBuilderResult result =
sharedModelBuilderSession.build(request);
+
+ // Walk the result tree and populate the cache.
+ Map<Path, Model> cache = new HashMap<>();
+ allResults(result).forEach(r -> {
+ Path path = r.getSource().getPath();
+ if (path != null) {
+ cache.put(path.toAbsolutePath().normalize(),
r.getEffectiveModel());
+ }
+ });
+ effectiveModelCache = cache;
+ context.debug("Reactor pre-build complete: cached " + cache.size()
+ " effective model(s)");
+
+ } catch (Exception e) {
+ context.warning("Reactor pre-build failed, falling back to per-POM
resolution: " + e.getMessage());
Review Comment:
**Stack trace is swallowed — diagnostics will be poor when the pre-build
fails.**
`e.getMessage()` loses the full stack trace. In offline environments or
projects with unresolvable external parents, this fires frequently. Add
debug-level logging of the full exception to make failures diagnosable without
polluting normal output:
```suggestion
context.warning("Reactor pre-build failed, falling back to
per-POM resolution: " + e.getMessage());
context.debug("Reactor pre-build exception detail: " + e);
```
--
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]
