[MJAVADOC-617] Normalize module path so that '..' in the path are resolved #27

weissreto · 2019-07-19T08:28:26Z

On line 2403 a path is removed from a map. The given path may not be normalized but the pathes in the map are. This leads to the fact that the project for the given path is not found. And therefore no javadoc for the project is generated.
By normalizing the module path on line 2396 the code works event if the module path has '..' in it.

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MJAVADOC-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MJAVADOC-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
[x ] Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

On line 2403 a path is removed from a map. The given path may not be normalized but the pathes in the map are. This leads to the fact that the project for the given path is not found. And therefore no javadoc for the project is generated. By normalizing the module path on line 2396 the code works event if the module path has '..' in it.

chrisinmtown · 2020-01-21T18:12:19Z

I see that this project is active. The javadoc plugin version 3.1.1 has this bug, @weissreto has proposed a fix. Is the team considering a merge and release of a new version? Thanks in advance for any info.

eolivelli · 2020-01-21T20:33:56Z

@chrisinmtown yes the project is hopefully still alive :)
We are very few, only volunteers, that work on Maven, that is composed of tens of repositories.
If you are interested in merging this fix you can ping politely for it and you can also bring to to the attention using dev@maven.apache.org mailing list.

I will check this issue as soon as I have cycles

chrisinmtown · 2020-01-22T13:19:18Z

Thanks @eolivelli for the quick reply.
@weissreto I guess there is no test case now that reveals this failure, may I suggest that you please add a case to test the behavior, the case should fail on the 3.1.0 and 3.1.1 code, and pass on the code you propose.

elharo · 2020-03-09T12:00:01Z

src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java

@@ -2393,7 +2393,7 @@ protected void executeReport( Locale unusedLocale )
        List<Path> modulePaths = new LinkedList<>();
        for ( String module :  aggregatedProject.getModules() )
        {
-            modulePaths.add( new File( aggregatedProject.getBasedir(), module ).toPath() );
+            modulePaths.add( new File( aggregatedProject.getBasedir(), module ).toPath().normalize() );


You can probably use Paths.get here and avoid the File class completely.

Have simpified code to use only class Path and not File.
Added also a regression test for the issue

elharo · 2020-03-30T15:32:32Z

src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java

@@ -1,5 +1,43 @@
 package org.apache.maven.plugins.javadoc;

+import static org.apache.maven.plugins.javadoc.JavadocUtil.isEmpty;


This is a not a well known class so static imports should probably be avoided.

Hi Elliotte
Sorry I messed up this commit, because my IDE reorganized the whole import section of this file. I reverted that in my last commit. I could squash the commits to get a clearer history if that is recommended. However I have not introduced this static import in this commit here I only moved it around.
Do I still have to refactor this static import? This will introduce some additional changes to this PR that are not relevant to the issue I try to fix.

I missed that the imports were reorganized. If this is existing code, no need to fix it here. I don't know if maven follows any conventions for import ordering, but absent that it's best not to reorder imports to avoid churn.

elharo · 2020-03-30T15:33:45Z

src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java

-        {
-            overviewSummary = new File( apidocs, "index.html" );
-        }
+        File overviewSummary = getOverviewSummary(apidocs);

        assertTrue( overviewSummary.exists() );
        String overview = readFile( overviewSummary ).toLowerCase();


toLowerCase needs a locale

elharo · 2020-03-30T15:34:42Z

src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java

+      mojo.execute();
+
+      File apidocs = new File( getBasedir(), "target/test/unit/aggregate-modules-not-in-subfolders-test/target/site/apidocs" );
+      assertTrue(apidocs.exists());


assertTrue( apidocs.exists() );

elharo · 2020-03-30T15:35:07Z

src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java

+
+      File apidocs = new File( getBasedir(), "target/test/unit/aggregate-modules-not-in-subfolders-test/target/site/apidocs" );
+      assertTrue(apidocs.exists());
+      assertTrue( getOverviewSummary(apidocs).exists() );


( apidocs )

Seems checkstyle does not run on test classes ...
Will fix

elharo · 2020-03-30T15:35:39Z

src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java

+      File overviewSummary;
+      if ( JavaVersion.JAVA_SPECIFICATION_VERSION.isBefore( "11" ) )
+      {
+          overviewSummary = new File( apidocs, "overview-summary.html" );


return new File...
is shorter and avoids an uninitialized variable.

elharo · 2020-03-30T15:35:51Z

src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java

+      }
+      else
+      {
+          overviewSummary = new File( apidocs, "index.html" );


return new File...

elharo · 2020-03-30T15:36:30Z

src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java

+      assertTrue( getOverviewSummary(apidocs).exists() );
+    }
+
+    private File getOverviewSummary(File apidocs)


elharo · 2020-03-30T15:37:15Z

.../apache/maven/plugins/javadoc/stubs/AggregateNotInSubFolderProject1TestMavenProjectStub.java

+import org.apache.maven.project.MavenProject;
+
+/**
+ * @author <a href="mailto:vincent.siveton@gmail.com">Vincent Siveton</a>


Is this you?

elharo · 2020-03-30T15:37:55Z

.../apache/maven/plugins/javadoc/stubs/AggregateNotInSubFolderProject2TestMavenProjectStub.java

+import org.apache.maven.project.MavenProject;
+
+/**
+ * @author <a href="mailto:vincent.siveton@gmail.com">Vincent Siveton</a>


elharo · 2020-03-30T15:38:37Z

.../apache/maven/plugins/javadoc/stubs/AggregateNotInSubFolderProject2TestMavenProjectStub.java

+{
+    public AggregateNotInSubFolderProject2TestMavenProjectStub()
+    {
+        readModel( new File( getBasedir(), "pom.xml" ) );


There's a lot of duplicate code here that could probably be refactored into a common location.

I agree that there is alot of duplicated code. I copied them from older test classes that made the same thing. Will refactor all this code.

However, why do I need this stub classes at all? I tried to avoid them but then the test seems not to work correctly?

- Refactor duplicated code into common new AbstractAggregateMavenProjectStub and AbstractAggregateChildMavenProjectStub classes - Fix toLowerCase without Locale - Simplify method getOverviewSummary and make it static

elharo

just some nits

elharo · 2020-04-01T10:14:04Z

src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java

+      mojo.execute();
+
+      File apidocs = new File( getBasedir(), "target/test/unit/aggregate-modules-not-in-subfolders-test/target/site/apidocs" );
+      assertTrue( apidocs.exists() );


apidocs.isDirectory() would be slightly stronger test

elharo · 2020-04-01T10:14:53Z

src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java

+
+      File apidocs = new File( getBasedir(), "target/test/unit/aggregate-modules-not-in-subfolders-test/target/site/apidocs" );
+      assertTrue( apidocs.exists() );
+      assertTrue( getOverviewSummary( apidocs ).exists() );


exists --> isFile?

weissreto · 2020-04-08T06:19:13Z

Hi Elliotte

Thanks a lot for reviewing and approving my PR.
What is next? Do I have to do something else so that my PR is merged?

Thanks for any advise.

Regards
Reto

weissreto · 2020-04-08T13:00:15Z

Thanks a lot for your help

elharo reviewed Mar 9, 2020

View reviewed changes

weissreto added 3 commits March 30, 2020 17:19

MJAVADOC-617 Add regression test

5f6d44c

MJAVADOC-617 Simplify code as suggested by review

437d736

MJAVADOC-617 Fix checkstyle problems

0faa689

elharo reviewed Mar 30, 2020

View reviewed changes

MJAVADOC-617 Refactor duplicated code into abstract sub classes

793f6f7

- Refactor duplicated code into common new AbstractAggregateMavenProjectStub and AbstractAggregateChildMavenProjectStub classes - Fix toLowerCase without Locale - Simplify method getOverviewSummary and make it static

elharo changed the title ~~MJAVADOC-617 Normalize module path so that '..' in the path are resolved~~ [MJAVADOC-617] Normalize module path so that '..' in the path are resolved Apr 1, 2020

elharo reviewed Apr 1, 2020

View reviewed changes

MJAVADOC-617 improve test assertions

cc66703

elharo approved these changes Apr 1, 2020

View reviewed changes

elharo merged commit d39b275 into apache:master Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MJAVADOC-617] Normalize module path so that '..' in the path are resolved #27

[MJAVADOC-617] Normalize module path so that '..' in the path are resolved #27

weissreto commented Jul 19, 2019 •

edited

chrisinmtown commented Jan 21, 2020 •

edited

eolivelli commented Jan 21, 2020

chrisinmtown commented Jan 22, 2020 •

edited

elharo Mar 9, 2020

weissreto Mar 30, 2020

elharo Mar 30, 2020

weissreto Apr 1, 2020

elharo Apr 1, 2020

elharo Mar 30, 2020

elharo Mar 30, 2020

elharo Mar 30, 2020

weissreto Apr 1, 2020

elharo Mar 30, 2020

elharo Mar 30, 2020

elharo Mar 30, 2020

elharo Mar 30, 2020

elharo Mar 30, 2020

elharo Mar 30, 2020

weissreto Apr 1, 2020

elharo left a comment

elharo Apr 1, 2020

elharo Apr 1, 2020

weissreto commented Apr 8, 2020

weissreto commented Apr 8, 2020

		@@ -1,5 +1,43 @@
		package org.apache.maven.plugins.javadoc;

		import static org.apache.maven.plugins.javadoc.JavadocUtil.isEmpty;

[MJAVADOC-617] Normalize module path so that '..' in the path are resolved #27

[MJAVADOC-617] Normalize module path so that '..' in the path are resolved #27

Conversation

weissreto commented Jul 19, 2019 • edited

chrisinmtown commented Jan 21, 2020 • edited

eolivelli commented Jan 21, 2020

chrisinmtown commented Jan 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissreto commented Apr 8, 2020

weissreto commented Apr 8, 2020

weissreto commented Jul 19, 2019 •

edited

chrisinmtown commented Jan 21, 2020 •

edited

chrisinmtown commented Jan 22, 2020 •

edited