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 PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
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, | |
| } |
coherence/coherence_test.go
Outdated
| 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.
coherence/coherence_test.go
Outdated
|
|
||
| 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (19)
badger/badger.go (3)
98-102: Context functionality is not implemented.Similar to
GetWithContext, this method provides only interface compliance without actual context support. Set operations will not respect context cancellation or timeouts.
115-119: Context functionality is not implemented.Delete operations will not respect context cancellation or timeouts with this dummy implementation.
126-130: Context functionality is not implemented.Reset operations will not respect context cancellation or timeouts with this dummy implementation.
ristretto/ristretto.go (3)
75-78: Context functionality is not implemented.Similar to other methods, this provides only interface compliance without actual context support.
89-92: Context functionality is not implemented.Delete operations will not respect context cancellation or timeouts.
100-103: Context functionality is not implemented.Reset operations will not respect context cancellation or timeouts.
bbolt/bbolt.go (4)
66-69: Context functionality is not implemented.The dummy context implementation means context cancellation, timeouts, and deadlines will not be respected. This appears to be for interface compliance only.
83-86: Context functionality is not implemented.Similar to other methods, this provides only interface compliance without actual context support.
100-103: Context functionality is not implemented.Delete operations will not respect context cancellation or timeouts.
115-118: Context functionality is not implemented.Reset operations will not respect context cancellation or timeouts.
memcache/memcache.go (4)
72-75: Context functionality is not implemented.The dummy context implementation means that memcache operations will not respect context cancellation, timeouts, or deadlines.
94-97: Context functionality is not implemented.Set operations will not respect context cancellation or timeouts with this dummy implementation.
107-110: Context functionality is not implemented.Delete operations will not respect context cancellation or timeouts.
117-120: Context functionality is not implemented.Reset operations will not respect context cancellation or timeouts.
pebble/pebble.go (4)
77-80: Context functionality is not implemented.The dummy context implementation means that Get operations will not respect context cancellation, timeouts, or deadlines.
105-108: Context functionality is not implemented.Set operations will not respect context cancellation or timeouts with this dummy implementation.
118-121: Context functionality is not implemented.Delete operations will not respect context cancellation or timeouts.
128-131: Context functionality is not implemented.Reset operations will not respect context cancellation or timeouts.
memory/memory.go (1)
59-62: Consider actual context utilization in future iterations.The dummy context support is acceptable for interface compliance, but consider implementing actual context checking for consistency with other storage backends that support cancellation.
For now, the implementation is correct for in-memory operations which are typically fast and don't benefit from cancellation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
badger/badger.go(4 hunks)bbolt/bbolt.go(4 hunks)memcache/memcache.go(4 hunks)memory/memory.go(7 hunks)mockstorage/mockstorage.go(7 hunks)mockstorage/mockstorage_test.go(2 hunks)pebble/pebble.go(4 hunks)ristretto/ristretto.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.20.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.22.x)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
🔇 Additional comments (20)
badger/badger.go (1)
76-80: Context cancellation and timeouts will not work with this implementation.The
GetWithContextmethod currently ignores the context parameter and delegates to the originalGetmethod. This means context cancellation, timeouts, and deadlines will not be respected.While this appears to be intentional based on the comment, consider if Badger operations could benefit from context support in the future, especially for long-running transactions or when implementing timeout behavior.
ristretto/ristretto.go (1)
56-59: Context cancellation and timeouts will not work.The dummy context implementation means that context cancellation, timeouts, and deadlines will not be respected. While this appears intentional for interface compliance, it limits the effectiveness of context-aware operations.
memcache/memcache.go (1)
132-132: Good comment improvement.The change from "Release item from pool" to "Release item back to pool" improves clarity.
pebble/pebble.go (2)
57-59: Good improvement to resource management.Adding explicit
closer.Close()before unmarshalling ensures proper resource cleanup and is a good practice when working with Pebble iterators.
88-92: Formatting improvement.The restructured
CacheTypeinitialization improves readability.memory/memory.go (4)
4-4: LGTM: Context import added for new interface compliance.The context import is necessary for the new context-aware method signatures.
21-23: LGTM: Minor formatting improvements.The field alignment and comment clarification improve code readability.
52-52: LGTM: Parentheses fix improves readability.The parentheses around the expiry condition make the logic clearer to read.
82-85: LGTM: Consistent dummy context pattern across all CRUD operations.All context-aware methods follow the same pattern of delegating to the original methods, maintaining consistency with the GetWithContext implementation.
Also applies to: 98-101, 112-115
mockstorage/mockstorage_test.go (5)
5-5: LGTM: Context import added for new test functions.The context import is necessary for the new context-aware test methods.
279-302: LGTM: Comprehensive test for GetWithContext.The test properly validates both fallback behavior to the original Get method and custom override functionality. Good use of different context values to verify the custom function is actually called.
304-330: LGTM: Thorough SetWithContext test with error handling.The test validates fallback behavior and includes proper error handling verification for the custom override case. The verification of the set value confirms the fallback actually works.
332-359: LGTM: Complete DeleteWithContext test coverage.The test verifies both successful deletion through fallback and error handling through custom override. The validation that the key is actually deleted is important.
361-385: LGTM: Solid ResetWithContext test implementation.The test follows the established pattern and properly validates both reset functionality and custom error handling.
mockstorage/mockstorage.go (6)
4-4: LGTM: Context import for new interface methods.The context import is required for the new context-aware method signatures.
30-41: LGTM: Well-structured extension of CustomFuncs.The addition of context-aware function fields maintains consistency with the existing pattern and provides comprehensive mock customization capabilities.
46-48: LGTM: Constructor improvement eliminates nil checks.Initializing CustomFuncs with an empty struct is cleaner than leaving it nil and eliminates the need to check for nil in the method implementations.
77-83: LGTM: Consistent context-aware method pattern.The GetWithContext implementation follows a clean pattern: check for custom function first, then fallback to the original method. This provides maximum flexibility for testing scenarios.
103-109: LGTM: Consistent implementation across all context-aware methods.All context-aware methods follow the same pattern as GetWithContext, providing consistent behavior and testing capabilities across the entire interface.
Also applies to: 123-129, 143-149
191-191: LGTM: Updated comment for clarity.The comment better describes the method's purpose for runtime function injection.
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
cassandra/cassandra.go (1)
268-297: Fix inconsistent context usage in expired key cleanup.The
GetWithContextmethod properly uses the provided context for the main query, but the expired key deletion on line 290 ignores the context by callings.Delete(key)instead ofs.DeleteWithContext(ctx, key).Apply this fix to ensure consistent context propagation:
// Check if the key has expired if !result.ExpiresAt.IsZero() && time.Now().After(result.ExpiresAt) { // Delete the expired key - if err := s.Delete(key); err != nil { + if err := s.DeleteWithContext(ctx, key); err != nil { return nil, err } return nil, ErrKeyExpired }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
aerospike/aerospike.go(5 hunks)cassandra/cassandra.go(6 hunks)cassandra/cassandra_test.go(4 hunks)surrealdb/surrealdb.go(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (60)
- GitHub Check: lint (postgres)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: lint (postgres)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: Tests (1.22.x)
- GitHub Check: lint (postgres)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: Tests (1.22.x)
- GitHub Check: lint (postgres)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: Tests (1.22.x)
- GitHub Check: lint (postgres)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: Tests (1.22.x)
- GitHub Check: lint (postgres)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: Tests (1.22.x)
- GitHub Check: lint (postgres)
- GitHub Check: lint (mssql)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: Tests (1.22.x)
- GitHub Check: lint (postgres)
- GitHub Check: lint (mssql)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: Tests (1.22.x)
- GitHub Check: lint (postgres)
- GitHub Check: lint (mssql)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: Tests (1.22.x)
- GitHub Check: lint (mssql)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x, 8)
- GitHub Check: Tests (1.22.x)
🔇 Additional comments (14)
cassandra/cassandra_test.go (4)
69-83: LGTM! Well-structured context cancellation test.The test correctly verifies that
SetWithContextrespects context cancellation and returns the appropriate error while ensuring no side effects occur on the storage.
105-125: LGTM! Comprehensive context cancellation test for Get operation.The test properly sets up data first, then verifies that
GetWithContextwith a canceled context returns the expected error while leaving the underlying data intact.
151-175: LGTM! Thorough context cancellation test for Delete operation.The test establishes the key exists, verifies
DeleteWithContextfails with canceled context, and confirms the key remains undeleted - excellent coverage of the expected behavior.
247-272: LGTM! Complete context cancellation test for Reset operation.The test correctly verifies that
ResetWithContextrespects context cancellation by ensuring both test keys remain in storage after the canceled operation.cassandra/cassandra.go (8)
4-4: LGTM! Required import for context support.The context import is necessary for the new context-aware storage methods.
222-261: LGTM! Well-implemented context-aware Set method.The
SetWithContextmethod properly integrates context support by passing it through to the underlying gocqlx query execution viaWithContext(ctx).
263-266: LGTM! Clean refactor maintaining backward compatibility.The original
Setmethod correctly delegates toSetWithContextwith a background context, preserving existing behavior while enabling context support.
299-302: LGTM! Clean refactor maintaining backward compatibility.The original
Getmethod correctly delegates toGetWithContextwith a background context.
304-315: LGTM! Well-implemented context-aware Delete method.The
DeleteWithContextmethod properly integrates context support into the gocqlx query execution.
317-321: LGTM! Clean refactor maintaining backward compatibility.The original
Deletemethod correctly delegates toDeleteWithContextwith a background context.
323-328: LGTM! Well-implemented context-aware Reset method.The
ResetWithContextmethod properly passes the context through to the underlying query execution.
330-334: LGTM! Clean refactor maintaining backward compatibility.The original
Resetmethod correctly delegates toResetWithContextwith a background context, maintaining existing behavior.aerospike/aerospike.go (1)
4-4: Context import added for dummy implementations.The context package is imported but the new context-aware methods don't actually utilize the context for cancellation, deadlines, or request tracing.
surrealdb/surrealdb.go (1)
4-4: Context and time imports added for enhanced functionality.The imports support the new context-aware methods and time-based operations.
Also applies to: 7-7
| // DeleteWithContext deletes key (dummy context support) | ||
| func (s *Storage) DeleteWithContext(ctx context.Context, key string) error { | ||
| return s.Delete(key) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper context support for Delete operation.
The DeleteWithContext method should check for context cancellation and integrate with the Aerospike client operations.
🤖 Prompt for AI Agents
In aerospike/aerospike.go around lines 267 to 270, the DeleteWithContext method
currently ignores the provided context and simply calls Delete. Modify this
method to properly support context by checking if the context is done or
cancelled before proceeding. Integrate the context with the Aerospike client's
delete operation, ensuring the delete respects context cancellation and
deadlines. This may involve using Aerospike client methods that accept context
or implementing manual context checks before and during the delete call.
| // SetWithContext sets value by key (dummy context support) | ||
| func (s *Storage) SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error { | ||
| return s.Set(key, val, exp) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper context support for Set operation.
Similar to GetWithContext, this method should utilize the context for proper cancellation and timeout handling rather than ignoring it.
-// SetWithContext sets value by key (dummy context support)
-func (s *Storage) SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error {
- return s.Set(key, val, exp)
-}
+// SetWithContext sets value by key with context support
+func (s *Storage) SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error {
+ // Check context cancellation before proceeding
+ select {
+ case <-ctx.Done():
+ return ctx.Err()
+ default:
+ }
+
+ k, err := aerospike.NewKey(s.namespace, s.setName, key)
+ if err != nil {
+ return err
+ }
+
+ expiration := s.expiration
+ if exp != 0 {
+ expiration = exp
+ }
+
+ // Convert to seconds with a minimum of 1
+ ttl := uint32(expiration.Seconds())
+ if ttl < 1 {
+ ttl = 1
+ }
+
+ writePolicy := aerospike.NewWritePolicy(0, ttl)
+ bins := aerospike.BinMap{
+ "value": val,
+ }
+
+ return s.client.Put(writePolicy, k, bins)
+}📝 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.
| // SetWithContext sets value by key (dummy context support) | |
| func (s *Storage) SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error { | |
| return s.Set(key, val, exp) | |
| } | |
| // SetWithContext sets value by key with context support | |
| func (s *Storage) SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error { | |
| // Check context cancellation before proceeding | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| default: | |
| } | |
| k, err := aerospike.NewKey(s.namespace, s.setName, key) | |
| if err != nil { | |
| return err | |
| } | |
| expiration := s.expiration | |
| if exp != 0 { | |
| expiration = exp | |
| } | |
| // Convert to seconds with a minimum of 1 | |
| ttl := uint32(expiration.Seconds()) | |
| if ttl < 1 { | |
| ttl = 1 | |
| } | |
| writePolicy := aerospike.NewWritePolicy(0, ttl) | |
| bins := aerospike.BinMap{ | |
| "value": val, | |
| } | |
| return s.client.Put(writePolicy, k, bins) | |
| } |
🤖 Prompt for AI Agents
In aerospike/aerospike.go around lines 251 to 254, the SetWithContext method
currently ignores the provided context and directly calls Set. Modify this
method to properly support context by integrating context cancellation and
timeout handling into the Set operation, similar to how GetWithContext handles
context. This may involve passing the context down to Aerospike client calls or
checking context.Done() to abort the operation if cancelled.
| // GetWithContext gets value by key (dummy context support) | ||
| func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) { | ||
| return s.Get(key) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper context support instead of dummy delegation.
The GetWithContext method ignores the context parameter and delegates to the original Get method. This misses the opportunity to provide proper cancellation and timeout handling.
Consider integrating the context with the Aerospike client operations:
-// GetWithContext gets value by key (dummy context support)
-func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) {
- return s.Get(key)
-}
+// GetWithContext gets value by key with context support
+func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) {
+ k, err := aerospike.NewKey(s.namespace, s.setName, key)
+ if err != nil {
+ return nil, err
+ }
+
+ // Create a policy with context
+ policy := aerospike.NewPolicy()
+
+ // Check context cancellation
+ select {
+ case <-ctx.Done():
+ return nil, ctx.Err()
+ default:
+ }
+
+ record, err := s.client.Get(policy, k, "value")
+ if err != nil {
+ if err.Matches(aerospike.ErrKeyNotFound.ResultCode) {
+ return nil, nil
+ }
+ return nil, err
+ }
+
+ data, ok := record.Bins["value"].([]byte)
+ if !ok {
+ return nil, nil
+ }
+
+ return data, nil
+}🤖 Prompt for AI Agents
In aerospike/aerospike.go around lines 220 to 223, the GetWithContext method
currently ignores the context parameter and simply calls Get, missing proper
context support. Modify this method to integrate the provided context with the
Aerospike client operation, enabling cancellation and timeout handling. Use the
context to control the Aerospike client's request lifecycle, ensuring that the
operation respects deadlines and cancellations signaled by the context.
| // ResetWithContext resets all keys (dummy context support) | ||
| func (s *Storage) ResetWithContext(ctx context.Context) error { | ||
| return s.Reset() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper context support for Reset operation.
The ResetWithContext method should periodically check for context cancellation during the potentially long-running reset operation that scans and deletes all records.
-// ResetWithContext resets all keys (dummy context support)
-func (s *Storage) ResetWithContext(ctx context.Context) error {
- return s.Reset()
-}
+// ResetWithContext resets all keys with context support
+func (s *Storage) ResetWithContext(ctx context.Context) error {
+ // Check context cancellation before starting
+ select {
+ case <-ctx.Done():
+ return ctx.Err()
+ default:
+ }
+
+ // Use ScanAll which returns a Recordset
+ scanPolicy := aerospike.NewScanPolicy()
+ recordset, err := s.client.ScanAll(scanPolicy, s.namespace, s.setName)
+ if err != nil {
+ return err
+ }
+
+ defer func() {
+ if err := recordset.Close(); err != nil {
+ log.Printf("Error closing recordset: %v\n", err)
+ }
+ }()
+
+ writePolicy := aerospike.NewWritePolicy(0, 0)
+
+ for result := range recordset.Results() {
+ // Check context cancellation periodically
+ select {
+ case <-ctx.Done():
+ return ctx.Err()
+ default:
+ }
+
+ if result.Err != nil {
+ log.Printf("Error scanning: %v\n", result.Err)
+ continue
+ }
+
+ _, err = s.client.Delete(writePolicy, result.Record.Key)
+ if err != nil {
+ log.Printf("Error deleting key %v: %v\n", result.Record.Key, 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 keys (dummy context support) | |
| func (s *Storage) ResetWithContext(ctx context.Context) error { | |
| return s.Reset() | |
| } | |
| // ResetWithContext resets all keys with context support | |
| func (s *Storage) ResetWithContext(ctx context.Context) error { | |
| // Check context cancellation before starting | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| default: | |
| } | |
| // Use ScanAll which returns a Recordset | |
| scanPolicy := aerospike.NewScanPolicy() | |
| recordset, err := s.client.ScanAll(scanPolicy, s.namespace, s.setName) | |
| if err != nil { | |
| return err | |
| } | |
| defer func() { | |
| if err := recordset.Close(); err != nil { | |
| log.Printf("Error closing recordset: %v\n", err) | |
| } | |
| }() | |
| writePolicy := aerospike.NewWritePolicy(0, 0) | |
| for result := range recordset.Results() { | |
| // Check context cancellation periodically | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| default: | |
| } | |
| if result.Err != nil { | |
| log.Printf("Error scanning: %v\n", result.Err) | |
| continue | |
| } | |
| _, err = s.client.Delete(writePolicy, result.Record.Key) | |
| if err != nil { | |
| log.Printf("Error deleting key %v: %v\n", result.Record.Key, err) | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In aerospike/aerospike.go around lines 312 to 315, the ResetWithContext method
currently ignores the provided context and directly calls Reset. Modify this
method to implement proper context support by periodically checking if the
context has been cancelled or timed out during the reset operation. Integrate
context cancellation checks within the reset logic to safely abort the operation
if the context signals done, ensuring the method respects context deadlines and
cancellations.
| // DeleteWithContext dummy context support: calls Delete ignoring ctx | ||
| func (s *Storage) DeleteWithContext(ctx context.Context, key string) error { | ||
| return s.Delete(key) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add context cancellation check to DeleteWithContext.
Similar to other context methods, add basic context cancellation support.
🤖 Prompt for AI Agents
In surrealdb/surrealdb.go around lines 124 to 127, the DeleteWithContext method
ignores the provided context and directly calls Delete. Modify this method to
first check if the context has been cancelled or expired by inspecting
ctx.Err(), and if so, return the context error immediately. Only proceed to call
s.Delete(key) if the context is still valid, thereby adding basic context
cancellation support.
| // ResetWithContext dummy context support: calls Reset ignoring ctx | ||
| func (s *Storage) ResetWithContext(ctx context.Context) error { | ||
| return s.Reset() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add context cancellation check to ResetWithContext.
The Reset operation could be long-running, so context cancellation checking is particularly important here.
-// ResetWithContext dummy context support: calls Reset ignoring ctx
-func (s *Storage) ResetWithContext(ctx context.Context) error {
- return s.Reset()
-}
+// ResetWithContext clears all keys with context support
+func (s *Storage) ResetWithContext(ctx context.Context) error {
+ // Check context cancellation before proceeding
+ select {
+ case <-ctx.Done():
+ return ctx.Err()
+ default:
+ }
+
+ _, err := surrealdb.Delete[[]model](s.db, models.Table(s.table))
+ return err
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In surrealdb/surrealdb.go around lines 135 to 138, the ResetWithContext method
currently ignores the provided context and directly calls Reset. Modify this
method to periodically check if the context has been cancelled or timed out
during the Reset operation. If the context is done, return the context error
immediately to respect cancellation requests and avoid unnecessary work.
| // SetWithContext dummy context support: calls Set ignoring ctx | ||
| func (s *Storage) SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error { | ||
| return s.Set(key, val, exp) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add context cancellation check to SetWithContext.
While delegating to the original method, at minimum check for context cancellation before proceeding.
-// SetWithContext dummy context support: calls Set ignoring ctx
-func (s *Storage) SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error {
- return s.Set(key, val, exp)
-}
+// SetWithContext sets value by key with context support
+func (s *Storage) SetWithContext(ctx context.Context, key string, val []byte, exp time.Duration) error {
+ // Check context cancellation before proceeding
+ select {
+ case <-ctx.Done():
+ return ctx.Err()
+ default:
+ }
+
+ return s.Set(key, val, exp)
+}🤖 Prompt for AI Agents
In surrealdb/surrealdb.go around lines 109 to 112, the SetWithContext method
currently ignores the context parameter and directly calls Set. Modify this
method to first check if the context has been cancelled or expired by inspecting
ctx.Err(), and if so, return the context error immediately. Only proceed to call
s.Set if the context is still valid.
| // GetWithContext dummy context support: calls Get ignoring ctx | ||
| func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) { | ||
| return s.Get(key) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper context support instead of dummy delegation.
The GetWithContext method ignores the context parameter. While the SurrealDB Go client may not directly support context parameters, you should still check for context cancellation before proceeding with operations.
-// GetWithContext dummy context support: calls Get ignoring ctx
-func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) {
- return s.Get(key)
-}
+// GetWithContext gets value by key with context support
+func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) {
+ // Check context cancellation before proceeding
+ select {
+ case <-ctx.Done():
+ return nil, ctx.Err()
+ default:
+ }
+
+ if len(key) == 0 {
+ return nil, errors.New("key is required")
+ }
+
+ recordID := models.NewRecordID(s.table, key)
+ m, err := surrealdb.Select[model, models.RecordID](s.db, recordID)
+ if err != nil {
+ return nil, err
+ }
+
+ if m.Exp > 0 && time.Now().Unix() > m.Exp {
+ _ = s.Delete(key)
+ return nil, nil
+ }
+
+ return m.Body, 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.
| // GetWithContext dummy context support: calls Get ignoring ctx | |
| func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) { | |
| return s.Get(key) | |
| } | |
| // GetWithContext gets value by key with context support | |
| func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) { | |
| // Check context cancellation before proceeding | |
| select { | |
| case <-ctx.Done(): | |
| return nil, ctx.Err() | |
| default: | |
| } | |
| if len(key) == 0 { | |
| return nil, errors.New("key is required") | |
| } | |
| recordID := models.NewRecordID(s.table, key) | |
| m, err := surrealdb.Select[model, models.RecordID](s.db, recordID) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if m.Exp > 0 && time.Now().Unix() > m.Exp { | |
| _ = s.Delete(key) | |
| return nil, nil | |
| } | |
| return m.Body, nil | |
| } |
🤖 Prompt for AI Agents
In surrealdb/surrealdb.go at lines 85 to 88, the GetWithContext method currently
ignores the context parameter and directly calls Get. Modify this method to
check if the context is already cancelled or expired before calling Get,
returning an appropriate error if so. This ensures proper context support by
respecting cancellation signals even if the underlying Get method does not
accept a context.
Add WithContext methods to storage.go
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
29-62: Fix markdown formatting: Replace hard tabs with spaces.The markdown linter has flagged multiple lines using hard tabs instead of spaces, which violates markdown formatting standards.
Apply this formatting fix to replace hard tabs with spaces throughout the interface documentation:
- // GetWithContext gets the value for the given key with a context. - // `nil, nil` is returned when the key does not exist - GetWithContext(ctx context.Context, key string) ([]byte, error) + // GetWithContext gets the value for the given key with a context. + // `nil, nil` is returned when the key does not exist + GetWithContext(ctx context.Context, key string) ([]byte, error)Similar replacements should be applied to all other lines flagged by the linter (lines 29-62).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)storage.go(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
29-29: Hard tabs
Column: 1
(MD010, no-hard-tabs)
30-30: Hard tabs
Column: 1
(MD010, no-hard-tabs)
31-31: Hard tabs
Column: 1
(MD010, no-hard-tabs)
33-33: Hard tabs
Column: 1
(MD010, no-hard-tabs)
34-34: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
38-38: Hard tabs
Column: 1
(MD010, no-hard-tabs)
39-39: Hard tabs
Column: 1
(MD010, no-hard-tabs)
41-41: Hard tabs
Column: 1
(MD010, no-hard-tabs)
42-42: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
54-54: Hard tabs
Column: 1
(MD010, no-hard-tabs)
55-55: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
60-60: Hard tabs
Column: 1
(MD010, no-hard-tabs)
61-61: Hard tabs
Column: 1
(MD010, no-hard-tabs)
62-62: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
🔇 Additional comments (9)
storage.go (5)
3-6: LGTM: Clean context package integration.The addition of the context import is necessary and properly placed to support the new context-aware methods.
11-13: LGTM: Well-designed context-aware getter method.The
GetWithContextmethod signature follows Go conventions with context as the first parameter and maintains consistency with the originalGetmethod.
19-21: LGTM: Consistent context-aware setter design.The
SetWithContextmethod properly extends the originalSetmethod with context support while maintaining the same parameter order and types.
28-30: LGTM: Context-aware delete method implementation.The
DeleteWithContextmethod follows the established pattern and correctly documents the no-error behavior for non-existent keys.
36-37: LGTM: Context-aware reset method completes the interface.The
ResetWithContextmethod rounds out the context-aware interface additions with proper documentation and signature.README.md (4)
29-31: LGTM: Documentation accurately reflects the new GetWithContext method.The interface documentation correctly matches the implementation in storage.go with proper parameter types and return values.
37-39: LGTM: SetWithContext documentation is consistent and accurate.The method documentation properly describes the context-aware version while maintaining consistency with the original method description.
46-48: LGTM: DeleteWithContext documentation maintains interface contract.The documentation correctly preserves the no-error behavior specification for non-existent keys while adding context support.
54-55: LGTM: ResetWithContext documentation completes the context-aware interface.The documentation accurately describes the context-aware reset functionality with clear and concise wording.
This PR adds context management support to support as a part of gofiber/fiber#2839
Todo List:
Summary by CodeRabbit
Summary by CodeRabbit