[MDEP-679] mvn dependency:analyze detected wrong transitive dependency

[johnlinp]

[MDEP-679] Should not include string literals when parsing references

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. Also be sure having selected the correct component.
• Each commit in the pull request should have a meaningful subject line and body.
• Format the pull request title like [MSHARED-XXX] - Fixes bug in ApproximateQuantiles,
  where you replace MSHARED-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.
• Run mvn clean verify to make sure basic checks pass. A more thorough check will
  be performed on your pull request automatically.
• You have run the integration tests successfully (mvn -Prun-its clean verify).

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.

[elharo]

This needs a test, probably an integration test.

[johnlinp]

Hi @elharo,

I've added a test case for this patch and fixed the issue you mentioned. Please take a look at it again. Thanks!

[rfscholte]

I'm just wondering what the original classname was where you hit this issue, lower case coffee doesn't make sense. Current implementation will support Class.forName("coffee"), which is important to keep working (should be added as a separate test).
For this reason there are options to ignore dependencies, see https://maven.apache.org/plugins/maven-dependency-plugin/analyze-mojo.html

[johnlinp]

Hi @rfscholte,

The original class name I encountered was update.class in the artifact dnsjava, as you can see at https://issues.apache.org/jira/browse/MDEP-679. There are more classes in dnsjava: dig.class, lookup.class, etc. You can see the full list at http://www.java2s.com/Code/Jar/d/Downloaddnsjava211jar.htm.

[johnlinp]

Also, I am aware of the the usage of the option ignoredUsedUndeclaredDependencies in the mojo dependency:analyze. I think detecting cases like Class.forName("coffee") is not maven-dependency-analyzer's responsibility. If user needs "coffee" to be treated as a used dependency, the option usedDependencies should be used.

[rfscholte]

Those are terrible classnames and in the end they will cause trouble in the modular system (once adopted). Better ask the maintainers of that project to add reasonable packages.
I understand your opinion about the scope of the maven-dependency-plugin, but so far it has prevented a lot of false negatives.
I don't think we should change the current behavior just for this. I'm pretty sure we'll get bug reports if we would merge this PR.

[johnlinp]

If I don't specify version when running mvn dependency:analyze, the default version of maven-dependency-plugin will be 2.8. When running with version 2.8, it doesn't treat string literals like Class.forName("update") as used dependency.

Therefore, I think this PR merely makes the behavior of the dependency detection as before, which I think is more reasonable than current behavior.

[rfscholte]

Suppose I would except this change, you can't use it as long as you call dependency:analyze. Your reference should be the latest, not 2.8 from 2015.
The reason why Maven is still using this old 2.8 as default is that we don't want to change behavior when people use a different version of Maven. It is the responsibility of the project maintainer to lock the plugin version.

[hboutemy]

Current implementation will support Class.forName("coffee"), which is important to keep working (should be added as a separate test).

Seriously, this is a supported use case?
When/in which Jira issue was it added?

[hboutemy]

@johnlinp can you please check with every past release of the plugin to find when this change was introduced, please?
then we'll try to see if this was an intentional change (without Unit test = something we'll need to change) or if this was a typo

[hboutemy]

ok, found the commit that introduced this change: 32ee156 for https://issues.apache.org/jira/browse/MSHARED-428 for maven-dependency-analyzer-1.7 used by maven-dependency-plugin 3.0.1 with https://issues.apache.org/jira/browse/MDEP-471
not clear from that issue that this Class.forName() case was an intent: there was a unit test added for the intent, that was lambdas IIUC

[johnlinp]

Hi @hboutemy,

After knowing the related issue MSHARED-428, did you decide whether we should merge this PR?

IMHO, it's really a surprise to see a line of code simply like String s = "org.apache.commons.collections4.CollectionUtils"; to be treated as really using the dependency collections4.

[johnlinp]

Ping @hboutemy?

[johnlinp]

Hi @khmarbaise & @rfscholte,

Would you please review my PR? Thank you.

[johnlinp]

Thank you for integrating my commits into master.