enhancement: add context management support to storages #1569
Conversation
efectn
requested review from
gaby,
sixcolors and
ReneWerner87
and removed request for
a team
WalkthroughA comprehensive refactor has been applied across many storage driver packages to add context-aware operations. The standard Get, Set, Delete, and Reset methods have been replaced or extended with versions that accept a context parameter, allowing cancellation and timeout handling. Existing methods now delegate to these new implementations using a background context, ensuring backward compatibility while improving concurrent operation management. Test suites have been updated to simulate context cancellations and validate proper error handling. Minor changes include configuration updates and HTTP request modifications to ensure context propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Storage
Client->>Storage: Call Get(key) / Set(key, val, exp)
Storage->>Storage: Create background context
Storage->>Storage: Call GetWithContext(ctx, key) / SetWithContext(ctx, key, val, exp)
Storage-->>Client: Return value or error (context canceled if applicable)
Possibly related issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (18)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (6)
mssql/mssql_test.go (1)
13-18:⚠️ Potential issueFix test initialization failure in CI pipeline.
The test initialization fails in the CI pipeline, likely due to missing environment variables or connection issues.
Consider these improvements:
- Add environment variable validation:
var testStore = New(Config{ + // Fail fast if required environment variables are not set + Database: func() string { + db := os.Getenv("MSSQL_DATABASE") + if db == "" { + panic("MSSQL_DATABASE environment variable is required") + } + return db + }(), + Username: func() string { + user := os.Getenv("MSSQL_USERNAME") + if user == "" { + panic("MSSQL_USERNAME environment variable is required") + } + return user + }(), - Database: os.Getenv("MSSQL_DATABASE"), - Username: os.Getenv("MSSQL_USERNAME"), Password: os.Getenv("MSSQL_PASSWORD"), Reset: true, })
- Add a test skip when running in CI:
+func init() { + if os.Getenv("CI") != "" && os.Getenv("MSSQL_DATABASE") == "" { + fmt.Println("Skipping MSSQL tests in CI environment") + os.Exit(0) + } +}🧰 Tools
🪛 GitHub Actions: Benchmark
[error] 13-13: Initialization failed in mssql_test.go.
mssql/mssql.go (1)
93-95:⚠️ Potential issueReplace panic with proper error handling in New function.
Panicking on connection failure makes error handling difficult for consumers and could cause issues in production.
- if err := db.Ping(); err != nil { - panic(err) - } + if err := db.Ping(); err != nil { + _ = db.Close() + return nil, fmt.Errorf("failed to connect to MSSQL: %w", err) + }This requires updating the function signature:
-func New(config ...Config) *Storage { +func New(config ...Config) (*Storage, error) {And updating the test initialization:
var testStore *Storage func init() { store, err := New(Config{ Database: os.Getenv("MSSQL_DATABASE"), Username: os.Getenv("MSSQL_USERNAME"), Password: os.Getenv("MSSQL_PASSWORD"), Reset: true, }) if err != nil { if os.Getenv("CI") != "" { fmt.Printf("Skipping MSSQL tests: %v\n", err) os.Exit(0) } panic(err) } testStore = store }🧰 Tools
🪛 GitHub Actions: Benchmark
[error] 94-94: Panic occurred: read tcp 127.0.0.1:49246->127.0.0.1:1433: read: connection reset by peer.
azureblob/azureblob.go (1)
158-163: 🛠️ Refactor suggestionConsider returning errors instead of panicking.
The
handleErrorhelper function panics on configuration errors. Consider returning these errors to allow better error handling by the caller.-func handleError(err error) { - if err != nil { - panic(fmt.Sprintf("invalid config:, %v", err)) - } +func handleError(err error) error { + if err != nil { + return fmt.Errorf("invalid config: %w", err) + } + return nil }valkey/valkey.go (1)
67-67:⚠️ Potential issueFix the benchmark pipeline failure.
The benchmark is failing due to a Redis connection refused error. This suggests that Redis is not running or not accessible during the benchmark test.
Please ensure that Redis is properly configured and running in the CI environment before running the benchmarks. You might need to:
- Add Redis service to the GitHub Actions workflow
- Wait for Redis to be ready before running the benchmarks
- Verify the Redis connection settings
🧰 Tools
🪛 GitHub Actions: Benchmark
[error] 67-67: Panic occurred: dial tcp 127.0.0.1:6379: connect: connection refused
dynamodb/dynamodb_test.go (1)
14-23:⚠️ Potential issueFix DynamoDB test configuration.
The test is failing with
ResourceNotFoundException. Ensure the table exists before running the tests.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Create DynamoDB table for tests aws dynamodb create-table \ --table-name fiber_storage \ --attribute-definitions AttributeName=id,AttributeType=S \ --key-schema AttributeName=id,KeyType=HASH \ --provisioned-throughput ReadCapacityUnits=5,WriteCapacityUnits=5 \ --endpoint-url http://localhost:8000Length of output: 351
DynamoDB Table Creation Fails Due to Missing AWS CLI
The supplied shell script intended to create the table isn't executing because the AWS CLI isn't installed in the current environment. Please ensure you run the table-creation command in an environment where the AWS CLI is available (or install/configure it in your test setup) so that the "fiber_storage" table exists before the tests are executed.
minio/minio_test.go (1)
334-334: 💡 Verification agent🧩 Analysis chain
Docker pull rate limit affecting benchmarks.
The benchmarks are failing due to Docker pull rate limits. Consider:
- Authenticating with Docker Hub
- Using GitHub Container Registry as a fallback
- Caching the container image
Also applies to: 345-345, 362-362
🏁 Script executed:
#!/bin/bash # Check if GitHub Container Registry has the minio image gh api \ -H "Accept: application/vnd.oci.image.index.v1+json" \ /v2/minio/minio/tags/listLength of output: 393
Action Required: Docker Hub Auth & Fallback for Image Pulls Not Implemented
The API call to GitHub Container Registry failed with an unsupported Accept header, confirming the fallback isn’t configured as intended. The benchmark code still appears to trigger Docker pulls without authentication or caching, leading to rate limit failures.
- The attempted fallback check using an incorrect header (should use
application/json) failed.- No changes addressing authentication with Docker Hub or image caching were detected in the affected benchmark sections.
Please update the Docker image handling by authenticating to Docker Hub, correctly querying GitHub Container Registry (using
application/jsonin the Accept header), and caching the container image to avoid pull rate limits.🧰 Tools
🪛 GitHub Actions: Benchmark
[error] 334-334: Received unexpected error: create container: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
🧹 Nitpick comments (16)
sqlite3/sqlite3_test.go (1)
27-191: Consider adding more test cases for context-aware operations.While the current tests cover context cancellation scenarios well, consider adding:
- Positive test cases with non-canceled context
- Timeout test cases using
context.WithTimeout- Test cases with parent context cancellation
Example for SetWithContext positive case:
func Test_SQLite3_SetWithContext_Success(t *testing.T) { var ( key = "john" val = []byte("doe") ) ctx := context.Background() err := testStore.SetWithContext(ctx, key, val, 0) require.NoError(t, err) result, err := testStore.Get(key) require.NoError(t, err) require.Equal(t, val, result) }sqlite3/sqlite3.go (2)
122-134: Consider adding context cancellation handling.While the context is correctly passed to database operations, consider adding explicit error checks for context cancellation to provide more specific error messages.
Example implementation for SetWithContext:
func (s *Storage) SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error { + if err := ctx.Err(); err != nil { + return fmt.Errorf("context error in SetWithContext: %w", err) + } // Ain't Nobody Got Time For That if len(key) <= 0 || len(val) <= 0 { return nil }Also applies to: 141-149, 156-160
187-190: Consider adding context support to garbage collection.The gc operation could benefit from context management to handle timeouts and cancellations during cleanup operations, especially for large datasets.
This could be implemented in a future update by:
- Adding a context parameter to the gc method
- Using ExecContext instead of Exec
- Propagating the context through the gcTicker
mssql/mssql_test.go (1)
30-41: Enhance test coverage for SetWithContext.The test verifies the error case but doesn't confirm that the value wasn't actually set in the database.
func Test_MSSQL_SetWithContext(t *testing.T) { var ( key = "john" val = []byte("doe") ) ctx, cancel := context.WithCancel(context.Background()) cancel() err := testStore.SetWithContext(ctx, key, val, 0) require.ErrorIs(t, err, context.Canceled) + + // Verify the value wasn't set + result, err := testStore.Get(key) + require.NoError(t, err) + require.Zero(t, len(result)) }mssql/mssql.go (3)
136-163: Enhance GetWithContext documentation and validation.The implementation is correct but could benefit from improved documentation and validation.
-// GetWithContext gets value by key with context +// GetWithContext gets value by key with context. +// Returns nil, ctx.Err() if the context is canceled or times out. +// Returns nil, nil if the key doesn't exist or is expired. func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) { + if ctx == nil { + ctx = context.Background() + } + if len(key) <= 0 { return nil, nil }
170-183: Enhance SetWithContext documentation and validation.The implementation is correct but could benefit from improved documentation and validation.
-// SetWithContext key with value and expiration time with context +// SetWithContext sets a key-value pair with an optional expiration time. +// Returns ctx.Err() if the context is canceled or times out. +// The operation is atomic; if the context is canceled, the value won't be set. func (s *Storage) SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error { + if ctx == nil { + ctx = context.Background() + } + if len(key) <= 0 || len(val) <= 0 { return nil }
241-244: Add context support to garbage collection.The gc method should support context for graceful shutdown and preventing long-running operations.
-func (s *Storage) gc(t time.Time) { +func (s *Storage) gc(ctx context.Context, t time.Time) { - _, _ = s.db.Exec(s.sqlGC, t.Unix()) + _, _ = s.db.ExecContext(ctx, s.sqlGC, t.Unix()) } func (s *Storage) gcTicker() { ticker := time.NewTicker(s.gcInterval) defer ticker.Stop() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + for { select { case <-s.done: return case t := <-ticker.C: - s.gc(t) + s.gc(ctx, t) } } }valkey/valkey_test.go (2)
27-38: Enhance context-aware test coverage.While the current tests verify canceled context behavior, consider adding tests for:
- Context timeout scenarios
- Parent context propagation
- Non-canceled context success cases
Here's an example of additional test cases:
+func Test_Valkey_SetWithContextTimeout(t *testing.T) { + var ( + key = "john" + val = []byte("doe") + ) + + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) + defer cancel() + time.Sleep(1 * time.Millisecond) // Ensure timeout + + err := testStore.SetWithContext(ctx, key, val, 0) + require.ErrorIs(t, err, context.DeadlineExceeded) +} + +func Test_Valkey_SetWithContextSuccess(t *testing.T) { + var ( + key = "john" + val = []byte("doe") + ) + + ctx := context.Background() + err := testStore.SetWithContext(ctx, key, val, 0) + require.NoError(t, err) + + // Verify the value was set + result, err := testStore.Get(key) + require.NoError(t, err) + require.Equal(t, val, result) +}Also applies to: 67-82, 128-146, 169-191
315-352: Add benchmarks for context-aware operations and improve test organization.Consider the following improvements:
- Add benchmarks for context-aware operations
- Use table-driven tests with t.Run for better organization
Here's an example of a context-aware benchmark:
+func Benchmark_Valkey_SetWithContext(b *testing.B) { + b.ReportAllocs() + b.ResetTimer() + + ctx := context.Background() + var err error + for i := 0; i < b.N; i++ { + err = testStore.SetWithContext(ctx, "john", []byte("doe"), 0) + } + + require.NoError(b, err) +}And here's how to organize tests using sub-tests:
+func Test_Valkey_Set_Operations(t *testing.T) { + tests := []struct { + name string + key string + value []byte + exp time.Duration + wantErr bool + }{ + { + name: "basic set", + key: "john", + value: []byte("doe"), + exp: 0, + wantErr: false, + }, + // Add more test cases + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := testStore.Set(tt.key, tt.value, tt.exp) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +}etcd/etcd.go (1)
95-102: Consider adding error documentation.While the implementation is correct, consider documenting the possible errors that could be returned from the etcd Delete operation.
clickhouse/clickhouse.go (1)
79-112: Consider simplifying the time comparison logic.The time comparison logic is complex and includes a comment about inconsistent behavior. Consider extracting this logic into a helper function for better maintainability.
+func isExpired(t time.Time) bool { + return !time.Unix(0, 0).Equal(t) && t.Before(time.Now().UTC()) +} func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) { // ... existing code ... - if !time.Unix(0, 0).Equal(result.Expiration) && result.Expiration.Before(time.Now().UTC()) { + if isExpired(result.Expiration) { return []byte{}, nil } // ... existing code ... }azureblob/azureblob.go (1)
111-113: Review commented-out code.There's commented-out code for container deletion. Either remove it or document why it's kept as a reference.
- //_, err := s.client.DeleteContainer(ctx, s.container, nil) - //return errcloudflarekv/cloudflarekv.go (1)
110-157: Consider adding pagination size limit.While the implementation of
ResetWithContextis good, consider adding a limit to the number of keys retrieved in each pagination request to prevent potential memory issues with large datasets.resp, err := s.api.ListWorkersKVKeys(ctx, cloudflare.AccountIdentifier(s.accountID), cloudflare.ListWorkersKVsParams{ NamespaceID: s.namespaceID, Cursor: cursor, + Limit: 1000, // Add reasonable pagination size })cloudflarekv/cloudflarekv_test.go (1)
70-70: Remove debug print statement.Remove the
fmt.Println(err)statement as it's not needed in the test.-fmt.Println(err)arangodb/arangodb_test.go (1)
174-174: Maintain consistency in error checking style.Consider using
require.NoError(t, err)instead ofrequire.Equal(t, err, nil)to maintain consistency with other tests.- require.Equal(t, err, nil) + require.NoError(t, err)dynamodb/dynamodb.go (1)
163-169: Consider using BatchWriteItem for Reset operation.The current implementation deletes and recreates the table, which might not be ideal for production environments due to:
- Potential permission issues (requires table deletion rights)
- Downtime during table recreation
- Loss of table configuration (indexes, capacity settings)
Consider using BatchWriteItem to delete items in batches instead:
-func (s *Storage) ResetWithContext(ctx context.Context) error { - deleteTableInput := awsdynamodb.DeleteTableInput{ - TableName: &s.table, - } - _, err := s.db.DeleteTable(ctx, &deleteTableInput) - return err +func (s *Storage) ResetWithContext(ctx context.Context) error { + scanInput := &awsdynamodb.ScanInput{ + TableName: &s.table, + AttributesToGet: []string{keyAttrName}, + } + + var lastKey map[string]types.AttributeValue + for { + scanInput.ExclusiveStartKey = lastKey + result, err := s.db.Scan(ctx, scanInput) + if err != nil { + return err + } + + if len(result.Items) > 0 { + writeRequests := make([]types.WriteRequest, len(result.Items)) + for i, item := range result.Items { + writeRequests[i] = types.WriteRequest{ + DeleteRequest: &types.DeleteRequest{ + Key: item, + }, + } + } + + batchInput := &awsdynamodb.BatchWriteItemInput{ + RequestItems: map[string][]types.WriteRequest{ + s.table: writeRequests, + }, + } + + if _, err := s.db.BatchWriteItem(ctx, batchInput); err != nil { + return err + } + } + + lastKey = result.LastEvaluatedKey + if lastKey == nil { + break + } + } + + return nil +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
arangodb/README.md(1 hunks)arangodb/arangodb.go(3 hunks)arangodb/arangodb_test.go(5 hunks)azureblob/azureblob.go(2 hunks)azureblob/azureblob_test.go(5 hunks)clickhouse/clickhouse.go(4 hunks)clickhouse/clickhouse_test.go(4 hunks)cloudflarekv/cloudflarekv.go(6 hunks)cloudflarekv/cloudflarekv_test.go(4 hunks)cloudflarekv/test_module.go(5 hunks)dynamodb/dynamodb.go(5 hunks)dynamodb/dynamodb_test.go(5 hunks)etcd/etcd.go(3 hunks)etcd/etcd_test.go(4 hunks)minio/minio.go(6 hunks)minio/minio_test.go(4 hunks)mongodb/mongodb.go(3 hunks)mongodb/mongodb_test.go(5 hunks)mssql/mssql.go(4 hunks)mssql/mssql_test.go(5 hunks)mysql/mysql.go(4 hunks)mysql/mysql_test.go(5 hunks)nats/config.go(1 hunks)nats/nats.go(10 hunks)nats/nats_test.go(6 hunks)postgres/postgres.go(3 hunks)postgres/postgres_test.go(4 hunks)redis/redis.go(1 hunks)redis/redis_test.go(5 hunks)rueidis/rueidis.go(1 hunks)rueidis/rueidis_test.go(5 hunks)s3/s3.go(5 hunks)s3/s3_test.go(5 hunks)scylladb/scylladb.go(5 hunks)scylladb/scylladb_test.go(5 hunks)sqlite3/sqlite3.go(4 hunks)sqlite3/sqlite3_test.go(5 hunks)valkey/valkey.go(1 hunks)valkey/valkey_test.go(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Benchmark
mssql/mssql.go
[error] 94-94: Panic occurred: read tcp 127.0.0.1:49246->127.0.0.1:1433: read: connection reset by peer.
valkey/valkey_test.go
[error] 13-13: Initialization failed in valkey_test.go due to connection issue.
mssql/mssql_test.go
[error] 13-13: Initialization failed in mssql_test.go.
minio/minio_test.go
[error] 334-334: Received unexpected error: create container: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
[error] 345-345: Received unexpected error: create container: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
[error] 362-362: Received unexpected error: create container: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
valkey/valkey.go
[error] 67-67: Panic occurred: dial tcp 127.0.0.1:6379: connect: connection refused
🪛 GitHub Actions: Tests DynamoDB
dynamodb/dynamodb_test.go
[error] 162-162: Received unexpected error: operation error DynamoDB: PutItem, https response error StatusCode: 400, RequestID: 2e1849af-f3c3-45c7-9a07-d99ee1d1192c, ResourceNotFoundException.
🔇 Additional comments (137)
sqlite3/sqlite3_test.go (5)
4-4: LGTM: Context import added for new test functions.The context import is correctly added to support the new context-aware test functions.
27-38: LGTM: SetWithContext test implementation.The test correctly verifies context cancellation behavior for the Set operation.
67-82: LGTM: GetWithContext test implementation.The test properly verifies context cancellation behavior and empty result for the Get operation.
128-146: LGTM: DeleteWithContext test implementation.The test effectively verifies context cancellation behavior and data persistence for the Delete operation.
169-191: LGTM: ResetWithContext test implementation.The test thoroughly verifies context cancellation behavior and data persistence for the Reset operation.
sqlite3/sqlite3.go (3)
4-4: LGTM!The context import is correctly added and properly ordered.
92-115: LGTM! GetWithContext implementation is robust.The implementation correctly uses QueryRowContext and handles all edge cases including:
- Empty key validation
- SQL no rows error
- Expiration time check
117-120: LGTM! Original methods correctly delegate to context-aware versions.The implementation maintains backward compatibility while leveraging the new context-aware methods. The use of context.Background() is appropriate for these default methods.
Also applies to: 136-139, 151-154, 162-165
mssql/mssql_test.go (3)
70-85: LGTM! Thorough test coverage.The test effectively verifies both the error condition and the empty result when the context is canceled.
131-149: LGTM! Comprehensive test coverage.The test effectively verifies both the error condition and data persistence when the context is canceled.
172-194: LGTM! Thorough test coverage.The test effectively verifies both the error condition and data persistence for multiple values when the context is canceled.
valkey/valkey_test.go (1)
13-15: Investigate initialization failure in CI pipeline.The pipeline is reporting initialization failures for the test store. This could be due to connection issues with the underlying storage service.
Run the following script to check if the service is properly configured and accessible:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if the storage service is accessible # Note: Using netcat to check port availability echo "Checking default port..." nc -zv localhost 6379 2>&1 echo "Checking TLS port..." nc -zv localhost 6380 2>&1 echo "Checking cluster ports..." for port in {7000..7005}; do nc -zv localhost $port 2>&1 doneLength of output: 1274
Investigate missing connectivity diagnostic tool in CI environment.
- The connectivity check script failed because the
nc(netcat) command is not installed on the CI runner.- Due to this, we cannot verify if the storage service ports (6379, 6380, 7000-7005) are accessible, which might be contributing to the initialization failure of the test store.
- Please consider either installing netcat in the CI environment or using an alternative connectivity check (e.g., using bash’s
/dev/tcpmechanism) to validate service accessibility.🧰 Tools
🪛 GitHub Actions: Benchmark
[error] 13-13: Initialization failed in valkey_test.go due to connection issue.
nats/config.go (1)
25-26: LGTM! Clear and well-documented addition.The new
Resetfield in theConfigstruct is well-documented and maintains backward compatibility with its default value offalse.etcd/etcd.go (3)
35-49: LGTM! Well-implemented context support for Get operation.The implementation correctly handles empty keys and properly uses the context in the etcd operation.
55-72: LGTM! Robust implementation of Set with context.The implementation includes proper validation and correctly uses context for both lease grant and put operations.
78-89: LGTM! Clean implementation of Delete with context.The implementation includes proper validation and correctly uses context in the delete operation.
clickhouse/clickhouse.go (1)
53-73: LGTM! Well-structured Set implementation with context.The implementation includes proper validation and correctly uses context in the database operation.
azureblob/azureblob.go (1)
150-156: LGTM! Well-implemented timeout management.The
requestContexthelper function provides a clean way to handle request timeouts with proper cleanup through cancel functions.redis/redis.go (8)
78-88: LGTM! Well-implemented context support for Get operation.The implementation correctly handles context propagation while preserving the existing error handling and empty key validation.
90-93: LGTM! Proper backward compatibility.The original Get method maintains backward compatibility by delegating to GetWithContext with a background context.
95-101: LGTM! Well-implemented context support for Set operation.The implementation correctly handles context propagation while preserving the existing error handling and empty key/value validation.
103-106: LGTM! Proper backward compatibility.The original Set method maintains backward compatibility by delegating to SetWithContext with a background context.
108-114: LGTM! Well-implemented context support for Delete operation.The implementation correctly handles context propagation while preserving the existing error handling and empty key validation.
116-119: LGTM! Proper backward compatibility.The original Delete method maintains backward compatibility by delegating to DeleteWithContext with a background context.
121-124: LGTM! Well-implemented context support for Reset operation.The implementation correctly handles context propagation.
126-129: LGTM! Proper backward compatibility.The original Reset method maintains backward compatibility by delegating to ResetWithContext with a background context.
rueidis/rueidis.go (8)
88-98: LGTM! Well-implemented context support for Get operation with caching.The implementation correctly handles context propagation while preserving the existing error handling, empty key validation, and cache TTL configuration.
100-103: LGTM! Proper backward compatibility.The original Get method maintains backward compatibility by delegating to GetWithContext with a background context.
105-115: LGTM! Well-implemented context support for Set operation.The implementation correctly handles context propagation while preserving the existing error handling, empty key/value validation, and expiration logic.
117-120: LGTM! Proper backward compatibility.The original Set method maintains backward compatibility by delegating to SetWithContext with a background context.
122-128: LGTM! Well-implemented context support for Delete operation.The implementation correctly handles context propagation while preserving the existing error handling and empty key validation.
130-133: LGTM! Proper backward compatibility.The original Delete method maintains backward compatibility by delegating to DeleteWithContext with a background context.
135-138: LGTM! Well-implemented context support for Reset operation.The implementation correctly handles context propagation.
140-143: LGTM! Proper backward compatibility.The original Reset method maintains backward compatibility by delegating to ResetWithContext with a background context.
valkey/valkey.go (8)
88-98: LGTM! Well-implemented context support for Get operation with caching.The implementation correctly handles context propagation while preserving the existing error handling, empty key validation, and cache TTL configuration.
100-103: LGTM! Proper backward compatibility.The original Get method maintains backward compatibility by delegating to GetWithContext with a background context.
105-115: LGTM! Well-implemented context support for Set operation.The implementation correctly handles context propagation while preserving the existing error handling, empty key/value validation, and expiration logic.
117-120: LGTM! Proper backward compatibility.The original Set method maintains backward compatibility by delegating to SetWithContext with a background context.
122-128: LGTM! Well-implemented context support for Delete operation.The implementation correctly handles context propagation while preserving the existing error handling and empty key validation.
130-133: LGTM! Proper backward compatibility.The original Delete method maintains backward compatibility by delegating to DeleteWithContext with a background context.
135-138: LGTM! Well-implemented context support for Reset operation.The implementation correctly handles context propagation.
140-143: LGTM! Proper backward compatibility.The original Reset method maintains backward compatibility by delegating to ResetWithContext with a background context.
s3/s3_test.go (4)
20-31: LGTM! Well-implemented test for SetWithContext.The test correctly verifies context cancellation behavior for the Set operation.
60-75: LGTM! Well-implemented test for GetWithContext.The test correctly verifies:
- Context cancellation behavior
- Error handling
- Empty result on cancellation
100-118: LGTM! Well-implemented test for DeleteWithContext.The test correctly verifies:
- Context cancellation behavior
- Error handling
- Data persistence on cancellation
141-163: LGTM! Well-implemented test for ResetWithContext.The test correctly verifies:
- Context cancellation behavior
- Error handling
- Data persistence on cancellation
etcd/etcd_test.go (4)
53-64: LGTM! Well-structured test for context cancellation.The test properly verifies that
SetWithContexthandles context cancellation correctly.
66-81: LGTM! Comprehensive test for GetWithContext.The test effectively verifies both the error condition and the nil result when context is canceled.
106-119: LGTM! Thorough test for DeleteWithContext.The test properly verifies that the operation is canceled and the data remains unchanged.
132-145: LGTM! Complete test for ResetWithContext.The test effectively verifies context cancellation and data preservation.
cloudflarekv/cloudflarekv.go (3)
58-71: LGTM! Clean implementation of context-aware Get methods.Good pattern of implementing
GetWithContextand havingGetdelegate to it with a background context.
73-90: LGTM! Well-structured context-aware Set methods.Good implementation of
SetWithContextand delegation pattern inSet.
92-108: LGTM! Proper implementation of context-aware Delete methods.Good pattern of implementing
DeleteWithContextand havingDeletedelegate to it.cloudflarekv/cloudflarekv_test.go (1)
92-103: LGTM! Comprehensive context cancellation tests.The tests for
SetWithContext,DeleteWithContext, andResetWithContextare well-structured and verify both the cancellation error and data preservation.Also applies to: 122-140, 152-174
dynamodb/dynamodb_test.go (1)
41-52: LGTM! Well-structured context cancellation tests.The tests for context-aware methods are comprehensive and verify both cancellation errors and data preservation.
Also applies to: 81-92, 117-135, 158-180
minio/minio.go (1)
65-86: Well-implemented context management support!The implementation follows best practices:
- Each operation has a WithContext variant
- Original methods delegate to WithContext versions with background context
- Context is properly propagated to underlying MinIO operations
- Error handling is consistent across all methods
Also applies to: 88-91, 93-111, 113-116, 118-128, 130-133, 135-160, 162-164
mongodb/mongodb_test.go (1)
35-46: Excellent test coverage for context-aware operations!The tests thoroughly verify:
- Context cancellation handling
- Error propagation
- Data integrity after failed operations
Also applies to: 75-90, 136-154, 177-199
scylladb/scylladb_test.go (1)
23-34: Well-structured tests maintaining consistency across storage implementations!The tests follow the same robust pattern as other storage implementations, ensuring:
- Proper context cancellation handling
- Error type verification
- Data consistency checks
Also applies to: 72-87, 130-148, 171-193
arangodb/arangodb_test.go (1)
25-36: Well-implemented tests following the established pattern!The tests maintain consistency with other storage implementations, providing thorough coverage of:
- Context cancellation scenarios
- Error propagation
- Data integrity verification
Also applies to: 65-80, 126-144, 167-189
azureblob/azureblob_test.go (4)
46-61: LGTM! Well-structured test for context cancellation.The test effectively validates the behavior of
GetWithContextwhen the context is canceled, ensuring proper error propagation.
73-84: LGTM! Well-structured test for context cancellation.The test effectively validates the behavior of
SetWithContextwhen the context is canceled, ensuring proper error propagation.
108-126: LGTM! Comprehensive test for context cancellation.The test effectively validates the behavior of
DeleteWithContextwhen the context is canceled, including verification that the data remains intact after the failed deletion.
183-205: LGTM! Comprehensive test for context cancellation.The test effectively validates the behavior of
ResetWithContextwhen the context is canceled, including verification that all data remains intact after the failed reset.postgres/postgres.go (4)
100-124: LGTM! Well-implemented context support.The function correctly uses
QueryRowContextand handles all edge cases appropriately.
143-155: LGTM! Well-implemented context support.The function correctly uses
ExecContextand handles all edge cases appropriately.
162-170: LGTM! Well-implemented context support.The function correctly uses
ExecContextand handles all edge cases appropriately.
177-181: LGTM! Well-implemented context support.The function correctly uses
ExecContextfor the reset operation.mysql/mysql.go (4)
109-136: LGTM! Well-implemented context support.The function correctly uses
QueryRowContextand handles all edge cases appropriately.
143-155: LGTM! Well-implemented context support.The function correctly uses
ExecContextand handles all edge cases appropriately.
162-170: LGTM! Well-implemented context support.The function correctly uses
ExecContextand handles all edge cases appropriately.
177-181: LGTM! Well-implemented context support.The function correctly uses
ExecContextfor the reset operation.scylladb/scylladb.go (4)
137-147: LGTM! Well-implemented context support.The function correctly uses
WithContextand handles all edge cases appropriately.
154-161: LGTM! Well-implemented context support.The function correctly uses
WithContextand handles expiration appropriately.
168-171: LGTM! Well-implemented context support.The function correctly uses
WithContextfor the delete operation.
178-181: LGTM! Well-implemented context support.The function correctly uses
WithContextfor the reset operation.mongodb/mongodb.go (8)
124-150: LGTM!The implementation correctly uses the context parameter in the FindOne operation and handles errors appropriately.
152-155: LGTM!The function maintains backward compatibility by delegating to GetWithContext with a background context.
157-179: LGTM!The implementation correctly uses the context parameter in the ReplaceOne operation and properly handles upsert functionality.
181-187: LGTM!The function maintains backward compatibility by delegating to SetWithContext with a background context.
189-197: LGTM!The implementation correctly uses the context parameter in the DeleteOne operation and handles empty key checks.
199-202: LGTM!The function maintains backward compatibility by delegating to DeleteWithContext with a background context.
204-207: LGTM!The implementation correctly uses the context parameter in the Drop operation.
209-212: LGTM!The function maintains backward compatibility by delegating to ResetWithContext with a background context.
postgres/postgres_test.go (4)
30-41: LGTM!The test correctly verifies that SetWithContext handles context cancellation.
70-85: LGTM!The test correctly verifies that GetWithContext handles context cancellation and returns appropriate results.
131-145: LGTM!The test correctly verifies that DeleteWithContext handles context cancellation.
168-190: LGTM!The test correctly verifies that ResetWithContext handles context cancellation and maintains data integrity.
cloudflarekv/test_module.go (5)
57-57: LGTM!The function correctly uses NewRequestWithContext for context-aware HTTP request creation.
98-98: LGTM!The function correctly uses NewRequestWithContext for context-aware HTTP request creation.
137-137: LGTM!The function correctly uses NewRequestWithContext for context-aware HTTP request creation.
176-176: LGTM!The function correctly uses NewRequestWithContext for context-aware HTTP request creation.
228-228: LGTM!The function correctly uses NewRequestWithContext for context-aware HTTP request creation.
s3/s3.go (8)
81-100: LGTM!The implementation correctly uses the context parameter in the Download operation and handles errors appropriately.
102-108: LGTM!The function maintains backward compatibility by delegating to GetWithContext with a timeout-aware context.
110-123: LGTM!The implementation correctly uses the context parameter in the Upload operation and handles empty key checks.
125-131: LGTM!The function maintains backward compatibility by delegating to SetWithContext with a timeout-aware context.
133-145: LGTM!The implementation correctly uses the context parameter in the DeleteObject operation and handles empty key checks.
147-153: LGTM!The function maintains backward compatibility by delegating to DeleteWithContext with a timeout-aware context.
155-186: LGTM!The implementation correctly uses the context parameter in both ListObjectsV2Paginator and DeleteObjects operations, with proper pagination handling.
188-194: LGTM!The function maintains backward compatibility by delegating to ResetWithContext with a timeout-aware context.
mysql/mysql_test.go (4)
61-72: LGTM! Well-structured context cancellation test.The test correctly verifies that SetWithContext fails with context.Canceled when the context is canceled before the operation.
70-89: LGTM! Comprehensive context cancellation test.The test thoroughly verifies GetWithContext behavior by:
- Setting up test data
- Verifying context.Canceled error
- Ensuring empty result on cancellation
134-156: LGTM! Thorough deletion verification with context.The test effectively verifies that:
- DeleteWithContext fails with context.Canceled
- The data remains intact after the failed deletion
178-204: LGTM! Comprehensive reset verification with context.The test effectively verifies that:
- ResetWithContext fails with context.Canceled
- Multiple keys remain intact after the failed reset
arangodb/arangodb.go (4)
119-150: LGTM! Well-implemented context-aware Get operation.The implementation correctly:
- Uses context for all database operations
- Handles document existence check
- Manages expiration logic
158-191: LGTM! Robust context-aware Set operation.The implementation correctly:
- Uses context for all database operations
- Handles document existence and updates
- Manages expiration time calculation
199-206: LGTM! Clean context-aware Delete operation.The implementation correctly:
- Uses context for the database operation
- Includes proper input validation
214-216: LGTM! Concise context-aware Reset operation.The implementation correctly uses context for the truncate operation.
dynamodb/dynamodb.go (3)
80-105: LGTM! Well-implemented context-aware Get operation.The implementation correctly:
- Uses context for DynamoDB operations
- Handles ResourceNotFoundException
- Unmarshals the result properly
112-132: LGTM! Robust context-aware Set operation.The implementation correctly:
- Uses context for DynamoDB operations
- Includes input validation
- Constructs DynamoDB attributes properly
139-156: LGTM! Clean context-aware Delete operation.The implementation correctly:
- Uses context for DynamoDB operations
- Includes input validation
- Constructs delete request properly
rueidis/rueidis_test.go (4)
27-42: LGTM! Well-structured context cancellation test.The test correctly verifies that SetWithContext fails with context.Canceled when the context is canceled before the operation.
70-89: LGTM! Comprehensive context cancellation test.The test thoroughly verifies GetWithContext behavior by:
- Setting up test data
- Verifying context.Canceled error
- Ensuring empty result on cancellation
134-156: LGTM! Thorough deletion verification with context.The test effectively verifies that:
- DeleteWithContext fails with context.Canceled
- The data remains intact after the failed deletion
178-204: LGTM! Comprehensive reset verification with context.The test effectively verifies that:
- ResetWithContext fails with context.Canceled
- Multiple keys remain intact after the failed reset
clickhouse/clickhouse_test.go (4)
79-93: LGTM! Test verifies context cancellation for SetWithContext.The test correctly verifies that a canceled context prevents the operation and returns the expected error.
121-139: LGTM! Test verifies context cancellation for GetWithContext.The test correctly verifies that a canceled context prevents the operation, returns the expected error, and ensures no value is returned.
186-208: LGTM! Test verifies context cancellation for DeleteWithContext.The test correctly verifies that a canceled context prevents the operation, returns the expected error, and ensures the value remains accessible.
232-254: LGTM! Test verifies context cancellation for ResetWithContext.The test correctly verifies that a canceled context prevents the operation, returns the expected error, and ensures values remain accessible.
nats/nats.go (5)
37-50: LGTM! Initialization uses background context.The function correctly uses a background context for initialization, ensuring proper setup of the NATS key-value store.
169-204: LGTM! Get operations properly handle context.The implementation correctly:
- Adds context support through GetWithContext
- Maintains backward compatibility through Get
- Includes proper error handling
207-249: LGTM! Set operations properly handle context.The implementation correctly:
- Adds context support through SetWithContext
- Maintains backward compatibility through Set
- Includes proper error handling
252-271: LGTM! Delete operations properly handle context.The implementation correctly:
- Adds context support through DeleteWithContext
- Maintains backward compatibility through Delete
- Includes proper error handling
274-306: LGTM! Reset operations properly handle context.The implementation correctly:
- Adds context support through ResetWithContext
- Maintains backward compatibility through Reset
- Includes proper error handling
minio/minio_test.go (4)
80-98: LGTM! Test verifies context cancellation for GetWithContext.The test correctly verifies that a canceled context prevents the operation and returns the expected error.
156-170: LGTM! Test verifies context cancellation for SetWithContext.The test correctly verifies that a canceled context prevents the operation and returns the expected error.
221-242: LGTM! Test verifies context cancellation for DeleteWithContext.The test correctly verifies that a canceled context prevents the operation and returns the expected error.
297-320: LGTM! Test verifies context cancellation for ResetWithContext.The test correctly verifies that a canceled context prevents the operation and returns the expected error.
nats/nats_test.go (4)
44-61: LGTM! Test verifies context cancellation for SetWithContext.The test correctly verifies that a canceled context prevents the operation and returns the expected error.
106-127: LGTM! Test verifies context cancellation for GetWithContext.The test correctly verifies that a canceled context prevents the operation and returns the expected error.
224-250: LGTM! Test verifies context cancellation for DeleteWithContext.The test correctly verifies that a canceled context prevents the operation and returns the expected error.
283-315: LGTM! Test verifies context cancellation for ResetWithContext.The test correctly verifies that a canceled context prevents the operation and returns the expected error.
redis/redis_test.go (4)
26-40: LGTM! Test correctly verifies context cancellation for Set operation.The test properly validates that a canceled context prevents the Set operation and returns the expected context.Canceled error.
83-101: LGTM! Test correctly verifies context cancellation for Get operation.The test effectively:
- Sets up test data
- Verifies that a canceled context prevents retrieval
- Confirms zero-length result on failure
160-181: LGTM! Test correctly verifies context cancellation for Delete operation.The test properly:
- Sets up test data
- Verifies that a canceled context prevents deletion
- Confirms data persistence after failed deletion
215-244: LGTM! Test correctly verifies context cancellation for Reset operation.The test effectively:
- Sets up multiple test entries
- Verifies that a canceled context prevents reset
- Confirms data persistence after failed reset
arangodb/README.md (1)
26-33: LGTM! Documentation properly reflects the new context-aware methods.The signatures section correctly documents all new context-aware methods while maintaining the existing method documentation.
|
think it would make sense to put everything in a new branch for v3 and upgrade everyone to the latest version of the dependencies and only support the latest go version
|
Agree with this plan. We should also make note that we will only support the latest 2 versions of Go going forward. |
|
i just created v3 branch |
|
I had a short chat with @efectn since it is just an extension of the interface and no breaking changes, we leave the target on main and do the rest testcontainers, version update, dependencie update in a followup task this makes sense because the storages can also be used independently of the fiber web framework @coderabbitai please create a cleanup task as a new issue so that we do not forget it
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ReneWerner87 I've created a new issue tracking the cleanup tasks as requested. The issue includes all the specified tasks along with proper context about its relationship to the current PR and future Fiber v3 integration. Let me know if you'd like any adjustments to the issue. 🏁 Script executed: Length of output: 988 @ReneWerner87 I've created issue #1570 tracking all the cleanup tasks. The issue has been assigned to you and includes all the specified tasks along with proper context. Let me know if you need any adjustments to the issue. ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
couchbase/couchbase.go (1)
114-116: Consider adding context support to Close methodFor consistency with the other methods, consider adding a
CloseWithContextmethod that accepts a context parameter, and refactor the existingClosemethod to use it with a background context.+ func (s *Storage) CloseWithContext(ctx context.Context) error { + return s.cb.Close(&gocb.ClusterCloseOptions{ + Context: ctx, + }) + } func (s *Storage) Close() error { - return s.cb.Close(nil) + return s.CloseWithContext(context.Background()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
couchbase/couchbase.go(3 hunks)couchbase/couchbase_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Tests (1.20.x, 7.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.22.x)
- GitHub Check: Tests (1.20.x)
- GitHub Check: Tests (1.20.x)
- GitHub Check: Tests (1.22.x)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.22.x)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.19.x)
- GitHub Check: Tests (1.19.x)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.20.x)
- GitHub Check: Tests (1.21.x)
🔇 Additional comments (10)
couchbase/couchbase_test.go (1)
65-75: Good addition of context cancellation testThis test validates that the
SetWithContextmethod properly handles context cancellation, which is essential for ensuring the reliability of context-aware operations. The test creates a context with a very short timeout to simulate context cancellation and verifies that an error is returned.couchbase/couchbase.go (9)
4-4: Correctly added context importThe addition of the context package import is necessary for implementing the context-aware methods.
47-70: Well-implemented GetWithContext methodThe
GetWithContextimplementation correctly integrates context support with Couchbase operations. It preserves the original error handling logic while allowing timeout and cancellation control through the provided context.
72-74: Good refactoring of Get methodThe
Getmethod has been properly refactored to callGetWithContextwith a background context, maintaining backward compatibility while leveraging the new context-aware implementation.
76-85: Well-implemented SetWithContext methodThe
SetWithContextimplementation correctly passes the context to the Couchbase operation. This allows for better control over timeouts and cancellations during write operations.
87-89: Good refactoring of Set methodThe
Setmethod has been cleanly refactored to callSetWithContextwith a background context, maintaining backward compatibility.
91-98: Well-implemented DeleteWithContext methodThe
DeleteWithContextimplementation correctly incorporates context support for delete operations, allowing for better control over timeouts and cancellations.
100-102: Good refactoring of Delete methodThe
Deletemethod has been effectively refactored to useDeleteWithContextwith a background context, maintaining backward compatibility.
104-108: Well-implemented ResetWithContext methodThe
ResetWithContextimplementation correctly passes the context to the bucket flush operation, enabling timeout and cancellation control for this potentially long-running operation.
110-112: Good refactoring of Reset methodThe
Resetmethod has been properly refactored to callResetWithContextwith a background context, maintaining backward compatibility.
|
Hi @mdelapenya. When i run couchbase tests with testcontainers-go, a timeout error raises after 30 seconds, especially with context tests; however, when i run tests with couchbase image on docker, everything works well. Do you have an idea why? I also run into the same issue for neo4j storage. Thanks |
|
@mdelapenya can you support here , do you know the reason ? |
|
Sorry, I read the ping today 🤦 When we merged the couchbase module, we needed to increase the max timeout of the go test command to 600s. I can imagine the differences running couchbase with plain docker come from the lack of a consistent wait strategy: if you just "docker run" the couchbase image, does it have a healthcheck to wait for it? In TC we wait for the cluster to be ready (see https://github.com/testcontainers/testcontainers-go/blob/3298e9a4d9c48615715f892e8e6e5505384270aa/modules/couchbase/couchbase.go#L189) I'd suggest increasing the go test timeout to 300s (in the benchmarks it was higher because of the load, my guess). Please increase that test timeout in your IDE settings too (I do it in my VSCode/Cursor) |
|
@efectn is it fine if I add some commits on top of yours updating the tests? |
Yes of course, feel free to play. |
|
@efectn I think there are no more compile-error, so the failures seem related to the changes in this PR. See https://github.com/gofiber/storage/actions/runs/14128624646/job/39583343922?pr=1569#step:4:53 |
There was a problem hiding this comment.
Actionable comments posted: 26
🔭 Outside diff range comments (2)
postgres/postgres.go (2)
132-156:⚠️ Potential issueFix: Use QueryRowContext instead of QueryRow for context support.
The method is using
QueryRowwhich doesn't respect the passed context. You should useQueryRowContextinstead to properly support context cancellation and timeout.- row := s.db.QueryRow(ctx, s.sqlSelect, key) + row := s.db.QueryRowContext(ctx, s.sqlSelect, key)
163-175:⚠️ Potential issueFix: Use ExecContext instead of Exec for context support.
The method is using
Execwhich doesn't respect the passed context. You should useExecContextinstead to properly support context cancellation and timeout.- _, err := s.db.Exec(ctx, s.sqlInsert, key, val, expSeconds, val, expSeconds) + _, err := s.db.ExecContext(ctx, s.sqlInsert, key, val, expSeconds, val, expSeconds)
♻️ Duplicate comments (6)
mysql/mysql_test.go (3)
143-162: Fix inconsistency between function signature and usageThe same issue with
newTestStoreappears here. The function signature and usage are inconsistent.
224-246: Fix inconsistency between function signature and usageThe same issue with
newTestStoreappears here. The function signature and usage are inconsistent.
272-298: Fix inconsistency between function signature and usageThe same issue with
newTestStoreappears here. The function signature and usage are inconsistent.mongodb/mongodb_test.go (3)
119-138: Fix inconsistency between function signature and usageThe same issue with
newTestStoreappears here. The function signature and usage are inconsistent.
200-222: Fix inconsistency between function signature and usageThe same issue with
newTestStoreappears here. The function signature and usage are inconsistent.
248-274: Fix inconsistency between function signature and usageThe same issue with
newTestStoreappears here. The function signature and usage are inconsistent.
🧹 Nitpick comments (11)
rueidis/rueidis_test.go (4)
27-41: Good test for context cancellation in SetWithContext.This test correctly verifies that SetWithContext returns a context.Canceled error when called with a canceled context.
Note that this test creates a new test store instance instead of using the global
testStorevariable. While this is good for test isolation, it's inconsistent with other tests in this file. Consider standardizing the approach across all tests in this file for better maintainability.
70-88: Good test for context cancellation in GetWithContext.This test correctly verifies that GetWithContext returns a context.Canceled error when called with a canceled context and confirms the result is empty.
As with the other new tests, this creates a separate store instance rather than using the global
testStore.
134-155: Good test for context cancellation in DeleteWithContext.This test correctly verifies that DeleteWithContext returns a context.Canceled error when called with a canceled context and confirms the data remains unchanged.
This also creates a separate store instance rather than using the global
testStore.
178-203: Good test for context cancellation in ResetWithContext.This test correctly verifies that ResetWithContext returns a context.Canceled error when called with a canceled context and confirms the data remains intact.
This test creates a separate store instance rather than using the global
testStore, which is inconsistent with other tests in this file.cloudflarekv/cloudflarekv_test.go (1)
69-72: Remove debugging statement and make error checking consistentThere's a debugging
fmt.Println(err)statement that should be removed. Also, for consistency with other storage adapters, consider usingrequire.ErrorIs(t, err, context.Canceled)instead ofrequire.ErrorContains.- fmt.Println(err) - require.ErrorContains(t, err, context.Canceled.Error()) + require.ErrorIs(t, err, context.Canceled)arangodb/arangodb_test.go (1)
174-174: Inconsistent error checking style.This line uses a different style of error checking compared to the rest of the file. For consistency, use
require.NoError(t, err)instead.- require.Equal(t, err, nil) + require.NoError(t, err)redis/redis.go (1)
142-168: Consider adding context support for the Keys methodWhile all other operations have been updated with context support, the Keys method still uses a hardcoded background context in the Scan operation on line 150. For consistency, consider adding a KeysWithContext method and updating this implementation.
+ // KeysWithContext returns all the keys with context + func (s *Storage) KeysWithContext(ctx context.Context) ([][]byte, error) { + var keys [][]byte + var cursor uint64 + var err error + + for { + var batch []string + + if batch, cursor, err = s.db.Scan(ctx, cursor, "*", 10).Result(); err != nil { + return nil, err + } + + for _, key := range batch { + keys = append(keys, []byte(key)) + } + + if cursor == 0 { + break + } + } + + if len(keys) == 0 { + return nil, nil + } + + return keys, nil + } + + // Keys returns all the keys + func (s *Storage) Keys() ([][]byte, error) { + return s.KeysWithContext(context.Background()) + }minio/minio.go (1)
176-193: Consider adding WithContext variants for bucket operations.For consistency with the other methods, consider adding
CheckBucketWithContext,CreateBucketWithContext, andRemoveBucketWithContextmethods that accept a context parameter, making the bucket operations also context-aware.nats/nats.go (2)
44-44: Consider using a passed context for this operation.The connectHandler still uses
context.Background()when callingnewNatsKV. For complete context handling, consider passing a context parameter to this method as well.
332-332: Consider adding KeysWithContext method.For consistency with other methods, consider adding a
KeysWithContextmethod that accepts a context parameter, and make the currentKeysmethod delegate to it with a background context.+// KeysWithContext returns all the keys with context +func (s *Storage) KeysWithContext(ctx context.Context) ([]string, error) { + s.mu.RLock() + kv := s.kv + s.mu.RUnlock() + if kv == nil { + return nil, fmt.Errorf("kv not initialized: %v", s.err) + } + + keyLister, err := kv.ListKeys(ctx) + + if err != nil { + return nil, fmt.Errorf("keys: %w", err) + } + + var keys []string + for key := range keyLister.Keys() { + keys = append(keys, key) + } + _ = keyLister.Stop() + + // Double check if no valid keys were found + if len(keys) == 0 { + return nil, nil + } + + return keys, nil +} + // Return all the keys func (s *Storage) Keys() ([]string, error) { - s.mu.RLock() - kv := s.kv - s.mu.RUnlock() - if kv == nil { - return nil, fmt.Errorf("kv not initialized: %v", s.err) - } - - keyLister, err := kv.ListKeys(context.Background()) - - if err != nil { - return nil, fmt.Errorf("keys: %w", err) - } - - var keys []string - for key := range keyLister.Keys() { - keys = append(keys, key) - } - _ = keyLister.Stop() - - // Double check if no valid keys were found - if len(keys) == 0 { - return nil, nil - } - - return keys, nil + return s.KeysWithContext(context.Background()) }mssql/mssql.go (1)
241-244: Consider adding context support to gc method.For consistency with the other database operations, consider making the
gcmethod context-aware as well. This would allow for proper cancellation and timeout handling during garbage collection.- func (s *Storage) gc(t time.Time) { - _, _ = s.db.Exec(s.sqlGC, t.Unix()) - } + func (s *Storage) gc(t time.Time) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + _, _ = s.db.ExecContext(ctx, s.sqlGC, t.Unix()) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
arangodb/README.md(1 hunks)arangodb/arangodb.go(3 hunks)arangodb/arangodb_test.go(5 hunks)azureblob/azureblob.go(2 hunks)azureblob/azureblob_test.go(4 hunks)clickhouse/clickhouse.go(4 hunks)clickhouse/clickhouse_test.go(4 hunks)cloudflarekv/cloudflarekv.go(6 hunks)cloudflarekv/cloudflarekv_test.go(4 hunks)cloudflarekv/test_module.go(5 hunks)coherence/coherence.go(3 hunks)coherence/coherence_test.go(2 hunks)couchbase/couchbase.go(3 hunks)couchbase/couchbase_test.go(1 hunks)dynamodb/dynamodb.go(5 hunks)dynamodb/dynamodb_test.go(4 hunks)etcd/etcd.go(3 hunks)etcd/etcd_test.go(4 hunks)minio/minio.go(6 hunks)minio/minio_test.go(4 hunks)mongodb/mongodb.go(3 hunks)mongodb/mongodb_test.go(4 hunks)mssql/mssql.go(4 hunks)mssql/mssql_test.go(5 hunks)mysql/mysql.go(4 hunks)mysql/mysql_test.go(4 hunks)nats/config.go(1 hunks)nats/nats.go(10 hunks)nats/nats_test.go(5 hunks)neo4j/neo4j.go(3 hunks)neo4j/neo4j_test.go(1 hunks)postgres/postgres.go(3 hunks)postgres/postgres_test.go(4 hunks)redis/redis.go(1 hunks)redis/redis_test.go(5 hunks)rueidis/rueidis.go(1 hunks)rueidis/rueidis_test.go(5 hunks)s3/s3.go(5 hunks)s3/s3_test.go(5 hunks)scylladb/scylladb.go(3 hunks)scylladb/scylladb_test.go(4 hunks)sqlite3/sqlite3.go(4 hunks)sqlite3/sqlite3_test.go(5 hunks)valkey/valkey.go(1 hunks)valkey/valkey_test.go(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
minio/minio_test.go (1)
clickhouse/clickhouse_test.go (4)
Test_GetWithContext(126-144)Test_SetWithContext(86-100)Test_DeleteWithContext(189-211)Test_ResetWithContext(234-256)
clickhouse/clickhouse_test.go (2)
minio/minio_test.go (4)
Test_SetWithContext(159-172)Test_GetWithContext(84-101)Test_DeleteWithContext(222-242)Test_ResetWithContext(296-318)clickhouse/config.go (2)
Config(29-52)Memory(21-21)
cloudflarekv/cloudflarekv_test.go (2)
cloudflarekv/cloudflarekv.go (2)
Storage(19-24)New(26-56)cloudflarekv/config.go (1)
Config(4-30)
valkey/valkey.go (1)
rueidis/rueidis.go (1)
Storage(13-15)
rueidis/rueidis.go (1)
valkey/valkey.go (1)
Storage(13-15)
🪛 GitHub Actions: Tests Coherence
coherence/coherence_test.go
[error] 77-77: Test failure in Test_Coherence_SetContext_And_Get: assertion failed - Should be true.
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: compare (rueidis)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.19.x)
- GitHub Check: Tests (1.21.x, 7.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.19.x)
- GitHub Check: Tests (1.20.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.20.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.23.x, 7.x)
- GitHub Check: Tests (1.23.x, 8.x)
🔇 Additional comments (224)
neo4j/neo4j_test.go (1)
82-93: LGTM! Good context timeout testThis test appropriately verifies that the
SetWithContextmethod handles context timeout correctly by checking for the expectedcontext.DeadlineExceedederror.cloudflarekv/test_module.go (5)
57-57: LGTM! Proper context propagationGood change to use
http.NewRequestWithContextinstead ofhttp.NewRequest, ensuring that context cancellation and timeout signals are properly propagated to the HTTP request.
98-98: LGTM! Proper context propagationGood change to use
http.NewRequestWithContextinstead ofhttp.NewRequest, ensuring that context cancellation and timeout signals are properly propagated to the HTTP request.
137-137: LGTM! Proper context propagationGood change to use
http.NewRequestWithContextinstead ofhttp.NewRequest, ensuring that context cancellation and timeout signals are properly propagated to the HTTP request.
176-176: LGTM! Proper context propagationGood change to use
http.NewRequestWithContextinstead ofhttp.NewRequest, ensuring that context cancellation and timeout signals are properly propagated to the HTTP request.
228-228: LGTM! Proper context propagationGood change to use
http.NewRequestWithContextinstead ofhttp.NewRequest, ensuring that context cancellation and timeout signals are properly propagated to the HTTP request.coherence/coherence_test.go (1)
71-72: Good use of context.DeadlineExceeded error verificationThe test correctly checks that the operation returns the timeout error.
arangodb/README.md (1)
24-31: Clear documentation of new context-aware methodsThe documentation properly reflects the addition of context-aware variants to the API. This enhances the Arangodb storage driver by allowing callers to control operation lifecycle through contexts.
valkey/valkey_test.go (4)
27-38: Well-structured test for context cancellationThis test effectively verifies that
SetWithContextproperly handles a canceled context by returning the appropriate error.
67-82: Properly validates GetWithContext cancellation behaviorThe test correctly verifies both the error returned and that no data is returned when the context is canceled.
128-146: Well-implemented test for DeleteWithContext cancellationThis test correctly verifies that a canceled context prevents deletion and preserves the original data.
169-191: Comprehensive test for ResetWithContext cancellationThe test thoroughly verifies that when the context is canceled:
- ResetWithContext returns context.Canceled error
- No data is lost (both test keys remain intact)
This ensures proper context handling in storage operations.
s3/s3_test.go (1)
23-34: Good implementation of context cancellation testThe test properly creates a canceled context and verifies that
SetWithContextreturns the expected error.mssql/mssql_test.go (5)
4-4: Added context import for new context-aware tests.The addition of the context import is necessary for the new context-aware tests added in this file.
30-41: Good test for context cancellation in SetWithContext.This test correctly verifies that SetWithContext returns a context.Canceled error when called with a canceled context, ensuring proper context propagation.
70-85: Good test for context cancellation in GetWithContext.This test appropriately sets up a value, then verifies GetWithContext fails with context.Canceled error when called with a canceled context. The additional validation of an empty result is a good practice.
131-149: Good test for context cancellation in DeleteWithContext.This test correctly verifies that DeleteWithContext returns a context.Canceled error with a canceled context and confirms the data remains unchanged, which is the expected behavior.
172-194: Good test for context cancellation in ResetWithContext.This test correctly verifies that ResetWithContext returns a context.Canceled error with a canceled context and confirms the data remains intact, providing thorough validation of the context-aware reset functionality.
minio/minio_test.go (3)
84-101: Good test for context cancellation in GetWithContext.This test correctly verifies that GetWithContext returns a context.Canceled error when called with a canceled context. It properly sets up test data beforehand and verifies the result is empty.
159-172: Good test for context cancellation in SetWithContext.This test correctly verifies that SetWithContext returns a context.Canceled error when called with a canceled context.
222-242: Good test for context cancellation in DeleteWithContext.This test properly verifies that DeleteWithContext returns a context.Canceled error when called with a canceled context, and it confirms that the data remains unchanged after the operation fails.
rueidis/rueidis_test.go (1)
4-4: Added context import for new context-aware tests.The addition of the context import is necessary for the new context-aware tests added in this file.
nats/nats_test.go (5)
144-144: Good addition of the Reset option.Adding
Reset: trueto the config ensures that each test starts with a clean state, which improves test isolation and reliability.
165-182: Well-implemented context cancellation test for SetWithContext.This test correctly verifies that the SetWithContext method respects context cancellation by:
- Creating a canceled context
- Verifying the method returns context.Canceled
- Ensuring no state changes occurred in the storage
The test structure is clean and follows best practices.
229-251: Great validation of GetWithContext context handling.The test properly verifies that:
- GetWithContext respects context cancellation
- It returns the expected context.Canceled error
- No data is returned when context is canceled
- The original data remains intact
This ensures proper context propagation throughout the operation.
351-378: Thorough test for DeleteWithContext with canceled context.This test correctly validates the DeleteWithContext method's behavior when the context is canceled:
- Data is set up properly before the test
- Context cancellation is correctly propagated
- The method returns context.Canceled as expected
- The stored data remains unchanged, verifying no partial operations occurred
Well structured and comprehensive.
412-445: Complete test coverage for ResetWithContext.This test thoroughly validates the context cancellation behavior for the ResetWithContext method:
- Proper test setup with multiple keys
- Verification that operation is aborted on context cancellation
- Confirmation that all data remains intact
- Proper error type validation
The test structure is consistent with other context-aware method tests, which is excellent for maintainability.
etcd/etcd_test.go (4)
53-64: Well-implemented context cancellation test for SetWithContext.This test correctly verifies that the SetWithContext method respects context cancellation by returning the appropriate context.Canceled error.
66-81: Good test for GetWithContext with canceled context.The test properly validates that GetWithContext:
- Returns context.Canceled when the context is canceled
- Returns nil result when the operation is canceled
- Does not affect the underlying data structure
This ensures consistent behavior across context-aware operations.
106-119: Thorough test for DeleteWithContext context handling.This test correctly verifies that:
- DeleteWithContext respects context cancellation
- The method returns context.Canceled error
- The data remains unchanged after the canceled operation
Well-structured test that ensures proper context propagation.
132-145: Complete test coverage for ResetWithContext cancellation.This test accurately validates that:
- ResetWithContext returns context.Canceled when the context is canceled
- The storage data remains intact after a canceled reset operation
- Context cancellation is properly propagated throughout the operation
The test is consistent with other context-aware method tests in the codebase.
redis/redis_test.go (4)
26-40: Well-implemented test for SetWithContext with canceled context.This test correctly verifies that SetWithContext respects context cancellation by returning context.Canceled error. The test follows the pattern established across other storage implementations, ensuring consistent behavior.
83-101: Good validation of GetWithContext context handling.The test properly verifies that:
- GetWithContext returns context.Canceled when the context is canceled
- No data is returned when operation is canceled
- Original data remains accessible through non-canceled operations
This ensures proper context propagation throughout Redis operations.
160-181: Thorough test for DeleteWithContext with canceled context.This test correctly validates the DeleteWithContext method's behavior with a canceled context:
- Data is set up properly before the test
- Context cancellation properly aborts the operation
- The appropriate error is returned
- The original data remains intact
Well-structured and consistent with other context-aware tests.
215-244: Complete test coverage for ResetWithContext cancellation.This test thoroughly validates the ResetWithContext method's behavior when the context is canceled:
- Multiple keys are set up to verify reset behavior
- Context cancellation is correctly handled
- Method returns the expected context.Canceled error
- All data remains unchanged after the canceled operation
The consistent test structure across all context-aware methods enhances readability and maintainability.
sqlite3/sqlite3_test.go (4)
27-38: Well-implemented test for SetWithContext with canceled context.This test correctly verifies that the SetWithContext method respects context cancellation by returning the appropriate context.Canceled error.
67-82: Good validation of GetWithContext context handling.The test properly verifies that:
- GetWithContext returns context.Canceled when the context is canceled
- No data is returned when the operation is canceled
- Context cancellation is properly propagated
This ensures consistent behavior with other storage implementations.
128-146: Thorough test for DeleteWithContext with canceled context.The test correctly validates that:
- DeleteWithContext respects context cancellation
- The method returns context.Canceled error
- The data remains unchanged after the canceled operation
Well-structured test that ensures proper context propagation.
169-191: Complete test coverage for ResetWithContext cancellation.This test thoroughly validates the ResetWithContext method's behavior when the context is canceled:
- Data is set up correctly before testing
- Context cancellation is properly handled
- The expected error is returned
- Storage state remains unchanged after the canceled operation
The test maintains consistency with other context-aware method tests, which is excellent for code maintainability.
azureblob/azureblob_test.go (4)
66-84: Well-implemented context cancellation test for GetWithContextThe test correctly verifies that the GetWithContext method properly handles context cancellation by returning context.Canceled and preserving data integrity.
99-113: Good implementation of SetWithContext cancellation testThe test correctly verifies that SetWithContext returns context.Canceled when provided with a canceled context.
140-161: Well-implemented DeleteWithContext cancellation testThe test effectively verifies two important aspects:
- The DeleteWithContext method properly propagates context cancellation by returning context.Canceled
- The operation doesn't actually delete the data when the context is canceled, maintaining data integrity
This ensures proper context propagation without side effects.
234-259: Well-structured ResetWithContext cancellation testThe test thoroughly verifies both the error handling and data preservation aspects of ResetWithContext when context is canceled. It correctly checks that both test keys remain intact after the operation fails due to cancellation.
cloudflarekv/cloudflarekv_test.go (4)
13-26: Good refactoring with global testStoreMoving to a global testStore in TestMain with Reset enabled is a good approach that reduces redundancy in test setup across multiple test functions.
92-103: Well-implemented SetWithContext cancellation testThe test correctly verifies that SetWithContext returns context.Canceled when provided with a canceled context.
122-140: Well-implemented DeleteWithContext cancellation testThe test properly verifies that DeleteWithContext returns context.Canceled when the context is canceled, and confirms that the data remains intact.
152-174: Well-structured ResetWithContext cancellation testThe test thoroughly verifies both error handling and data preservation when the context is canceled during a Reset operation.
arangodb/arangodb_test.go (2)
3-9: Appropriate context import.Good job adding the context import which is required for the new context-aware test functions.
25-36: Well-implemented context cancellation tests.The context-aware test functions correctly verify that operations return context.Canceled errors when the context is canceled, and that canceled operations don't modify state. This pattern is consistently implemented across all context-aware methods.
Also applies to: 65-80, 126-144, 167-189
dynamodb/dynamodb_test.go (1)
63-78: Well-structured context testing.These test functions provide thorough testing of context cancellation behavior, correctly:
- Creating and immediately canceling a context
- Verifying that the context-aware operation returns a context.Canceled error
- Confirming that no state changes occur when operations are canceled
- Following a consistent pattern for all context-aware methods
I particularly like the clean error handling and the verification that data remains unchanged after canceled operations.
Also applies to: 113-128, 159-181, 207-233
etcd/etcd.go (9)
7-7: Changed import alias for better readabilityThe import alias has been renamed from
etcdtoclientv3, which better reflects the actual package name and improves code readability.
35-49: LGTM: New context-aware method for Get operationThe new
GetWithContextmethod properly accepts a context parameter and propagates it to the underlying etcd client call.
51-53: LGTM: Backward compatibility maintainedThe original
Getmethod now callsGetWithContextwith a background context, maintaining backward compatibility while allowing for context propagation in new code.
55-72: LGTM: New context-aware method for Set operationThe new
SetWithContextmethod properly accepts a context parameter and propagates it to all underlying etcd client calls, including bothGrantandPutoperations.
74-76: LGTM: Backward compatibility maintainedThe original
Setmethod now callsSetWithContextwith a background context, maintaining backward compatibility.
78-89: LGTM: New context-aware method for Delete operationThe new
DeleteWithContextmethod properly accepts a context parameter and propagates it to the underlying etcd client call.
91-93: LGTM: Backward compatibility maintainedThe original
Deletemethod now callsDeleteWithContextwith a background context, maintaining backward compatibility.
95-102: LGTM: New context-aware method for Reset operationThe new
ResetWithContextmethod properly accepts a context parameter and propagates it to the underlying etcd client call.
104-106: LGTM: Backward compatibility maintainedThe original
Resetmethod now callsResetWithContextwith a background context, maintaining backward compatibility.couchbase/couchbase.go (9)
4-4: Added context importThe
contextpackage has been added to the imports to support the new context-aware methods.
47-70: LGTM: New context-aware method for Get operationThe new
GetWithContextmethod properly accepts a context parameter and propagates it to the underlying Couchbase client call through theContextoption.
72-74: LGTM: Backward compatibility maintainedThe original
Getmethod now callsGetWithContextwith a background context, maintaining backward compatibility.
76-85: LGTM: New context-aware method for Set operationThe new
SetWithContextmethod properly accepts a context parameter and propagates it to the underlying Couchbase client call through theContextoption.
87-89: LGTM: Backward compatibility maintainedThe original
Setmethod now callsSetWithContextwith a background context, maintaining backward compatibility.
91-98: LGTM: New context-aware method for Delete operationThe new
DeleteWithContextmethod properly accepts a context parameter and propagates it to the underlying Couchbase client call through theContextoption.
100-102: LGTM: Backward compatibility maintainedThe original
Deletemethod now callsDeleteWithContextwith a background context, maintaining backward compatibility.
104-108: LGTM: New context-aware method for Reset operationThe new
ResetWithContextmethod properly accepts a context parameter and propagates it to the underlying Couchbase client call through theContextoption.
110-112: LGTM: Backward compatibility maintainedThe original
Resetmethod now callsResetWithContextwith a background context, maintaining backward compatibility.clickhouse/clickhouse.go (8)
53-73: LGTM: New context-aware method for Set operationThe new
SetWithContextmethod properly accepts a context parameter and propagates it to the underlying ClickHouse clientExeccall.
75-77: LGTM: Backward compatibility maintainedThe original
Setmethod now callsSetWithContextwith a background context, maintaining backward compatibility.
79-112: LGTM: New context-aware method for Get operationThe new
GetWithContextmethod properly accepts a context parameter and propagates it to the underlying ClickHouse clientQueryRowcall.
114-116: LGTM: Backward compatibility maintainedThe original
Getmethod now callsGetWithContextwith a background context, maintaining backward compatibility.
118-124: LGTM: New context-aware method for Delete operationThe new
DeleteWithContextmethod properly accepts a context parameter and propagates it to the underlying ClickHouse clientExeccall.
126-128: LGTM: Backward compatibility maintainedThe original
Deletemethod now callsDeleteWithContextwith a background context, maintaining backward compatibility.
130-132: LGTM: New context-aware method for Reset operationThe new
ResetWithContextmethod properly accepts a context parameter and propagates it to the underlying ClickHouse clientExeccall.
134-136: LGTM: Backward compatibility maintainedThe original
Resetmethod now callsResetWithContextwith a background context, maintaining backward compatibility.dynamodb/dynamodb.go (10)
20-22: LGTM: Simplified Storage structThe
requestTimeoutfield has been removed from theStoragestruct, centralizing context management in the caller rather than in the storage implementation.
79-105: LGTM: New context-aware method for Get operationThe new
GetWithContextmethod properly accepts a context parameter and propagates it to the underlying DynamoDB client call.
107-109: LGTM: Backward compatibility maintainedThe original
Getmethod now callsGetWithContextwith a background context, maintaining backward compatibility.
111-132: LGTM: New context-aware method for Set operationThe new
SetWithContextmethod properly accepts a context parameter and propagates it to the underlying DynamoDB client call.
134-136: LGTM: Backward compatibility maintainedThe original
Setmethod now callsSetWithContextwith a background context, maintaining backward compatibility.
138-156: LGTM: New context-aware method for Delete operationThe new
DeleteWithContextmethod properly accepts a context parameter and propagates it to the underlying DynamoDB client call.
158-160: LGTM: Backward compatibility maintainedThe original
Deletemethod now callsDeleteWithContextwith a background context, maintaining backward compatibility.
162-169: LGTM: New context-aware method for Reset operationThe new
ResetWithContextmethod properly accepts a context parameter and propagates it to the underlying DynamoDB client call.
171-173: LGTM: Backward compatibility maintainedThe original
Resetmethod now callsResetWithContextwith a background context, maintaining backward compatibility.
181-182: LGTM: Using background contextThe
createTablemethod now uses a background context directly, which is appropriate since it's an internal method called during initialization.coherence/coherence.go (8)
146-155: Implementation of GetWithContext looks goodThe context parameter is properly passed to the Coherence client's Get method, allowing the operation to be canceled or to respect deadlines.
157-159: Well-implemented backward compatibilityGood approach delegating to the new context-aware method with a background context, maintaining backward compatibility while enabling the new functionality.
161-164: SetWithContext implementation looks goodThe context parameter is properly utilized in the PutWithExpiry method, allowing the operation to be canceled or to respect deadlines.
166-168: Consistent delegation to WithContext methodGood implementation of the backward compatibility pattern by delegating to SetWithContext with a background context.
170-173: DeleteWithContext implementation looks goodThe context parameter is correctly passed to the Remove method, maintaining the consistent pattern.
175-177: Consistent delegation pattern for DeleteGood consistency in implementing the backward compatibility pattern for Delete.
179-181: ResetWithContext implementation looks goodThe context parameter is properly utilized in the Truncate method.
183-185: Consistent delegation pattern for ResetThe implementation maintains consistency with the other methods by delegating to ResetWithContext with a background context.
sqlite3/sqlite3.go (9)
4-4: Correctly added context importProper addition of the context package import required for the new context-aware methods.
92-115: Well-implemented GetWithContext methodThe GetWithContext method is implemented correctly, using QueryRowContext to ensure the database operation respects the provided context. The core logic remains unchanged from the original implementation, maintaining consistency.
117-120: Good backward compatibility for Get methodThe Get method now delegates to GetWithContext with a background context, preserving backward compatibility while enabling the new context-aware functionality.
122-134: Well-implemented SetWithContext methodThe context parameter is properly used with ExecContext, and the implementation preserves all the existing logic for handling empty keys/values and expiration time conversion.
136-139: Good backward compatibility for Set methodConsistent implementation of the delegation pattern for maintaining backward compatibility.
141-149: Well-implemented DeleteWithContext methodThe context parameter is correctly used with ExecContext, and the early return for empty keys is preserved.
151-154: Consistent delegation for Delete methodMaintains the pattern of delegating to the context-aware version with a background context.
156-160: Well-implemented ResetWithContext methodProperly uses ExecContext with the provided context parameter.
162-165: Consistent delegation for Reset methodMaintains the established pattern for backward compatibility.
scylladb/scylladb.go (9)
4-4: Correctly added context importThe context package import is properly added for the new context-aware methods.
138-148: Well-implemented GetWithContext methodThe method correctly uses WithContext to ensure the ScyllaDB query respects the provided context. The error handling logic remains unchanged, preserving the special case for gocql.ErrNotFound.
150-153: Good backward compatibility for Get methodProperly delegates to GetWithContext with a background context to maintain backward compatibility.
155-162: Well-implemented SetWithContext methodThe context parameter is properly passed to the query execution with WithContext, while preserving the expiration time conversion logic.
164-167: Consistent delegation for Set methodFollows the established pattern for backward compatibility.
169-172: Well-implemented DeleteWithContext methodCorrectly uses WithContext for context propagation in the query execution.
174-177: Consistent delegation for Delete methodMaintains the pattern of delegating to the context-aware version with a background context.
179-182: Well-implemented ResetWithContext methodProperly uses WithContext for context propagation in the truncate operation.
184-187: Consistent delegation for Reset methodFollows the established pattern for maintaining backward compatibility.
redis/redis.go (8)
78-88: Well-implemented GetWithContext methodThe implementation correctly uses the provided context with the Redis Get operation. The error handling logic remains unchanged, including the special case for redis.Nil and early return for empty keys.
90-93: Good backward compatibility for Get methodProperly delegates to GetWithContext with a background context to maintain backward compatibility.
95-101: Well-implemented SetWithContext methodThe context parameter is correctly passed to the Redis Set operation, while preserving the early return logic for empty keys or values.
103-106: Consistent delegation for Set methodFollows the established pattern for backward compatibility.
108-114: Well-implemented DeleteWithContext methodThe method correctly uses the provided context with the Redis Del operation, preserving the early return logic for empty keys.
116-119: Consistent delegation for Delete methodMaintains the pattern of delegating to the context-aware version with a background context.
121-124: Well-implemented ResetWithContext methodProperly passes the context to the FlushDB operation.
126-129: Consistent delegation for Reset methodFollows the established pattern for maintaining backward compatibility.
minio/minio.go (9)
43-43: Storage initialization updated correctly.The embedded context field has been properly removed from the Storage struct initialization to support the new context management approach.
65-90: Well-implemented context propagation for Get operation.The new
GetWithContextmethod correctly accepts and propagates the context parameter to the MinIO client call, enabling proper cancellation and timeout handling.
92-95: Good backward compatibility pattern.The original
Getmethod now delegates toGetWithContextwith a background context, maintaining backward compatibility while enabling the new context-aware functionality.
97-115: Context propagation correctly implemented for Set operation.The
SetWithContextmethod properly propagates the context to the MinIO PutObject operation while maintaining the existing locking mechanism for content type handling.
117-120: Backward compatibility maintained for Set operation.The original
Setmethod correctly delegates to the new context-aware implementation with a background context.
122-132: Context properly propagated for Delete operation.The
DeleteWithContextmethod correctly accepts and uses the context parameter in the MinIO RemoveObject call.
134-137: Backward compatibility preserved for Delete operation.The original
Deletemethod appropriately delegates to the new context-aware implementation with a background context.
139-165: Context propagation for Reset operation looks good.The
ResetWithContextmethod correctly uses the provided context in both the ListObjects and RemoveObjects operations, ensuring proper context handling throughout the Reset flow.
167-169: Proper delegation for Reset operation.The original
Resetmethod correctly delegates to the new context-aware implementation with a background context.s3/s3.go (8)
81-100: Well-implemented context propagation for Get operation.The new
GetWithContextmethod correctly accepts and uses the context parameter in the S3 download operation, maintaining the same error handling approach as the original method.
102-108: Good use of request timeout in context.The original
Getmethod now creates a context with timeout using therequestContext()helper method before delegating to the context-aware implementation, which is a good practice for handling timeouts.
110-123: Context properly propagated for Set operation.The
SetWithContextmethod correctly accepts and uses the context parameter in the S3 upload operation.
125-131: Consistent timeout handling for Set operation.The original
Setmethod correctly creates a context with timeout before delegating to the context-aware implementation, maintaining a consistent pattern across methods.
133-145: Context propagation for Delete operation looks good.The
DeleteWithContextmethod correctly accepts and uses the context parameter in the S3 delete operation.
147-153: Consistent timeout handling for Delete operation.The original
Deletemethod properly creates a context with timeout before delegating to the context-aware implementation, following the established pattern.
155-186: Context properly propagated throughout Reset operation.The
ResetWithContextmethod correctly uses the provided context in both paginator operations and delete operations, ensuring proper context propagation throughout the entire reset flow.
188-194: Consistent timeout handling for Reset operation.The original
Resetmethod properly creates a context with timeout before delegating to the context-aware implementation, maintaining consistency with other methods.valkey/valkey.go (8)
88-98: Context properly propagated to DoCache operation.The new
GetWithContextmethod correctly accepts and uses the context parameter in the Valkey DoCache operation, maintaining the same error handling for nil values.
100-103: Good backward compatibility for Get operation.The original
Getmethod now delegates toGetWithContextwith a background context, preserving backward compatibility while enabling the new context-aware functionality.
105-115: Context properly handled in Set operation with expiration support.The
SetWithContextmethod correctly accepts and uses the context parameter in the Valkey Do operation, maintaining the conditional logic for handling expiration times.
117-120: Backward compatibility maintained for Set operation.The original
Setmethod correctly delegates to the new context-aware implementation with a background context.
122-128: Context properly propagated for Delete operation.The
DeleteWithContextmethod correctly accepts and uses the context parameter in the Valkey Do operation for key deletion.
130-133: Backward compatibility preserved for Delete operation.The original
Deletemethod appropriately delegates to the new context-aware implementation with a background context.
135-138: Context properly propagated for Reset operation.The
ResetWithContextmethod correctly uses the provided context in the Flushdb operation, ensuring proper context handling.
141-143: Proper delegation for Reset operation.The original
Resetmethod correctly delegates to the new context-aware implementation with a background context.rueidis/rueidis.go (9)
88-98: Context properly propagated to DoCache operation.The new
GetWithContextmethod correctly accepts and uses the context parameter in the Rueidis DoCache operation, maintaining the same error handling for Redis nil values.
100-103: Good backward compatibility for Get operation.The original
Getmethod now delegates toGetWithContextwith a background context, preserving backward compatibility while enabling the new context-aware functionality.
105-115: Context properly handled in Set operation with expiration support.The
SetWithContextmethod correctly accepts and uses the context parameter in the Rueidis Do operation, maintaining the conditional logic for handling expiration times.
117-120: Backward compatibility maintained for Set operation.The original
Setmethod correctly delegates to the new context-aware implementation with a background context.
122-128: Context properly propagated for Delete operation.The
DeleteWithContextmethod correctly accepts and uses the context parameter in the Rueidis Do operation for key deletion.
130-133: Backward compatibility preserved for Delete operation.The original
Deletemethod appropriately delegates to the new context-aware implementation with a background context.
135-138: Context properly propagated for Reset operation.The
ResetWithContextmethod correctly uses the provided context in the Flushdb operation, ensuring proper context handling.
141-143: Proper delegation for Reset operation.The original
Resetmethod correctly delegates to the new context-aware implementation with a background context.
1-154: Consistent implementation with valkey package.The implementation of context support in rueidis.go matches the pattern used in valkey.go, which is good for consistency as both are Redis-compatible clients. This consistent approach will make the codebase easier to maintain.
neo4j/neo4j.go (8)
104-138: Good implementation of GetWithContext!The method correctly implements context-aware functionality for Neo4j data retrieval. The function signature and implementation align well with the overall PR objectives of adding context support to storage adapters.
140-142: Good backward compatibility approach.The original
Getmethod now delegates toGetWithContextwith a background context, maintaining backward compatibility while allowing new code to use the context-aware version.
144-168: Good implementation of SetWithContext!The method properly accepts a context parameter and correctly passes it to
neo4j.ExecuteQuery, allowing for context-based cancellation and timeout handling.
170-173: Good backward compatibility approach.The original
Setmethod now properly delegates toSetWithContextwith a background context, maintaining consistent behavior for existing code.
175-183: Good implementation of DeleteWithContext!The method correctly implements context-aware functionality for Neo4j deletions, properly passing the context to
neo4j.ExecuteQuery.
185-188: Good backward compatibility approach.The original
Deletemethod now delegates toDeleteWithContextwith a background context, maintaining backward compatibility.
190-199: Good implementation of ResetWithContext!The method properly accepts a context parameter and correctly passes it to
neo4j.ExecuteQuery, allowing for context-based cancellation and timeout handling during reset operations.
201-204: Good backward compatibility approach.The original
Resetmethod now delegates toResetWithContextwith a background context, maintaining consistent behavior for existing code.arangodb/arangodb.go (8)
119-150: Good implementation of GetWithContext!The method properly implements context-aware functionality for ArangoDB data retrieval, correctly passing the context to
s.collection.DocumentExistsands.collection.ReadDocument.
152-155: Good backward compatibility approach.The original
Getmethod now delegates toGetWithContextwith a background context, maintaining backward compatibility while allowing new code to use the context-aware version.
157-191: Good implementation of SetWithContext!The method properly accepts a context parameter and correctly passes it to ArangoDB client methods, allowing for context-based cancellation and timeout handling.
193-196: Good backward compatibility approach.The original
Setmethod now delegates toSetWithContextwith a background context, maintaining consistent behavior for existing code.
198-206: Good implementation of DeleteWithContext!The method correctly implements context-aware functionality for ArangoDB deletions, properly passing the context to
s.collection.RemoveDocument.
208-211: Good backward compatibility approach.The original
Deletemethod now delegates toDeleteWithContextwith a background context, maintaining backward compatibility.
213-216: Good implementation of ResetWithContext!The method properly accepts a context parameter and correctly passes it to
s.collection.Truncate, allowing for context-based cancellation and timeout handling during reset operations.
218-222: Good backward compatibility approach.The original
Resetmethod now delegates toResetWithContextwith a background context, maintaining consistent behavior for existing code.nats/nats.go (9)
157-163: Good enhancement to support configuration-based reset.The addition of reset functionality in the initialization is a good enhancement. It ensures the storage adapter's state is clean before it's used if specified in the configuration.
168-199: Good implementation of GetWithContext!The method properly implements context-aware functionality for NATS data retrieval, correctly passing the context to
kv.Get.
201-204: Good backward compatibility approach.The original
Getmethod now delegates toGetWithContextwith a background context, maintaining backward compatibility while allowing new code to use the context-aware version.
206-244: Good implementation of SetWithContext!The method properly accepts a context parameter and correctly passes it to the NATS client methods, allowing for context-based cancellation and timeout handling.
246-249: Good backward compatibility approach.The original
Setmethod now delegates toSetWithContextwith a background context, maintaining consistent behavior for existing code.
251-266: Good implementation of DeleteWithContext!The method correctly implements context-aware functionality for NATS deletions, properly passing the context to
kv.Delete.
268-271: Good backward compatibility approach.The original
Deletemethod now delegates toDeleteWithContextwith a background context, maintaining backward compatibility.
273-301: Good implementation of ResetWithContext!The method properly accepts a context parameter and correctly passes it to the NATS client methods, allowing for context-based cancellation and timeout handling during reset operations.
303-306: Good backward compatibility approach.The original
Resetmethod now delegates toResetWithContextwith a background context, maintaining consistent behavior for existing code.azureblob/azureblob.go (7)
51-63: Good implementation of GetWithContext!The method properly implements context-aware functionality for Azure Blob data retrieval, correctly passing the context to
s.client.DownloadStream.
65-71: Good backward compatibility approach.The original
Getmethod now creates a request context usings.requestContext()and delegates toGetWithContext, maintaining backward compatibility while allowing new code to use the context-aware version.
73-81: Good implementation of SetWithContext!The method properly accepts a context parameter and correctly passes it to
s.client.UploadBuffer, allowing for context-based cancellation and timeout handling.
83-89: Good backward compatibility approach.The original
Setmethod now creates a request context and delegates toSetWithContext, maintaining consistent behavior for existing code.
91-99: Good implementation of DeleteWithContext!The method correctly implements context-aware functionality for Azure Blob deletions, properly passing the context to
s.client.DeleteBlob.
101-107: Good backward compatibility approach.The original
Deletemethod now creates a request context and delegates toDeleteWithContext, maintaining backward compatibility.
132-138: Good backward compatibility approach.The original
Resetmethod now creates a request context and delegates toResetWithContext, maintaining consistent behavior for existing code.cloudflarekv/cloudflarekv.go (8)
58-67: Implementation of GetWithContext looks good.The method properly uses the passed context with the Cloudflare API call. This will allow for proper timeout and cancellation handling.
69-71: Good backward compatibility implementation.The original
Getmethod now properly delegates toGetWithContextusing a background context, maintaining backward compatibility while enabling context-aware operations.
73-86: Implementation of SetWithContext looks good.The method properly uses the passed context with the Cloudflare API call, following the same pattern as other methods.
88-90: Good backward compatibility implementation.The original
Setmethod correctly delegates toSetWithContextwith a background context.
92-104: Implementation of DeleteWithContext looks good.The method correctly passes the context to the Cloudflare API call for proper cancellation support.
106-108: Good backward compatibility implementation.The original
Deletemethod correctly delegates toDeleteWithContextwith a background context.
110-153: Implementation of ResetWithContext looks good.The method properly uses the passed context with all Cloudflare API calls within the loop, ensuring that cancellation can be propagated at any point in the operation.
155-157: Good backward compatibility implementation.The original
Resetmethod correctly delegates toResetWithContextwith a background context.mysql/mysql.go (8)
109-136: Implementation of GetWithContext looks good.The method correctly uses
QueryRowContextwith the passed context, allowing for proper cancellation and timeout handling in database operations.
138-141: Good backward compatibility implementation.The original
Getmethod correctly delegates toGetWithContextusing a background context.
143-155: Implementation of SetWithContext looks good.The method correctly uses
ExecContextwith the passed context for database operations.
157-160: Good backward compatibility implementation.The original
Setmethod correctly delegates toSetWithContextusing a background context.
162-170: Implementation of DeleteWithContext looks good.The method correctly uses
ExecContextwith the passed context for database operations.
172-175: Good backward compatibility implementation.The original
Deletemethod correctly delegates toDeleteWithContextusing a background context.
177-181: Implementation of ResetWithContext looks good.The method correctly uses
ExecContextwith the passed context for database operations.
183-186: Good backward compatibility implementation.The original
Resetmethod correctly delegates toResetWithContextusing a background context.postgres/postgres.go (5)
158-161: Good backward compatibility implementation.The original
Getmethod correctly delegates toGetWithContextusing a background context.
177-180: Good backward compatibility implementation.The original
Setmethod correctly delegates toSetWithContextusing a background context.
192-195: Good backward compatibility implementation.The original
Deletemethod correctly delegates toDeleteWithContextusing a background context.
203-206: Good backward compatibility implementation.The original
Resetmethod correctly delegates toResetWithContextusing a background context.
197-201:⚠️ Potential issueFix: Use ExecContext instead of Exec for context support.
The method is using
Execwhich doesn't respect the passed context. You should useExecContextinstead to properly support context cancellation and timeout.- _, err := s.db.Exec(ctx, s.sqlReset) + _, err := s.db.ExecContext(ctx, s.sqlReset)Likely an incorrect or invalid review comment.
mongodb/mongodb.go (8)
124-150: Implementation of GetWithContext looks good.The method properly uses the passed context with the MongoDB FindOne operation, allowing for cancellation and timeout control.
152-155: Good backward compatibility implementation.The original
Getmethod correctly delegates toGetWithContextusing a background context.
157-179: Implementation of SetWithContext looks good.The method properly passes the context to the MongoDB ReplaceOne operation, ensuring context cancellation propagates correctly.
181-187: Good backward compatibility implementation.The original
Setmethod correctly delegates toSetWithContextusing a background context.
189-197: Implementation of DeleteWithContext looks good.The method properly passes the context to the MongoDB DeleteOne operation for cancellation support.
199-202: Good backward compatibility implementation.The original
Deletemethod correctly delegates toDeleteWithContextusing a background context.
204-207: Implementation of ResetWithContext looks good.The method properly passes the context to the MongoDB Drop operation for cancellation support.
210-212: Good backward compatibility implementation.The original
Resetmethod correctly delegates toResetWithContextusing a background context.mssql/mssql.go (9)
4-4: Import of context package is appropriate.Adding the context package is necessary for implementing the context-aware methods that are being introduced in this PR.
136-163: Good implementation of context-aware Get operation.The
GetWithContextmethod properly accepts a context parameter and usesQueryRowContextto propagate cancellation signals. The implementation maintains the same logic as the originalGetmethod while adding context support.
165-168: Appropriate pattern for backward compatibility.The updated
Getmethod correctly delegates toGetWithContextwith a background context, maintaining backward compatibility while allowing the new context-aware functionality to be used.
170-183: Good implementation of context-aware Set operation.The
SetWithContextmethod properly accepts a context parameter and usesExecContextto propagate cancellation signals. The implementation maintains the same logic as the originalSetmethod while adding context support.
185-188: Appropriate pattern for backward compatibility.The updated
Setmethod correctly delegates toSetWithContextwith a background context, maintaining backward compatibility while allowing the new context-aware functionality to be used.
190-198: Good implementation of context-aware Delete operation.The
DeleteWithContextmethod properly accepts a context parameter and usesExecContextto propagate cancellation signals. The implementation maintains the same logic as the originalDeletemethod while adding context support.
200-203: Appropriate pattern for backward compatibility.The updated
Deletemethod correctly delegates toDeleteWithContextwith a background context, maintaining backward compatibility while allowing the new context-aware functionality to be used.
205-209: Good implementation of context-aware Reset operation.The
ResetWithContextmethod properly accepts a context parameter and usesExecContextto propagate cancellation signals. The implementation maintains the same logic as the originalResetmethod while adding context support.
211-214: Appropriate pattern for backward compatibility.The updated
Resetmethod correctly delegates toResetWithContextwith a background context, maintaining backward compatibility while allowing the new context-aware functionality to be used.
| // Reset clears any existing keys in existing bucket default: false | ||
| Reset bool |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add default value for the new Reset field in ConfigDefault
You've added a new Reset field to the Config struct with documentation stating the default is false, but you need to explicitly set this default value in the ConfigDefault variable to ensure consistent behavior.
var ConfigDefault = Config{
URLs: nats.DefaultURL,
Context: context.Background(),
ClientName: "fiber_storage",
KeyValueConfig: jetstream.KeyValueConfig{
Bucket: "fiber_storage",
},
WaitForConnection: 250 * time.Millisecond,
+ Reset: false,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Reset clears any existing keys in existing bucket default: false | |
| Reset bool | |
| var ConfigDefault = Config{ | |
| URLs: nats.DefaultURL, | |
| Context: context.Background(), | |
| ClientName: "fiber_storage", | |
| KeyValueConfig: jetstream.KeyValueConfig{ | |
| Bucket: "fiber_storage", | |
| }, | |
| WaitForConnection: 250 * time.Millisecond, | |
| Reset: false, | |
| } |
| ctx, cancel := context.WithTimeout(context.Background(), time.Microsecond) | ||
| defer cancel() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set a longer timeout for reliable testing
Using a 1 microsecond timeout makes the test extremely timing-sensitive and could lead to flaky test behavior depending on the environment.
-ctx, cancel := context.WithTimeout(context.Background(), time.Microsecond)
+ctx, cancel := context.WithTimeout(context.Background(), time.Nanosecond)Committable suggestion skipped: line range outside the PR's diff.
dynamodb/dynamodb_test.go
Outdated
| testStore, err := newTestStore(t) | ||
| require.NoError(t, err) | ||
| defer testStore.Close() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Same potential type mismatch in other test functions.
The same issue with newTestStore appears in the GetWithContext, DeleteWithContext, and ResetWithContext tests. It seems that the function signature might have changed from returning just a store to returning a store and an error.
Let's verify what's being returned from newTestStore in the original functions:
Also applies to: 165-167, 210-212
🏁 Script executed:
#!/bin/bash
# Check how newTestStore is used in original tests
grep -A 3 "testStore := newTestStore" dynamodb/dynamodb_test.goLength of output: 1070
Handle the newTestStore error return in all tests
The helper newTestStore now returns (store, error), so every call must capture and assert the error before using the store. For example:
- dynamodb/dynamodb_test.go, lines 165–167 (GetWithContext test)
- dynamodb/dynamodb_test.go, lines 210–212 (DeleteWithContext / ResetWithContext tests)
- Any other occurrence of
testStore := newTestStore(t) defer testStore.Close()
should be updated to:
- testStore := newTestStore(t)
- defer testStore.Close()
+ testStore, err := newTestStore(t)
+ require.NoError(t, err)
+ defer testStore.Close()| // ResetWithContext resets all entries | ||
| func (s *Storage) ResetWithContext(ctx context.Context) error { | ||
| //_, err := s.client.DeleteContainer(ctx, s.container, nil) | ||
| //return err | ||
| pager := s.client.NewListBlobsFlatPager(s.container, nil) | ||
| errCounter := 0 | ||
|
|
||
| for pager.More() { | ||
| resp, err := pager.NextPage(ctx) | ||
| if err != nil { | ||
| errCounter = errCounter + 1 | ||
| return err | ||
| } | ||
|
|
||
| for _, v := range resp.Segment.BlobItems { | ||
| _, err = s.client.DeleteBlob(ctx, s.container, *v.Name, nil) | ||
| if err != nil { | ||
| errCounter = errCounter + 1 | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| if errCounter > 0 { | ||
| return fmt.Errorf("%d errors occured while resetting", errCounter) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review error handling change in ResetWithContext.
The error handling in ResetWithContext has been changed to return immediately on any error (lines 118 and 124). Previously, it would continue trying to delete other blobs even if some deletions failed. This is a behavior change that could impact users who expect the method to delete as many blobs as possible even if some operations fail.
Also, there's a commented-out implementation (lines 111-112) that might be more efficient. Consider adding a comment explaining why the current approach was chosen over deleting the container.
// ResetWithContext resets all entries
func (s *Storage) ResetWithContext(ctx context.Context) error {
- //_, err := s.client.DeleteContainer(ctx, s.container, nil)
- //return err
+ // Note: We're not using DeleteContainer and recreating it because...
+ // [Add explanation here for why individual blob deletion is preferred]
pager := s.client.NewListBlobsFlatPager(s.container, nil)
for pager.More() {
resp, err := pager.NextPage(ctx)
if err != nil {
return err
}
for _, v := range resp.Segment.BlobItems {
_, err = s.client.DeleteBlob(ctx, s.container, *v.Name, nil)
if err != nil {
return err
}
}
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ResetWithContext resets all entries | |
| func (s *Storage) ResetWithContext(ctx context.Context) error { | |
| //_, err := s.client.DeleteContainer(ctx, s.container, nil) | |
| //return err | |
| pager := s.client.NewListBlobsFlatPager(s.container, nil) | |
| errCounter := 0 | |
| for pager.More() { | |
| resp, err := pager.NextPage(ctx) | |
| if err != nil { | |
| errCounter = errCounter + 1 | |
| return err | |
| } | |
| for _, v := range resp.Segment.BlobItems { | |
| _, err = s.client.DeleteBlob(ctx, s.container, *v.Name, nil) | |
| if err != nil { | |
| errCounter = errCounter + 1 | |
| return err | |
| } | |
| } | |
| } | |
| if errCounter > 0 { | |
| return fmt.Errorf("%d errors occured while resetting", errCounter) | |
| } | |
| return nil | |
| } | |
| // ResetWithContext resets all entries | |
| func (s *Storage) ResetWithContext(ctx context.Context) error { | |
| // Note: We're not using DeleteContainer and recreating it because... | |
| // [Add explanation here for why individual blob deletion is preferred] | |
| pager := s.client.NewListBlobsFlatPager(s.container, nil) | |
| for pager.More() { | |
| resp, err := pager.NextPage(ctx) | |
| if err != nil { | |
| return err | |
| } | |
| for _, v := range resp.Segment.BlobItems { | |
| _, err = s.client.DeleteBlob(ctx, s.container, *v.Name, nil) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| } | |
| return nil | |
| } |
| // DeleteWithContext deletes entry by key | ||
| func (s *Storage) DeleteWithContext(ctx context.Context, key string) error { | ||
| // Ain't Nobody Got Time For That | ||
| if len(key) <= 0 { | ||
| return nil | ||
| } | ||
| _, err := s.db.Exec(context.Background(), s.sqlDelete, key) | ||
| _, err := s.db.Exec(ctx, s.sqlDelete, key) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Fix: Use ExecContext instead of Exec for context support.
The method is using Exec which doesn't respect the passed context. You should use ExecContext instead to properly support context cancellation and timeout.
- _, err := s.db.Exec(ctx, s.sqlDelete, key)
+ _, err := s.db.ExecContext(ctx, s.sqlDelete, key)
efectn
force-pushed
the
add-withcontext
branch
2 times, most recently
from
0127b30 to
93da22f
Compare
|
@efectn can you update, maybe its aready working with the new test containers @mdelapenya can you try to support here |
|
Indeed, the arangodb and redis code must be updated to use the creation of the testStore using the testcontainers code. |
|
@mdelapenya would be cool if you could support here |
@mdelapenya |
* main: (168 commits) chore: read redis image for ruedis chore: align ruedis expiration test with redis chore: skip cluster tests chore: align valkey expiration test with redis chore: refine message fix: reuse valkey container in benchmarks chore: pass the redis-like image explicitly in tests fix: missing eval of redis image from env var chore: proper parallel layout chore: add integration tests to the redis helper module chore: verify mtls properly in tests chore(deps): bump github.com/aws/aws-sdk-go-v2/feature/s3/manager in /s3 chore(deps): bump github.com/aws/aws-sdk-go-v2/service/s3 in /s3 chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 in /clickhouse chore(deps): bump github.com/minio/minio-go/v7 in /minio chore(deps): bump github.com/valkey-io/valkey-go in /valkey chore(deps): bump github.com/jackc/pgx/v5 in /postgres chore: add tests for the helper module chore: make sure the client gets the variable locally fix(coherence): set coherence log level to ERROR in benchmarks ...
|
I resolved the conflicts, updating the test for the following modules:
I noticed the aerospike module needs the Context support, as this PR was created before it was merged |
|
@efectn I think the test errors are real, as the build‚GH workflows fail for the new added tests. As the tests are in a good shape now, you can continue with your work from the current state of the PR |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances all storage drivers by introducing context‐aware methods (GetWithContext, SetWithContext, DeleteWithContext, ResetWithContext) and updates their default operations to delegate to these context methods. It also adds tests to verify proper handling of canceled contexts across various backends.
- Add
WithContextvariants for Get/Set/Delete/Reset in storage drivers. - Update existing methods to call their
WithContextcounterpart withcontext.Background(). - Extend test suites to cover cancellation behavior for new context methods.
Reviewed Changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| minio/minio.go | Added WithContext methods, removed embedded context field. |
| minio/minio_test.go | Added tests for GetWithContext, SetWithContext, etc. |
| etcd/etcd.go | Introduced context variants and removed embedded contexts. |
| etcd/etcd_test.go | Added tests for context methods in etcd storage. |
| dynamodb/dynamodb.go | Added WithContext methods, removed requestContext. |
| dynamodb/dynamodb_test.go | Added context cancellation tests. |
| couchbase/couchbase.go | Added WithContext methods for Couchbase operations. |
| couchbase/couchbase_test.go | Added a context‐cancelled test case. |
| coherence/coherence.go | Introduced context methods, removed stored context. |
| coherence/coherence_test.go | Added context cancellation test for Coherence. |
| cloudflarekv/cloudflarekv.go | Added WithContext methods for Cloudflare KV API calls. |
| cloudflarekv/cloudflarekv_test.go | Extended tests to cover context‐aware operations. |
| cloudflarekv/test_module.go | Switched HTTP calls to NewRequestWithContext. |
| clickhouse/clickhouse.go | Added context methods, removed stored context. |
| clickhouse/clickhouse_test.go | Added context cancellation tests. |
| azureblob/azureblob.go | Introduced WithContext variants for Azure Blob. |
| azureblob/azureblob_test.go | Added cancellation tests for Azure Blob operations. |
| arangodb/arangodb.go | Added context‐aware methods, updated existing ones. |
| arangodb/arangodb_test.go | Added cancellation tests for ArangoDB. |
| arangodb/README.md | Updated README with new method signatures. |
| err := testStore.SetWithContext(ctx, key, val, 0) | ||
| require.ErrorIs(t, err, context.Canceled) | ||
| } | ||
|
|
||
| func TestSetAndGetWithContext(t *testing.T) { |
There was a problem hiding this comment.
testStore is not initialized in this test. You should create a new store instance (e.g., testStore := newTestStore(t)) before invoking SetWithContext.
| err := testStore.SetWithContext(ctx, key, val, 0) | |
| require.ErrorIs(t, err, context.Canceled) | |
| } | |
| func TestSetAndGetWithContext(t *testing.T) { | |
| err := store.SetWithContext(ctx, key, val, 0) | |
| require.ErrorIs(t, err, context.Canceled) | |
| } | |
| func TestSetAndGetWithContext(t *testing.T) { | |
| store := newTestStore(t) |
Copilot uses AI. Check for mistakes.
|
|
||
| val, err = testStore.Get(key1) | ||
| require.NoError(t, err) | ||
| fmt.Println(string(val)) |
There was a problem hiding this comment.
[nitpick] This debug fmt.Println appears unnecessary in the test. Consider removing it to keep tests focused and clean.
| fmt.Println(string(val)) |
Copilot uses AI. Check for mistakes.
| cancel() | ||
|
|
||
| val, err = testStore.GetWithContext(ctx, key) | ||
| fmt.Println(err) |
There was a problem hiding this comment.
[nitpick] Debug printing of err is not required; tests should use assertions only. You can remove this fmt.Println.
| fmt.Println(err) |
Copilot uses AI. Check for mistakes.
| // Get value by key | ||
| func (s *Storage) Get(key string) ([]byte, error) { | ||
|
|
||
| // GetWithContext value by key with context |
There was a problem hiding this comment.
[nitpick] The comment for GetWithContext should be a full sentence and start with the method name (e.g., // GetWithContext gets the value by key using the provided context).
| // GetWithContext value by key with context | |
| // GetWithContext retrieves the value associated with the given key using the provided context. |
Copilot uses AI. Check for mistakes.
This PR adds context management support to support as a part of gofiber/fiber#2839
Todo List:
Summary by CodeRabbit