Fix: Implement and test basic key-value storeImplement basic key-value store with persistence and tests #49
Conversation
WalkthroughImplements a persistent, thread-safe key-value store with constructor, Load/Save (atomic file writes), Set/Get/Delete with RWMutex, and sentinel ErrKeyNotFound. Tests updated to exercise persistence, missing-key behavior, delete semantics, and loading a missing file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test
participant S as KeyValueStore
participant FS as FileSystem
Note over T,S: Persisting data
T->>S: Set(key1, val1)
T->>S: Set(key2, val2)
T->>S: Save()
activate S
S->>FS: create temp file in same dir
FS-->>S: file handle / error
S->>FS: write buffered "key=value\n" lines
FS-->>S: bytes written / error
S->>FS: fs.Rename(temp -> target) /* atomic */
FS-->>S: success / error
S-->>T: return err/nil
deactivate S
Note over T,S: Loading persisted data (new instance)
T->>S: NewKeyValueStore(path)
T->>S: Load()
activate S
S->>FS: open file for read
FS-->>S: file handle / not found
alt file exists
S->>S: parse lines -> map under write lock
S-->>T: return nil
else file missing
S-->>T: return nil (no-op)
end
deactivate S
Note over T,S: Lookup / Delete
T->>S: Get(key1)
S-->>T: value or ErrKeyNotFound
T->>S: Delete(key1)
S-->>T: nil or ErrKeyNotFound
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (6)
internal/exercises/templates/35_basic_key_value_store/key_value_store.go (3)
42-64: Avoid holding the write lock during file I/O; replace state atomically and handle long lines.Reading from disk while holding s.mu blocks all readers/writers and can stall the app with large files. Also, Scanner’s default 64K token limit risks ErrTooLong. Build a temporary map without locking, then swap under a short write lock; increase the scanner buffer and trim whitespace. This also clears stale keys by replacing the map rather than merging.
Apply this diff:
func (s *KeyValueStore) Load() error { - s.mu.Lock() - defer s.mu.Unlock() - - file, err := os.Open(s.filepath) + file, err := os.Open(s.filepath) if err != nil { if os.IsNotExist(err) { return nil } return err } defer file.Close() - scanner := bufio.NewScanner(file) - for scanner.Scan() { - line := scanner.Text() - parts := strings.SplitN(line, "=", 2) - if len(parts) == 2 { - s.data[parts[0]] = parts[1] - } - } - - return scanner.Err() + scanner := bufio.NewScanner(file) + scanner.Buffer(make(byte, 0), 1024*1024) // allow long lines (1MB) + tmp := make(map[string]string) + for scanner.Scan() { + line := scanner.Text() + if line == "" { + continue + } + parts := strings.SplitN(line, "=", 2) + if len(parts) == 2 { + k := strings.TrimSpace(parts[0]) + v := strings.TrimSpace(parts[1]) + tmp[k] = v + } + } + if err := scanner.Err(); err != nil { + return err + } + s.mu.Lock() + s.data = tmp + s.mu.Unlock() + return nil }
66-84: Make Save atomic and reduce lock scope to avoid torn files and long write lock holds.Current Save holds a write lock across disk I/O and truncates the file in-place; a crash during write can corrupt the store. Snapshot under RLock, write to a temp file, fsync, then rename to the target (atomic on most OSes).
Apply these diffs (also adds a small import):
import ( "bufio" "fmt" "os" + "path/filepath" "strings" "sync" )func (s *KeyValueStore) Save() error { - s.mu.Lock() - defer s.mu.Unlock() - file, err := os.Create(s.filepath) - if err != nil { - return err - } - defer file.Close() - - writer := bufio.NewWriter(file) - for k, v := range s.data { - _, err := fmt.Fprintf(writer, "%s=%s\n", k, v) - if err != nil { - return err - } - } - return writer.Flush() + // Snapshot under read lock + s.mu.RLock() + snapshot := make(map[string]string, len(s.data)) + for k, v := range s.data { + snapshot[k] = v + } + s.mu.RUnlock() + + dir := filepath.Dir(s.filepath) + tmp, err := os.CreateTemp(dir, ".kvtmp-*") + if err != nil { + return err + } + writer := bufio.NewWriter(tmp) + for k, v := range snapshot { + if _, err := fmt.Fprintf(writer, "%s=%s\n", k, v); err != nil { + tmp.Close() + _ = os.Remove(tmp.Name()) + return err + } + } + if err := writer.Flush(); err != nil { + tmp.Close() + _ = os.Remove(tmp.Name()) + return err + } + if err := tmp.Sync(); err != nil { + tmp.Close() + _ = os.Remove(tmp.Name()) + return err + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmp.Name()) + return err + } + return os.Rename(tmp.Name(), s.filepath) }
24-30: Use a sentinel error for missing keys to enable errors.Is checks and avoid allocations.Returning new &StoreError each time makes comparisons clumsy. Use a package-level sentinel and reuse it in Get/Delete.
Apply these diffs:
type StoreError struct { Message string } func (e *StoreError) Error() string { return e.Message } + +// Sentinel error for missing keys. +var ErrKeyNotFound = &StoreError{Message: "key not found"}val, ok := s.data[key] if !ok { - return "", &StoreError{Message: "key not found"} + return "", ErrKeyNotFound } return val, nilif _, ok := s.data[key]; !ok { - return &StoreError{Message: "key not found"} + return ErrKeyNotFound } delete(s.data, key) return nilAlso applies to: 93-102, 104-113
internal/exercises/templates/35_basic_key_value_store/key_value_store_test.go (3)
13-16: Use t.TempDir instead of manual cleanup; drop ad-hoc helper.Avoids leftover files and enables safe parallel runs.
Apply these diffs (also updates imports):
import ( - "os" + "errors" + "path/filepath" "testing" )-// helper to clean up after tests -func cleanup(filename string) { - _ = os.Remove(filename) -} +// no-op: using t.TempDir in tests belowfunc TestLoadAndSave(t *testing.T) { - filename := "test_store.db" - defer cleanup(filename) + dir := t.TempDir() + filename := filepath.Join(dir, "test_store.db")func TestMissingKey(t *testing.T) { - filename := "test_store_missing.db" - defer cleanup(filename) + dir := t.TempDir() + filename := filepath.Join(dir, "test_store_missing.db")Also applies to: 55-58
61-65: Assert the precise error using errors.Is and the sentinel.Stronger assertion and future-proofing.
Apply this diff:
- _, err := store.Get("notthere") - if err == nil { - t.Errorf("expected error for missing key, got nil") - } + _, err := store.Get("notthere") + if !errors.Is(err, ErrKeyNotFound) { + t.Errorf("expected ErrKeyNotFound, got %v", err) + }
1-66: Consider adding Delete and “missing file Load()” tests.
- Add a Delete test (delete once ok, twice returns ErrKeyNotFound; Get after delete returns ErrKeyNotFound).
- Add a Load() on non-existent file test (should return nil).
I can open a follow-up PR; or you can paste this test:
func TestDelete(t *testing.T) { t.Parallel() dir := t.TempDir() filename := filepath.Join(dir, "test_store.db") store := NewKeyValueStore(filename) store.Set("k", "v") if err := store.Delete("k"); err != nil { t.Fatalf("unexpected delete error: %v", err) } if _, err := store.Get("k"); !errors.Is(err, ErrKeyNotFound) { t.Fatalf("expected ErrKeyNotFound after delete, got %v", err) } if err := store.Delete("k"); !errors.Is(err, ErrKeyNotFound) { t.Fatalf("expected ErrKeyNotFound on second delete, got %v", err) } } func TestLoadMissingFileIsNoop(t *testing.T) { t.Parallel() dir := t.TempDir() filename := filepath.Join(dir, "does_not_exist.db") store := NewKeyValueStore(filename) if err := store.Load(); err != nil { t.Fatalf("expected nil error on missing file, got %v", err) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/exercises/templates/35_basic_key_value_store/key_value_store.go(2 hunks)internal/exercises/templates/35_basic_key_value_store/key_value_store_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/exercises/templates/35_basic_key_value_store/key_value_store_test.go (1)
internal/exercises/templates/35_basic_key_value_store/key_value_store.go (1)
NewKeyValueStore(32-38)
🔇 Additional comments (2)
internal/exercises/templates/35_basic_key_value_store/key_value_store.go (2)
18-22: Solid, idiomatic state + locking.Map + RWMutex + filepath fields look good for a basic, thread-safe store.
32-37: Constructor is correct.Initializes map and records path; no issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
internal/exercises/templates/35_basic_key_value_store/key_value_store.go (5)
91-97: Make persisted output deterministic (sorted keys).This stabilizes diffs and eases debugging.
-writer := bufio.NewWriter(tmp) -for k, v := range snapshot { - if _, err := fmt.Fprintf(writer, "%s=%s\n", k, v); err != nil { +writer := bufio.NewWriter(tmp) +keys := make([]string, 0, len(snapshot)) +for k := range snapshot { + keys = append(keys, k) +} +sort.Strings(keys) +for _, k := range keys { + v := snapshot[k] + if _, err := fmt.Fprintf(writer, "%s=%s\n", k, v); err != nil { tmp.Close() _ = os.Remove(tmp.Name()) return err } }Add import:
import ( "bufio" + "errors" "fmt" "os" "path/filepath" + "sort" "strings" "sync" )
57-63: Preserve value whitespace; only trim keys.Trimming value loses intentional leading/trailing spaces on round-trip.
- k := strings.TrimSpace(parts[0]) - v := strings.TrimSpace(parts[1]) + k := strings.TrimSpace(parts[0]) + v := parts[1] // preserve value as-is tmp[k] = vIf preserving value whitespace is undesirable, document it explicitly in the package comment.
57-63: Guard against non-round-trippable keys/values.Newlines in values and '=' in keys break the parser. Either encode (e.g., base64 or JSON lines) or validate inputs in Set (reject keys containing "\n", "\r", or "=").
Example minimal validation in Set:
func (s *KeyValueStore) Set(key, value string) { s.mu.Lock() defer s.mu.Unlock() + if strings.ContainsAny(key, "\n\r=") { + // alternatively: log/return error by changing signature + key = strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(key, "\n", " "), "\r", " "), "=", ":") + } s.data[key] = value }Also applies to: 115-120
19-29: *Return error, not StoreError (idiomatic API).Using concrete *StoreError in signatures is unusual; prefer error with a sentinel error value.
Imports:
import ( "bufio" + "errors" "fmt" "os" "path/filepath" "strings" "sync" )Replace custom type + sentinel:
-type StoreError struct { - Message string -} - -func (e *StoreError) Error() string { return e.Message } - -// Sentinel error for missing keys. -var ErrKeyNotFound = &StoreError{Message: "key not found"} +// Sentinel error for missing keys. +var ErrKeyNotFound = errors.New("key not found")Adjust Get/Delete:
-func (s *KeyValueStore) Get(key string) (string, *StoreError) { +func (s *KeyValueStore) Get(key string) (string, error) { s.mu.RLock() defer s.mu.RUnlock() val, ok := s.data[key] if !ok { return "", ErrKeyNotFound } return val, nil } -func (s *KeyValueStore) Delete(key string) *StoreError { +func (s *KeyValueStore) Delete(key string) error { s.mu.Lock() defer s.mu.Unlock() if _, ok := s.data[key]; !ok { return ErrKeyNotFound } delete(s.data, key) return nil }Tests already use errors.Is and should continue to pass.
Also applies to: 122-131, 133-142, 3-10
30-36: Avoid parameter name shadowing the filepath import.Minor readability nit.
-func NewKeyValueStore(filepath string) *KeyValueStore { +func NewKeyValueStore(path string) *KeyValueStore { return &KeyValueStore{ data: make(map[string]string), - filepath: filepath, + filepath: path, } }internal/exercises/templates/35_basic_key_value_store/key_value_store_test.go (2)
9-19: Run this test in parallel.Safe with t.TempDir; speeds up the suite.
-func TestLoadAndSave(t *testing.T) { +func TestLoadAndSave(t *testing.T) { + t.Parallel()
67-76: Nice: Load is a no-op for absent file.Consider adding a companion test that Save creates missing parent directories (after implementing MkdirAll).
I can add tests for:
- Save to nested non-existent dir.
- Round-trip of values with leading/trailing spaces or '='.
Want me to push those?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/exercises/templates/35_basic_key_value_store/key_value_store.go(2 hunks)internal/exercises/templates/35_basic_key_value_store/key_value_store_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/exercises/templates/35_basic_key_value_store/key_value_store_test.go (1)
internal/exercises/templates/35_basic_key_value_store/key_value_store.go (2)
NewKeyValueStore(31-36)ErrKeyNotFound(28-28)
🔇 Additional comments (4)
internal/exercises/templates/35_basic_key_value_store/key_value_store.go (2)
68-71: LGTM: atomic map replacement under write lock.Clear, simple, and avoids partial state during Load.
112-113: Confirm atomic rename semantics on target OS.os.Rename within the same directory is typically atomic; on some platforms behavior differs when destination exists.
Would Windows support be required? If yes, consider documenting guarantees or switching to a well-known atomic write helper (e.g., rename-on-same-dir with replace semantics).
Also applies to: 84-89
internal/exercises/templates/35_basic_key_value_store/key_value_store_test.go (2)
35-46: LGTM: correct sentinel check for missing key.
48-66: LGTM: delete semantics covered (including idempotent second delete).
| dir := filepath.Dir(s.filepath) | ||
| tmp, err := os.CreateTemp(dir, ".kvtmp-*") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Create parent directory before atomic write.
Save fails if s.filepath’s directory doesn’t exist. Ensure dir is created.
dir := filepath.Dir(s.filepath)
+if err := os.MkdirAll(dir, 0o755); err != nil {
+ return err
+}
tmp, err := os.CreateTemp(dir, ".kvtmp-*")
if err != nil {
return err
}📝 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.
| dir := filepath.Dir(s.filepath) | |
| tmp, err := os.CreateTemp(dir, ".kvtmp-*") | |
| if err != nil { | |
| return err | |
| } | |
| dir := filepath.Dir(s.filepath) | |
| if err := os.MkdirAll(dir, 0o755); err != nil { | |
| return err | |
| } | |
| tmp, err := os.CreateTemp(dir, ".kvtmp-*") | |
| if err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
In internal/exercises/templates/35_basic_key_value_store/key_value_store.go
around lines 84 to 89, the code attempts to create a temp file in dir but fails
when the parent directory for s.filepath does not exist; call os.MkdirAll(dir,
0o755) (and check/return its error) before os.CreateTemp to ensure the parent
directory exists, then proceed with creating the temp file and the existing
atomic write flow.
|
@tendinginfinity24: Thank you so much for contributing to the issue!! Really appreaciate your efforts!
Already made those changes you can just move your change to that folder |
kaushalyap
left a comment
kaushalyap
left a comment
There was a problem hiding this comment.
Haven't updated the catalog.yaml
@kaushalyap Not just that, exercise has been solved, that is wrong. This solved should go to solution and exercise should remain as it is, incomplete. |
|
@zhravan yeah, also this should go under projects part. There are other things to discuss, I'll message you on Discord |
3 participants
This PR implements the KeyValueStore with the following features:
Thread-safe in-memory key-value storage using sync.RWMutex.
Persistent storage with Load() and Save() methods.
Set, Get, and Delete operations with proper error handling.
Added and fixed unit tests to validate functionality (TestLoadAndSave, TestMissingKey).
✅ All tests are passing.
Closes #44
Summary by CodeRabbit
New Features
Tests