Skip to content

Support multipage reports when the goal is invoked directly - #244

Open
slachiewicz wants to merge 1 commit into
apache:masterfrom
slachiewicz:multipage-standalone-report
Open

Support multipage reports when the goal is invoked directly#244
slachiewicz wants to merge 1 commit into
apache:masterfrom
slachiewicz:multipage-standalone-report

Conversation

@slachiewicz

Copy link
Copy Markdown
Member

Fixes #217.

reportToSite() passed null as the SinkFactory to generate(Sink, SinkFactory, Locale), with a TODO saying multipage reports would fail with an NPE. They do:

mvn plugin-report:3.15.1:report
...
Cannot invoke "org.apache.maven.doxia.sink.SinkFactory.createSink(java.io.File, String)"
because the return value of "PluginReport.getSinkFactory()" is null
    at PluginReport.generateMojosDocumentation (PluginReport.java:280)
    at AbstractMavenReport.reportToSite (AbstractMavenReport.java:266)

Only the site path was affected. reportToMarkup(), taken when output.format is set, already builds a sink factory, which is why use-as-direct-mojo-markup exercises the multi-page mojo happily while use-as-direct-mojo could not.

The change

Hand generate() a MultiPageSinkFactory mirroring the one in Maven Site Plugin's ReportDocumentRenderer: each createSink(File, String) builds a DocumentRenderingContext derived from the main one, wraps it in a SiteRendererSink that remembers where it belongs, and records it. After the main document is merged into the site, every collected sub-sink is merged the same way.

Keeping the structure identical to ReportDocumentRenderer is deliberate, so the two stay easy to compare, and so a report behaves the same whether its goal is invoked directly or through the site.

Test

The integration test for exactly this was already written and disabled with a pointer to the issue, in src/it/use-as-direct-mojo. This PR enables invoker.goals.4 = custom-reporting:multi-page again and uncomments the verify.groovy assertions, extended to also check the rendered content rather than mere file existence.

I confirmed it is a real regression guard: reverting only AbstractMavenReport and rerunning the IT reproduces the reported failure.

Cannot invoke "org.apache.maven.doxia.sink.SinkFactory.createSink(java.io.File, String)"
because the return value of "MultiPageReport.getSinkFactory()" is null
Passed: 1, Failed: 1

With the change, mvn verify is green over the full IT suite (6 passed), and the multipage goal logs both pages:

--- custom-reporting:1.0-SNAPSHOT:multi-page (default-cli) @ use-as-direct-mojo ---
Rendering report to target/reports/multi-page.html
          using org.apache.maven.skins:maven-fluido-skin:jar:2.0.0-M9 site skin
Rendering report to target/reports/multi-second.html

I did not rebuild maven-plugin-report-plugin against this branch to re-verify the original report from the issue; the IT covers the same code path with the same API.

reportToSite() passed a null SinkFactory to generate(), so any report
that creates sub-sinks for additional pages failed with an NPE as soon as
its goal was run from the command line rather than through the site. The
maven-plugin-report-plugin report is one such report.

Provide the same MultiPageSinkFactory that Maven Site Plugin's
ReportDocumentRenderer provides, collect the sub-sinks it hands out and
merge each of them into the site next to the main document.

The integration test for this was already written and disabled with a
pointer to the issue, so it is simply enabled again.
@slachiewicz
slachiewicz requested review from hboutemy and michael-o and a lite review from Copilot August 8, 2026 14:25
@slachiewicz slachiewicz added enhancement New feature or request maintenance labels Aug 8, 2026

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 a NullPointerException when invoking reporting goals directly for multipage reports by providing a SinkFactory during site rendering, aligning behavior with Maven Site Plugin so multipage reports work consistently in both direct-goal and site modes.

Changes:

  • Provide a MultiPageSinkFactory to generate(...) in reportToSite() and render any collected subpage sinks after the main page is merged into the site.
  • Add internal MultiPageSubSink/MultiPageSinkFactory helpers to track subpage output locations and rendering contexts.
  • Re-enable and strengthen the existing IT (use-as-direct-mojo) to verify both multipage outputs exist and contain expected content.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/main/java/org/apache/maven/reporting/AbstractMavenReport.java Passes a sink factory for multipage sub-sinks during direct-goal site rendering and merges generated subpages into the site output.
src/it/use-as-direct-mojo/verify.groovy Re-enables assertions for multipage output and validates rendered content.
src/it/use-as-direct-mojo/invoker.properties Re-enables the multipage goal in the direct-mojo IT.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +381 to +385
String document = PathTool.getRelativeFilePath(
report.getReportOutputDirectory().getPath(), new File(outputDirectory, outputName).getPath());
// Remove .html suffix since we know that we are in Site Renderer context
document = document.substring(0, document.lastIndexOf('.'));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The claim is right — lastIndexOf returns -1 for a name with no dot and substring(0, -1) throws.

I have left it, deliberately. That line is copied verbatim from MultiPageSinkFactory in maven-site-plugin's ReportDocumentRenderer, and the point of this PR is that a multipage report behaves the same whether its goal is invoked directly or through the site. Making only this copy lenient would mean a report using an extension-less name works standalone and throws under mvn site, which is a worse failure mode than both rejecting it consistently: it would only show up in one of the two paths.

The implied contract is that the name ends in .html — the site plugin's own comment on that line says "Remove .html suffix since we know that we are in Site Renderer context" — so an extension-less name is invalid input either way. Happy to fix it in both places in a follow-up if a maintainer wants the diagnostic improved; it should not be fixed in just one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NPE on reporting goal when multi-pages report

2 participants