Issue #4387: problem with usage of third-party Check libraries and ch…

[Luolc]

#4387

@romani
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java#L202
The direct cause is that we have duplicate keys(the class simple name) in this stream.

I guess the reason is we have some classes that have the same simple name in both checkstyle project and sevntu.checkstyle project.
We could see HideUtilityClassConstructorCheck in the log, and there are https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/design/HideUtilityClassConstructorCheck.java and https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/HideUtilityClassConstructorCheck.java.

I am not sure which class should I use here. More specifically, to use checkstyle's HideUtilityClassConstructorCheck or sevntu.checkstyle's HideUtilityClassConstructorCheck? (same question with other classes with same name) Currently I just choose the new one in the stream.

I don't know how to reproduce the bug in IDE currently. So I was not able to use a debug mode to check the packages field when the bug occurs.

[rnveach]

I am not sure which class should I use here.

Should we change the map from Map<String, String> to Map<String, List<String>> and loop through when we find same named checks?

I don't know how to reproduce the bug in IDE currently.
to check the packages field when the bug occurs.

Change the POM for the checkstyle:check run of sevntu's sample to use snapshot version of checkstyle. Install a snapshot version of checkstyle (without your fix) where you print out the field to the system output. Then run checkstyle:check on sevntu's sample.

[Luolc]

I am not sure which class should I use here

Now I think that we should not use the checkstyle's class. Before excute this line: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java#L146, we have already search the name in the hard coded map, that means we are not able to generate a checkstyle's class already.

@rnveach

Should we change the map from Map<String, String> to Map<String, List> and loop through when we find same named checks?

IMO it could not solve the problem. When users write a "FooCheck" in the config xml, and there are more than one check named "FooCheck", we don't know which one users exactly want. IMO, trying to instantiate them one by one and return the first one successful is not a good solution.
For example, there are "com.foo.FooCheck" and "com.bar.FooCheck". Users want to use "com.bar.FooCheck" and don't know there is a "com.foo.FooCheck". If we just try each "FooCheck" and return the first successful one("com.foo.FooCheck"), users would be puzzled.

[rnveach]

Now I think that we should not use the checkstyle's class.

@Luolc Yes, I agree. For our checks we should drop them from 3rd party checks. We should identify our checks as the ones in the field NAME_TO_FULL_MODULE_NAME.

[romani]

@Luolc , @rnveach , we will never guess what used meant by his config.

All we need is to throw exception as we do now, but clearly define what we do not like:
In config there is "FooCheck", but in classpath there are"com.foo.FooCheck" and "com.bar.FooCheck", please resolve ambiguity by specifying fully qualifies name in config.

Internally in maps we should keep know about all classes.

[Luolc]

@romani
So should we throw the exception when users do write the ambiguous name in config? Or earlier when we find that there are ambiguous classes name, no matter users use them or not?
For the former, we might need to use the data-structure suggested by @rnveach , a Map<String, List<String>>, and to throw the exception when we do map.get and find the list size is bigger than 1.
For the latter, we could keep the <String, String> map and throw the exception when generating the map.

[codecov-io]

Codecov Report

Merging #4389 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4389   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         285     285           
  Lines       15292   15310   +18     
  Branches     3477    3480    +3     
======================================
+ Hits        15292   15310   +18

Impacted Files	Coverage Δ
...pycrawl/tools/checkstyle/PackageObjectFactory.java	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 015ecca...0d7e24c. Read the comment docs.

[romani]

current fix can not be merged as it will not allow to use non-standard Check as override for standard, that probably all users did.

Or earlier when we find that there are ambiguous classes name, no matter users use them or not?

We should not care about any madness in classpath of user till it make problem to us.

So should we throw the exception when users do write the ambiguous name in config?

Yes, when we are not sure, the whole responsibility in user.

[Luolc]

@rnveach
I cannot running the command with the SNAPSHOT version with my fix. I don't know what step is wrong. Please help. :)
I changed this line to 7.9-SNAPSHOT: https://github.com/sevntu-checkstyle/checkstyle-samples/blob/master/maven-project/pom.xml#L14
and installed SNAPSHOT version with my fix by running mvn clean install -DskipTests -Dcobertura.skip=true -Dfindbugs.skip=true -Dpmd.skip=true at checkstyle dir. (I commented the sevntu check in pom.xml or it would fail)
But running mvn -X checkstyle:check on checkstyle-samples still throw the "duplicate key" exception. I even tried changing the generateThirdPartyNameToFullModuleName method (https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java#L199-L209) to just throw a RuntimeException, but it didn't work. It seems that the command still using the old version checkstyle, but not the SNAPSHOT with new fix.

@romani

current fix can not be merged as it will not allow to use non-standard Check as override for standard

The filter could drop the standard checkstyle's check(by checking whether NAME_TO_FULL_MODULE_NAME contains it). Only for this issue, there would be only sevntu's check so that no "duplicate key" exception would be raised, IMO.
But now I am facing the problem stated above... Thus I am not able to prove it.

I will move on to the ambiguous name problem after I confirm whether the filter way could work for the sevntu issue. BTW, would I fix the ambiguous name problem in this PR as well or split a new issue?

[romani]

@Luolc ,

Please help. :)

please share all changes with us, to help you, looks like problem is in tiny nuance. If share your sources , I can you code.
Please recheck following:

• in ~/.m2/....../checkstyle you have only SNAPSHOT version. If there is any other - please remove them.
• run, if after launch, another version is appear - it mean that POM demand it somehow, so that version should be eliminated and example project is forced to use only one jar.

But now I am facing the problem stated above... Thus I am not able to prove it.

You can not reproduce problem or check fix ? All CIs reproduced it.

BTW, would I fix the ambiguous name problem in this PR as well or split a new issue?

filter fix is not acceptable, please provide fix with exception for ambiguous name in this PR.

[rnveach]

I cannot running the command with the SNAPSHOT version with my fix.

Most likely your install wasn't successful as long as you saved your changes to that pom. I was able to do it and print out the results of the packages field in generateThirdPartyNameToFullModuleName using System.out.println("packages field: " + Arrays.toString(packages.toArray()));.
I will help you in gitter if you are still having issues.

So I was not able to use a debug mode to check the packages field when the bug occurs.

This is the print out:

[INFO] --- maven-checkstyle-plugin:2.17:check (default-cli) @ maven-project ---
packages field: [com.puppycrawl.tools.checkstyle.checks.annotation, com.puppycrawl.tools.checkstyle.checks.blocks, com.puppycrawl.tools.checkstyle.checks.coding, com.puppycrawl.tools.checkstyle.checks.design, com.puppycrawl.tools.checkstyle.checks.header, com.puppycrawl.tools.checkstyle.checks.imports, com.puppycrawl.tools.checkstyle.checks.indentation, com.puppycrawl.tools.checkstyle.checks.javadoc, com.puppycrawl.tools.checkstyle.checks.metrics, com.puppycrawl.tools.checkstyle.checks.modifier, com.puppycrawl.tools.checkstyle.checks.naming, com.puppycrawl.tools.checkstyle.checks.regexp, com.puppycrawl.tools.checkstyle.checks.sizes, com.puppycrawl.tools.checkstyle.checks.whitespace, com.puppycrawl.tools.checkstyle.checks, com.puppycrawl.tools.checkstyle.filefilters, com.puppycrawl.tools.checkstyle.filters, com.puppycrawl.tools.checkstyle, com.github.sevntu.checkstyle.checks.annotation, com.github.sevntu.checkstyle.checks.coding, com.github.sevntu.checkstyle.checks.design, com.github.sevntu.checkstyle.checks.naming, com.github.sevntu.checkstyle.checks.sizes, com.github.sevntu.checkstyle.checks.whitespace, com.github.sevntu.checkstyle.checks, com.github.sevntu.checkstyle.grammars, com.github.sevntu.checkstyle, com.puppycrawl.tools.checkstyle.checks.duplicates, com.puppycrawl.tools.checkstyle.checks.j2ee]

current fix can not be merged as it will not allow to use non-standard Check as override for standard

@romani As long as they use a different package for their check, they can use the non-standard check by using the fully qualified path in the configuration. We should always default to our checks first if just the check name is given IMO. If they use the EXACT same package and class name as our checks, we cannot guarantee which one the java classloader will use.

[Luolc]

I removed all files in ~/.m2/repository/com/puppycrawl/tools/checkstyle/ and re-install the SNAPSHOT version and it works. Thanks :D

@romani

You can not reproduce problem or check fix ? All CIs reproduced it.

I can reproduce the problem. I mean I could not check my fix before, now the SNAPSHOT works.

filter fix is not acceptable, please provide fix with exception for ambiguous name in this PR.

Sure.

With the current changes(the filter), mvn checkstyle:check could success in "checkstyle-samples".

So there are some strategies now:

1. If there is one and only one check that has the same simple name as any check in checkstyle, use the third-party one.
2. If ... (same with 1), use checkstyle's check.
3. If ... (same with 1), throw "ambiguous name" Exception.

I think if there are two third-party checks with same name, we should throw exception obviously.

@romani @rnveach , I would work on the fix once you confirm to use which strategy :D

[romani]

I vote for "3)" and only when we try to instantiate Module (unclear name is on config)

[Luolc]

@romani
And that means we will need to change all the check names in sevntu's config xml to full qualified ones(i.e. in https://github.com/checkstyle/checkstyle/blob/master/config/checkstyle_sevntu_checks.xml), and the users who are using sevntu-checkstyle would be affected (I don't know whether sevntu is only used ourselves or there are others using it). Is that OK?

[rnveach]

@Luolc @romani I agree with point 3 as long as that simple check name with the conflict is used in config.
We shouldn't get this exception if check is never used or if only used full paths for the check.

[romani]

And that means we will need to change all the check names to full qualified ones

yes, for all conflicting names ONLY.

I don't know whether sevntu is only used ourselves or there are others using it

There are bunch of users, you can find them even on github.
They might not be affected, as usage of all Checks is madness only in checkstyle project.
All other projects use only part of checkstyle's Checks (not all).

[Luolc]

@romani

They might not be affected, as usage of all Checks is madness only in checkstyle project.
All other projects use only part of checkstyle's Checks (not all).

Got it.

So (3) wins. Last thing to confirm, should I also change the sevntu's config xml in this PR? If answer is yes, in one commit or separate commits?

[rnveach]

should I also change the sevntu's config xml in this PR?

Most likely yes. Sevntu config is used in our other projects, so it has to stand up to this fix and not cause issues. See https://github.com/checkstyle/sonar-checkstyle/blob/master/checkstyle-sonar-plugin/pom.xml#L201 .
We need to be sure everything is fixed and working this time.

If answer is yes, in one commit or separate commits?

2 is fine.

Edit: sorry for the multiple posts.

[Luolc]

With version 7.8, this fix won't affect the maven checkstyle check task. CI would still fail. So I change it to 7.9-SNAPSHOT. Is it OK? Why did we use stable version but not current SNAPSHOT version before?

[rnveach]

I didn't think maven would work with dependency that defines itself if said dependency hasn't been installed yet as it gets all dependencies from the maven cache folder.
Can someone confirm it isn't using the previously installed SNAPSHOT version? (I believe this is what is happening)

If this does work as intended than #4403 isn't needed.

[rnveach]

It looks like it does work. I can run the checkstyle phase without the dependency installed.
If a dependency is already installed, it still seems to use the current project and not the installed one.
I'm still looking if maven documents this behavior somewhere.

[romani]

Why did we use stable version but not current SNAPSHOT version before?

Maven release plugin is failing if any snapshot version is found, as release version have to be stable and based on strict versions

[rnveach]

Maven release plugin is failing if any snapshot version is found

@Luolc So we can't keep this change. We will get around limitation by using #4403 then.
Passing CI is still a good sign that the fix will work.

[Luolc]

Smth to confirm:

1. The maven-checkstyle-plugin dependency version. See the review above.
2. In the past CI, the sevntu check did not execute at all if it has a same name with the standard one. In the past, we instantiate an object with the order: checkstyle's check, third-party check, full qualified name. If a name could be found in the hardcode map, we would not check whether there is a third-party one with the same name. It also explains why CI is green now. (I haven't changed sevntu config xml yet) If we want to implement the strategy (3), we would need to check the third-party map first, and in that case the hardcode map would be sort of meaningless.

@romani @rnveach What's your opinions?

[rnveach]

to implement the strategy (3), we would need to check the third-party map first

Option 3 is only if simple module name is used and it exists in 3rd party and checkstyle.
We would need to check 3rd party and hardcoded map at the same time.

the hardcode map would be sort of meaningless

Hardcoded map isn't meaningless.

Here is the new order of operation as I have understood the discussion:

1. Check for simple name conflict in 3rd party and hardcoded, exception if conflict
2. Find simple name module in hardcoded
3. Find simple name module in 3rd party
4. Try to instantiate object as if it was full path

1-3 should only be done if module is simple name (no period).
3rd party checks should never include hardcoded checks (via full class path).

I haven't changed sevntu config xml yet

With option 3, we should get exception with sevntu config as it is, since it uses simple names, once changed, it shouldn't fail.
sevntu should be changed to full class path.

[romani]

to my mind problem is in loading of Checks from class path.
By loading thirdparty Checks we do load all Checks from classpath - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java#L199

private Map<String, String> generateThirdPartyNameToFullModuleName(ClassLoader loader) {
....
returnValue = ModuleReflectionUtils.getCheckstyleModules(packages, loader).stream()
     .collect(Collectors.toMap(Class::getSimpleName, Class::getCanonicalName));

so we cheated on name and it is now a problem. In code above there is no filter for known standard Check classes by fully qualified names.
So we have to filter out all Checks that belong to standard Checkstyle here. So by such fix we will NOT resolve problem but we will be honest on what is inside of this collection. It could be even workaround.
As it will return behavior to how it was before - standard Checks got preference in name recognition so in case of name conflict, standard Check is used, no exceptions, user have to use fully qualified name.

But there will be still possibility to have name conflicts in few thirdparty jars. So lets create separate issue on this and process it separately, as we need to fix our CIs(builds) ASAP. So current fix branch we be moved to next PR/issue. And we need to back to one of first solution with filtering.

@Luolc , @rnveach are you agree to make fix in few steps/PRs ?
we do need to make our CIs green again as soon as possible.
I will not switch codebase to checkstyle8 till we make final fix and release it (I will do hot fix release).

[rnveach]

@romani I agree, just to re-confirm what needs to be done:

1. remove 'ambiguous name' exception code and any other changes in PR and start fresh
2. add code to remove hardcoded checks from 3rd party list
3. create new issue on duplicate keys found in only 3rd party checks

[romani]

yes, just "remove 'ambiguous name' exception" will be copy all changes to another branch on Luolc repo, as we will continue discussion .

[romani]

I did a fix to unblock CIs - 064cd90

new issue - #4414

I am going to do hot fix release to make CIs green again. Unfortunately we depend on previous checkstyle version, without release it will not pass.

[romani]

I am closing this PR, as it need to be recreated with reference to new issue.