Skip to content
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.

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

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

Merged
merged 4 commits into from
May 21, 2024

Conversation

stIncMale
Copy link
Member

@stIncMale stIncMale commented May 16, 2024

To get the rank data, I had enabled SpotBugs XML reports, then was commenting out parts of exclude.xml and manually get the rank data from the XML reports.

JAVA-5431

@stIncMale stIncMale requested a review from jyemin May 16, 2024 16:24
@stIncMale stIncMale self-assigned this May 16, 2024
@stIncMale stIncMale requested a review from rozza May 16, 2024 16:25
<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

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".

Comment on lines 145 to 147
<!-- 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

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
Contributor

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.

bson/src/main/org/bson/types/ObjectId.java Outdated Show resolved Hide resolved
config/spotbugs/exclude.xml Outdated Show resolved Hide resolved
config/spotbugs/exclude.xml Show resolved Hide resolved
config/spotbugs/exclude.xml Show resolved Hide resolved
config/spotbugs/exclude.xml Outdated Show resolved Hide resolved
@stIncMale stIncMale requested a review from jyemin May 16, 2024 22:20
Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

LGTM - not sure if you want #1395 merged first. If you are happy with the PR feel free to merge.

We discussed it, experimented with it during a catchup meeting, and established that this finding is a false positive.

JAVA-5431
@stIncMale stIncMale merged commit c161afc into mongodb:master May 21, 2024
59 checks passed
@stIncMale stIncMale deleted the JAVA-5431 branch May 21, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants