Skip to content

Augment config/spotbugs/exclude.xml with finding status and rank #1392

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 21, 2024
Merged

Augment config/spotbugs/exclude.xml with finding status and rank #1392

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8020057
Augment `config/spotbugs/exclude.xml` with finding status and rank
stIncMale May 16, 2024
d06e1ff
Remove an unnecessary exclusion
stIncMale May 16, 2024
d949b60
Address review concerns
stIncMale May 16, 2024
f6dd54b
`RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE` is a "False Positive"
stIncMale May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 48 additions & 111 deletions config/spotbugs/exclude.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,251 +14,186 @@
~ limitations under the License.
-->

<!-- See https://spotbugs.readthedocs.io/en/latest/filter.html#rank
for the mapping from SpotBugs rank to verbal labels.
-->
<FindBugsFilter>
<!-- specific excludes which should be fixed -->
<Match>
<Class name="org.bson.JSONWriterSettings"/>
</Match>
<Match>
<Bug code="EI,EI2"/>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 18 -->
<Bug pattern="EI_EXPOSE_REP,EI_EXPOSE_REP2"/>
Copy link
Member Author

@stIncMale stIncMale May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Used a more specific filter here than the previous code="EI,EI2".

</Match>

<Match>
<!-- MongoDB status: "False Positive", SpotBugs rank: 14 -->
<Class name="org.bson.types.ObjectId"/>
<Bug pattern="DMI_RANDOM_USED_ONLY_ONCE"/>
</Match>

<!-- Once we stop supporting Java SE 8,
we can start relying on `SecureRandom` being thread-safe,
as per https://docs.oracle.com/javase/9/docs/api/java/security/SecureRandom.html,
and reuse its instances. -->
<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 14 -->
<Class name="com.mongodb.internal.connection.AwsAuthenticator$AwsSaslClient"/>
<Bug pattern="DMI_RANDOM_USED_ONLY_ONCE"/>
</Match>

<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 14 -->
<Class name="com.mongodb.internal.connection.ScramShaAuthenticator$DefaultRandomStringGenerator"/>
<Bug pattern="DMI_RANDOM_USED_ONLY_ONCE"/>
</Match>

<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 18 -->
<Class name="org.bson.codecs.BsonValueCodecProvider"/>
<Method name="getBsonTypeClassMap"/>
<Bug pattern="MS_EXPOSE_REP"/>
</Match>

<!-- these specific issues are deliberate design decisions -->

<!-- Deliberately ignoring this, as the check for a null SSLParameters is actually necessary.
See https://jira.mongodb.org/browse/JAVA-2876 for details. -->
<Match>
<!-- MongoDB status: "False Positive", SpotBugs rank: 18 -->
<!-- Deliberately ignoring this, as the check for a null SSLParameters is actually necessary.
See https://jira.mongodb.org/browse/JAVA-2876 for details. -->
<Class name="com.mongodb.client.internal.KeyManagementService"/>
<Method name="enableHostNameVerification" params="javax.net.ssl.SSLSocket"/>
<Bug pattern="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE"/>
</Match>

<!-- Deliberately ignoring this, as many BSONObject subclasses don't do it -->
<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 17 -->
<!-- Deliberately ignoring this, as many BSONObject subclasses don't do it -->
<Package name="com.mongodb"/>
<Bug pattern="EQ_DOESNT_OVERRIDE_EQUALS"/>
</Match>

<!-- Deliberately ignoring return value of CountDownLatch.await -->
<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 5 -->
<!-- Deliberately ignoring return value of CountDownLatch.await -->
<Class name="com.mongodb.internal.connection.BaseCluster"/>
<Bug pattern="RV_RETURN_VALUE_IGNORED"/>
</Match>

<!-- The underlying call to GridFSInputFile closes the file -->
<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 20 -->
<!-- The underlying call to GridFSInputFile closes the file -->
<Class name="com.mongodb.gridfs.GridFS"/>
<Method name="createFile" params="java.io.File"/>
<Bug pattern="OBL_UNSATISFIED_OBLIGATION"/>
</Match>

<Match>
<Class name="com.mongodb.DocumentCodec"/>
<Bug pattern="NM_SAME_SIMPLE_NAME_AS_SUPERCLASS"/>
</Match>

<Match>
<Class name="org.mongodb.DatabaseTestCase" />
<Bug pattern="MS_PKGPROTECT"/>
</Match>

<Match>
<Class name="org.mongodb.FunctionalSpecification"/>
<Bug pattern="MS_PKGPROTECT"/>
</Match>

<Match>
<Class name="com.mongodb.DatabaseTestCase"/>
<Bug pattern="MS_PKGPROTECT"/>
</Match>

<Match>
<Class name="org.bson.types.StringRangeSet$NumberStringComparator"/>
<Bug pattern="SE_COMPARATOR_SHOULD_BE_SERIALIZABLE"/>
</Match>

<!-- Special handling of clone method for cloneable BsonValue subclasses. By design, they don't call super.clone() -->
<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 14 -->
<Class name="org.bson.BsonArray"/>
<Method name="clone"/>
<Bug pattern="CN_IDIOM_NO_SUPER_CALL"/>
</Match>

<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 14 -->
<Class name="org.bson.BsonDocument"/>
<Method name="clone"/>
<Bug pattern="CN_IDIOM_NO_SUPER_CALL"/>
</Match>

<Match>
<Class name="org.bson.BsonDocumentWrapper"/>
<Method name="clone"/>
<Bug pattern="CN_IDIOM_NO_SUPER_CALL"/>
</Match>

<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 16 -->
<Class name="org.bson.RawBsonArray"/>
<Method name="clone"/>
<Bug pattern="CN_IDIOM_NO_SUPER_CALL"/>
</Match>

<Match>
<Class name="org.bson.codecs.jsr310.Jsr310CodecProvider"/>
<Bug pattern="DE_MIGHT_IGNORE"/>
</Match>

<!-- Test exclusions -->
<!-- All bugs in test classes, except for JUnit-specific bugs -->
<Match>
<Class name="~.*\.*Test"/>
<Not>
<Bug code="IJU"/>
</Not>
</Match>

<!-- Deliberate use of an unused field in the Person POJO class -->
<Match>
<Class name="com.mongodb.acceptancetest.crud.pojo.Person"/>
<Bug pattern="SS_SHOULD_BE_STATIC"/>
</Match>

<!-- Spock tests seem to fail the serial version ID test when stubbing. Annoying. -->
<Match>
<Source name="~.*\.groovy"/>
<Bug pattern="SE_NO_SERIALVERSIONID,LI_LAZY_INIT_STATIC,EQ_UNUSUAL,IT_NO_SUCH_ELEMENT,RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT"/>
</Match>

<!-- Strange findbugs issues -->
<Match>
<Source name="~.*ClusterSettingsSpecification\.groovy"/>
<Bug pattern="RANGE_ARRAY_INDEX"/>
</Match>

<Match>
<Class name="~com.mongodb.internal.connection.AbstractByteBufBsonDocument.*"/>
<Bug pattern="NP_BOOLEAN_RETURN_NULL"/>
</Match>

<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 16 -->
<Class name="~com.mongodb.client.ClientSession.*"/>
<Bug pattern="NM_SAME_SIMPLE_NAME_AS_INTERFACE"/>
</Match>

<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 16 -->
<Class name="~com.mongodb.reactivestreams.client.ClientSession.*"/>
<Bug pattern="NM_SAME_SIMPLE_NAME_AS_INTERFACE"/>
</Match>

<Match>
<Class name="~com.mongodb.kotlin.client.ClientSession.*"/>
<Bug pattern="NM_SAME_SIMPLE_NAME_AS_INTERFACE"/>
</Match>

<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 16 -->
<Class name="~com.mongodb.kotlin.client.coroutine.ClientSession.*"/>
<Bug pattern="NM_SAME_SIMPLE_NAME_AS_INTERFACE"/>
</Match>

<Match>
<Class name="~com.mongodb.internal.async.client.ClientSession.*"/>
<Bug pattern="NM_SAME_SIMPLE_NAME_AS_INTERFACE"/>
</Match>

<Match>
<Class name="~com.mongodb.async.client.internal.AsyncCryptConnection.*"/>
<Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT"/>
</Match>

<!-- Spotbugs assumes that SSLSocket#getSSLParameters never returns null, when that is not the case for all JDKs -->
<Match>
<!-- MongoDB status: "False Positive", SpotBugs rank: 18 -->
<!-- Spotbugs assumes that SSLSocket#getSSLParameters never returns null, when that is not the case for all JDKs -->
<Class name="com.mongodb.internal.connection.SslHelper"/>
<Bug pattern="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE"/>
</Match>

<Match>
<!-- MongoDB status: "False Positive", SpotBugs rank: 17 -->
<Class name="com.mongodb.client.internal.CryptConnection"/>
<Method name="retain"/>
<Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT"/>
</Match>

<Match>
<Class name="~com.mongodb.reactivestreams.client.internal.crypt.*"/>
<!-- MongoDB status: "False Positive", SpotBugs rank: 17 -->
<Class name="com.mongodb.reactivestreams.client.internal.crypt.CryptConnection"/>
<Method name="retain"/>
<Bug pattern="RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT"/>
</Match>

<!-- Caused by Java 11 generating a null check on try with resources.
See https://github.com/spotbugs/spotbugs/issues/756 for details. -->
<Match>
<!-- MongoDB status: "TODO Ross", SpotBugs rank: 11 -->
<Class name="org.bson.codecs.kotlin.DataClassCodec$Companion"/>
<Method name="getCodec"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

@rozza, could you please look at this Kotlin finding and tell whether it is a "False Positive" or rather "No Fix Needed" one?

Copy link
Member

Choose a reason for hiding this comment

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

So this is a legit bug, not sure why it wasn't picked up by Spotbugs before. Created a fix in #1395

Copy link
Member Author

Choose a reason for hiding this comment

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

It was picked up, but it was ignored because we were ignoring all RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE findings, due to there being too many false positives related to try-with-resources statements. We don't have those false positives anymore (I am guessing, that's because we are not running SpotBugs with JDK 8 anymore, SpotBugs looks at the bytecode, and the bytecode denerated by different javac versions is different), I removed the corresponding exclusion, and we now can focus on those more reasonable RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE findings.

Copy link
Member Author

@stIncMale stIncMale May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I prefer this PR wait for #1395 to be merged in to master, then incorporate the change from master, get rid of the corresponding SpotBugs exclusion, then be merged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like a False Positive. The CodecRegistry contract requires that a CodecConfigurationException is thrown, and null is not returned, and there are many seemingly identical matches that SpotBugs does not report, e.g. RecordCodec.ComponentModel#computeCodec.

<Bug pattern="RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE"/>
</Match>

<!-- AWS use UNIX-style line endings -->
<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 20 -->
<!-- AWS use UNIX-style line endings -->
<Class name="com.mongodb.internal.connection.AuthorizationHeader"/>
<Bug pattern="VA_FORMAT_STRING_USES_NEWLINE"/>
</Match>

<!-- The method is a wrapper for `Condition.await`, naturally it does not call it in a loop. -->
<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 14 -->
<!-- The method is a wrapper for `Condition.await`, naturally it does not call it in a loop. -->
<Class name="com.mongodb.internal.connection.DefaultConnectionPool$OpenConcurrencyLimiter"/>
<Method name="awaitNanos"/>
<Bug pattern="WA_AWAIT_NOT_IN_LOOP"/>
</Match>

<!-- The checker fails to see that the code prevents concurrent incrementation by using exclusive locking. -->
<Match>
<!-- MongoDB status: "False Positive", SpotBugs rank: 14 -->
<Class name="com.mongodb.internal.connection.DefaultConnectionPool$StateAndGeneration"/>
<Field name="generation"/>
<Bug pattern="VO_VOLATILE_INCREMENT"/>
</Match>
<Match>
<!-- MongoDB status: "False Positive", SpotBugs rank: 14 -->
<Class name="com.mongodb.internal.connection.ConcurrentPool$StateAndPermits"/>
<Field name="permits"/>
<Bug pattern="VO_VOLATILE_INCREMENT"/>
</Match>

<!-- Non-short-circuit operators are used intentionally. -->
<Match>
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 15 -->
<!-- Non-short-circuit operators are used intentionally. -->
<Class name="com.mongodb.internal.connection.ConcurrentPool$StateAndPermits"/>
<Method name="acquirePermit"/>
<Bug pattern="NS_DANGEROUS_NON_SHORT_CIRCUIT"/>
</Match>
<Match>
<Class name="com.mongodb.internal.connection.DefaultConnectionPool$OpenConcurrencyLimiter"/>
<Method name="acquirePermitOrGetAvailableOpenedConnection"/>
<Bug pattern="NS_NON_SHORT_CIRCUIT"/>
</Match>

<!-- Kotlin false positive for validating annotations -->
<Match>
<!-- MongoDB status: "False Positive", SpotBugs rank: 17 -->
<Class name="org.bson.codecs.kotlin.DataClassCodec$Companion"/>
<Method name="~.*validateAnnotations.*"/>
<Bug pattern="UC_USELESS_OBJECT"/>
</Match>

<Match>
<!-- MongoDB status: "False Positive", SpotBugs rank: 17 -->
<Class name="org.bson.codecs.kotlinx.KotlinSerializerCodec$Companion"/>
<Method name="~.*validateAnnotations.*"/>
<Bug pattern="UC_USELESS_OBJECT"/>
Expand All @@ -268,11 +203,13 @@
see: https://github.com/Kotlin/kotlinx.coroutines/issues/3099
-->
<Match>
<!-- MongoDB status: "False Positive", SpotBugs rank: 13 -->
<Class name="com.mongodb.kotlin.client.coroutine.MongoClient"/>
<Method name="startSession"/>
<Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/>
</Match>
<Match>
<!-- MongoDB status: "False Positive", SpotBugs rank: 16 -->
<Class name="~com.mongodb.kotlin.client.coroutine.*"/>
<Bug pattern="NP_NONNULL_PARAM_VIOLATION"/>
</Match>
Expand Down