feat(simplestorage)!: achieve feature parity with TypeScript SDK#26
Conversation
Add missing object operations and extended options to match @tigrisdata/storage, plus a few correctness fixes on top. Object operations: - Head: metadata-only retrieval with a presigned URL - GetPresignedUrl: GET/PUT URL generation with expiry + content type - Put options: WithAccessType, WithRandomSuffix, WithAllowOverwrite, WithContentDisposition, WithMultipartUpload, WithUploadProgress - Get options: WithResponseContentType, WithResponseContentDisposition, WithResponseCacheControl, WithQuerySnapshotVersion - List options: WithDelimiter, WithPrefix, WithPaginationToken; now returns *ListResult with CommonPrefixes and HasMore Bucket options: - WithBucketAccess, WithDefaultTier, WithConsistentRead, WithForkSourceSnapshot Correctness: - WithAllowOverwrite(false) now uses Tigris' If-Match: "" header (WithCreateObjectIfNotExists) for a server-side atomic check instead of a racy Head-then-Put. - storage.Client.CreateBucketSnapshot returns a new CreateBucketSnapshotOutput exposing SnapshotVersion from the X-Tigris-Snapshot-Version response header, so callers no longer need to list snapshots to learn the version. - simplestorage.ListBucketSnapshots now uses the dedicated storage.Client.ListBucketSnapshots wrapper and leaves SnapshotInfo.Name empty since the listing API does not return the user-provided description. BREAKING CHANGE: storage.Client.CreateBucketSnapshot now returns *CreateBucketSnapshotOutput instead of *s3.CreateBucketOutput. The new type embeds *s3.CreateBucketOutput so existing field access still works; callers that bound the return value to *s3.CreateBucketOutput must re-type their variables. BREAKING CHANGE: simplestorage.Client.List now returns *ListResult instead of []Object to carry CommonPrefixes, PaginationToken, and HasMore. Assisted-by: Claude Opus 4.7 via Claude Code Signed-off-by: Xe Iaso <xe@tigrisdata.com>
Greptile SummaryThis PR achieves feature parity with the TypeScript SDK by adding
Confidence Score: 4/5Safe to merge after fixing the multipart + WithAllowOverwrite(false) bug; all other findings are P2 test-quality issues. One P1 defect: the multipart upload path drops o.S3Options entirely, silently invalidating the WithAllowOverwrite(false) no-overwrite contract for large files. All other findings (test cleanup pattern) are P2. simplestorage/client.go (multipart branch), simplestorage/client_new_features_test.go (env setup/cleanup pattern) Important Files Changed
|
There was a problem hiding this comment.
Pull request overview
This PR expands the Go simplestorage API to align with the TypeScript @tigrisdata/storage SDK by adding new operations (Head + presigned URL generation), extending Put/Get/List/CreateBucket options, improving overwrite correctness, and surfacing snapshot versions returned by CreateBucketSnapshot.
Changes:
- Add
HeadandGetPresignedUrl, plus extended options for Put/Get/List/CreateBucket to match TS SDK behavior. - Replace racy “Head-then-Put” no-overwrite logic with atomic server-side
If-Match: ""viatigrisheaders.WithCreateObjectIfNotExists(). - Surface snapshot versions from
CreateBucketSnapshotand switchListBucketSnapshotsto the dedicated storage wrapper.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
tigrisheaders/tigrisheaders.go |
Adds header helpers for storage class, consistency, and fork-from-snapshot support. |
simplestorage/client.go |
Implements Head, GetPresignedUrl, extended options, multipart Put support, and changes List return type. |
simplestorage/bucket_options.go |
Adds new bucket options (default tier, consistent read, fork source snapshot, bucket ACL). |
simplestorage/buckets.go |
Applies bucket ACL on create, surfaces snapshot version on snapshot creation, uses dedicated snapshot list wrapper. |
simplestorage/buckets_test.go |
Adds unit tests for new bucket options. |
simplestorage/client_new_features_test.go |
Adds tests for new simplestorage features/options and response types. |
simplestorage/client_example_test.go |
Adds documentation examples for new client APIs/options (compile-only examples). |
client.go |
Adds Client.S3() accessor and changes CreateBucketSnapshot to return snapshot version. |
README.md |
Documents new SnapshotVersion field on snapshot creation output. |
go.mod / go.sum |
Bumps AWS SDK deps and adds transfermanager dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| cleanup := tt.setupEnv |
There was a problem hiding this comment.
In these subtests, setupEnv is never executed before creating the client: cleanup := tt.setupEnv stores the function instead of calling it, and defer cleanup() only runs at the end (setting the env var late and never running the returned unset function). This both breaks the test (bucket env var not set) and leaks TIGRIS_STORAGE_BUCKET into subsequent tests. Call tt.setupEnv() up-front and defer the returned cleanup func.
| cleanup := tt.setupEnv | |
| cleanup := tt.setupEnv() |
| cleanup := tt.setupEnv | ||
| if cleanup == nil { | ||
| cleanup = func() func() { return func() {} } |
There was a problem hiding this comment.
Same issue here: setupEnv isn't invoked and the returned cleanup closure is never called, so the env var is set too late and not unset. This will make the test order-dependent and can cause failures when TIGRIS_STORAGE_BUCKET is required by New(). Call tt.setupEnv() and defer the returned cleanup.
| cleanup := tt.setupEnv | |
| if cleanup == nil { | |
| cleanup = func() func() { return func() {} } | |
| cleanup := func() {} | |
| if tt.setupEnv != nil { | |
| cleanup = tt.setupEnv() |
| { | ||
| name: "generates presigned URL with default expiration", | ||
| key: "test-key", | ||
| setupEnv: func() func() { | ||
| os.Setenv("TIGRIS_STORAGE_BUCKET", "test-bucket") | ||
| return func() { os.Unsetenv("TIGRIS_STORAGE_BUCKET") } | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "generates presigned URL with custom expiration", | ||
| key: "test-key", | ||
| opts: []ClientOption{ | ||
| WithPresignedExpiresIn(7200), | ||
| }, | ||
| setupEnv: func() func() { | ||
| os.Setenv("TIGRIS_STORAGE_BUCKET", "test-bucket") | ||
| return func() { os.Unsetenv("TIGRIS_STORAGE_BUCKET") } | ||
| }, | ||
| wantErr: false, | ||
| }, |
There was a problem hiding this comment.
These GetPresignedUrl cases expect success (wantErr: false) but presigning requires AWS credentials; in environments without TIGRIS/AWS creds this will fail non-deterministically. Consider either (a) using skipIfNoCreds(t) like other integration tests in this repo, (b) injecting static dummy credentials into the client config for presign-only tests, or (c) changing expectations to wantErr: true when creds are missing.
| if useMultipart { | ||
| tm := transfermanager.New(c.cli.S3()) | ||
| _, err = tm.UploadObject(ctx, &transfermanager.UploadObjectInput{ | ||
| Bucket: putInput.Bucket, | ||
| Key: putInput.Key, | ||
| Body: putInput.Body, | ||
| ContentType: putInput.ContentType, | ||
| ContentDisposition: putInput.ContentDisposition, | ||
| ACL: tmObjectACL(o.AccessType), | ||
| }) | ||
| } else { | ||
| _, err = c.cli.PutObject(ctx, putInput, o.S3Options...) | ||
| } |
There was a problem hiding this comment.
When useMultipart is true, the upload goes through transfermanager.UploadObject but none of o.S3Options are applied. This means headers like WithCreateObjectIfNotExists() (used for WithAllowOverwrite(false)) are silently ignored for multipart uploads, reintroducing the overwrite race/behavior difference. Ensure the multipart path also applies the same request headers/options (or explicitly disable multipart when AllowOverwrite=false if the transfermanager can't support it).
| b := make([]byte, length/2) | ||
| rand.Read(b) |
There was a problem hiding this comment.
generateRandomSuffix ignores the error from rand.Read, and will also panic for odd length values because length/2 truncates (hex string can be shorter than length before slicing). Handle the rand.Read error and compute the byte length as (length+1)/2 (or enforce an even length) before slicing.
| b := make([]byte, length/2) | |
| rand.Read(b) | |
| if length <= 0 { | |
| return "" | |
| } | |
| b := make([]byte, (length+1)/2) | |
| if _, err := rand.Read(b); err != nil { | |
| return "" | |
| } |
| // Put puts the contents of an object into Tigris. | ||
| // Returns PutResponse with metadata and a presigned URL for accessing the uploaded object. | ||
| func (c *Client) Put(ctx context.Context, obj *Object, opts ...ClientOption) (*PutResponse, error) { | ||
| o := new(ClientOptions).defaults(c.options) | ||
|
|
||
| for _, doer := range opts { | ||
| doer(&o) | ||
| } |
There was a problem hiding this comment.
This PR changes Put's return type from *Object to *PutResponse, which is a breaking API change for simplestorage callers. It’s not listed under the PR’s “Breaking changes” section; please document it there (or keep the old return type / provide a compatibility shim) so downstream users aren’t surprised.
| return &PutResponse{ | ||
| Path: key, | ||
| Size: obj.Size, | ||
| Modified: time.Now(), | ||
| ContentType: obj.ContentType, | ||
| ContentDisposition: lower(o.ContentDisposition, ""), | ||
| URL: presignResult.URL, |
There was a problem hiding this comment.
PutResponse.Modified is described as a “Last modified time”, but it is currently set to time.Now(), which may not match the server's recorded timestamp (and can be misleading across clock skew/timezones). Consider either leaving it unset/zero, renaming the field to clarify it's the client timestamp, or doing an explicit HEAD to retrieve authoritative metadata (if the extra call is acceptable).
| // WithBucketAccess sets the bucket-level canned ACL (public or private). | ||
| // Use AccessPublic to allow anonymous reads via public-read, or AccessPrivate | ||
| // (the default) to require authenticated access. | ||
| func WithBucketAccess(access AccessType) BucketOption { | ||
| return func(o *BucketOptions) { | ||
| o.Access = access | ||
| } | ||
| } |
There was a problem hiding this comment.
WithBucketAccess comment says AccessPrivate is the default, but BucketOptions.defaults() does not initialize Access (zero value ""), which leads bucketACL to send an empty canned ACL instead of "private". If private is the intended default, initialize Access: AccessPrivate in defaults (and consider adding a test asserting the default).
| cleanup := tt.setupEnv | ||
| if cleanup == nil { | ||
| cleanup = func() func() { return func() {} } |
There was a problem hiding this comment.
Same setup/cleanup bug as other tests: cleanup := tt.setupEnv doesn't invoke setupEnv before creating the client, and defer cleanup() only sets the env var at the end while never calling the returned unset func. Call tt.setupEnv() and defer the returned cleanup closure to avoid env leakage and ErrNoBucketName failures.
| cleanup := tt.setupEnv | |
| if cleanup == nil { | |
| cleanup = func() func() { return func() {} } | |
| cleanup := func() {} | |
| if tt.setupEnv != nil { | |
| cleanup = tt.setupEnv() |
Merge origin/main into Xe/simplestorage-feature-parity. Main landed simpler Head/List/PresignURL APIs and a Bundle API while this branch added atomic overwrite, snapshot query, response overrides, access control, random suffix, multipart, and progress-callback options. Resolution keeps main's API shape (Get/Head/Put return *Object, List takes only opts, PresignURL takes method+expiry) and re-layers the branch's unique additions on top. Drops the redundant GetPresignedUrl wrapper, HeadResponse, PutResponse, and GetPresignedUrlResult types in favor of main's PresignURL and *Object. Removes simplestorage/client_new_features_test.go (referenced removed types) and adds simplestorage/client_options_test.go covering the preserved option plumbing and progressReader. Signed-off-by: Xe Iaso <xe@tigrisdata.com> Assisted-by: Claude Opus 4.7 via Claude Code Signed-off-by: Xe Iaso <xe@tigrisdata.com>
BucketOptions.defaults() left Access as the empty zero value, which made bucketACL emit no canned ACL header. The WithBucketAccess docstring already documented private as the default, so align the implementation with the contract: seed Access in defaults() and have bucketACL fall through to BucketCannedACLPrivate so unset values still land on the wire as private. Add a regression test that asserts the default. Signed-off-by: Xe Iaso <xe@tigrisdata.com> Assisted-by: Claude Opus 4.7 via Claude Code Signed-off-by: Xe Iaso <xe@tigrisdata.com>
Two issues: - rand.Read could return an error that the previous code silently dropped, leaving the suffix all-zeros if entropy failed. - length/2 truncated for odd lengths, producing a hex string shorter than the requested length before slicing. Return (string, error), use (length+1)/2 bytes so the hex output is always at least length characters before truncation, and propagate the error from Put with bucket/key context. Signed-off-by: Xe Iaso <xe@tigrisdata.com> Assisted-by: Claude Opus 4.7 via Claude Code Signed-off-by: Xe Iaso <xe@tigrisdata.com>
Summary
Head,GetPresignedUrl, and extendedPut/Get/List/CreateBucketoptions to match the TypeScript@tigrisdata/storageSDK.WithAllowOverwrite(false)(Head-then-Put) with the server-side atomicIf-Match: ""header, and surface the snapshot version returned byCreateBucketSnapshot.simplestorage.ListBucketSnapshotsto the dedicatedstorage.Client.ListBucketSnapshotswrapper.Details
New object operations
Head(ctx, key, opts...)— metadata-only retrieval that also returns a presigned GET URL.GetPresignedUrl(ctx, key, opts...)— GET or PUT URL generation viaWithPresignedOperation,WithPresignedExpiresIn, andWithPresignedContentType.Extended options
Put:WithAccessType,WithRandomSuffix,WithAllowOverwrite,WithContentDisposition,WithMultipartUpload(viatransfermanager),WithUploadProgress.Get:WithResponseContentType,WithResponseContentDisposition,WithResponseCacheControl,WithQuerySnapshotVersion.List:WithDelimiter,WithPrefix,WithPaginationToken. Return type changes to*ListResult(carriesCommonPrefixes,PaginationToken,HasMore).CreateBucket:WithBucketAccess,WithDefaultTier,WithConsistentRead,WithForkSourceSnapshot.Correctness fixes
WithAllowOverwrite(false)now usestigrisheaders.WithCreateObjectIfNotExists()(If-Match: "") instead of a Head-then-Put check that was racy under concurrent writers.storage.Client.CreateBucketSnapshotnow returns*CreateBucketSnapshotOutput(embedding*s3.CreateBucketOutput) with aSnapshotVersionfield pulled from theX-Tigris-Snapshot-Versionresponse header.simplestorage.CreateBucketSnapshotsurfaces it throughSnapshotInfo.Version.simplestorage.ListBucketSnapshotsuses the dedicated wrapper and leavesSnapshotInfo.Nameempty, since the listing API does not return the user-provided description.Breaking changes
storage.Client.CreateBucketSnapshot: return type changed from*s3.CreateBucketOutputto*CreateBucketSnapshotOutput. The new type embeds*s3.CreateBucketOutputso field access is unchanged; callers that bound the value to a concrete*s3.CreateBucketOutputvariable need to re-type it.simplestorage.Client.List: return type changed from[]Objectto*ListResult.Non-goals (excluded from parity): browser-side uploads, force-delete for buckets, AbortController-style cancellation (use
context.Context).Test plan
go build ./...go vet ./...cleango test ./...passesnpm run formatHead,GetPresignedUrl,CreateBucketSnapshotversion header, andPutwithWithAllowOverwrite(false)under concurrent writers)