Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

ANY23-445 Review spotbugs issues #151

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

lewismc
Copy link
Member

@lewismc lewismc commented Oct 16, 2019

First pass at addressing https://issues.apache.org/jira/browse/ANY23-445.
Still loads to do.

@lewismc
Copy link
Member Author

lewismc commented Oct 17, 2019

Current branch results in following errors

[INFO] --- spotbugs-maven-plugin:3.1.12.2:check (default-cli) @ apache-any23-api ---
[INFO] BugInstance size is 2
[INFO] Error size is 0
[INFO] Total bugs: 2
[ERROR] org.apache.any23.mime.MIMEType defines compareTo(MIMEType) and uses Object.equals() [org.apache.any23.mime.MIMEType] At MIMEType.java:[line 134] EQ_COMPARETO_USE_OBJECT_EQUALS
[ERROR] Possible null pointer dereference in org.apache.any23.plugin.Any23PluginManager.loadJARDir(File) due to return value of called method [org.apache.any23.plugin.Any23PluginManager, org.apache.any23.plugin.Any23PluginManager] Dereferenced at Any23PluginManager.java:[line 201]Known null at Any23PluginManager.java:[line 201] NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE

@HansBrende if you get a chance can you review. You can run spotbugs analysis as follows

mvn spotbugs:check

You can also get detailed information by running

mvn clean install -DskipTests && mvn spotbugs:gui

You can then load the file in ./api/target/spotbugsXml.xml

@@ -78,6 +78,7 @@ public boolean allExtractorsSupportAllContentTypes() {
return true;
}

@SuppressWarnings("unlikely-arg-type")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's actually a bug here, so adding @SuppressWarnings kinda defeats the purpose of the spotbugs plugin. getSupportedMIMETypes() returns a list of MIMEType, so checking if it contains a String will always return false!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed both the SupressWarnings javac annotation and thought about this one again.

I've decided to introduce <omitVisitors>FindUnrelatedTypesInGenericContainer</omitVisitors> which will omit spotbugs from identifying this issue as a bug. My justification is that, in this case, because the ExtractorFactory#getSupportedMIMETypes() supports wildcards, and as far as I can see there is currently no way to determine the full Collection (e.g. */*) of supported MimeType's then we cannot enforce another check whether an ExtractorGroup supports all MimeType's.

What do you think about this assessment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wasn't clear. The code inside this method has a bug, as correctly identified by the spotbugs plugin. The code is testing if a Collection<MIMEType> contains a String. This will always return false, as a String cannot be an instance of MIMEType.

Therefore, the method needs to be fixed, and the spotbugs plugin is doing its job correctly: no suppression of warnings is needed here.

@@ -24,8 +24,8 @@
import java.util.Iterator;

/**
* It simple models a group of {@link ExtractorFactory} providing
* simple accessing methods.
* Models a group of {@link ExtractorFactory} objcts providing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@lewismc
Copy link
Member Author

lewismc commented Jun 3, 2020

@HansBrende do you have any issue with us pushing this off to the 2.5 development drive? This has blocked the 2.4 release for months and I don't really have the time to invest in addressing it right now. Thanks

@HansBrende
Copy link
Member

@lewismc I can potentially do a quick fix-up on this PR this weekend when I get a chance, and then we should be good to go.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants