From 1aed59469075569a47bc77682018a6095e849ea6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 31 Mar 2026 12:05:02 +0100 Subject: [PATCH] refactor(cache): AX compliance pass 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove banned encoding/json import; Entry.Data is now string using core.JSONMarshal/JSONMarshalString. Rename ds→separator, cwd→workingDirectory throughout. Add missing Good/Bad/Ugly tests for all public functions (29 tests, all pass). Co-Authored-By: Virgil --- cache.go | 55 +++++++------- cache_test.go | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 222 insertions(+), 28 deletions(-) diff --git a/cache.go b/cache.go index 0c36711..ba97f14 100644 --- a/cache.go +++ b/cache.go @@ -4,7 +4,6 @@ package cache import ( - "encoding/json" "io/fs" "time" @@ -28,9 +27,9 @@ type Cache struct { // Entry is the serialized cache record written to the backing Medium. type Entry struct { - Data json.RawMessage `json:"data"` - CachedAt time.Time `json:"cached_at"` - ExpiresAt time.Time `json:"expires_at"` + Data string `json:"data"` + CachedAt time.Time `json:"cached_at"` + ExpiresAt time.Time `json:"expires_at"` } // New creates a cache and applies default Medium, base directory, and TTL values @@ -43,12 +42,12 @@ func New(medium coreio.Medium, baseDir string, ttl time.Duration) (*Cache, error } if baseDir == "" { - cwd := currentDir() - if cwd == "" || cwd == "." { + workingDirectory := currentDir() + if workingDirectory == "" || workingDirectory == "." { return nil, core.E("cache.New", "failed to resolve current working directory", nil) } - baseDir = normalizePath(core.JoinPath(cwd, ".core", "cache")) + baseDir = normalizePath(core.JoinPath(workingDirectory, ".core", "cache")) } else { baseDir = absolutePath(baseDir) } @@ -123,8 +122,8 @@ func (c *Cache) Get(key string, dest any) (bool, error) { return false, nil } - if err := core.JSONUnmarshal(entry.Data, dest); !err.OK { - return false, core.E("cache.Get", "failed to unmarshal cached data", err.Value.(error)) + if result := core.JSONUnmarshalString(entry.Data, dest); !result.OK { + return false, core.E("cache.Get", "failed to unmarshal cached data", result.Value.(error)) } return true, nil @@ -152,18 +151,18 @@ func (c *Cache) Set(key string, data any) error { return core.E("cache.Set", "failed to marshal cache data", dataResult.Value.(error)) } - ttl := c.ttl - if ttl < 0 { + cacheTTL := c.ttl + if cacheTTL < 0 { return core.E("cache.Set", "cache ttl must be >= 0", nil) } - if ttl == 0 { - ttl = DefaultTTL + if cacheTTL == 0 { + cacheTTL = DefaultTTL } entry := Entry{ - Data: dataResult.Value.([]byte), + Data: string(dataResult.Value.([]byte)), CachedAt: time.Now(), - ExpiresAt: time.Now().Add(ttl), + ExpiresAt: time.Now().Add(cacheTTL), } entryResult := core.JSONMarshal(entry) @@ -258,22 +257,22 @@ func GitHubRepoKey(org, repo string) string { } func pathSeparator() string { - if ds := core.Env("DS"); ds != "" { - return ds + if separator := core.Env("DS"); separator != "" { + return separator } return "/" } func normalizePath(path string) string { - ds := pathSeparator() - normalized := core.Replace(path, "\\", ds) + separator := pathSeparator() + normalized := core.Replace(path, "\\", separator) - if ds != "/" { - normalized = core.Replace(normalized, "/", ds) + if separator != "/" { + normalized = core.Replace(normalized, "/", separator) } - return core.CleanPath(normalized, ds) + return core.CleanPath(normalized, separator) } func absolutePath(path string) string { @@ -282,18 +281,18 @@ func absolutePath(path string) string { return normalized } - cwd := currentDir() - if cwd == "" || cwd == "." { + workingDirectory := currentDir() + if workingDirectory == "" || workingDirectory == "." { return normalized } - return normalizePath(core.JoinPath(cwd, normalized)) + return normalizePath(core.JoinPath(workingDirectory, normalized)) } func currentDir() string { - cwd := normalizePath(core.Env("PWD")) - if cwd != "" && cwd != "." { - return cwd + workingDirectory := normalizePath(core.Env("PWD")) + if workingDirectory != "" && workingDirectory != "." { + return workingDirectory } return normalizePath(core.Env("DIR_CWD")) diff --git a/cache_test.go b/cache_test.go index b38f57b..8d2c702 100644 --- a/cache_test.go +++ b/cache_test.go @@ -75,6 +75,32 @@ func TestCache_New_Bad(t *testing.T) { } } +func TestCache_New_Ugly(t *testing.T) { + // New with zero ttl falls back to DefaultTTL; verify a set entry uses it. + c, m := newTestCache(t, "/tmp/cache-default-ttl", 0) + + const key = "ugly-key" + if err := c.Set(key, map[string]string{"x": "y"}); err != nil { + t.Fatalf("Set failed: %v", err) + } + + path, err := c.Path(key) + if err != nil { + t.Fatalf("Path failed: %v", err) + } + + raw, err := m.Read(path) + if err != nil { + t.Fatalf("Read failed: %v", err) + } + + entry := readEntry(t, raw) + appliedTTL := entry.ExpiresAt.Sub(entry.CachedAt) + if appliedTTL < cache.DefaultTTL || appliedTTL > cache.DefaultTTL+time.Second { + t.Fatalf("expected ttl near %v, got %v", cache.DefaultTTL, appliedTTL) + } +} + func TestCache_Path_Good(t *testing.T) { c, _ := newTestCache(t, "/tmp/cache-path", time.Minute) @@ -98,6 +124,15 @@ func TestCache_Path_Bad(t *testing.T) { } } +func TestCache_Path_Ugly(t *testing.T) { + // Path on a nil receiver returns an error rather than panicking. + var c *cache.Cache + _, err := c.Path("any-key") + if err == nil { + t.Fatal("expected Path to fail on nil receiver") + } +} + func TestCache_Get_Good(t *testing.T) { c, _ := newTestCache(t, "/tmp/cache", time.Minute) @@ -121,6 +156,20 @@ func TestCache_Get_Good(t *testing.T) { } } +func TestCache_Get_Bad(t *testing.T) { + // Get on a missing key returns (false, nil) — not an error. + c, _ := newTestCache(t, "/tmp/cache-get-bad", time.Minute) + + var retrieved map[string]string + found, err := c.Get("nonexistent-key", &retrieved) + if err != nil { + t.Fatalf("expected no error for missing key, got: %v", err) + } + if found { + t.Fatal("expected found=false for missing key") + } +} + func TestCache_Get_Ugly(t *testing.T) { c, _ := newTestCache(t, "/tmp/cache-expiry", 10*time.Millisecond) @@ -152,6 +201,84 @@ func TestCache_Age_Good(t *testing.T) { } } +func TestCache_Age_Bad(t *testing.T) { + // Age returns -1 for a key that was never written. + c, _ := newTestCache(t, "/tmp/cache-age-bad", time.Minute) + + if age := c.Age("missing-key"); age != -1 { + t.Errorf("expected -1 for missing key, got %v", age) + } +} + +func TestCache_Age_Ugly(t *testing.T) { + // Age returns -1 when the stored file contains corrupt JSON. + c, medium := newTestCache(t, "/tmp/cache-age-ugly", time.Minute) + + path, err := c.Path("corrupt-key") + if err != nil { + t.Fatalf("Path failed: %v", err) + } + + medium.Files[path] = "not-valid-json" + + if age := c.Age("corrupt-key"); age != -1 { + t.Errorf("expected -1 for corrupt entry, got %v", age) + } +} + +func TestCache_Set_Good(t *testing.T) { + // Set writes a valid entry that Get can later retrieve. + c, _ := newTestCache(t, "/tmp/cache-set-good", time.Minute) + + if err := c.Set("set-key", map[string]int{"count": 42}); err != nil { + t.Fatalf("Set failed: %v", err) + } + + var retrieved map[string]int + found, err := c.Get("set-key", &retrieved) + if err != nil { + t.Fatalf("Get after Set failed: %v", err) + } + if !found { + t.Fatal("expected to find item after Set") + } + if retrieved["count"] != 42 { + t.Errorf("expected count=42, got %v", retrieved["count"]) + } +} + +func TestCache_Set_Bad(t *testing.T) { + // Set on a nil receiver returns an error rather than panicking. + var c *cache.Cache + if err := c.Set("key", map[string]string{"a": "b"}); err == nil { + t.Fatal("expected Set to fail on nil receiver") + } +} + +func TestCache_Set_Ugly(t *testing.T) { + // Set overwrites an existing entry; subsequent Get returns the new value. + c, _ := newTestCache(t, "/tmp/cache-set-ugly", time.Minute) + + if err := c.Set("overwrite-key", map[string]string{"v": "first"}); err != nil { + t.Fatalf("first Set failed: %v", err) + } + if err := c.Set("overwrite-key", map[string]string{"v": "second"}); err != nil { + t.Fatalf("second Set failed: %v", err) + } + + var retrieved map[string]string + found, err := c.Get("overwrite-key", &retrieved) + if err != nil { + t.Fatalf("Get failed: %v", err) + } + if !found { + t.Fatal("expected to find item") + } + if retrieved["v"] != "second" { + t.Errorf("expected v=second after overwrite, got %q", retrieved["v"]) + } +} + func TestCache_NilReceiver_Good(t *testing.T) { var c *cache.Cache var target map[string]string @@ -231,6 +358,24 @@ func TestCache_Delete_Good(t *testing.T) { } } +func TestCache_Delete_Bad(t *testing.T) { + // Delete of a key that does not exist returns nil — idempotent. + c, _ := newTestCache(t, "/tmp/cache-delete-bad", time.Minute) + + if err := c.Delete("nonexistent-key"); err != nil { + t.Fatalf("expected Delete of missing key to return nil, got: %v", err) + } +} + +func TestCache_Delete_Ugly(t *testing.T) { + // Delete rejects a path traversal key with an error. + c, _ := newTestCache(t, "/tmp/cache-delete-ugly", time.Minute) + + if err := c.Delete("../../etc/shadow"); err == nil { + t.Fatal("expected Delete to reject path traversal key") + } +} + func TestCache_Clear_Good(t *testing.T) { c, _ := newTestCache(t, "/tmp/cache-clear", time.Minute) data := map[string]string{"foo": "bar"} @@ -255,6 +400,23 @@ func TestCache_Clear_Good(t *testing.T) { } } +func TestCache_Clear_Bad(t *testing.T) { + // Clear on a nil receiver returns an error rather than panicking. + var c *cache.Cache + if err := c.Clear(); err == nil { + t.Fatal("expected Clear to fail on nil receiver") + } +} + +func TestCache_Clear_Ugly(t *testing.T) { + // Clear on an empty cache (no entries written) returns nil — idempotent. + c, _ := newTestCache(t, "/tmp/cache-clear-ugly", time.Minute) + + if err := c.Clear(); err != nil { + t.Fatalf("expected Clear on empty cache to return nil, got: %v", err) + } +} + func TestCache_GitHubReposKey_Good(t *testing.T) { key := cache.GitHubReposKey("myorg") if key != "github/myorg/repos" { @@ -262,9 +424,42 @@ func TestCache_GitHubReposKey_Good(t *testing.T) { } } +func TestCache_GitHubReposKey_Bad(t *testing.T) { + // GitHubReposKey with an empty org still produces a structurally valid path. + key := cache.GitHubReposKey("") + if key == "" { + t.Error("expected a non-empty path even for empty org") + } +} + +func TestCache_GitHubReposKey_Ugly(t *testing.T) { + // GitHubReposKey with an org containing slashes produces a deterministic path. + key := cache.GitHubReposKey("org/sub") + if key == "" { + t.Error("expected a non-empty path for org with slash") + } +} + func TestCache_GitHubRepoKey_Good(t *testing.T) { key := cache.GitHubRepoKey("myorg", "myrepo") if key != "github/myorg/myrepo/meta" { t.Errorf("unexpected GitHubRepoKey: %q", key) } } + +func TestCache_GitHubRepoKey_Bad(t *testing.T) { + // GitHubRepoKey with empty args still returns a non-empty path. + key := cache.GitHubRepoKey("", "") + if key == "" { + t.Error("expected a non-empty path even for empty org and repo") + } +} + +func TestCache_GitHubRepoKey_Ugly(t *testing.T) { + // GitHubRepoKey differentiates between two repositories in the same org. + keyA := cache.GitHubRepoKey("org", "repo-a") + keyB := cache.GitHubRepoKey("org", "repo-b") + if keyA == keyB { + t.Errorf("expected different keys for different repos, both got %q", keyA) + } +}