mxtymoshyk opened a new pull request, #39951:
URL: https://github.com/apache/beam/pull/39951
`sdks/java/build-tools/src/main/resources/beam/spotbugs-filter.xml`
suppresses `MS_EXPOSE_REP` across every module. That hides 34 findings today
and silently absorbs every new one. This removes the filter line and annotates
the existing sites individually.
No runtime behaviour changes and no finding is "fixed" -- this is a ratchet.
After it, a newly added accessor that returns a mutable static fails
`spotbugsMain` and has to be argued for in review rather than inheriting a
blanket exemption.
Addresses #35312. That issue covers two patterns; `CT_CONSTRUCTOR_THROW` has
~140 unique sites and is untouched here.
### The 34 findings
| Cause | Sites |
|---|---|
| Relocated Guava immutable collections | 3 |
| `TpcdsSchemas` fixed table definitions | 24 |
| Metric handles that must be shared to work | 6 |
| Stateless flyweight | 1 |
11 `@SuppressFBWarnings` annotations across 9 files; `TpcdsSchemas` takes
one class-level annotation for all 24.
### Why suppression rather than code changes
I tried the code fixes first. Each one makes the code worse:
**Relocated Guava** (`ModelCoders.urns()`,
`PTransformTranslation.getKnownPayloadTranslators()`,
`NamedTestResult.getSchema()`) -- these already return
`ImmutableSet`/`ImmutableMap`. `Collections.unmodifiableSet(...)` does clear
the finding, verified, but it wraps something already immutable and leaves a
reader wondering what the second wrapper is defending against.
**`TpcdsSchemas`** -- inlining 24 `Schema.builder()` chains into their
getters rebuilds fixed table definitions on every call in a benchmark harness.
**`Lineage`, Spark `MetricsAccumulator` x2, `SourceMetrics` gauges** --
returning the shared instance *is* the contract. A defensive copy collects
metrics that nothing reports.
**`ThriftSchema.provider()`** -- dropping the flyweight allocates per call
and changes `provider() == provider()` from true to false.
This matches #35394, which fixed `DCN_NULLPOINTER_EXCEPTION`,
`PA_PUBLIC_PRIMITIVE_ATTRIBUTE` and `DMI_RANDOM_USED_ONLY_ONCE` -- patterns
where the finding is a genuine defect -- and left `MS_EXPOSE_REP` in the filter.
### Why the Guava sites cannot be resolved by configuration
`MutableClasses.mutableSignature()` decides immutability by fully-qualified
name against a hardcoded list that includes
`com.google.common.collect.ImmutableSet`, but not
`org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableSet`.
It then falls through to scanning for setter-named public methods, recursing
up the superclass chain, and vendored `ImmutableCollection` declares `add`,
`remove`, `addAll`, `removeAll`, `removeIf` and `clear`. Both checks fail.
The lists are `static final` with no property override in 4.8.3. The one
extension point is a class annotation whose descriptor ends in `/Immutable;`,
which would have to sit on Guava's own classes.
Upstream: spotbugs/spotbugs#1601 (44 comments, open), and the PR adding
configurability, spotbugs/spotbugs#1619, has been an unmerged draft since 2021
and was opposed on principle by a maintainer. Filing a new issue would
duplicate #1601.
The clearest evidence the detector is purely name-based is in
`SourceMetrics`: `elementsRead()` and `bytesRead()` return static `Counter`
fields and pass; `backlogBytes()` and `backlogElements()` return static `Gauge`
fields and fail. The only difference is that `Gauge` declares `set(long)` and
`Counter` declares `inc()`.
### Notes for reviewers
- **`edu.umd.cs.findbugs.annotations.SuppressFBWarnings` is LGPL**, but
`spotbugs-annotations` is already a `compileOnly` dependency and is not
shipped. `sdks/java/core` main source already uses it.
- **`matchType = EXACT` does not exist** in spotbugs-annotations 4.8.3 --
only `value()` and `justification()`. Suppression is prefix-matched, so
`MS_EXPOSE_REP` also covers `MS_EXPOSE_BUF`. No site here returns an array, so
nothing is over-suppressed.
- **`TpcdsSchemas` is annotated at class level** rather than on 24 getters.
All 24 share one reason and the per-method version would be 24 copies of the
same paragraph.
- **`ExecutionStateSampler.java:55`** already carried
`@SuppressFBWarnings("MS_EXPOSE_REP")` from #35313 and is left alone.
- **Justifications are terse except the Guava ones**, which name the
upstream issue because the cause is not guessable from the call site. Long ones
break only at sentence boundaries; `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`, `:runners:spark:3`.
`sdks/java/io/components:spotbugsMain` fails on master independently (2
`SE_BAD_FIELD` in `EnvoyRateLimiterFactory`) and is unrelated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
