feat: Improve Storage Service Coverage and Fix Versioning#58
Conversation
…t versioning support
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds bucket versioning awareness to multipart upload completion (generating and applying a versionID to storage keys and ARNs when enabled) and expands test infrastructure with an in-memory file store, a failing-encryption wrapper, failing reader, and broader integration tests covering multipart, encryption, versioning, presigned URLs, and error paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Svc as StorageService
participant DB as BucketRepo/DB
participant Store as FileStore
participant Enc as EncryptionService
Client->>Svc: CompleteMultipartUpload(bucket, key, parts)
Svc->>DB: GetBucket(bucket)
DB-->>Svc: Bucket{VersioningEnabled: true/false}
alt Versioning enabled
Svc->>Svc: Generate VersionID
Svc->>Store: Assemble(parts...) -> combined object (temp key)
else Versioning disabled
Svc->>Store: Assemble(parts...) -> combined object (temp key)
end
Svc->>Enc: (if encryption enabled) Decrypt(combined data)
Enc-->>Svc: plaintext bytes
Svc->>Store: Write(bucket, storeKey[+versionID], plaintext)
Store-->>Svc: bytesWritten
Svc->>DB: SaveObjectMetadata(bucket,key,storeKey,VersionID?,IsLatest,ARN)
DB-->>Svc: confirmation
Svc-->>Client: Complete response (ARN [+ versionId])
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/services/storage.go (1)
271-273:⚠️ Potential issue | 🟡 MinorStale doc comment — says "ListBuckets" but decorates
SetBucketVersioning.-// ListBuckets list buckets for the current user. +// SetBucketVersioning enables or disables versioning on a bucket. func (s *StorageService) SetBucketVersioning(ctx context.Context, name string, enabled bool) error {
🤖 Fix all issues with AI agents
In `@internal/core/services/storage_test.go`:
- Around line 131-133: The test silently discards the error returned by
NewEncryptionService which can leave realEncSvc nil and cause panics when
FailingEncryptionService delegates to it; change the assignment from
"realEncSvc, _ := services.NewEncryptionService(encRepo, masterKeyHex)" to
capture the error (realEncSvc, err := services.NewEncryptionService(...)) and
call require.NoError(t, err) (and optionally require.NotNil(t, realEncSvc)) so
the test fails fast on bad masterKeyHex or other initialization errors.
- Line 374: The call to svc.Upload in the test ignores its returned error, which
can mask failures and cause store.failNext to exercise the wrong path; update
the test to capture the returned error from svc.Upload(ctx, "obj-bucket",
"fail-store", strings.NewReader("data")), check it immediately, and fail the
test (e.g., t.Fatalf or t.Fatal) if non-nil before setting store.failNext so
subsequent assertions exercise the intended failure branch.
In `@internal/core/services/storage.go`:
- Around line 371-386: Extract the duplicated versioning logic into a small
helper: add a named constant (e.g., versionEpochBit = 1 << 62) and two functions
generateVersionID() string and versionedStoreKey(key, versionID string) string
that encapsulate the computation of the version ID and the sprintf of
versionQueryFormat; then replace the inline logic in CompleteMultipartUpload
(around the bucket.VersioningEnabled block) and the Upload code path (the
earlier Upload function where the same 1<<62 arithmetic and fmt.Sprintf are
used) to call generateVersionID() and versionedStoreKey(...) respectively so the
magic number and formatting are centralized.
🧹 Nitpick comments (8)
internal/core/services/storage.go (2)
371-375: Unwrapped error fromGetBucketis inconsistent with the rest of this method.Other error returns in
CompleteMultipartUploadwrap witherrors.Wrap(...)(e.g., lines 352, 358, 390, 413), but this one returns the raw repository error. This inconsistency can make it harder for callers to distinguish error origins.Proposed fix
bucket, err := s.repo.GetBucket(ctx, upload.Bucket) if err != nil { - return nil, err + return nil, errors.Wrap(errors.Internal, "failed to get bucket for versioning check", err) }As per coding guidelines, "Use service-layer error handling with
internal/errorspackage".
37-45: Constructor has 5 parameters — consider using a params struct.
NewStorageServiceaccepts 5 dependencies. The coding guidelines require a params struct for constructors with 3 or more dependencies.Example params struct
type StorageServiceParams struct { Repo ports.StorageRepository Store ports.FileStore AuditSvc ports.AuditService EncryptSvc ports.EncryptionService Cfg *platform.Config } func NewStorageService(p StorageServiceParams) *StorageService { return &StorageService{ repo: p.Repo, store: p.Store, auditSvc: p.AuditSvc, encryptSvc: p.EncryptSvc, cfg: p.Cfg, } }As per coding guidelines, "Use a params struct for service constructors with 3 or more dependencies".
internal/core/services/storage_test.go (6)
141-146:FailingReaderis defined but never used.This type is dead code in the current file. Either remove it or add a test that exercises reader failures (e.g., passing
&FailingReader{}toUpload).
22-27:failNextfield is accessed without synchronization — potential data race.
failNextis written from the test goroutine (e.g.,store.failNext = true) and read insideWrite/Read/Assemblewithout holding the mutex. While current tests are sequential, thego test -racedetector will flag this.Proposed fix — guard with the existing mutex
type InMemFileStore struct { mu sync.RWMutex files map[string][]byte - failNext bool + failNext bool // guarded by mu } + +func (s *InMemFileStore) SetFailNext(v bool) { + s.mu.Lock() + defer s.mu.Unlock() + s.failNext = v +} + +func (s *InMemFileStore) shouldFail() bool { + s.mu.Lock() + defer s.mu.Unlock() + if s.failNext { + s.failNext = false + return true + } + return false +}Then replace each
if s.failNext { s.failNext = false; ... }block with:if s.shouldFail() { return ..., fmt.Errorf("injected failure") }And call sites change from
store.failNext = truetostore.SetFailNext(true).Also applies to: 37-41, 52-56, 77-81
97-117:FailingEncryptionService.failNexthas the same unsynchronized access issue.Same race concern as
InMemFileStore.failNext—encSvc.failNext = true(line 393) is set from the test goroutine whileEncrypt/Decryptread it without synchronization.Consider adding a mutex or accessor methods here as well.
148-151:masterKeyHexis duplicated three times (lines 131, 151, 270).Extract it as a test-file-level constant to avoid drift.
+const testMasterKeyHex = "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f"As per coding guidelines, "Do not use magic numbers - use named constants instead".
Also applies to: 258-271
258-288: Encryption subtest shadows outerdband creates redundant resources.Line 264 calls
setupDB(t)again, shadowing thedbreturned by the setup helper on line 149. It also creates a secondencRepoandencSvc(lines 269-272) that duplicate what's already available from the outer scope. This makes the test harder to follow.Consider reusing the
dband repo from the setup function and only creating the bucket-level key:- db := setupDB(t) - _, err = db.Exec(ctx, "UPDATE buckets SET encryption_enabled = TRUE WHERE name = $1", bucketName) + _, err = db.Exec(ctx, "UPDATE buckets SET encryption_enabled = TRUE WHERE name = $1", bucketName) require.NoError(t, err) - encRepo := postgres.NewEncryptionRepository(db) - masterKeyHex := "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" - encSvc, _ := services.NewEncryptionService(encRepo, masterKeyHex) - _, err = encSvc.CreateKey(ctx, bucketName) + encRepo := postgres.NewEncryptionRepository(db) + tempEncSvc, _ := services.NewEncryptionService(encRepo, testMasterKeyHex) + _, err = tempEncSvc.CreateKey(ctx, bucketName)
237-240: Errors fromDownloadandReadAllare silently discarded in content verification.Lines 238-239 ignore errors when verifying content integrity. If the download or read fails, the assertion on line 240 would give a misleading diff instead of a clear failure message.
- r, _, _ := svc.Download(ctx, bucketName, key) - data, _ := io.ReadAll(r) + r, _, err := svc.Download(ctx, bucketName, key) + require.NoError(t, err) + data, err := io.ReadAll(r) + require.NoError(t, err)Same pattern applies at lines 206, 286, 324, 328-330.
There was a problem hiding this comment.
Pull request overview
This PR improves test coverage for the storage service and fixes a bug where CompleteMultipartUpload did not properly handle bucket versioning. The fix ensures that multipart uploads to versioned buckets correctly generate version IDs and store objects with versioned keys, mirroring the behavior of the regular Upload method.
Changes:
- Implemented
InMemFileStorefor reliable in-memory testing of file operations - Added comprehensive error-path tests covering store failures, encryption failures, and edge cases
- Fixed versioning bug in
CompleteMultipartUploadto properly handle versioned buckets
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| internal/core/services/storage_test.go | Added InMemFileStore and FailingEncryptionService helpers, comprehensive error tests, and versioning/encryption test scenarios |
| internal/core/services/storage.go | Fixed CompleteMultipartUpload to support bucket versioning by generating version IDs and using versioned store keys; removed outdated comments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| db := setupDB(t) | ||
| _, err = db.Exec(ctx, "UPDATE buckets SET encryption_enabled = TRUE WHERE name = $1", bucketName) | ||
| require.NoError(t, err) | ||
|
|
||
| // Initialize encryption key for this bucket | ||
| encRepo := postgres.NewEncryptionRepository(db) |
There was a problem hiding this comment.
Variable shadowing: The db variable is declared here but already exists in the outer scope (line 149). This shadows the outer db variable within the Encryption test scope. Use a different variable name or remove the redundant declaration to avoid confusion.
| db := setupDB(t) | |
| _, err = db.Exec(ctx, "UPDATE buckets SET encryption_enabled = TRUE WHERE name = $1", bucketName) | |
| require.NoError(t, err) | |
| // Initialize encryption key for this bucket | |
| encRepo := postgres.NewEncryptionRepository(db) | |
| encDB := setupDB(t) | |
| _, err = encDB.Exec(ctx, "UPDATE buckets SET encryption_enabled = TRUE WHERE name = $1", bucketName) | |
| require.NoError(t, err) | |
| // Initialize encryption key for this bucket | |
| encRepo := postgres.NewEncryptionRepository(encDB) |
| svc, _, ctx := setupStorageServiceIntegrationTest(t) | ||
| svc, _, store, encSvc, db, ctx := setupStorageServiceIntegrationTest(t) | ||
| userID := appcontext.UserIDFromContext(ctx) | ||
| masterKeyHex := "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" |
There was a problem hiding this comment.
The masterKeyHex variable is defined at the test function level but is only used within specific nested subtests (Encryption and ErrorPaths). Consider moving this declaration into the subtests where it's actually used to improve code clarity and reduce the scope of the variable.
| masterKeyHex := "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" |
| _, err = svc.CompleteMultipartUpload(ctx, upload2.ID) | ||
| assert.Error(t, err) | ||
| }) | ||
|
|
There was a problem hiding this comment.
The PR description states that a bug in CompleteMultipartUpload related to bucket versioning was fixed (lines 371-409 in storage.go). However, there's no test case that explicitly verifies CompleteMultipartUpload with versioning enabled. The MultipartUpload test uses a bucket without versioning, and the Versioning test only tests regular Upload(). Consider adding a test case that enables versioning on a bucket and then performs a multipart upload to ensure the versioning fix is properly covered.
| t.Run("MultipartUploadWithVersioning", func(t *testing.T) { | |
| bucketName := "mp-versioned-bucket" | |
| _, err := svc.CreateBucket(ctx, bucketName, true) | |
| require.NoError(t, err) | |
| key := "large.zip" | |
| // Success Case with versioning enabled | |
| upload, err := svc.CreateMultipartUpload(ctx, bucketName, key) | |
| require.NoError(t, err) | |
| _, err = svc.UploadPart(ctx, upload.ID, 1, strings.NewReader("part1")) | |
| require.NoError(t, err) | |
| _, err = svc.UploadPart(ctx, upload.ID, 2, strings.NewReader("part2")) | |
| require.NoError(t, err) | |
| obj, err := svc.CompleteMultipartUpload(ctx, upload.ID) | |
| require.NoError(t, err) | |
| assert.Equal(t, key, obj.Key) | |
| // Verify content for the completed multipart upload on a versioned bucket | |
| r, _, err := svc.Download(ctx, bucketName, key) | |
| require.NoError(t, err) | |
| data, err := io.ReadAll(r) | |
| require.NoError(t, err) | |
| assert.Equal(t, "part1part2", string(data)) | |
| }) |
| // FailingReader returns an error on Read. | ||
| type FailingReader struct{} | ||
|
|
||
| return svc, repo, ctx | ||
| func (f *FailingReader) Read(p []byte) (n int, err error) { | ||
| return 0, fmt.Errorf("read failure") |
There was a problem hiding this comment.
The FailingReader type is defined but never used in the test file. If it's not needed for testing error paths, consider removing it to keep the code clean. If it was intended for testing read failures during upload operations, add a test case that uses it.
| func (s *InMemFileStore) Write(ctx context.Context, bucket, key string, r io.Reader) (int64, error) { | ||
| if s.failNext { | ||
| s.failNext = false | ||
| return 0, fmt.Errorf("injected failure") | ||
| } | ||
| data, err := io.ReadAll(r) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| s.files[s.key(bucket, key)] = data | ||
| return int64(len(data)), nil | ||
| } | ||
|
|
||
| func (s *InMemFileStore) Read(ctx context.Context, bucket, key string) (io.ReadCloser, error) { | ||
| if s.failNext { | ||
| s.failNext = false | ||
| return nil, fmt.Errorf("injected failure") | ||
| } |
There was a problem hiding this comment.
The failNext field is accessed without proper synchronization. While this test runs sequentially, accessing failNext outside the mutex protection could lead to data races if tests are run in parallel or if the store is used concurrently. Consider protecting failNext with the mutex or using atomic operations for this field to ensure thread safety.
| // FailingEncryptionService wraps a real one but can fail. | ||
| type FailingEncryptionService struct { | ||
| ports.EncryptionService | ||
| failNext bool | ||
| } | ||
|
|
||
| func (f *FailingEncryptionService) Encrypt(ctx context.Context, bucket string, data []byte) ([]byte, error) { | ||
| if f.failNext { | ||
| f.failNext = false | ||
| return nil, fmt.Errorf("injected failure") | ||
| } | ||
| return f.EncryptionService.Encrypt(ctx, bucket, data) | ||
| } | ||
|
|
||
| func (f *FailingEncryptionService) Decrypt(ctx context.Context, bucket string, data []byte) ([]byte, error) { | ||
| if f.failNext { | ||
| f.failNext = false | ||
| return nil, fmt.Errorf("injected failure") | ||
| } | ||
| return f.EncryptionService.Decrypt(ctx, bucket, data) | ||
| } |
There was a problem hiding this comment.
The failNext field in FailingEncryptionService is not synchronized. If this service is called concurrently (which is possible in a storage service), this could lead to data races. Consider using atomic operations (e.g., atomic.Bool) or a mutex to protect access to this field.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/services/storage_test.go (1)
187-188:⚠️ Potential issue | 🟠 MajorPervasive silent error discards throughout tests mask real failures.
There are ~20 instances of
_, _ =orval, _ :=across this file (e.g., lines 187, 222, 240–241, 245, 251, 272, 293, 329, 331, 391–398, 406–407, 413, 420–423, 444). When setup steps fail silently, subsequent assertions test the wrong thing — an error path instead of the intended happy/error path.For test setup calls whose success is a precondition, capture the error and use
require.NoError:// Instead of: _, _ = svc.CreateBucket(ctx, bucketName, false) // Use: _, err := svc.CreateBucket(ctx, bucketName, false) require.NoError(t, err)For assertion-adjacent reads (e.g.,
io.ReadAll), at minimum useassert.NoErrorso a read failure surfaces clearly rather than comparing against empty/nil.As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()".Also applies to: 222-222, 240-242, 391-398
🤖 Fix all issues with AI agents
In `@internal/core/services/storage_test.go`:
- Around line 272-273: The call to NewEncryptionService currently ignores its
error return, which can leave encSvc nil and make the subsequent CreateKey and
assertions meaningless; update the test to check the error returned by
services.NewEncryptionService(encRepo, masterKeyHex) (the variable assigned to
encSvc) and fail the test immediately (e.g., t.Fatalf or require.NoError) if an
error is returned before calling encSvc.CreateKey(ctx, bucketName), so any
failure is surfaced rather than silently discarded.
- Around line 11-13: The test file imports "time" and "github.com/google/uuid"
but neither is referenced, causing CI failures; remove those unused imports from
the import block in storage_test.go (delete the "time" and
"github.com/google/uuid" entries) or alternatively use them in a test if
intended, ensuring there are no leftover references to time or uuid in the file.
🧹 Nitpick comments (3)
internal/core/services/storage_test.go (3)
39-43:failNextflag is not protected by the mutex.The
filesmap is correctly guarded bymu, butfailNextis read and written without synchronization. While current tests are single-goroutine, this is inconsistent and fragile if tests ever run concurrently or the store is used across goroutines. Consider guarding it with the same mutex, or usingatomic.Bool.Also applies to: 54-58, 79-83
343-377: Consider table-driven structure for the validation/error-path cases.The invalid-bucket-name checks (lines 345–352) and the "not found" checks (lines 355–377) follow a repetitive pattern that's well suited to table-driven tests. This improves readability and makes it easy to add new cases.
invalidNames := []struct { name string }{ {"INVALID NAME"}, {""}, {strings.Repeat("a", 65)}, {"-invalid"}, } for _, tc := range invalidNames { _, err := svc.CreateBucket(ctx, tc.name, false) assert.Error(t, err, "expected error for bucket name: %q", tc.name) }As per coding guidelines, "Use table-driven tests in test files".
130-131: Extract repeated test constants.
masterKeyHexand the config'sSecretsEncryptionKeyvalue appear identically at lines 130, 133, 153, 271, and 394/422. Define them once as package-level test constants to reduce duplication and make rotation easier.const testMasterKeyHex = "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" const testSecretKey = "test-secret-32-chars-long-needed-!!"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _, err = svc.CompleteMultipartUpload(ctx, upload2.ID) | ||
| assert.Error(t, err) | ||
| }) | ||
|
|
There was a problem hiding this comment.
The PR description states this fixes a bug where CompleteMultipartUpload did not correctly support bucket versioning, but the MultipartUpload test case (lines 220-258) does not test multipart upload with versioning enabled. Consider adding a test case that:
- Creates a bucket with versioning enabled
- Performs a multipart upload
- Verifies that the completed object has a non-null version ID
- Verifies that the object is stored with the correct versioned key format
This would ensure the versioning fix is properly tested and prevent regression.
| t.Run("MultipartUploadWithVersioning", func(t *testing.T) { | |
| versionedBucket := "mp-versioned-bucket" | |
| _, err := svc.CreateBucket(ctx, versionedBucket, true) | |
| require.NoError(t, err) | |
| key := "large.zip" | |
| upload, err := svc.CreateMultipartUpload(ctx, versionedBucket, key) | |
| require.NoError(t, err) | |
| _, err = svc.UploadPart(ctx, upload.ID, 1, strings.NewReader("part1")) | |
| require.NoError(t, err) | |
| _, err = svc.UploadPart(ctx, upload.ID, 2, strings.NewReader("part2")) | |
| require.NoError(t, err) | |
| obj, err := svc.CompleteMultipartUpload(ctx, upload.ID) | |
| require.NoError(t, err) | |
| // Verify object metadata | |
| assert.Equal(t, key, obj.Key) | |
| assert.Equal(t, int64(10), obj.SizeBytes) // "part1" (5) + "part2" (5) = 10 | |
| // With versioning enabled, the completed object should have a non-empty version ID. | |
| assert.NotEmpty(t, obj.VersionID) | |
| // Verify that the latest version is readable via the logical key. | |
| r, _, err := svc.Download(ctx, versionedBucket, key) | |
| require.NoError(t, err) | |
| data, err := io.ReadAll(r) | |
| require.NoError(t, err) | |
| assert.Equal(t, "part1part2", string(data)) | |
| }) |
| if s.failNext { | ||
| s.failNext = false | ||
| return nil, fmt.Errorf("injected failure") | ||
| } |
There was a problem hiding this comment.
The failNext field is accessed without mutex protection, which could lead to race conditions if tests are run concurrently. While these are integration tests that likely run sequentially, it's best practice to protect all shared mutable state with the mutex. Consider acquiring the lock before checking failNext, or use atomic operations for the boolean flag.
| if s.failNext { | ||
| s.failNext = false | ||
| return 0, fmt.Errorf("injected failure") | ||
| } |
There was a problem hiding this comment.
The failNext field is accessed without mutex protection, which could lead to race conditions if tests are run concurrently. While these are integration tests that likely run sequentially, it's best practice to protect all shared mutable state with the mutex. Consider acquiring the lock before checking failNext, or use atomic operations for the boolean flag.
| if s.failNext { | ||
| s.failNext = false | ||
| return 0, fmt.Errorf("injected failure") | ||
| } |
There was a problem hiding this comment.
The failNext field is accessed without mutex protection, which could lead to race conditions if tests are run concurrently. While these are integration tests that likely run sequentially, it's best practice to protect all shared mutable state with the mutex. Consider acquiring the lock before checking failNext, or use atomic operations for the boolean flag.
| if f.failNext { | ||
| f.failNext = false | ||
| return nil, fmt.Errorf("injected failure") | ||
| } |
There was a problem hiding this comment.
The failNext field is accessed without any synchronization. If tests are run concurrently or if the service is used from multiple goroutines, this could lead to race conditions. Consider using atomic operations (e.g., atomic.Bool or atomic.LoadUint32/StoreUint32) to access this field safely.
| // Setup encryption service | ||
| masterKeyHex := "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" | ||
| encRepo := postgres.NewEncryptionRepository(db) | ||
| realEncSvc, _ := services.NewEncryptionService(encRepo, masterKeyHex) |
There was a problem hiding this comment.
The error returned from services.NewEncryptionService is ignored (using blank identifier). While this is test code and may intentionally ignore errors for simplicity, it would be better to handle the error with require.NoError(t, err) to catch any setup issues early and make test failures clearer.
| t.Run("Versioning", func(t *testing.T) { | ||
| bucketName := "version-bucket" | ||
| _, _ = svc.CreateBucket(ctx, bucketName, false) | ||
| err := svc.SetBucketVersioning(ctx, bucketName, true) | ||
| assert.NoError(t, err) | ||
|
|
||
| // Verify versioning is enabled | ||
| b, err := svc.GetBucket(ctx, bucketName) | ||
| assert.NoError(t, err) | ||
| assert.True(t, b.VersioningEnabled) | ||
|
|
||
| key := "v.txt" | ||
|
|
||
| // Upload v1 | ||
| v1, err := svc.Upload(ctx, bucketName, key, strings.NewReader("v1")) | ||
| assert.NoError(t, err) | ||
| assert.NotEqual(t, "null", v1.VersionID) | ||
|
|
||
| // Upload v2 | ||
| v2, err := svc.Upload(ctx, bucketName, key, strings.NewReader("v2")) | ||
| assert.NoError(t, err) | ||
| assert.NotEqual(t, "null", v2.VersionID) | ||
| assert.NotEqual(t, v1.VersionID, v2.VersionID) | ||
|
|
||
| // List Versions | ||
| versions, err := svc.ListVersions(ctx, bucketName, key) | ||
| assert.NoError(t, err) | ||
| assert.GreaterOrEqual(t, len(versions), 2) | ||
|
|
||
| // Download v1 | ||
| r, meta, err := svc.DownloadVersion(ctx, bucketName, key, v1.VersionID) | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, r) | ||
| assert.Equal(t, v1.VersionID, meta.VersionID) | ||
| d1, _ := io.ReadAll(r) | ||
| assert.Equal(t, "v1", string(d1)) | ||
|
|
||
| // Download v2 | ||
| r2, meta2, _ := svc.DownloadVersion(ctx, bucketName, key, v2.VersionID) | ||
| assert.Equal(t, v2.VersionID, meta2.VersionID) | ||
| d2, _ := io.ReadAll(r2) | ||
| assert.Equal(t, "v2", string(d2)) | ||
|
|
There was a problem hiding this comment.
The Download function (line 128 in storage.go) does not correctly handle versioned objects. After retrieving the object metadata (which includes version_id), it reads from the store using just the key (line 136), but versioned objects are stored with a versioned key format (key?versionId=versionID). This will fail to find the file in the store for any versioned object.
The test does not catch this bug because it only tests DownloadVersion, not Download with a versioned bucket. After uploading v2 on line 310, add a test to verify that Download(ctx, bucketName, key) returns the latest version (v2) with the correct content.
| func (s *StorageService) Download(ctx context.Context, bucket, key string) (io.ReadCloser, *domain.Object, error) { | ||
| // 1. Get metadata | ||
| obj, err := s.repo.GetMeta(ctx, bucket, key) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| // 2. Open file | ||
| reader, err := s.store.Read(ctx, bucket, key) | ||
| if err != nil { | ||
| platform.StorageOperations.WithLabelValues("download", bucket, "error").Inc() | ||
| return nil, nil, err | ||
| } |
There was a problem hiding this comment.
The Download function does not correctly handle versioned objects. After retrieving the object metadata (which includes obj.VersionID on line 130), the function reads from the store using just the key on line 136. However, versioned objects are stored with a versioned key format (key?versionId=versionID as seen in Upload line 65 and DownloadVersion line 181).
This means Download will fail to find versioned objects in the store. The fix should be similar to DownloadVersion (lines 179-182): construct the storeKey based on obj.VersionID before reading from the store.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type InMemFileStore struct { | ||
| mu sync.RWMutex | ||
| files map[string][]byte | ||
| failNext bool | ||
| } |
There was a problem hiding this comment.
The failNext flag in InMemFileStore and FailingEncryptionService is not protected by mutex, which could cause race conditions if multiple goroutines access these test helpers concurrently. While the current tests appear to be sequential, this could lead to flaky tests if concurrency is introduced later. Consider protecting failNext with the existing mu mutex or using atomic operations.
| type FailingEncryptionService struct { | ||
| ports.EncryptionService | ||
| failNext bool | ||
| } |
There was a problem hiding this comment.
The failNext flag in FailingEncryptionService is not protected by any synchronization mechanism, which could cause race conditions if multiple goroutines access this test helper concurrently. While the current tests appear to be sequential, consider using atomic operations (e.g., atomic.Bool) to make this safer and prevent potential flaky tests.
| // Initialize encryption key for this bucket | ||
| encRepo := postgres.NewEncryptionRepository(db) | ||
| masterKeyHex := "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" | ||
| encSvc, _ := services.NewEncryptionService(encRepo, masterKeyHex) |
There was a problem hiding this comment.
The Encryption test creates a new EncryptionService instance on line 272, which bypasses the FailingEncryptionService wrapper set up in setupStorageServiceIntegrationTest. This means the encryption failure path cannot be tested through this wrapper. Consider using the encSvc from setup or documenting why a separate instance is needed here.
| // Initialize encryption key for this bucket | |
| encRepo := postgres.NewEncryptionRepository(db) | |
| masterKeyHex := "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" | |
| encSvc, _ := services.NewEncryptionService(encRepo, masterKeyHex) | |
| // Initialize encryption key for this bucket using the existing encryption service |
This PR increases the test coverage for the storage service to ~90% and fixes a bug where CompleteMultipartUpload did not correctly support bucket versioning. Key changes include the implementation of an InMemFileStore for reliable integration testing and the addition of comprehensive error-path tests.
Summary by CodeRabbit
New Features
Bug Fixes
Tests