Repository navigation
Treat a model built from a POM file as the project, whatever its validation level - #13394
Conversation
gnodet
left a comment
There was a problem hiding this comment.
Thanks for the fix — the IDE regression is real and this unblocks NetBeans/m2e users.
The pomFile == null addition is a pragmatic fix for a heuristic that was always indirect: validationLevel was never designed to carry the "this is a repository model" semantic, and now pomFile is being recruited for the same kind of duty. It works because DefaultArtifactDescriptorReader never sets a POM file for repository POMs, but it's a convention that callers need to know about — a tool that loads a project POM via setModelSource(new FileModelSource(f)) without calling setPomFile() would still be sandboxed.
For context, the v4 model builder (impl/maven-impl) solves this cleanly with RequestType + source provenance (isRepositoryResolved checks whether the source carries coordinates), so there's no heuristic at all. The compat layer can't easily use the same approach without API changes, so the pomFile check is the right tradeoff for 3.10.x and for the compat bridge on master.
A couple of observations:
-
The
setPomFile()Javadoc says it indicates "project mode", but this is the first time it's load-bearing for security-relevant sandboxing. It might be worth adding a note inModelBuildingRequest.setPomFile()that callers building their own project POM must usesetPomFile()(not justsetModelSource) to avoid being sandboxed. -
In
restrictExternalModelInterpolation(the interpolation side), thepomFilecheck is added but themaven.model.dependencyInterpolation.fullescape hatch still exists. Should the escape hatch also restore profile activation, or is the intent thatpomFilemakes it unnecessary? Right now a caller that can't setpomFilefor some reason has no workaround for the profile half.
The fix itself looks correct for the cases it targets. The test coverage (project-at-MINIMAL vs repository-at-MINIMAL) is good.
Claude Code on behalf of Guillaume Nodet
|
Fully automatic review from Claude Code This PR (#13394, fixing #13347) and #13451 (fixing #13390) are complementary fixes to the same
Both are needed. The production code changes are in different methods and merge cleanly. There will be a textual conflict in |
…dation level The repository-model sandbox (restricted interpolation, profile activation without user properties or file activation) keyed only on validationLevel < MAVEN_2_0. Embedders build their own project at MINIMAL, so -D profile activation broke for them (#13347). getPomFile() is the documented project-mode signal; repository models never set it.
f5f978e to
ba9605b
Compare
|
Why is this draft? Any special reason? |
gnodet-bot
left a comment
There was a problem hiding this comment.
Clean, minimal, well-tested fix for a real regression affecting IDE embedders (NetBeans, m2e) that build projects at VALIDATION_LEVEL_MINIMAL.
The fix correctly adds getPomFile() == null as an additional guard in both isExternalModelBuildingRequest() and restrictExternalModelInterpolation() — consistently applied. The invariant holds: the documented contract of ModelBuildingRequest.setPomFile already guarantees that repository-resolved POMs never set a POM file, so this is a safe discriminator.
The two new tests form a precise pair — same POM, same validation level, only setPomFile vs setModelSource differs — exactly covering the behavioral change. The PR description confirms the new test was failing before the fix, satisfying pre-fix validation.
The isFullExternalModelBuild opt-out path is preserved and unaffected. Javadoc update is accurate. No compat-layer changes. The companion fix for master's v3 bridge is tracked in #13395.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Fixes #13347.
The repository-model restrictions from #12953 (interpolation) and #13114 (profile activation sandbox) decided "this POM came from a repository" by
validationLevel < VALIDATION_LEVEL_MAVEN_2_0alone. IDEs build the user's own project from its POM file atVALIDATION_LEVEL_MINIMALfor leniency (NetBeansMavenEmbedder.readProjectWithDependencies, m2eMavenImpl.readProject), so their projects were sandboxed:-Dprofile activation stopped working, andmaven.model.dependencyInterpolation.fullonly restored the interpolation half (apache/netbeans#9642).Both checks now also require
getPomFile() == null.ModelBuildingRequest.setPomFilealready documents a POM file as project mode, and the repository paths never set one:DefaultArtifactDescriptorReadersets it only forWorkspaceRepositoryPOMs, BOM imports usesetModelSource, andDefaultProjectBuilder.build(Artifact)passes a POM file only for a project in the reactor. Callers that set neither keep the validation-level fallback, so nothing changes for the CLI or for repository resolution. No API change.master needs the equivalent change in the v3 bridge (
DefaultProjectBuilderrequest type), tracked in #13395.Verified:
mvn -pl maven-model-builder,maven-resolver-provider,maven-core -am install→ BUILD SUCCESS (maven-core 402 tests). With the main-code change reverted, the newtestProjectModeAtMinimalValidationEvaluatesUserPropertyAndFileActivatorsfails (expected: <activated> but was: <null>). The core ITs covering the hardening (MavenIT0085, mng3586, mng4379, mng4590) have not been run locally.