[Java] Remove CT_CONSTRUCTOR_THROW from the global SpotBugs filter - #40008
Open
mxtymoshyk wants to merge 1 commit into
Open
[Java] Remove CT_CONSTRUCTOR_THROW from the global SpotBugs filter#40008mxtymoshyk wants to merge 1 commit into
mxtymoshyk wants to merge 1 commit into
Conversation
The global filter suppressed CT_CONSTRUCTOR_THROW repo-wide, so a constructor
that throws after publishing `this` was never flagged in any module. This
removes the blanket suppression and resolves every resulting finding.
Two remediations were used:
- Marked the class `final` where it is not public, so no code outside the
repo can subclass it in a supported way. SpotBugs' detector short-circuits
on final classes, so the finding genuinely goes away: a finalizer attack
needs an attacker-supplied subclass, which a final class cannot have.
- Added a class-level @SuppressFBWarnings with a justification everywhere
`final` would be a source-incompatible change: every public class, every
abstract class, and anything already subclassed inside Beam.
A public class annotated @internal still gets the suppression rather than
`final`. @internal disclaims the compatibility guarantee, but the class is
reachable, so a forked runner may already extend it and sealing it would break
that fork at compile time.
The per-class suppressions are not equivalent to the global filter: a new
throwing constructor added anywhere now fails spotbugsMain instead of being
silently ignored.
github-actions
Bot
added
java
build
examples
io
extensions
runners
spark
sql
direct
elasticsearch
gcp
jdbc
kafka
kudu
parquet
rabbitmq
xml
core
flink
fn-execution
jet
sorter
delta
labels
Contributor
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
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
1 participant
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.
Part of #35312.
The global SpotBugs filter suppressed
CT_CONSTRUCTOR_THROWin every module, so a constructor that throws after publishingthiswas never reported anywhere. This deletes those three filter lines and fixes everything the detector then found.I used two fixes. Where the class is not public, I marked it
final. SpotBugs'ConstructorThrowdetector returns early on a final class, so the warning goes away because the problem is gone: a finalizer attack needs an attacker-supplied subclass, and a final class cannot have one. Every other class gets a@SuppressFBWarningswhose justification says why it has to stay extensible.A class was made
finalonly when it is neither abstract nor public. Code outside the repo can subclass a package-private class only by declaring itself inside a Beam package, which is unsupported and breaks under shaded jars, so sealing it cannot break a downstream user.Public classes annotated
@Internalget the suppression as well.@Internalwithdraws the compatibility guarantee, but the class is still reachable, so a forked runner may already extend it and sealing it would break that fork at compile time. Six classes fall in this category:BoundedTrieData,ExplicitShardedFile,PCollectionViews.SingletonViewFn,PCollectionViews.SingletonViewFn2,FnApiDoFnRunner, andSplittableSplitAndSizeRestrictionsDoFnRunner.I also checked that nothing in the repo extends any class I sealed. That check only proves the tree still compiles. It is not by itself a reason to seal a class, and it says nothing about out-of-tree code.
Verified locally with
./gradlew spotbugsMain spotlessJavaCheck --continue: 140 modules passspotbugsMain, 190 passspotlessJavaCheck, and no report contains aCT_CONSTRUCTOR_THROWfinding.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.