[Java] Remove MS_EXPOSE_REP from the global SpotBugs filter - #39951
Open
mxtymoshyk wants to merge 1 commit into
Open
[Java] Remove MS_EXPOSE_REP from the global SpotBugs filter#39951mxtymoshyk wants to merge 1 commit into
mxtymoshyk wants to merge 1 commit into
Conversation
mxtymoshyk
force-pushed
the
spotbugs-filter-ms-expose-rep
branch
from
September 1, 2026 11:24
1a384bd to
95b4bcd
Compare
Contributor
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
The global filter turns off MS_EXPOSE_REP for every module. That hides 34 findings today, and hides any new one automatically. Remove that line and mark the existing sites one by one with @SuppressFBWarnings, each carrying the reason it is exempt. Nothing changes at runtime. What changes is that the next accessor someone writes that returns a mutable static will fail spotbugsMain instead of passing quietly. The three Guava sites cannot be configured away. SpotBugs checks its list of known immutable types by fully qualified name, so it does not recognise Guava relocated into org.apache.beam.vendor.guava, and the fallback scan finds setter-named methods on ImmutableCollection. See spotbugs/spotbugs#1601. Addresses apache#35312.
mxtymoshyk
force-pushed
the
spotbugs-filter-ms-expose-rep
branch
from
September 2, 2026 04:02
95b4bcd to
40df57d
Compare
Contributor
|
Assigning reviewers: R: @chamikaramj for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sdks/java/build-tools/src/main/resources/beam/spotbugs-filter.xmlturns offMS_EXPOSE_REPfor every module. That hides 34 findings today, and hides any new one automatically. This removes that line and marks the existing sites one by one.Nothing changes at runtime and no finding is fixed. What changes is that the next accessor someone writes that returns a mutable static will fail
spotbugsMaininstead of passing quietly.Addresses #35312. That issue covers two patterns.
CT_CONSTRUCTOR_THROWhas about 140 sites and is not touched here.The 34 findings
TpcdsSchemasfixed table definitionsThat is 11
@SuppressFBWarningsannotations across 9 files.TpcdsSchemastakes one annotation on the class for all 24 of its findings.Why these are suppressed instead of fixed
I tried fixing them first. In each case the fix makes the code worse.
Relocated Guava, 3 sites:
ModelCoders.urns(),PTransformTranslation.getKnownPayloadTranslators(),NamedTestResult.getSchema(). These already return anImmutableSetorImmutableMap. Wrapping the return inCollections.unmodifiableSetdoes clear the finding, I checked, but it puts a second immutable wrapper around a collection that is already immutable, and the next reader has to work out why it is there.TpcdsSchemas, 24 sites. Moving the 24Schema.builder()chains into their getters means rebuilding fixed table definitions on every call, inside a benchmark.Lineage, both SparkMetricsAccumulatorclasses, and the twoSourceMetricsgauges, 6 sites. Returning the shared instance is the whole point. A copy would collect metrics that nothing reports.ThriftSchema.provider(), 1 site. It returns a single shared instance holding an empty map. Removing it means allocating on every call, andprovider() == provider()stops being true.This is the same split as #35394, which fixed
DCN_NULLPOINTER_EXCEPTION,PA_PUBLIC_PRIMITIVE_ATTRIBUTEandDMI_RANDOM_USED_ONLY_ONCE, where the findings were real bugs, and leftMS_EXPOSE_REPin the filter.Why the Guava sites cannot be configured away
MutableClasses.mutableSignature()checks the fully qualified name against a hardcoded list. That list hascom.google.common.collect.ImmutableSetbut notorg.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableSet. When that check misses, it scans the class for public methods with setter-like names and walks up the superclass chain. The relocatedImmutableCollectiondeclaresadd,remove,addAll,removeAll,removeIfandclear, so that check fails too.The list is
static finaland 4.8.3 reads no property that would extend it. The only hook is a class annotation whose descriptor ends in/Immutable;, which would have to be added to Guava's own source.Upstream, spotbugs/spotbugs#1601 is open with 44 comments. The pull request that would make the list configurable, spotbugs/spotbugs#1619, has been a draft since 2021 and a maintainer objected to it. Filing a new issue would just duplicate #1601.
SourceMetricsshows plainly that the check goes by name.elementsRead()andbytesRead()return staticCounterfields and pass.backlogBytes()andbacklogElements()return staticGaugefields and fail. The only difference is thatGaugehasset(long)andCounterhasinc().Notes for reviewers
edu.umd.cs.findbugs.annotations.SuppressFBWarningsis LGPL.spotbugs-annotationsis already acompileOnlydependency and is not shipped, andsdks/java/coremain source already imports it.matchType = EXACTis not available in spotbugs-annotations 4.8.3, which has onlyvalue()andjustification(). Suppression matches by prefix, soMS_EXPOSE_REPalso coversMS_EXPOSE_BUF. None of these sites return an array, so nothing extra gets suppressed.TpcdsSchemasis annotated on the class rather than on 24 getters, since all 24 have the same reason.ExecutionStateSampler.java:55already had@SuppressFBWarnings("MS_EXPOSE_REP")from Fix Spotbugs #35313 and is unchanged.LineLengthis disabled incheckstyle.xml, and google-java-format does not split string literals, sospotlessJavaCheckis stable on them.Verified
spotbugsMainandspotlessJavaCheckpass on:sdks:java:core,:sdks:java:io:thrift,:sdks:java:testing:tpcds,:sdks:java:testing:test-utilsand:runners:spark:3.sdks/java/io/components:spotbugsMainalready fails on master for an unrelated reason, twoSE_BAD_FIELDfindings inEnvoyRateLimiterFactory.