Fix NPE in S3PinotFS credential refresh when initialized with a provided S3Client#18644
Open
noob-se7en wants to merge 1 commit into
Open
Fix NPE in S3PinotFS credential refresh when initialized with a provided S3Client#18644noob-se7en wants to merge 1 commit into
noob-se7en wants to merge 1 commit into
Conversation
…ded S3Client `S3PinotFS.init(S3Client, String, PinotConfiguration)` set `_s3Client` but left the `_s3Config` field null — it built an `S3Config` only as a local for the server-side-encryption / multipart / ACL setup. The single-arg `init(PinotConfiguration)` is the only path that assigned `_s3Config`. Any later operation routed through `retryWithS3CredentialRefresh(...)` -> `initOrRefreshS3Client()` reads `_s3Config.getRegion()`, so an instance created with a caller-provided client threw a NullPointerException on the first credential refresh. Assign the already-constructed `S3Config` to the `_s3Config` field (and reuse it for the SSE/multipart/ACL setup). No extra object construction; the field is now populated so a refresh rebuilds the client from the same config instead of NPEing. Adds `S3PinotFSInitTest` which initializes via the three-arg `init` and then calls `initOrRefreshS3Client()` — it reproduces the NPE before this change and passes after. Static credentials keep the rebuild fully offline (no container). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
noob-se7en
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18644 +/- ##
============================================
+ Coverage 64.27% 64.42% +0.14%
- Complexity 1137 1299 +162
============================================
Files 3314 3362 +48
Lines 204064 207907 +3843
Branches 31760 32463 +703
============================================
+ Hits 131165 133945 +2780
- Misses 62373 63180 +807
- Partials 10526 10782 +256
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xiangfu0
reviewed
Jun 1, 2026
Contributor
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal issue; see inline comment.
| // through initOrRefreshS3Client(), which reads _s3Config.getRegion(); leaving the field null | ||
| // caused a NullPointerException on the first refresh for instances initialized with a provided | ||
| // client. | ||
| _s3Config = new S3Config(serverSideEncryptionConfig); |
Contributor
There was a problem hiding this comment.
This fixes the init(S3Client, ..., PinotConfiguration) path, but the other public provided-client overload at init(S3Client) still leaves _s3Config null. If a caller uses that overload and later hits the 401/403 retry path, initOrRefreshS3Client() will still dereference _s3Config.getRegion() and fail before the retry happens. Please either initialize _s3Config there as well or explicitly guard/disable credential refresh for bare-client initialization.
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
3 participants
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.
Problem
S3PinotFS.init(S3Client, String, PinotConfiguration)(the "bring your own client" overload) sets_s3Clientbut leaves the_s3Configfieldnull. It constructs anS3Configonly as a local variable, used for the server-side-encryption / multipart / ACL setup. The single-arginit(PinotConfiguration)is currently the only method that assigns the_s3Configfield.Any later operation that goes through the credential-refresh path —
retryWithS3CredentialRefresh(...)→initOrRefreshS3Client()— reads_s3Config.getRegion(). For an instance initialized with a caller-provided client,_s3Configis stillnull, so the first credential refresh throws aNullPointerException.Fix
Assign the
S3Configthat the three-arginitalready constructs to the_s3Configfield (and reuse it for the existing SSE/multipart/ACL calls). No extra object construction. The field is now populated, so a later refresh rebuilds the client from the same config instead of NPEing.Testing
Adds
S3PinotFSInitTest.testInitWithProvidedClientPopulatesConfigForRefresh: initializes via the three-arginit, then callsinitOrRefreshS3Client(). It reproduces theNullPointerExceptionbefore this change and passes after. Static credentials keep the rebuild fully offline (no S3 mock container required).