feat: support intra-bucket copy/move in GCP Storage Bucket binding#4410
Draft
nelson-parente wants to merge 4 commits into
Draft
feat: support intra-bucket copy/move in GCP Storage Bucket binding#4410nelson-parente wants to merge 4 commits into
nelson-parente wants to merge 4 commits into
Conversation
Add optional `destinationKey` to copy/move operation payloads. Make `destinationBucket` optional, defaulting to the component's configured bucket when omitted. Reject only the case where both fields are absent (that would be a no-op onto itself). Preserves GCS server-side copy semantics so content-type, metadata, and ACLs are retained without streaming bytes through the sidecar. Fixes dapr#4377 Signed-off-by: Nelson Parente <nelson@diagrid.io>
nelson-parente
force-pushed
the
feat/gcp-bucket-intra-bucket-copy-move
branch
from
e12eb88 to
c83cc6d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the GCP Storage Bucket output binding to support intra-bucket copy/move across key prefixes by allowing callers to optionally specify a destinationKey, and by making destinationBucket optional (defaulting to the component’s configured bucket).
Changes:
- Added
destinationKeytocopyandmovepayloads and centralized destination resolution/defaulting inresolveCopyDestination. - Relaxed
destinationBucketrequirement by defaulting it to the configured bucket when omitted. - Added/updated unit tests to cover the destination resolution matrix and updated error assertions for wrapped JSON unmarshal errors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| bindings/gcp/bucket/metadata.yaml | Documents the new copy/move behavior (needs wording alignment with actual validation). |
| bindings/gcp/bucket/bucket.go | Implements destination defaulting for copy/move and uses resolved bucket/key for the GCS CopierFrom operation. |
| bindings/gcp/bucket/bucket_test.go | Adds unit tests for destination resolution and adjusts copy/move validation expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ve docs (Copilot review) Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
… behavior Reword the move operation metadata.yaml description to state that the resolved destination (after defaults are applied) must not equal the source, matching the actual guard in move(). Add a comment in copy() documenting that copy-onto-self is intentionally allowed (GCS creates a new object generation), unlike move. Addresses Copilot review feedback. Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
The both-omitted validation message described the resolved source==destination case as a no-op for copy, which contradicts the copy() comment noting that copy-onto-self is a valid GCS operation (it rewrites the object as a new generation). Reword the error and the copy operation description to say it resolves to the source object (rewritten for copy, deleted for move). Addresses Copilot review feedback. Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
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
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.
What
Today the GCP Storage Bucket binding's
copyandmoveoperations requiredestinationBucketand hardcode the destination key to the source key. This makes it impossible to copy/move an object between key prefixes within the same bucket (e.g.public/file.xlsx→adhoc/file.xlsx) without a costly get+create round-trip that also loses content-type, custom metadata, and ACLs.Changes
destinationKeyfield to thecopyandmoveoperationdatapayloads. When omitted it defaults to the source key.destinationBucketoptional in both operations. When omitted it defaults to the component's configured bucket.destinationBucketanddestinationKeyare omitted — that would produce a no-op copy/move onto itself. A clear error is returned in that case.CopierFromAPI, so content-type, custom metadata, and ACLs are preserved without streaming bytes through the app/sidecar.Example — intra-bucket prefix change (new)
{ "operation": "copy", "metadata": { "key": "public/file.xlsx" }, "data": { "destinationKey": "adhoc/file.xlsx" } }{ "operation": "move", "metadata": { "key": "public/file.xlsx" }, "data": { "destinationKey": "adhoc/file.xlsx" } }Example — cross-bucket, different key (unchanged behaviour still works)
{ "operation": "copy", "metadata": { "key": "reports/q1.pdf" }, "data": { "destinationBucket": "archive-bucket", "destinationKey": "2024/q1.pdf" } }Validation matrix
destinationBucketdestinationKeyTests
Unit tests added/updated in
bucket_test.go:destinationKeyfrom payloadcopyandmoveConformance tests that require a live GCS connection are not affected and must be run separately.
Fixes #4377
📖 Documentation: dapr/docs#5218