Skip to content

[Java] Remove MS_EXPOSE_REP from the global SpotBugs filter - #39951

Open
mxtymoshyk wants to merge 1 commit into
apache:masterfrom
mxtymoshyk:spotbugs-filter-ms-expose-rep
Open

[Java] Remove MS_EXPOSE_REP from the global SpotBugs filter#39951
mxtymoshyk wants to merge 1 commit into
apache:masterfrom
mxtymoshyk:spotbugs-filter-ms-expose-rep

Conversation

@mxtymoshyk

@mxtymoshyk mxtymoshyk commented Sep 1, 2026

Copy link
Copy Markdown

sdks/java/build-tools/src/main/resources/beam/spotbugs-filter.xml turns off MS_EXPOSE_REP for 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 spotbugsMain instead of passing quietly.

Addresses #35312. That issue covers two patterns. CT_CONSTRUCTOR_THROW has about 140 sites and is not touched here.

The 34 findings

Cause Sites
Relocated Guava immutable collections 3
TpcdsSchemas fixed table definitions 24
Metric handles that have to be shared to work 6
One shared empty instance 1

That is 11 @SuppressFBWarnings annotations across 9 files. TpcdsSchemas takes 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 an ImmutableSet or ImmutableMap. Wrapping the return in Collections.unmodifiableSet does 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 24 Schema.builder() chains into their getters means rebuilding fixed table definitions on every call, inside a benchmark.

Lineage, both Spark MetricsAccumulator classes, and the two SourceMetrics gauges, 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, and provider() == provider() stops being true.

This is the same split as #35394, which fixed DCN_NULLPOINTER_EXCEPTION, PA_PUBLIC_PRIMITIVE_ATTRIBUTE and DMI_RANDOM_USED_ONLY_ONCE, where the findings were real bugs, and left MS_EXPOSE_REP in the filter.

Why the Guava sites cannot be configured away

MutableClasses.mutableSignature() checks the fully qualified name against a hardcoded list. That list has com.google.common.collect.ImmutableSet but not org.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 relocated ImmutableCollection declares add, remove, addAll, removeAll, removeIf and clear, so that check fails too.

The list is static final and 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.

SourceMetrics shows plainly that the check goes by name. elementsRead() and bytesRead() return static Counter fields and pass. backlogBytes() and backlogElements() return static Gauge fields and fail. The only difference is that Gauge has set(long) and Counter has inc().

Notes for reviewers

  • edu.umd.cs.findbugs.annotations.SuppressFBWarnings is LGPL. spotbugs-annotations is already a compileOnly dependency and is not shipped, and sdks/java/core main source already imports it.
  • matchType = EXACT is not available in spotbugs-annotations 4.8.3, which has only value() and justification(). Suppression matches by prefix, so MS_EXPOSE_REP also covers MS_EXPOSE_BUF. None of these sites return an array, so nothing extra gets suppressed.
  • TpcdsSchemas is annotated on the class rather than on 24 getters, since all 24 have the same reason.
  • ExecutionStateSampler.java:55 already had @SuppressFBWarnings("MS_EXPOSE_REP") from Fix Spotbugs #35313 and is unchanged.
  • The Guava justifications are longer than the rest because they name the upstream issue, which nobody could guess from the call site. Long strings break at sentence ends only. LineLength is disabled in checkstyle.xml, and google-java-format does not split string literals, so spotlessJavaCheck is stable on them.

Verified

spotbugsMain and spotlessJavaCheck pass on :sdks:java:core, :sdks:java:io:thrift, :sdks:java:testing:tpcds, :sdks:java:testing:test-utils and :runners:spark:3.

sdks/java/io/components:spotbugsMain already fails on master for an unrelated reason, two SE_BAD_FIELD findings in EnvoyRateLimiterFactory.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

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
mxtymoshyk force-pushed the spotbugs-filter-ms-expose-rep branch from 95b4bcd to 40df57d Compare September 2, 2026 04:02
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Assigning reviewers:

R: @chamikaramj for label java.
R: @damccorm for label build.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant