fix(gcs): map cacheControl and customTime on the gRPC object path - #184
Conversation
UpdateObject rejected the mask paths. Both are now mapped like their siblings. A live GCS probe also set the rules: an unset custom_time or null customTime keeps the value, a decrease is rejected, and only contentEncoding is honoured as an upload query parameter.
|
| Filename | Overview |
|---|---|
| src/main/java/io/floci/gcp/services/gcs/GcsCustomTime.java | Centralizes custom-time normalization, protobuf conversion, bounds handling, and monotonicity validation. |
| src/main/java/io/floci/gcp/services/gcs/GcsGrpcMapper.java | Maps cache control and custom time between stored metadata and gRPC objects and update masks. |
| src/main/java/io/floci/gcp/services/gcs/GcsService.java | Applies normalized custom-time patch semantics and rejects decreases. |
| src/main/java/io/floci/gcp/services/gcs/GcsUploadController.java | Limits upload query metadata to content encoding and validates custom time from JSON metadata. |
| src/test/java/io/floci/gcp/services/gcs/GcsGrpcControllerTest.java | Covers gRPC metadata round-tripping, clearing behavior, monotonicity, and malformed timestamp handling. |
| src/test/java/io/floci/gcp/services/gcs/GcsSystemMetadataRestIntegrationTest.java | Covers REST upload and patch behavior against the documented GCS semantics. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
REST[REST upload or patch] --> CT[GcsCustomTime validation]
GRPC[gRPC write or update] --> CT
CT --> META[GcsObjectMeta]
META --> RESTREAD[REST object response]
META --> MAPPER[GcsGrpcMapper]
MAPPER --> GRPCREAD[gRPC GetObject or ListObjects]
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
One thing, and it is not about your code: #180 merged since I looked at this and added to Blocking: a rebase, whenever you get a moment. That is our merge ordering, not anything you did. Follow-up: worth recording, since it makes your case stronger than the PR text does — the upload-URL narrowing is in the spec, not only probe-confirmed. Greptile's legacy-timestamp point: your refutation holds, and I checked it rather than taking it. Thank you for chasing the live behaviour rather than guessing — the cross-protocol test is exactly the shape these gaps needed. |
…ntrol-custom-time # Conflicts: # src/test/java/io/floci/gcp/services/gcs/GcsGrpcControllerTest.java
hectorvent
left a comment
hectorvent
left a comment
There was a problem hiding this comment.
Approving. The conflict from #180 is resolved and all 12 checks are green, including the seven SDK compat suites and the native image.
Read through the logic rather than just the tests. A few things I checked and liked:
patchObjectLockednormalises and validatescustomTimebefore any field is mutated, so a rejected decrease leaves the object untouched rather than half-patched.- The
"*"mask path addscustom_timeto the selected set but still gates onhasCustomTime(), so a star update with the field unset keeps the existing value instead of clearing it.objectUpdateWithAnUnsetCustomTimeKeepsTheValuepins both shapes. - The int64-nanosecond saturation in
GcsCustomTime.fromProtois odd-looking on its own, but it is exactly what GCS does, and the 1677/2262 boundary values inmalformedCustomTimeIsRejectedOrSaturatedLikeGcsmake that explicit rather than incidental. Thanks for pinning the error strings too — those are the part that silently drifts. - Narrowing
systemMetadataFromQuerytocontentEncodingis a behaviour removal, but it matches GCS and the docs change says so plainly.
Two non-blocking notes, neither worth holding the merge for:
-
GcsCustomTime.requireNotDecreasedcallsInstant.parse(previous)on the stored value. Everything written from now on is normalised, but an object persisted by an older build could hold acustomTimein offset form (...+09:00), whichInstant.parserejects — that would surface as an uncaughtDateTimeParseException(500) instead of a clean 400 on the next patch. Only reachable from pre-existing state in a persisted/app/data, so it is an edge, but atry/catcharound thepreviousparse that treats an unparsable stored value as "no previous" would close it. -
patch.get("customTime") instanceof Stringmeans a non-stringcustomTimein a JSON patch (say a number) is now silently ignored, where the old cast would have thrown. GCS answers 400 there. Worth a follow-up if you want the fidelity.
Neither needs to block this — happy to file them if you would rather not fold them in.
|
Thank you for the rebase, that is sorted. I checked the merge rather than just the mergeable flag: #180's additions to Nothing further from my side. The mask handling and the upload-URL narrowing both still read correct against the reference, so this is good to go whenever it gets picked up. |
hectorvent
left a comment
hectorvent
left a comment
There was a problem hiding this comment.
Rebase verified: #180's additions to GcsGrpcControllerTest survived the merge. Code findings all resolve clean. Ready to merge.
2 participants
Summary
UpdateObject rejected the mask paths. Both are now mapped like their siblings.
A live GCS probe also set the rules: an unset custom_time or null customTime keeps the value, a decrease is rejected, and only contentEncoding is honoured as an upload query parameter.
Type of change
fix:)feat:)feat!:orfix!:)GCP Compatibility
GetObject/ListObjectsreturned an emptycache_controland nocustom_timefor objects written over REST, andUpdateObjectrejected both mask paths. They now maps both fields.custom_timeunder the mask, or a JSONcustomTime: null, cleared the field. They are now kept.customTimewas accepted. It is now rejected with INVALID_ARGUMENT / 400; the message is pinned in tests.cache_controlunder the mask stored"". It is now unset.customTimewas stored as sent. It is now rendered in UTC and rejected with 400 if unparsable values.contentEncodingis honoured now.Checklist
./mvnw testpasses locally