Fix for #118: Fixed NPE in SonarDevon4jPlugin class #120
Conversation
There was a problem hiding this comment.
@lmarniazman thanks for your quick fix. I am approving as merging this PR increases the quality and is fine for me, but our plugin might still be buggy even with this PR merged.
| this.pluginList = Arrays.asList(fileList).stream().map(f -> f.getName()).collect(Collectors.toList()); | ||
| if (fileList != null) { | ||
| this.pluginList = Arrays.asList(fileList).stream().map(f -> f.getName()).collect(Collectors.toList()); | ||
| } else { | ||
| this.pluginList = new ArrayList<>(); | ||
| } |
There was a problem hiding this comment.
OK, so fileList may be null and you are checking for this. This is a strange API of Java JDK, but indeed this can be null if already the directory (this.pluginDirectory) does not exist:
https://docs.oracle.com/javase/7/docs/api/java/io/File.html#listFiles(java.io.FileFilter)
So there must be another error if that directory does not exist. With this fix, we will prevent NPE but wont be able to detect the plugins as the pluginsDirectory seems to be determined wrong.
| @@ -17,7 +17,7 @@ | |||
|
|
|||
| private static final String DISABLED = "Disabled"; | |||
|
|
|||
| private static final String QUALINSIGHT = "qualinsight-plugins-sonarqube-smell-plugin"; | |||
| private static final String QUALINSIGHT = "qualinsight-sonarqube-smell-plugin"; | |||
There was a problem hiding this comment.
was this wrong from the start or has that plugin be renamed and the name might depend on the version of SonarQube server?
There was a problem hiding this comment.
For recent plugin the new name looks sane:
https://github.com/QualInsight/qualinsight-plugins-sonarqube-smell
There was a problem hiding this comment.
The naming of this plugin is a little inconsistent... Most of the released versions are using qualinsight-sonarqube-smell-plugin, except for 3.0.0 & 2.0.0-RC2 (see releases)
There was a problem hiding this comment.
Thanks for that explanation. It might have been better then to implement some magic in our plugin that will try both names and see if one of them works. We might break existing installations using the old name with this change...
hohwille
added
bug
dependencies
|
I will merge this now, but my comments indicate that IMHO #118 is not properly fixed. We only prevent NPE now but the root of the problem is IMHO still present. |
Fixes #118
Added an if-condition checking if the array
fileListis null. If yes, it is not used to fillpluginListwith values, instead an emptyArrayListis returned.