From d1d7b0e34a2f717725ed054b5ca1d466ba7f0ce0 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Tue, 25 Nov 2025 11:30:41 -0800 Subject: [PATCH] refactor(deletions): improve API based on code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Return LoadResult struct instead of multiple values, with Warnings slice for testability (no more hardcoded stderr output) - Add ID validation in AppendDeletion to prevent invalid records - Add Sync() call in AppendDeletion for durability - Document that timestamps may lose sub-second precision - Document that empty slice in WriteDeletions clears all deletions - Add test for empty ID validation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/deletions/deletions.go | 64 ++++++++++----- internal/deletions/deletions_test.go | 113 ++++++++++++++++----------- 2 files changed, 113 insertions(+), 64 deletions(-) diff --git a/internal/deletions/deletions.go b/internal/deletions/deletions.go index 7eea4ea9..6db1eb29 100644 --- a/internal/deletions/deletions.go +++ b/internal/deletions/deletions.go @@ -13,29 +13,40 @@ import ( ) // DeletionRecord represents a single deletion entry in the manifest. +// Timestamps are serialized as RFC3339 and may lose sub-second precision. type DeletionRecord struct { - ID string `json:"id"` // Issue ID that was deleted - Timestamp time.Time `json:"ts"` // When the deletion occurred - Actor string `json:"by"` // Who performed the deletion + ID string `json:"id"` // Issue ID that was deleted + Timestamp time.Time `json:"ts"` // When the deletion occurred + Actor string `json:"by"` // Who performed the deletion Reason string `json:"reason,omitempty"` // Optional reason for deletion } -// LoadDeletions reads the deletions manifest and returns a map for O(1) lookup. -// It returns the records, the count of skipped (corrupt) lines, and any error. -// Corrupt JSON lines are skipped with a warning rather than failing the load. -func LoadDeletions(path string) (map[string]DeletionRecord, int, error) { +// LoadResult contains the result of loading deletions, including any warnings. +type LoadResult struct { + Records map[string]DeletionRecord + Skipped int + Warnings []string +} + +// LoadDeletions reads the deletions manifest and returns a LoadResult. +// Corrupt JSON lines are skipped rather than failing the load. +// Warnings about skipped lines are collected in LoadResult.Warnings. +func LoadDeletions(path string) (*LoadResult, error) { + result := &LoadResult{ + Records: make(map[string]DeletionRecord), + Warnings: []string{}, + } + f, err := os.Open(path) // #nosec G304 - controlled path from caller if err != nil { if os.IsNotExist(err) { - // No deletions file yet - return empty map - return make(map[string]DeletionRecord), 0, nil + // No deletions file yet - return empty result + return result, nil } - return nil, 0, fmt.Errorf("failed to open deletions file: %w", err) + return nil, fmt.Errorf("failed to open deletions file: %w", err) } defer f.Close() - records := make(map[string]DeletionRecord) - skipped := 0 lineNo := 0 scanner := bufio.NewScanner(f) @@ -51,33 +62,40 @@ func LoadDeletions(path string) (map[string]DeletionRecord, int, error) { var record DeletionRecord if err := json.Unmarshal([]byte(line), &record); err != nil { - // Skip corrupt line with warning to stderr - fmt.Fprintf(os.Stderr, "Warning: skipping corrupt line %d in deletions manifest: %v\n", lineNo, err) - skipped++ + warning := fmt.Sprintf("skipping corrupt line %d in deletions manifest: %v", lineNo, err) + result.Warnings = append(result.Warnings, warning) + result.Skipped++ continue } // Validate required fields if record.ID == "" { - fmt.Fprintf(os.Stderr, "Warning: skipping line %d in deletions manifest: missing ID\n", lineNo) - skipped++ + warning := fmt.Sprintf("skipping line %d in deletions manifest: missing ID", lineNo) + result.Warnings = append(result.Warnings, warning) + result.Skipped++ continue } // Use the most recent record for each ID (last write wins) - records[record.ID] = record + result.Records[record.ID] = record } if err := scanner.Err(); err != nil { - return nil, skipped, fmt.Errorf("error reading deletions file: %w", err) + return nil, fmt.Errorf("error reading deletions file: %w", err) } - return records, skipped, nil + return result, nil } // AppendDeletion appends a single deletion record to the manifest. // Creates the file if it doesn't exist. +// Returns an error if the record has an empty ID. func AppendDeletion(path string, record DeletionRecord) error { + // Validate required fields + if record.ID == "" { + return fmt.Errorf("cannot append deletion record: ID is required") + } + // Ensure directory exists dir := filepath.Dir(path) if err := os.MkdirAll(dir, 0755); err != nil { @@ -102,11 +120,17 @@ func AppendDeletion(path string, record DeletionRecord) error { return fmt.Errorf("failed to write deletion record: %w", err) } + // Sync to ensure durability for append-only log + if err := f.Sync(); err != nil { + return fmt.Errorf("failed to sync deletions file: %w", err) + } + return nil } // WriteDeletions atomically writes the entire deletions manifest. // Used for compaction to deduplicate and prune old entries. +// An empty slice will create an empty file (clearing all deletions). func WriteDeletions(path string, records []DeletionRecord) error { // Ensure directory exists dir := filepath.Dir(path) diff --git a/internal/deletions/deletions_test.go b/internal/deletions/deletions_test.go index 56067046..d455cb64 100644 --- a/internal/deletions/deletions_test.go +++ b/internal/deletions/deletions_test.go @@ -8,16 +8,19 @@ import ( ) func TestLoadDeletions_Empty(t *testing.T) { - // Non-existent file should return empty map - records, skipped, err := LoadDeletions("/nonexistent/path/deletions.jsonl") + // Non-existent file should return empty result + result, err := LoadDeletions("/nonexistent/path/deletions.jsonl") if err != nil { t.Fatalf("expected no error for non-existent file, got: %v", err) } - if skipped != 0 { - t.Errorf("expected 0 skipped, got %d", skipped) + if result.Skipped != 0 { + t.Errorf("expected 0 skipped, got %d", result.Skipped) } - if len(records) != 0 { - t.Errorf("expected empty map, got %d records", len(records)) + if len(result.Records) != 0 { + t.Errorf("expected empty map, got %d records", len(result.Records)) + } + if len(result.Warnings) != 0 { + t.Errorf("expected no warnings, got %d", len(result.Warnings)) } } @@ -48,19 +51,19 @@ func TestRoundTrip(t *testing.T) { } // Load and verify - records, skipped, err := LoadDeletions(path) + result, err := LoadDeletions(path) if err != nil { t.Fatalf("LoadDeletions failed: %v", err) } - if skipped != 0 { - t.Errorf("expected 0 skipped, got %d", skipped) + if result.Skipped != 0 { + t.Errorf("expected 0 skipped, got %d", result.Skipped) } - if len(records) != 2 { - t.Fatalf("expected 2 records, got %d", len(records)) + if len(result.Records) != 2 { + t.Fatalf("expected 2 records, got %d", len(result.Records)) } // Verify record1 - r1, ok := records["bd-123"] + r1, ok := result.Records["bd-123"] if !ok { t.Fatal("record bd-123 not found") } @@ -72,7 +75,7 @@ func TestRoundTrip(t *testing.T) { } // Verify record2 - r2, ok := records["bd-456"] + r2, ok := result.Records["bd-456"] if !ok { t.Fatal("record bd-456 not found") } @@ -96,20 +99,23 @@ this is not valid json t.Fatalf("failed to write test file: %v", err) } - records, skipped, err := LoadDeletions(path) + result, err := LoadDeletions(path) if err != nil { t.Fatalf("LoadDeletions should not fail on corrupt lines: %v", err) } - if skipped != 2 { - t.Errorf("expected 2 skipped lines, got %d", skipped) + if result.Skipped != 2 { + t.Errorf("expected 2 skipped lines, got %d", result.Skipped) } - if len(records) != 3 { - t.Errorf("expected 3 valid records, got %d", len(records)) + if len(result.Records) != 3 { + t.Errorf("expected 3 valid records, got %d", len(result.Records)) + } + if len(result.Warnings) != 2 { + t.Errorf("expected 2 warnings, got %d", len(result.Warnings)) } // Verify valid records were loaded for _, id := range []string{"bd-001", "bd-002", "bd-003"} { - if _, ok := records[id]; !ok { + if _, ok := result.Records[id]; !ok { t.Errorf("expected record %s to be loaded", id) } } @@ -128,16 +134,16 @@ func TestLoadDeletions_MissingID(t *testing.T) { t.Fatalf("failed to write test file: %v", err) } - records, skipped, err := LoadDeletions(path) + result, err := LoadDeletions(path) if err != nil { t.Fatalf("LoadDeletions failed: %v", err) } // Two lines should be skipped: one missing "id" field, one with empty "id" - if skipped != 2 { - t.Errorf("expected 2 skipped lines (missing/empty ID), got %d", skipped) + if result.Skipped != 2 { + t.Errorf("expected 2 skipped lines (missing/empty ID), got %d", result.Skipped) } - if len(records) != 1 { - t.Errorf("expected 1 valid record, got %d", len(records)) + if len(result.Records) != 1 { + t.Errorf("expected 1 valid record, got %d", len(result.Records)) } } @@ -153,18 +159,18 @@ func TestLoadDeletions_LastWriteWins(t *testing.T) { t.Fatalf("failed to write test file: %v", err) } - records, skipped, err := LoadDeletions(path) + result, err := LoadDeletions(path) if err != nil { t.Fatalf("LoadDeletions failed: %v", err) } - if skipped != 0 { - t.Errorf("expected 0 skipped, got %d", skipped) + if result.Skipped != 0 { + t.Errorf("expected 0 skipped, got %d", result.Skipped) } - if len(records) != 1 { - t.Errorf("expected 1 record (deduplicated), got %d", len(records)) + if len(result.Records) != 1 { + t.Errorf("expected 1 record (deduplicated), got %d", len(result.Records)) } - r := records["bd-001"] + r := result.Records["bd-001"] if r.Actor != "user2" { t.Errorf("expected last write to win (user2), got '%s'", r.Actor) } @@ -188,15 +194,15 @@ func TestWriteDeletions_Atomic(t *testing.T) { } // Verify by loading - loaded, skipped, err := LoadDeletions(path) + result, err := LoadDeletions(path) if err != nil { t.Fatalf("LoadDeletions failed: %v", err) } - if skipped != 0 { - t.Errorf("expected 0 skipped, got %d", skipped) + if result.Skipped != 0 { + t.Errorf("expected 0 skipped, got %d", result.Skipped) } - if len(loaded) != 2 { - t.Errorf("expected 2 records, got %d", len(loaded)) + if len(result.Records) != 2 { + t.Errorf("expected 2 records, got %d", len(result.Records)) } } @@ -225,14 +231,14 @@ func TestWriteDeletions_Overwrite(t *testing.T) { } // Verify only compacted records remain - loaded, _, err := LoadDeletions(path) + result, err := LoadDeletions(path) if err != nil { t.Fatalf("LoadDeletions failed: %v", err) } - if len(loaded) != 1 { - t.Errorf("expected 1 record after compaction, got %d", len(loaded)) + if len(result.Records) != 1 { + t.Errorf("expected 1 record after compaction, got %d", len(result.Records)) } - if _, ok := loaded["bd-002"]; !ok { + if _, ok := result.Records["bd-002"]; !ok { t.Error("expected bd-002 to remain after compaction") } } @@ -297,14 +303,33 @@ func TestLoadDeletions_EmptyLines(t *testing.T) { t.Fatalf("failed to write test file: %v", err) } - records, skipped, err := LoadDeletions(path) + result, err := LoadDeletions(path) if err != nil { t.Fatalf("LoadDeletions failed: %v", err) } - if skipped != 0 { - t.Errorf("empty lines should not count as skipped, got %d", skipped) + if result.Skipped != 0 { + t.Errorf("empty lines should not count as skipped, got %d", result.Skipped) } - if len(records) != 2 { - t.Errorf("expected 2 records, got %d", len(records)) + if len(result.Records) != 2 { + t.Errorf("expected 2 records, got %d", len(result.Records)) + } +} + +func TestAppendDeletion_EmptyID(t *testing.T) { + tmpDir := t.TempDir() + path := filepath.Join(tmpDir, "deletions.jsonl") + + record := DeletionRecord{ + ID: "", + Timestamp: time.Now(), + Actor: "testuser", + } + + err := AppendDeletion(path, record) + if err == nil { + t.Fatal("AppendDeletion should fail with empty ID") + } + if err.Error() != "cannot append deletion record: ID is required" { + t.Errorf("unexpected error message: %v", err) } }