Skip to content

Fix MojoExtension.beforeEach to use merged model instead of raw parsed model#12242

Open
UjjwalChitransh wants to merge 2 commits into
apache:masterfrom
UjjwalChitransh:fix/12201-mojo-extension-use-merged-model
Open

Fix MojoExtension.beforeEach to use merged model instead of raw parsed model#12242
UjjwalChitransh wants to merge 2 commits into
apache:masterfrom
UjjwalChitransh:fix/12201-mojo-extension-use-merged-model

Conversation

@UjjwalChitransh

Copy link
Copy Markdown

Problem

In MojoExtension.beforeEach(), alignToBaseDirectory was called on tmodel (the raw parsed POM) and the result was stored in the extension context. This caused two bugs:

  1. NPE when no POM exists: tmodel is null when no POM file is found, causing a NullPointerException.
  2. Missing defaults: The stored model was the raw parsed POM, missing defaults merged in from defaultModel (groupId, artifactId, build paths, etc.).

Fixes #12201

Fix

Call alignToBaseDirectory on model (the merged result of tmodel + defaultModel) and store that aligned model. The createProject provider inside Foo is also updated to use alignedModel consistently.

Testing

  • All 22 existing tests in maven-testing pass.
  • The NPE scenario (no POM file) is now safe: model is never null since defaultModel is always the fallback.

…d model

Previously, alignToBaseDirectory was called on tmodel (the raw parsed POM)
and the result was stored in the extension context. This caused two bugs:
1. NPE when no POM file exists, since tmodel is null in that case
2. The stored model missed defaults injected during merging (groupId,
   artifactId, build paths, etc.)

Fix: apply alignToBaseDirectory to the merged model and store that result.

Fixes: apache#12201

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes MojoExtension.beforeEach() in maven-testing to store an aligned merged Maven Model (defaults + parsed POM) rather than aligning/storing the raw parsed POM model, preventing NPEs when no POM is present and ensuring default values (groupId/artifactId/build paths, etc.) are included.

Changes:

  • Align base-directory paths on the merged model and store the resulting alignedModel in the JUnit extension context.
  • Update the internal Project provider to consistently use alignedModel (packaging + artifact coordinates + stub model).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, @UjjwalChitransh. The root-cause analysis in the PR description is accurate.

Correctness — The old code called alignToBaseDirectory(tmodel, ...) which was wrong in two ways: tmodel is null when no POM is provided (NPE), and even when non-null it lacks the defaults merged from defaultModel (missing build paths, coordinates, etc.). Passing the merged model instead fixes both issues.

Consistency — Before this fix there was also a subtle inconsistency: the extension context stored the aligned tmodel while createProject() used the non-aligned model. The two could diverge. Now both consistently use alignedModel, which is cleaner.

Scoping — The final qualifier on alignedModel is needed for the inner-class capture in Foo.createProject() (line 488+). Looks correct.

Tests — Good coverage: the no-POM default path, the merged-model-with-custom-coordinates path, and the absolute-path invariant after alignment. The three tests map directly to the three things the fix is supposed to guarantee.

LGTM from a code-review standpoint. Leaving as COMMENT so Guillaume can give the final approval.

Claude Code on behalf of Guillaume Nodet

Comment on lines +440 to +441
final Model alignedModel = new DefaultModelPathTranslator(new DefaultPathTranslator())
.alignToBaseDirectory(model, Paths.get(getBasedir()), null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the heart of the fix: model is guaranteed non-null here (either defaultModel or the merge result), so the NPE is eliminated. And since model carries the merged defaults, the aligned result now includes build paths and coordinates from defaultModel — which was the other half of the bug.

Nit: the local model variable (line 434) is now only used as an intermediate to compute alignedModel. That's fine — keeping it separate is more readable than inlining the ternary.

@gnodet gnodet added this to the 4.0.0-rc-6 milestone Jun 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix MojoExtension.beforeEach to use proper model

3 participants