blob: use toBuilder for correlationId stamping; fix async BlobMetadata.checksum drop - #583
Conversation
BucketClient rebuilt UploadResponse/DownloadResponse/BlobMetadata/CopyResponse and UploadRequest/MultipartUploadRequest field-by-field to stamp the resolved correlationId. Any newly added field on these classes would be silently dropped in the rebuild until every callsite was updated. Add toBuilder support to the six model classes (@builder(toBuilder = true) for the Lombok-built ones; hand-written toBuilder() for the two request classes whose builders carry defaulting logic) and collapse each rebuild in BucketClient into a one-line toBuilder().<field>(...).build() call. Adding a new field to any of these classes now flows through automatically. Add a regression test that populates every field of BlobMetadata to a distinct non-default value and verifies each survives the rebuild, with only the correlationId overwritten by the resolved OperationContext.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #583 +/- ##
============================================
- Coverage 83.61% 83.58% -0.03%
Complexity 674 674
============================================
Files 215 215
Lines 15010 14930 -80
Branches 2076 2076
============================================
- Hits 12550 12479 -71
+ Misses 1636 1628 -8
+ Partials 824 823 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The Builder defaults metadata/tags to Collections.emptyMap() and the withMetadata/withTags setters throw NPE on null via unmodifiableMap, so the fields can never be null when the request is constructed through the builder (the only construction path). The `metadata != null ? metadata : Map.of()` guard in toBuilder() was dead defensive code — remove it so a broken invariant would fail loudly instead of being silently papered over.
| } | ||
| return DownloadResponse.builder() | ||
| .key(r.getKey()) | ||
| return r.toBuilder() |
There was a problem hiding this comment.
Nice catch keeping .metadata(withCorrelationId(r.getMetadata(), ctx)) explicit here rather than relying on toBuilder() to shallow-copy the nested BlobMetadata — a plain r.toBuilder().correlationId(...).build() would've silently kept the driver's original (unstamped) metadata correlationId, which is exactly the class of bug this PR is trying to kill. Worth a one-line comment on this call noting the nested rebuild is intentional, not leftover ceremony — future readers might "simplify" it away.
There was a problem hiding this comment.
Added an inline comment in third commit flagging the nested rebuild as intentional, so a future reader doesn't collapse it into a plain toBuilder().correlationId(...).
| assertContextPropagated(captured); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The new testGetMetadataPreservesAllFieldsWhenStampingCorrelationId is a genuinely good regression test — it would have caught the original silent-drop bug. But it only covers BlobMetadata. UploadResponse went through the identical conversion and has no equivalent test: getTestUploadResponse() only builds key/versionId/eTag, leaving checksumValue unset, so a dropped checksumValue in the toBuilder() rebuild wouldn't fail today. DownloadResponse has no field-survival coverage at all — the download tests only assert the request was forwarded correctly, never construct or compare a DownloadResponse. (CopyResponse is fine — testCopy already sets all four non-correlationId fields.) Given the whole point of this PR is closing that drift hazard, should UploadResponse and DownloadResponse get the same field-completeness test, or is BlobMetadata considered the representative case?
There was a problem hiding this comment.
Added testUploadPreservesAllFieldsWhenStampingCorrelationId and testDownloadPreservesAllFieldsWhenStampingCorrelationId mirroring the BlobMetadata one.
| when(mockBlobStore.upload(any(), any(File.class))).thenThrow(RuntimeException.class); | ||
| when(mockBlobStore.upload(any(), any(Path.class))).thenThrow(RuntimeException.class); | ||
| UploadRequest request = mock(UploadRequest.class); | ||
| UploadRequest request = UploadRequest.builder().withKey("object-1").build(); |
There was a problem hiding this comment.
Good fix swapping mock(UploadRequest.class) for a real UploadRequest — the old mock would've returned null from toBuilder() and NPE'd before ever reaching the exception-mapping path this test is actually checking. Confirms the toBuilder migration needed to hunt down every mocked request/response of these six types across the test suite; did a pass over AsyncBucketClientTest.java turn up the same landmine (it also has mock(UploadRequest.class) at line 257)? [Thinking out loud] since the async client isn't being touched in this PR, its test file's mocks won't NPE today — but the moment the async migration happens, that test needs the identical fix.
There was a problem hiding this comment.
Swapped mock(UploadRequest.class) for a real UploadRequest preemptively
iamabhilaksh
left a comment
iamabhilaksh
left a comment
There was a problem hiding this comment.
Clean refactor — the toBuilder() swap kills the field-drift hazard on the sync path, and the explicit nested-metadata rebuild is the right call. Left a few inline questions on test coverage completeness (UploadResponse/DownloadResponse don't have the field-survival test that BlobMetadata now has).
One scope question I couldn't anchor inline: AsyncBucketClient still has the exact same six withCorrelationId/withResolvedContext methods doing the field-by-field rebuild this PR calls a drift hazard. Since BlobMetadata, UploadResponse, DownloadResponse, and CopyResponse all get @Builder(toBuilder = true) here, the async client could take the identical one-line fix with zero additional model changes. Was sync-only deliberate, or is the async migration a planned follow-up? If it's not planned, we're back to the same drift hazard, just in the other client.
- BucketClient.java: add a comment on withCorrelationId(DownloadResponse) noting the nested BlobMetadata rebuild is intentional; a plain r.toBuilder().correlationId(...) would shallow-copy the driver's unstamped metadata and defeat the purpose of the rebuild. - BucketClientTest.java: add field-survival tests for UploadResponse and DownloadResponse that mirror the existing BlobMetadata test, covering the same drift-hazard the PR closes for all four response types stamped through toBuilder() (CopyResponse is already covered by testCopy). - AsyncBucketClientTest.java: replace mock(UploadRequest.class) in testUploadThrowsFutureException with a real UploadRequest — it doesn't NPE today because the async client isn't in this PR, but it will the moment the async client migrates, so fixing it preemptively.
Mirror the sync-client refactor on AsyncBucketClient: replace the six field-by-field rebuilds in withCorrelationId(UploadResponse/DownloadResponse/ BlobMetadata/CopyResponse) and withResolvedContext(UploadRequest/ MultipartUploadRequest) with toBuilder() copies. Closes the same drift hazard in the async path — the BlobMetadata rebuild had already silently drifted (the checksum field was never being copied through). Add field-survival tests mirroring the sync suite (getMetadata, upload, download) that populate every field to a distinct value and assert every field survives the rebuild so any future field addition is caught by a failing test. Swap the four DownloadResponse-mock success-path tests to real builder-produced responses; toBuilder() on a mock returns null, which previously masked the missing checksum field and would otherwise NPE under the new rebuild path.
|
@iamabhilaksh Good catch — sync-only wasn't deliberate, just missed it in the initial cut. Just pushed the async migration in the follow-up commit: same six methods collapsed to toBuilder(), plus mirror field-survival tests for getMetadata/upload/download in AsyncBucketClientTest. While in there, the drift hazard turned out to already be a latent bug — withCorrelationId(BlobMetadata) on the async path was only copying 10 of the 11 fields and silently dropping checksum. The new field-survival test would have failed against the pre-refactor code; it passes after the toBuilder() swap. So this PR now closes the drift class on both sides. |
iamabhilaksh
left a comment
iamabhilaksh
left a comment
There was a problem hiding this comment.
LGTM 👍 — thanks for extending it to AsyncBucketClient and adding the field-survival tests; that checksum drift the async path had is exactly the failure mode this closes. Nicely done.
| * {@link UploadRequest} requires only extending this method (and the corresponding builder | ||
| * setter) — not every callsite that copies a request. | ||
| */ | ||
| public Builder toBuilder() { |
There was a problem hiding this comment.
UploadRequest.toBuilder() and MultipartUploadRequest.toBuilder() are the only two copy helpers in this PR that can drift — Lombok generates the other four, so those are structurally safe. But only the multipart one is actually guarded by a test.
I checked this by mutation rather than by reading: deleting one field-copy line at a time from this method and re-running mvn test -pl blob/blob-client. 10 of the 11 copied fields can be deleted with the suite still 100% green. Only .withKey(key) is caught:
CAUGHT | .withKey(key) -> 8 failures
SURVIVED | .withContentLength(contentLength) -> 316 pass, 0 failures
SURVIVED | .withMetadata(metadata) -> 316 pass, 0 failures
SURVIVED | .withTags(tags) -> 316 pass, 0 failures
SURVIVED | .withStorageClass(storageClass) -> 316 pass, 0 failures
SURVIVED | .withKmsKeyId(kmsKeyId) -> 316 pass, 0 failures
SURVIVED | .withUseKmsManagedKey(...) -> 316 pass, 0 failures
SURVIVED | .withObjectLock(objectLock) -> 316 pass, 0 failures
SURVIVED | .withChecksumValue(checksumValue) -> 316 pass, 0 failures
SURVIVED | .withChecksumAlgorithm(...) -> 316 pass, 0 failures
SURVIVED | .withContentType(contentType) -> 316 pass, 0 failures
The withKey result is worth noting on its own: it proves this branch really is executed by testUpload{InputStream,ByteArray,File,Path}. So the method has line coverage but no assertion coverage for 10 of its fields.
Some of those are more than cosmetic. Silently dropping kmsKeyId or useKmsManagedKey writes an object without the intended CMK; dropping objectLock writes it without WORM retention or legal hold; dropping checksumValue/checksumAlgorithm skips upload integrity validation. Those are encryption and compliance settings failing open with nothing in CI going red.
The path is reachable in normal use: MultiCloudJLogger.resolveContext() returns a new OperationContext when the caller supplies none, and withResolvedContext short-circuits on reference equality (ctx == req.getOperationContext()), so for the common case of a caller omitting OperationContext this rebuild runs on every upload.
The fix is to mirror what already exists for the sibling class: testWithResolvedContextMultipartRebuildPreservesAllFields (AsyncBucketClientTest) calls withResolvedContext directly with a deliberately distinct context to force the rebuild branch, then asserts every field. withResolvedContext is package-private and the test classes share the package, so a UploadRequest equivalent is straightforward. Under the same sweep, that shape catches all 10 currently-unguarded drops.
One small thing while you're in there: the multipart test guards 8 of its 9 fields, but .withChecksumEnabled(checksumEnabled) also survives mutation. The fixture sets both withChecksumEnabled(true) and withChecksumAlgorithm(SHA256), and since the constructor derives checksumEnabled = builder.checksumEnabled || builder.checksumAlgorithm != null, the assertTrue(rebuilt.isChecksumEnabled()) passes via the algorithm regardless of whether the flag was copied. A case with checksumEnabled = true and no explicit algorithm — a valid configuration, meaning "checksum on, substrate-native default" — would close it.
There was a problem hiding this comment.
Added three tests in most recent commit populating every field with distinct non-defaults and asserting each survives the rebuild:
- AsyncBucketClientTest.testWithResolvedContextUploadRebuildPreservesAllFields (async side already had the MultipartUploadRequest variant)
- BucketClientTest.testWithResolvedContextUploadRebuildPreservesAllFields
- BucketClientTest.testWithResolvedContextMultipartRebuildPreservesAllFields
|
|
||
| /** Blob metadata data object */ | ||
| @Builder | ||
| @Builder(toBuilder = true) |
There was a problem hiding this comment.
metadata is @Singular("metadata"), which gives the generated toBuilder() a behavior most callers will not expect: Lombok's plural setter adds entries rather than replacing them.
Verified against the compiled class:
BlobMetadata m = ...; // metadata = {a=1, b=2}
m.toBuilder().metadata(Map.of("c", "3")).build().getMetadata();
// => {a=1, b=2, c=3} and not {c=3}This PR's own code path is unaffected, since it only overrides correlationId. The concern is that toBuilder() is now public API on a published SDK, and "copy this metadata but swap out the user-metadata map" is the natural next use — which will silently union the two maps instead of replacing. clearMetadata() is the escape hatch, but nothing signals that a caller needs it.
Worth a javadoc line on the class (or the field) noting that the plural metadata(Map) setter accumulates, and that replacing requires clearMetadata() first.
There was a problem hiding this comment.
Added a javadoc note on the field documenting that .metadata(Map) on a toBuilder() copy unions with the existing entries rather than replacing them, and how to replace (clearMetadata() first) or append (.metadata(key, value)).
| .objectLockInfo(m.getObjectLockInfo()) | ||
| .correlationId(ctx.getCorrelationId()) | ||
| .build(); | ||
| return m.toBuilder().correlationId(ctx.getCorrelationId()).build(); |
There was a problem hiding this comment.
This line is a user-visible bug fix rather than a pure refactor, and it is currently undisclosed.
git log -S '.checksum(m.getChecksum())' against BucketClient shows #540 added BlobMetadata.checksum and updated the field-by-field rebuild there. The same search against AsyncBucketClient returns nothing — the async rebuild never received it. So since #540, AsyncBucketClient.getMetadata() and download() have returned checksum == null unconditionally on every provider, even though the transformers populate it (AwsTransformer.toMetadata / toDownloadResponse and the GCP/Ali equivalents). Collapsing to toBuilder() fixes that, and the new assertEquals(fromDriver.getChecksum(), actual.getChecksum()) assertion would fail against main — which is exactly the right guard.
The issue is disclosure. The PR title says "refactor BucketClient correlationId stamping", and the description mentions neither the async client nor the checksum. Since this repo squash-merges and release-please maps the blobstore: type into the changelog, a behavior fix would ship described as a refactor, leaving consumers who suddenly see a populated checksum nothing to correlate it against.
Suggest retitling, or adding a short "Behavior change" line naming AsyncBucketClient.getMetadata() / download() and referencing #540 as the regression source.
Two smaller description drifts worth fixing in the same pass: it says six methods in BucketClient (now twelve across two files), and "311 tests pass in blob-client" (actual is 316, up from 310 on base). A release note would also help for the fact that Mockito mocks of UploadRequest / DownloadResponse no longer work through these clients — toBuilder() returns null on a mock, which is what forced the mock-to-real swaps here, and downstream test suites will hit the same NPE on upgrade.
There was a problem hiding this comment.
Updated title and description to name the AsyncBucketClient checksum fix as a behavior change (references #540), correct the scope to both clients, and add a Mockito heads-up for downstream tests.
|
|
||
| /** Wrapper object for copy result data */ | ||
| @Builder | ||
| @Builder(toBuilder = true) |
There was a problem hiding this comment.
Now that CopyResponse has toBuilder(), there is one remaining field-by-field CopyResponse copy in the repo that this PR's rationale covers verbatim. AliBlobStore.buildCopyResponse (blob/blob-ali/src/main/java/com/salesforce/multicloudj/blob/ali/AliBlobStore.java:581-586) rebuilds a real CopyResponse — the one returned by transformer.toCopyResponse(...) — purely to override lastModified:
return CopyResponse.builder()
.key(response.getKey())
.versionId(response.getVersionId())
.eTag(response.getETag())
.lastModified(transformer.parseLastModified(headResult.lastModified()))
.build();That is now expressible as response.toBuilder().lastModified(...).build(), and any future CopyResponse field would flow through automatically instead of needing to be remembered here.
Not blocking, and it sits outside this diff — but it is the same drift hazard, and the tool to remove it lands in this PR.
There was a problem hiding this comment.
Collapsed to response.toBuilder().lastModified(...).build(). All 272 blob-ali tests pass.
graceli5428
changed the title
- Add rebuild-branch tests for withResolvedContext on UploadRequest and MultipartUploadRequest to close the mutation-testing gap that only caught .withKey(key) — the new tests populate every field with distinct non-defaults and assert each survives the rebuild - Document the @Singular("metadata") accumulation footgun on BlobMetadata.metadata — .metadata(newMap) on a toBuilder() copy unions entries with the existing map rather than replacing them - Collapse AliBlobStore.buildCopyResponse field-by-field rebuild to use response.toBuilder().lastModified(...).build()
iamabhilaksh
left a comment
iamabhilaksh
left a comment
There was a problem hiding this comment.
LGTM 👍 — re-approving on the current head; the follow-up commits addressed the second round cleanly (toBuilder now covers CopyResponse, and the field-preservation tests close the mutation-coverage gap).
LihaoLiuXs
left a comment
LihaoLiuXs
left a comment
There was a problem hiding this comment.
In the provider implementation why we only updated the AliBlobStore.java? Do we need to apply the same change in aws and gcp?
|
The AWS and GCP paths don't have the same pattern. The Ali change was for a specific two-stage rebuild: AWS and GCP just build the |
| private final String eTag; | ||
| private final long objectSize; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Lets remove this comments
4 participants
Summary
Refactor
BucketClientandAsyncBucketClientcorrelationId-stamping helpers so they no longer rebuild response/request objects field-by-field. AddstoBuilder()support to the six affected model classes and collapses each rebuild into a one-line copy-with-override (twelve rebuilds across the two clients).Motivation
Both clients copy the resolved
OperationContext.correlationIdonto every driver-returned response and onto request objects before they reach the driver. Because these model classes are immutable, the previous implementation rebuilt each object field-by-field. Any new field added to these classes was silently dropped by the copy until every callsite was updated — six such methods in each client, twelve independent drift hazards.Behavior change
This PR fixes a silent regression from #540 , which added
BlobMetadata.checksumand updated the syncBucketClientrebuild but not the async one. Since #540 merged, on every provider (AWS / GCP / Ali):AsyncBucketClient.getMetadata(...)returnedchecksum == nullAsyncBucketClient.download(...)returnedgetMetadata().getChecksum() == nulleven when the transformer had populated the field. The
toBuilder()collapse closes this gap; async callers will now see the real checksum value. Callers that assumedchecksum == nullon the async path should be reviewed.Changes
toBuilder()toBlobMetadata,DownloadResponse,UploadResponse,CopyResponse,UploadRequest,MultipartUploadRequest. The four response classes use Lombok@Builder(toBuilder = true); the two request classes have a hand-writtentoBuilder()since they already carry custom builder defaulting.BucketClient, six inAsyncBucketClient) into one-linetoBuilder().(...).build()calls. New fields on these classes now flow through both correlationId paths automatically.Downstream note: Mockito
The request-side helpers now call
.toBuilder()on their argument. Mockito mocks returnnullfrom object-returning methods by default, so downstream tests that domock(UploadRequest.class),mock(MultipartUploadRequest.class), ormock(DownloadResponse.class)and hand them to these clients will NPE after upgrade. Fix in test code: swap for real builders (UploadRequest.builder().key(...).build(), etc.). Five such sites inside this repo were updated in this PR.Testing
testGetMetadataPreservesAllFieldsWhenStampingCorrelationIdpopulates everyBlobMetadatafield (includingchecksum) with a distinct non-default value, has the driver return it, and asserts each field survives the rebuild while onlycorrelationIdis overwritten. Fails againstmain— this is the guard against the regression class blobstore: add the checksum data in the get and metadata #540 introduced. Companion tests coverUploadResponseandDownloadResponsefield survival.