[MSHARED-1032] API change: let MavenReport#canGenerateReport() throw an exception

[jira-importer]

Benjamin Marwell opened MSHARED-1032 and commented

Hi everyone,

the AbstractReportMojo in reporting-impl implements a method canGenerateReport() from reporting-api.

However, it is unable to throw any exceptions. Not even MojoExecutionEx or MavenReportEx, which is most unfortunate.

It is being used twice:

Once in execute() throws MojoExEx

and in

generate() throws MavenReportEx (and is called by execute()).

This way, there is no way for reporting plugins to scan for files, because FileUtils::getFiles DOES throw a {}IOException{}, which then cannot be wrapped. However, the IOException from that method is useless anyway, because it is never declared in any methods it calls.

Therefore please consider:

• Declaring any Exception on canGenerateReport()
• Removing the declared IOException in PlexusUtils (PR exists) and Maven-Utils (issue: tbd).

Thanks!

---

Affects: maven-reporting-api-3.0

Issue Links:

• MSHARED-1275 MavenReport#canGenerateReport() is invoked twice
  ("causes")
• MSHARED-193 API change: let MavenReportRenderer#render() throw an exception

Remote Links:

• GitHub Pull Request #17
  
• GitHub Pull Request #22
  
• GitHub Pull Request #156
  

[jira-importer]

Michael Osipov commented

You can always use runtime exceptions...

[jira-importer]

Benjamin Marwell commented

You should know better than me that this is not a good idea, especially in this case. A MojoExecutionException and/or MavenReportException (wrapped around the {}IOException{}) is much better handled and printed.

I am not really sure what to make from your comment. Yes, I know I could throw a {}RuntimeException{}. But as you could have guessed from the fact that I created this issue in the first place, I sincerely think changing the API is a cleaner solution here. ;)

[jira-importer]

Michael Osipov commented

Quick hack solution ;-) I will try to check this in 2022-02.

[jira-importer]

Hervé Boutemy commented

I added links to javadoc for reporting-impl and reporting-api, to better describe where the API update questions really happen :)

[jira-importer]

Michael Osipov commented

Hervé Boutemy, really in a minor version?

[jira-importer]

Hervé Boutemy commented

we'll define everything step by step:

1. it will be for the next version of reporting-api https://maven.apache.org/shared/maven-reporting-api/ (unrelated to Maven 4)
2. we'll see if finally the next version of maven-reporting-api is 3.1 or 4.0
3. we'll se also which exception to add: reading the javadoc, it seems that MavenReportException looks like a better choice https://maven.apache.org/shared/maven-reporting-api/apidocs/index.html , because reporting-api is not tied to Mojo at all (that's the role of reporting-impl to fill the gap: https://maven.apache.org/shared/maven-reporting-impl/ )

on major and minor question, IMHO the important aspect is binary compatibility:

1. will a new report using this modified API with an Exception still be usable from maven-site-plugin 3.x?
2. will an old report using unmodified 3.0 API without exception still be usable from maven-site-plugin that support the new API signature?

IMHO, that will impact our choice of version both for maven-reporting-api AND maven-site-plugin = the real user-visible impact

 

[jira-importer]

Michael Osipov commented

Don't confuse generate() with execute() one is report while the latter is mojo. Hervé Boutemy, the major/minor issue needs to be analyzed. I'd like to finalize the lift to Doxia (Sitetools) 1.11.1 first and then move to issues like this.

[jira-importer]

Benjamin Marwell commented

canGenrateReport() is actually called twice: in execute() and then in generate() again (which is also called by execute()).

[jira-importer]

Michael Osipov commented

The reason is that one is report and the other mojo. We have components which do both. Maybe this can be better solved.

[jira-importer]

Michael Osipov commented

bmarwell, looking again at this before GA: Regardless of the exception you need canGenerateReport() should not be called twice since it may contain expensive calls. I'd say that we first need to solve the double call before this can be discussed. WDYT?

[jira-importer]

Hervé Boutemy commented

execute() and generate() does not mean executed twice: it means called in 2 conditions, either from a direct goal invocation or as from a report in site:site goal

generate() is the important case (as execute() just adds a basic site:site simplified goal to call generate as site does)

[jira-importer]

Hervé Boutemy commented

now I re-read the issue from the start: "of course", not "throw Exception"
but something like renderexception (precise exception has to be chosen)

and "of course", we're talking about changing reporting-api , and it will impact m-site-p and reporting-impl (which is the mini site:site implementation)

I've been confused with the related MSHARED-193 that has the same discussion for another API call of reporting-api

notice: these issues should be associated to reporting-api only
impact on maven-site-plugin and reporting-api will be when they upgrade their dependency on reporting-api

[jira-importer]

Benjamin Marwell commented

Ha, I wish I knew which plugin I was trying to modify when I stumbled over this. Probably the checkstyle-plugin.

Maybe I did not mean "double call", but instead "called on two occurences", but that is hard to guess from now. If it was from two different entry points, it should be two different methods (imho). The default can be that one calls another...

 

Then there's the exception issue, yes, which can be solved right afterwards.

[jira-importer]

Michael Osipov commented

bmarwell, you are right. It is indeed called twice for standalone AND Maven Site Plugin use case in org.apache.maven.reporting.AbstractMavenReport.generate(Sink, SinkFactory, Locale) also both uses cases make sure that canGenerateReport() is invoked before generate() is invoked. This is clearly redudant an can incur a performance overhead. I will create a new issue and provide a PR.

As for the actual request: There are two spots which are subject to the call, I think we can add this exception, I have already found a suitable usecase, but need to check how the system behavior since the signature has changed.

[jira-importer]

Michael Osipov commented

Duplcate call has been resolved, let's move on with the actual request...