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)
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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? |
This PR adds context management support to support as a part of gofiber/fiber#2839
Todo List:
Summary by CodeRabbit