-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
<Match> | ||
<Bug code="EI,EI2"/> | ||
<!-- MongoDB status: "No Fix Needed", SpotBugs rank: 18 --> | ||
<Bug pattern="EI_EXPOSE_REP,EI_EXPOSE_REP2"/> |
There was a problem hiding this comment.
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"
.
JAVA-5431
config/spotbugs/exclude.xml
Outdated
<!-- MongoDB status: "TODO Ross", SpotBugs rank: 11 --> | ||
<Class name="org.bson.codecs.kotlin.DataClassCodec$Companion"/> | ||
<Method name="getCodec"/> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
JAVA-5431
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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
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