Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return null for module name when extraction fails #30

Closed
wants to merge 1 commit into from
Closed

Return null for module name when extraction fails #30

wants to merge 1 commit into from

Conversation

ctubbsii
Copy link

As detailed in https://issues.apache.org/jira/browse/MJAVADOC-609,
module name extraction can easily fail, due to a legacy jar having a
file naming convention that doesn't match what ModuleFinder expects,
when determining an automatic module name from the file name.

This failure to determine the automatic module name should not cause the
jar to be missing from the resulting class path.

This patch handles the FindException that ModuleFinder throws when
looking for module descriptors, and returns null, indicating no name.
This should allow the jar to still be added as a class path element,
even though it can't be added as a module path element. Named modules
won't be able to use the jar from the module path, but automatically
named modules should still be able to use the jar from the UNNAMED
module on the class path.

Also:

  • Update LocationManagerIT to test the change
  • Make the LocationManager perform strict check for file existence
  • Fix broken LocationManagerIT tests (references to non-existent
    test/resource/dir.* directories) revealed by strict file existence
    checks in LocationManager

As detailed in https://issues.apache.org/jira/browse/MJAVADOC-609,
module name extraction can easily fail, due to a legacy jar having a
file naming convention that doesn't match what ModuleFinder expects,
when determining an automatic module name from the file name.

This failure to determine the automatic module name should not cause the
jar to be missing from the resulting class path.

This patch handles the FindException that ModuleFinder throws when
looking for module descriptors, and returns null, indicating no name.
This should allow the jar to still be added as a class path element,
even though it can't be added as a module path element. Named modules
won't be able to use the jar from the module path, but automatically
named modules should still be able to use the jar from the UNNAMED
module on the class path.

Also:

* Update LocationManagerIT to test the change
* Make the LocationManager perform strict check for file existence
* Fix broken LocationManagerIT tests (references to non-existent
  test/resource/dir.* directories) revealed by strict file existence
  checks in LocationManager
@rfscholte
Copy link
Member

I'm not really happy with this proposal. Now it will always swallow the exception in case of an invalid filename, whereas the exception contains proper information what the issue is.
I'd prefer to leave it up to the consumer of plexus-java to decide what to do with the exception.

@ctubbsii
Copy link
Author

Now it will always swallow the exception in case of an invalid filename

The change attempts to determine the likely cause of the FindException (because the cause isn't clear from the exception alone, which is poor API design on the part of ModuleFinder, I think), and handle it. It will only swallow the exception if the likely reason was because the answer to the question getModuleName is null (which is already a valid response from that method).

We could make that "likely reason" check more precise, by checking the file name against the regex described in the ModuleFinder javadoc, but I thought that might be too brittle (since it's possible that regex could change over time as ModuleFinder improves).

For what it's worth, I made several earlier attempts to preserve the exception and add the path item to the class path elements, but those attempts broke many more tests, and required a larger code change. In the end, I came to the conclusion that the exception shouldn't have been thrown from ModuleFinder in the first place, when the situation is not exceptional (it's quite common, and very distinct from actual exceptions, like not having permission to read the file or the file being corrupt). ModuleFinder should have returned null or an empty list for this situation, but since it didn't, I decided it should be fixed as close as possible to the ModuleFinder usage.

To be honest, I'm not 100% comfortable with swallowing the exception this way either. I'd be much more comfortable swallowing a subclass of FindException, or checking a reason code that was specifically for this case, but alas, the ModuleFinder API only throws this catch-all exception... leaving the caller to try to figure out the reason on their own.

The main reason I care about this is because of the impact on MJAVADOC-609, so if this solution is rejected, I'd be happy to help review/test other solutions (here or in maven-javadoc-plugin) that gets the class path correct for these kinds of classic jars with irregular file names. Anything to help get this fixed sooner.

@rfscholte
Copy link
Member

Let's fix it at the maven-javadoc-plugin

@rfscholte rfscholte closed this Nov 23, 2019
@ctubbsii
Copy link
Author

@rfscholte Okay, sounds good.

@ctubbsii
Copy link
Author

In that case, it looks like the pull request in apache/maven-javadoc-plugin#35 is what I would have come up with.

@ctubbsii ctubbsii deleted the fix-MJAVADOC-609 branch November 27, 2019 22:55
@olamy olamy added the bug label Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants