🔥 feat: Add SurrealDB driver#1638
Conversation
… configuration and tests
Implementing surrealdb
aliyasirnac
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
WalkthroughThis pull request introduces a SurrealDB storage driver for Go, featuring a new README file that outlines the driver's functionality, a configuration structure with default values, and a storage implementation that includes methods for CRUD operations and expiration handling. Additionally, a comprehensive suite of unit tests and benchmarks has been added to validate the functionality. The main components include the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as Storage
participant DB as SurrealDB
C->>S: New(config)
S->>DB: Connect & authenticate with config
DB-->>S: Connection established
S-->>C: Return Storage instance
C->>S: Set(key, value, exp)
S->>DB: Insert/Update record with expiration
DB-->>S: Acknowledge operation
S-->>C: Operation successful
C->>S: Get(key)
S->>DB: Retrieve record
DB-->>S: Return record (or indicate expiration)
S-->>C: Return value or nil
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches
🪧 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: 3
🧹 Nitpick comments (12)
surrealdb/config.go (2)
15-39: Consider secure and flexible handling of credentials.Storing credentials (username/password) unencrypted in config structs is convenient but may pose a security risk if the config is logged or inadvertently exposed. Consider using environment variables or a secrets manager.
52-94: Exposing possible validation for config fields.While
configDefaultcorrectly merges user overrides, consider validating critical fields. For instance, verifying thatConnectionStringis a valid URL, or checking thatUsernameandPasswordmeet certain requirements (if any). This helps avoid edge cases at runtime.surrealdb/surrealdb.go (4)
24-28: Consider optional TTL support at the schema level.Storing expiration in each record is helpful, but SurrealDB may provide alternate TTL or timed record management approaches. If SurrealDB supports TTL natively, this custom approach may be streamlined further.
63-80: Log errors from theDeletecall on expiration.You gracefully delete an expired entry, but any error from
s.Deleteis ignored. Consider logging or handling it to catch unexpected DB issues.
109-112: Risk of losing data in multi-tenant usage.
Resetdeletes the entire table. In multi-tenant or shared usage scenarios, this is destructive. Confirm whether usage contexts require typed or namespaced resets.
118-136: HandleDeleteerrors for expired records.Similar to
Get(), you ignore errors when deleting expired records. Logging and retrying could be beneficial if SurrealDB experiences transient errors.surrealdb/README.md (1)
12-13: Heading style lint warning (MD036).You’ve used emphasis instead of a heading for “Note: Requires Go 1.20 and above.” Consider changing this to a proper heading to avoid markdown lint warnings.
-**Note: Requires Go 1.20 and above** +#### Note +Requires Go 1.20 and above🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
12-12: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
surrealdb/surrealdb_test.go (5)
42-42: Remove extra blank line.There's an unnecessary blank line that can be removed for better code formatting.
require.NotEmpty(t, get) - }
44-48: Improve test isolation.This test depends on data from previous tests, which could lead to flaky tests if test execution order changes. Consider adding setup code to ensure the test has its own data.
func Test_Surrealdb_ListTable(t *testing.T) { + // Ensure we have some data for this test + err := testStore.Set("list-test-key", []byte("list-test-value"), 0) + require.NoError(t, err) + bytes, err := testStore.List() require.NoError(t, err) require.NotEmpty(t, bytes) }
89-93: Translate comments to English.The comments are currently in Turkish. For international collaboration and consistency, please translate them to English.
- // Geçerli kayıt + // Valid record _ = testStore.Set("valid", []byte("123"), 0) - // Süresi geçen kayıt + // Expired record _ = testStore.Set("expired", []byte("456"), 1*time.Second)
73-74: Consider alternatives to time.Sleep in tests.Using
time.Sleepin tests can make them slow and potentially flaky. While sometimes necessary for testing time-dependent functionality like expiration, consider whether there are alternative approaches that could be more reliable.If the SurrealDB driver allows it, you might consider implementing a mock time provider for testing to avoid actual sleeping.
114-117: Simplify key generation in benchmark.The current key generation is redundant - it creates a key with format
bench-key-bench-key-%d-%d. This can be simplified for better readability.for i := 0; i < b.N; i++ { - key := fmt.Sprintf("bench-key-%d-%d", i, time.Now().UnixNano()) - err := store.Set(fmt.Sprintf("bench-key-%s", key), []byte("value"), 0) + key := fmt.Sprintf("bench-key-%d-%d", i, time.Now().UnixNano()) + err := store.Set(key, []byte("value"), 0) if err != nil { b.Errorf("Set failed: %v", err) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/test-surrealdb.ymlis excluded by!**/*.ymlsurrealdb/go.modis excluded by!**/*.modsurrealdb/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (4)
surrealdb/README.md(1 hunks)surrealdb/config.go(1 hunks)surrealdb/surrealdb.go(1 hunks)surrealdb/surrealdb_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
surrealdb/surrealdb.go (1)
surrealdb/config.go (1)
Config(15-39)
surrealdb/surrealdb_test.go (2)
surrealdb/surrealdb.go (2)
Storage(12-15)New(32-61)surrealdb/config.go (1)
ConfigDefault(41-50)
🪛 markdownlint-cli2 (0.17.2)
surrealdb/README.md
12-12: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🔇 Additional comments (7)
surrealdb/config.go (2)
3-14: Well-documented purpose and usage.The doc comments are clear and thorough, which is helpful for new users.
41-50: Good practice providing sensible defaults.Supplying a default configuration is handy for rapid development. The chosen defaults look appropriate for a local development environment.
surrealdb/surrealdb.go (4)
12-15: Check handling of an empty table name.Currently, there's no safeguard if
cfg.DefaultTableturns out empty. SurrealDB calls may fail downstream. It might be worth validating the table name before usage.
82-98: Confirm record overwrite behavior.
surrealdb.Createmight fail or duplicate if the record already exists. If you intend to overwrite an existing record, check whether you needUpdateor an upsert-like method.
100-107: Error handling for nonexistent keys.Deleting a key that doesn't exist is not an error here, which is acceptable. Just ensure this behavior is intentional and documented for users who expect an error on missing keys.
114-116: Straightforward resource cleanup.Your
Close()method correctly callsdb.Close(). This is clean and expected.surrealdb/surrealdb_test.go (1)
1-121: Good test coverage of SurrealDB storage driver functionality.The test file provides comprehensive coverage of the SurrealDB storage driver's functionality, including CRUD operations, expiration handling, and performance benchmarking. The tests are well-structured and use appropriate assertions.
| _ = testStore.Reset() | ||
|
|
||
| // Geçerli kayıt | ||
| _ = testStore.Set("valid", []byte("123"), 0) | ||
|
|
||
| // Süresi geçen kayıt | ||
| _ = testStore.Set("expired", []byte("456"), 1*time.Second) | ||
| time.Sleep(2 * time.Second) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle returned errors in test setup.
Error values from Reset() and Set() calls are being ignored. Even in tests, it's good practice to check these errors to ensure proper test setup.
func Test_Surrealdb_ListSkipsExpired(t *testing.T) {
- _ = testStore.Reset()
+ err := testStore.Reset()
+ require.NoError(t, err)
// Geçerli kayıt
- _ = testStore.Set("valid", []byte("123"), 0)
+ err = testStore.Set("valid", []byte("123"), 0)
+ require.NoError(t, err)
// Süresi geçen kayıt
- _ = testStore.Set("expired", []byte("456"), 1*time.Second)
+ err = testStore.Set("expired", []byte("456"), 1*time.Second)
+ require.NoError(t, err)
time.Sleep(2 * time.Second)📝 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.
| _ = testStore.Reset() | |
| // Geçerli kayıt | |
| _ = testStore.Set("valid", []byte("123"), 0) | |
| // Süresi geçen kayıt | |
| _ = testStore.Set("expired", []byte("456"), 1*time.Second) | |
| time.Sleep(2 * time.Second) | |
| func Test_Surrealdb_ListSkipsExpired(t *testing.T) { | |
| err := testStore.Reset() | |
| require.NoError(t, err) | |
| // Geçerli kayıt | |
| err = testStore.Set("valid", []byte("123"), 0) | |
| require.NoError(t, err) | |
| // Süresi geçen kayıt | |
| err = testStore.Set("expired", []byte("456"), 1*time.Second) | |
| require.NoError(t, err) | |
| time.Sleep(2 * time.Second) | |
| } |
gaby
left a comment
gaby
left a comment
There was a problem hiding this comment.
Looks good overall, just a few comments here and there. Will re-review once those are done. Thanks! 💪
|
@gaby thanks for your feedbacks |
- Replaced Create with Upsert to allow overwriting existing keys - Removed trailing spaces from line 2 and 3 in README - Addressed other code review suggestions (naming, consistency, etc.)
|
@aliyasirnac What's left on this PR? |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new SurrealDB storage driver for the project, implementing the Storage interface with support for key-value operations, TTL expiration, and integrations with tests, benchmarks, and documentation updates.
- Implements Storage interface methods (Get, Set, Delete, List, Reset, Close) for SurrealDB.
- Adds comprehensive tests and benchmarks to validate driver functionality.
- Updates README and workflow configurations to include SurrealDB support.
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| surrealdb/surrealdb_test.go | Adds tests for CRUD, expiration, list behavior, and benchmarks |
| surrealdb/surrealdb.go | Implements the storage driver using SurrealDB SDK with panic on errors |
| surrealdb/config.go | Provides configuration defaults and overrides |
| surrealdb/README.md | Documents usage, configuration, and examples for the new driver |
| README.md | Updates main README to reference the new SurrealDB driver |
| .github/workflows/test-surrealdb.yml | Adds CI workflow to test the SurrealDB driver |
| .github/workflows/benchmark.yml | Integrates SurrealDB into the benchmark workflow |
Files not reviewed (1)
- surrealdb/go.mod: Language not supported
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
81-82: Maintain alphabetical order in storage implementations listThe new SurrealDB entry should appear before Valkey to keep the list sorted alphabetically by driver name.
- - [Valkey](./valkey/README.md) <a href="/gofiber/storage/actions?query=workflow%3A%22Tests+valkey%22"> <img src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-valkey.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" /> </a> - - [SurrealDB](./surrealdb/README.md) <a href="/gofiber/storage/actions?query=workflow%3A%22Tests+surrealdb%22"> <img src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-surrealdb.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" /> </a> + - [SurrealDB](./surrealdb/README.md) <a href="/gofiber/storage/actions?query=workflow%3A%22Tests+surrealdb%22"> <img src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-surrealdb.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" alt="SurrealDB Tests Status" /> </a> + - [Valkey](./valkey/README.md) <a href="/gofiber/storage/actions?query=workflow%3A%22Tests+valkey%22"> <img src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-valkey.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" alt="Valkey Tests Status" /> </a>🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
81-81: Images should have alternate text (alt text)
null(MD045, no-alt-text)
82-82: Images should have alternate text (alt text)
null(MD045, no-alt-text)
81-81: Add alt attribute to Valkey badge for accessibilityThe
<img>tag on this line is missing analtattribute, which is required for accessibility and to satisfy markdownlint (MD045).- <img src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-valkey.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" /> + <img src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-valkey.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" alt="Valkey Tests Status" />🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
81-81: Images should have alternate text (alt text)
null(MD045, no-alt-text)
82-82: Add alt attribute to SurrealDB badge for accessibilitySimilarly, this
<img>tag needs analtattribute to improve accessibility and satisfy markdownlint (MD045).- <img src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-surrealdb.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" /> + <img src="https://img.shields.io/github/actions/workflow/status/gofiber/storage/test-surrealdb.yml?branch=main&label=%F0%9F%A7%AA%20&style=flat&color=75C46B" alt="SurrealDB Tests Status" />🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
82-82: Images should have alternate text (alt text)
null(MD045, no-alt-text)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/benchmark.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
81-81: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
82-82: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
I don't have much of an idea. KV and in-memory are not currently supported in the official sdk, so I couldn't add them. Some repositories had garbage collectors, maybe I can add them and maybe we can add and refactor the bots' suggestions. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
surrealdb/config.go (2)
40-46: Consider removing or better documenting the commented-out Mode field.The commented-out
Modefield suggests planned support for different operational modes, but it's currently not implemented. From the PR objectives, I understand that SurrealDB SDK doesn't support embedded database modes (KV and memory) yet.Instead of keeping commented code, consider either:
- Remove the commented field entirely until support is added
- Add a more detailed comment explaining why it's commented out and any plans for future implementation
If you decide to keep it commented, consider adding a reference to the SDK issue tracking this limitation.
100-102: Missing Mode field initialization in configDefault.The
configDefaultfunction doesn't set the default Mode if it's not provided, unlike other fields.if cfg.DefaultTable == "" { cfg.DefaultTable = ConfigDefault.DefaultTable } + if cfg.Mode == "" { + cfg.Mode = ConfigDefault.Mode + } return cfg
🧹 Nitpick comments (3)
surrealdb/config.go (3)
48-57: Add a security note about default credentials.The default configuration uses "root"/"root" as credentials, which is fine for development but could be dangerous if used in production. Consider adding a comment warning users to change these values in production environments.
var ConfigDefault = Config{ ConnectionString: "ws://localhost:8000", Namespace: "fiber_storage", Database: "fiber_storage", Username: "root", // Warning: Change in production Password: "root", // Warning: Change in production Access: "full", Scope: "all", DefaultTable: "fiber_storage", + // Warning: Default credentials are for development only. + // Use secure credentials in production environments. }
59-105: Consider adding basic configuration validation.The
configDefaultfunction merges default values but doesn't validate the correctness of the configuration. Consider adding basic validation, such as checking if the connection string is a valid URL or if the table name contains invalid characters.This would help users identify configuration issues early before attempting to establish a connection.
Here's an example implementation:
func configDefault(config ...Config) Config { // Return default config if nothing provided if len(config) < 1 { return ConfigDefault } // Override default config cfg := config[0] // Set default values if cfg.ConnectionString == "" { cfg.ConnectionString = ConfigDefault.ConnectionString } // ...other default settings... + // Basic validation + if !strings.HasPrefix(cfg.ConnectionString, "ws://") && !strings.HasPrefix(cfg.ConnectionString, "wss://") && + !strings.HasPrefix(cfg.ConnectionString, "http://") && !strings.HasPrefix(cfg.ConnectionString, "https://") { + // Don't fail here, just log a warning + log.Printf("Warning: ConnectionString %s doesn't start with ws://, wss://, http:// or https://", cfg.ConnectionString) + } + + // Additional validations could be added here return cfg }
16-18: Clarify the preferred connection protocol.Based on the PR discussions, WebSocket connections work better than HTTP for SurrealDB. Consider enhancing the documentation to recommend using WebSocket connections (ws:// or wss://) over HTTP connections.
// The connection URL to connect to SurrealDB + // WebSocket connections (ws:// or wss://) are recommended over HTTP for better reliability ConnectionString string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
surrealdb/README.md(1 hunks)surrealdb/config.go(1 hunks)surrealdb/surrealdb_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- surrealdb/README.md
- surrealdb/surrealdb_test.go
…ion url validation
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
surrealdb/config.go (2)
20-44: The Config struct is missing the Mode field mentioned in previous reviews.According to past review comments, the Config struct should include a Mode field with supported values like "default", "memory", and "kv" to handle different SurrealDB operating modes.
Add a Mode field to the Config struct:
type Config struct { // WebSocket connections (ws:// or wss://) are recommended over HTTP for better reliability ConnectionString string // The namespace to be used in SurrealDB Namespace string // The database to be used within the specified namespace Database string // The application username to connect to SurrealDB Username string // The application password to connect to SurrealDB Password string // Optional access token or access type Access string // Optional scope for scoped logins (e.g., user-defined scopes) Scope string // The default table used to store key-value records DefaultTable string + + // Mode defines the operating mode ("default", "memory", "kv") + Mode string }
94-96: Missing Mode field initialization in configDefault.The configDefault function doesn't set the default Mode if it's not provided, unlike other fields.
Add the missing default assignment:
if cfg.DefaultTable == "" { cfg.DefaultTable = ConfigDefault.DefaultTable } + + if cfg.Mode == "" { + cfg.Mode = ConfigDefault.Mode + }
🧹 Nitpick comments (3)
surrealdb/config.go (3)
46-55: Default credentials could pose a security risk.The default credentials (
root/root) might be used in production if not explicitly overridden. Consider using empty strings as defaults to force explicit configuration.var ConfigDefault = Config{ ConnectionString: "ws://localhost:8000", Namespace: "fiber_storage", Database: "fiber_storage", - Username: "root", - Password: "root", + Username: "", + Password: "", Access: "full", Scope: "all", DefaultTable: "fiber_storage", }Then update the
Newfunction in surrealdb.go to check if credentials are empty and handle accordingly.
3-6: Import statements could be more comprehensive.Consider adding
fmtfor improved error formatting if implementing validation, and organize imports alphabetically.import ( + "fmt" "log" "strings" )
57-104: Missing configuration options for timeouts and connection pooling.The configuration lacks options for connection timeouts, retry policies, and connection pooling, which are important for production deployments.
Consider extending the Config struct to include these additional options:
type Config struct { // ... existing fields ... + // ConnectionTimeout specifies the maximum time to wait for a connection + ConnectionTimeout time.Duration + + // MaxConnections specifies the maximum number of connections in the pool + MaxConnections int + + // RetryAttempts specifies the number of retry attempts for failed operations + RetryAttempts int }Then update the corresponding defaults in ConfigDefault and initialization in configDefault.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
surrealdb/surrealdb.go (1)
50-57: 🛠️ Refactor suggestionAdd cleanup on authentication failure.
If authentication fails, the connection should be closed to prevent resource leaks.
token, err := db.SignIn(authData) if err != nil { - panic(err) + // Close the connection to prevent leaks + _ = db.Close() + return nil, fmt.Errorf("failed to sign in: %w", err) } if err = db.Authenticate(token); err != nil { - panic(err) + // Close the connection to prevent leaks + _ = db.Close() + return nil, fmt.Errorf("failed to authenticate: %w", err) }
🧹 Nitpick comments (9)
surrealdb/config.go (2)
103-106: Connection string validation could be improved.Currently, invalid connection strings only trigger a warning log. Consider providing a validation method that users can call to check configuration validity before initializing storage.
// Add a validation method to Config struct + +// Validate checks if the configuration is valid and returns an error if not +func (c *Config) Validate() error { + if !strings.HasPrefix(c.ConnectionString, "ws://") && !strings.HasPrefix(c.ConnectionString, "wss://") && + !strings.HasPrefix(c.ConnectionString, "http://") && !strings.HasPrefix(c.ConnectionString, "https://") { + return fmt.Errorf("invalid connection string format: %s", c.ConnectionString) + } + return nil +}This would allow users to validate their configuration before using it, and the
Newfunction insurrealdb.gocould call this validation method.
108-110: Consider adding an upper bound check for GCInterval.While there's a check for a positive GCInterval, extremely large values might not be practical and could impact performance.
- if int(cfg.GCInterval.Seconds()) <= 0 { + if int(cfg.GCInterval.Seconds()) <= 0 { cfg.GCInterval = ConfigDefault.GCInterval + } else if cfg.GCInterval > time.Hour { + log.Printf("Warning: GCInterval is very large (%v). This might impact performance.", cfg.GCInterval) }surrealdb/surrealdb.go (2)
165-176: Improve error handling in cleanupExpired method.Currently, errors from
Deleteoperations in the cleanup process are silently ignored, which might hide issues.func (s *Storage) cleanupExpired() { records, err := surrealdb.Select[[]model, models.Table](s.db, models.Table(s.table)) if err != nil { + // Consider logging the error or having a way to monitor cleanup failures return } now := time.Now().Unix() for _, item := range *records { if item.Exp > 0 && now > item.Exp { - _ = s.Delete(item.Key) + if err := s.Delete(item.Key); err != nil { + // Consider logging errors during cleanup + // log.Printf("Error deleting expired key %s: %v", item.Key, err) + } } } }
12-17: Consider adding mutex for concurrent access protection.The
Storagestruct doesn't protect against concurrent access to the DB connection, which could lead to race conditions in a multi-goroutine environment.Add a mutex to protect concurrent access:
type Storage struct { db *surrealdb.DB table string stopGC chan struct{} interval time.Duration + mu sync.RWMutex // Protects concurrent access to db }Then use the mutex in methods that access the db field:
func (s *Storage) Get(key string) ([]byte, error) { if len(key) == 0 { return nil, errors.New("key is required") } s.mu.RLock() defer s.mu.RUnlock() recordID := models.NewRecordID(s.table, key) // ... rest of the method }surrealdb/surrealdb_test.go (5)
169-172: Handle returned errors in test setup.Error values from
Set()calls are being ignored. Even in tests, it's good practice to check these errors to ensure proper test setup.- _ = testStore.Set("valid", []byte("123"), 0) + err = testStore.Set("valid", []byte("123"), 0) + require.NoError(t, err) - _ = testStore.Set("expired", []byte("456"), 1*time.Second) + err = testStore.Set("expired", []byte("456"), 1*time.Second) + require.NoError(t, err) time.Sleep(2 * time.Second)
172-172: Replace fixed sleep with a more reliable approach.Using a fixed sleep duration can lead to flaky tests depending on the test environment. Consider using
require.Eventuallyas implemented in other tests.- time.Sleep(2 * time.Second) + require.Eventually(t, func() bool { + val, _ := testStore.Get("expired") + return val == nil + }, 3*time.Second, 100*time.Millisecond)This approach is more reliable and consistent with the
Test_Surrealdb_GetExpiredtest implementation.
214-219: Handle errors for each operation in benchmark.The error check is outside the loop and will only check the last iteration. For comprehensive benchmarking, check errors inside the loop.
for i := 0; i < b.N; i++ { - err = testStore.Set("john", []byte("doe"), 0) + if err = testStore.Set("john", []byte("doe"), 0); err != nil { + b.Fatal(err) + } } - require.NoError(b, err)
232-237: Handle errors for each operation in Get benchmark.Similar to the Set benchmark, the error check is outside the loop and will only check the last iteration.
for i := 0; i < b.N; i++ { - _, err = testStore.Get("john") + _, err = testStore.Get("john") + if err != nil { + b.Fatal(err) + } } - require.NoError(b, err)
247-253: Handle errors for both operations in SetAndDelete benchmark.The error from
Set()is ignored, and the error check forDelete()is outside the loop, only checking the last iteration.for i := 0; i < b.N; i++ { - testStore.Set("john", []byte("doe"), 0) - err = testStore.Delete("john") + if err = testStore.Set("john", []byte("doe"), 0); err != nil { + b.Fatal(err) + } + if err = testStore.Delete("john"); err != nil { + b.Fatal(err) + } } - require.NoError(b, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.github/workflows/benchmark.ymlis excluded by!**/*.ymlsurrealdb/go.modis excluded by!**/*.modsurrealdb/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (4)
surrealdb/README.md(1 hunks)surrealdb/config.go(1 hunks)surrealdb/surrealdb.go(1 hunks)surrealdb/surrealdb_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- surrealdb/README.md
🧰 Additional context used
🧠 Learnings (1)
surrealdb/surrealdb_test.go (2)
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
🧬 Code Graph Analysis (2)
surrealdb/surrealdb.go (1)
surrealdb/config.go (1)
Config(21-48)
surrealdb/surrealdb_test.go (2)
surrealdb/surrealdb.go (2)
Storage(12-17)New(34-68)surrealdb/config.go (1)
Config(21-48)
This PR adds a new storage driver for SurrealDB.
Note: Since SurrealDB SDK is generic, a custom
List()was implemented to fetch all entries.Fixes #1532
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Tests