cleanup the null cache impl

Author: Thomas Stromberg

cmd/prx/main.go

@@ -17,6 +17,7 @@ import (
 	"time"
 
 	"github.com/codeGROOVE-dev/prx/pkg/prx"
+	"github.com/codeGROOVE-dev/sfcache/pkg/store/null"
 )
 
 const (
@@ -78,7 +79,7 @@ func main() {
 
 	// Configure client options
 	if *noCache {
-		opts = append(opts, prx.WithNoCache())
+		opts = append(opts, prx.WithCacheStore(null.New[string, prx.PullRequestData]()))
 	}
 
 	client := prx.NewClient(token, opts...)

go.mod

@@ -4,6 +4,7 @@ go 1.25.4
 
 require (
 	github.com/codeGROOVE-dev/retry v1.3.0
-	github.com/codeGROOVE-dev/sfcache v1.3.0
-	github.com/codeGROOVE-dev/sfcache/pkg/persist/localfs v1.3.0
+	github.com/codeGROOVE-dev/sfcache v1.4.0
 )
+
+require github.com/codeGROOVE-dev/sfcache/pkg/store/localfs v1.4.0

go.sum

@@ -1,6 +1,6 @@
 github.com/codeGROOVE-dev/retry v1.3.0 h1:/+ipAWRJLL6y1R1vprYo0FSjSBvH6fE5j9LKXjpD54g=
 github.com/codeGROOVE-dev/retry v1.3.0/go.mod h1:8OgefgV1XP7lzX2PdKlCXILsYKuz6b4ZpHa/20iLi8E=
-github.com/codeGROOVE-dev/sfcache v1.3.0 h1:Ew900GWXkZhMEU560kz0Nk1HvhGshiqaIASbTH+ihHg=
-github.com/codeGROOVE-dev/sfcache v1.3.0/go.mod h1:ksV5Y1RwKmOPZZiV0zXpsBOENGUCgO0fVgr/P8f/DJM=
-github.com/codeGROOVE-dev/sfcache/pkg/persist/localfs v1.3.0 h1:7zKbd7aHVzmbK2eEbdsf6Dknrte/o8+7I6GkNob8bGA=
-github.com/codeGROOVE-dev/sfcache/pkg/persist/localfs v1.3.0/go.mod h1:vHDjjehmi+yjXD+DXVG0rnc4iOBsiIknLThqpkYwA/c=
+github.com/codeGROOVE-dev/sfcache v1.4.0 h1:F00PqIYuenn/t2uK72MJ2zUCrrhW8WC5xaV+C16VNE4=
+github.com/codeGROOVE-dev/sfcache v1.4.0/go.mod h1:ksV5Y1RwKmOPZZiV0zXpsBOENGUCgO0fVgr/P8f/DJM=
+github.com/codeGROOVE-dev/sfcache/pkg/store/localfs v1.4.0 h1:oyHwsawHd9kUFdAuXSCKYiEQTXuHAVd4y7cdBatjsLY=
+github.com/codeGROOVE-dev/sfcache/pkg/store/localfs v1.4.0/go.mod h1:D/Y1sYT1m3VXHZRkQjHjFwtKQOsb587QZP/GtfJEDSE=

pkg/prx/cache.go

@@ -1,20 +1,17 @@
 package prx
 
 import (
-	"context"
 	"crypto/sha256"
 	"encoding/hex"
 	"errors"
 	"fmt"
-	"log/slog"
 	"os"
 	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
 
-	"github.com/codeGROOVE-dev/sfcache"
-	"github.com/codeGROOVE-dev/sfcache/pkg/persist/localfs"
+	"github.com/codeGROOVE-dev/sfcache/pkg/store/localfs"
 )
 
 const (
@@ -24,51 +21,6 @@ const (
 	collaboratorsCacheTTL = 4 * time.Hour
 )
 
-// prxCache provides tiered caching with disk persistence using sfcache.
-type prxCache struct {
-	pr     *sfcache.TieredCache[string, PullRequestData]
-	logger *slog.Logger
-}
-
-// newCache creates a new cache with disk persistence at the given directory.
-func newCache(cacheDir string, logger *slog.Logger) (*prxCache, error) {
-	if logger == nil {
-		logger = slog.Default()
-	}
-
-	cleanPath := filepath.Clean(cacheDir)
-	if !filepath.IsAbs(cleanPath) {
-		return nil, errors.New("cache directory must be absolute path")
-	}
-
-	if err := os.MkdirAll(cleanPath, 0o700); err != nil {
-		return nil, fmt.Errorf("creating cache directory: %w", err)
-	}
-
-	prStore, err := localfs.New[string, PullRequestData]("prx-pr", cleanPath)
-	if err != nil {
-		return nil, fmt.Errorf("creating PR cache store: %w", err)
-	}
-
-	prCache, err := sfcache.NewTiered(prStore, sfcache.TTL(prCacheTTL))
-	if err != nil {
-		return nil, fmt.Errorf("creating PR cache: %w", err)
-	}
-
-	return &prxCache{
-		pr:     prCache,
-		logger: logger,
-	}, nil
-}
-
-// close releases cache resources.
-func (c *prxCache) close() error {
-	if c.pr != nil {
-		return c.pr.Close()
-	}
-	return nil
-}
-
 // prCacheKey generates a cache key for PR data.
 func prCacheKey(owner, repo string, prNumber int) string {
 	key := strings.Join([]string{"graphql", "pr_graphql", owner, repo, strconv.Itoa(prNumber)}, "/")
@@ -81,108 +33,34 @@ func collaboratorsCacheKey(owner, repo string) string {
 	return fmt.Sprintf("%s/%s", owner, repo)
 }
 
-// CacheClient wraps the regular Client and adds disk-based caching.
-type CacheClient struct {
-	*Client
-
-	cache *prxCache
-}
-
-// NewCacheClient creates a new caching client with the given cache directory.
-func NewCacheClient(token string, cacheDir string, opts ...Option) (*CacheClient, error) {
-	// Create a temporary logger to use during cache creation
-	logger := slog.Default()
-
-	cache, err := newCache(cacheDir, logger)
+// CacheClient is an alias for Client, kept for backward compatibility.
+//
+// Deprecated: Use NewClient with WithCacheStore instead.
+type CacheClient = Client
+
+// NewCacheClient creates a client with a specific cache directory.
+//
+// Deprecated: Use NewClient with WithCacheStore instead.
+func NewCacheClient(token, dir string, opts ...Option) (*Client, error) {
+	dir = filepath.Clean(dir)
+	if !filepath.IsAbs(dir) {
+		return nil, errors.New("cache directory must be absolute path")
+	}
+	if err := os.MkdirAll(dir, 0o700); err != nil {
+		return nil, fmt.Errorf("creating cache directory: %w", err)
+	}
+	store, err := localfs.New[string, PullRequestData]("prx-pr", dir)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("creating PR cache store: %w", err)
 	}
-
-	// Create client with no cache since CacheClient handles caching
-	opts = append(opts, WithNoCache())
-	client := NewClient(token, opts...)
-
-	// Update logger to the one from client options
-	cache.logger = client.logger
-
-	return &CacheClient{
-		Client: client,
-		cache:  cache,
-	}, nil
+	opts = append(opts, WithCacheStore(store))
+	return NewClient(token, opts...), nil
 }
 
 // Close releases cache resources.
-func (c *CacheClient) Close() error {
-	if c.cache != nil {
-		return c.cache.close()
+func (c *Client) Close() error {
+	if c.prCache != nil {
+		return c.prCache.Close()
 	}
 	return nil
 }
-
-// PullRequest fetches a pull request with all its events and metadata, with caching support.
-func (c *CacheClient) PullRequest(ctx context.Context, owner, repo string, prNumber int, referenceTime time.Time) (*PullRequestData, error) {
-	return c.PullRequestWithReferenceTime(ctx, owner, repo, prNumber, referenceTime)
-}
-
-// PullRequestWithReferenceTime fetches PR data using GetSet for thundering herd protection.
-func (c *CacheClient) PullRequestWithReferenceTime(
-	ctx context.Context, owner, repo string, prNumber int, referenceTime time.Time,
-) (*PullRequestData, error) {
-	if c.cache == nil || c.cache.pr == nil {
-		return c.pullRequestViaGraphQL(ctx, owner, repo, prNumber)
-	}
-
-	cacheKey := prCacheKey(owner, repo, prNumber)
-
-	// Try to get from cache first with time validation
-	cached, found, err := c.cache.pr.Get(ctx, cacheKey)
-	if err != nil {
-		c.logger.WarnContext(ctx, "cache get error",
-			"owner", owner,
-			"repo", repo,
-			"pr", prNumber,
-			"error", err)
-	}
-	if found {
-		// Check if cache entry is fresh enough (cached after reference time)
-		// Note: sfcache handles TTL expiration; we check freshness against referenceTime
-		if cached.CachedAt.After(referenceTime) || cached.CachedAt.Equal(referenceTime) {
-			c.logger.InfoContext(ctx, "cache hit: GraphQL pull request",
-				"owner", owner,
-				"repo", repo,
-				"pr", prNumber,
-				"cached_at", cached.CachedAt)
-			return &cached, nil
-		}
-		c.logger.InfoContext(ctx, "cache miss: GraphQL pull request expired",
-			"owner", owner,
-			"repo", repo,
-			"pr", prNumber,
-			"cached_at", cached.CachedAt,
-			"reference_time", referenceTime)
-		// Delete stale entry so GetSet will fetch fresh data
-		if delErr := c.cache.pr.Delete(ctx, cacheKey); delErr != nil {
-			c.logger.WarnContext(ctx, "failed to delete stale cache entry", "error", delErr)
-		}
-	} else {
-		c.logger.InfoContext(ctx, "cache miss: GraphQL pull request not in cache",
-			"owner", owner,
-			"repo", repo,
-			"pr", prNumber)
-	}
-
-	// Fetch from API using GetSet for thundering herd protection
-	result, err := c.cache.pr.GetSet(ctx, cacheKey, func(ctx context.Context) (PullRequestData, error) {
-		prData, err := c.pullRequestViaGraphQL(ctx, owner, repo, prNumber)
-		if err != nil {
-			return PullRequestData{}, err
-		}
-		prData.CachedAt = time.Now()
-		return *prData, nil
-	})
-	if err != nil {
-		return nil, err
-	}
-
-	return &result, nil
-}

pkg/prx/cache_test.go

@@ -148,7 +148,7 @@ func TestCacheClient(t *testing.T) {
 
 	// First request - should hit the API
 	beforeFirstRequest := requestCount
-	events1, err := client.PullRequest(ctx, "test", "repo", 1, time.Now().Add(-2*time.Hour))
+	events1, err := client.PullRequestWithReferenceTime(ctx, "test", "repo", 1, time.Now().Add(-2*time.Hour))
 	if err != nil {
 		t.Fatalf("First request failed: %v", err)
 	}
@@ -164,7 +164,7 @@ func TestCacheClient(t *testing.T) {
 
 	// Second request with same reference time - should use cache for most endpoints
 	beforeSecondRequest := requestCount
-	events2, err := client.PullRequest(ctx, "test", "repo", 1, time.Now().Add(-2*time.Hour))
+	events2, err := client.PullRequestWithReferenceTime(ctx, "test", "repo", 1, time.Now().Add(-2*time.Hour))
 	if err != nil {
 		t.Fatalf("Second request failed: %v", err)
 	}
@@ -181,7 +181,7 @@ func TestCacheClient(t *testing.T) {
 
 	// Third request with future reference time - should hit API again
 	beforeThirdRequest := requestCount
-	_, err = client.PullRequest(ctx, "test", "repo", 1, time.Now().Add(1*time.Hour))
+	_, err = client.PullRequestWithReferenceTime(ctx, "test", "repo", 1, time.Now().Add(1*time.Hour))
 	if err != nil {
 		t.Fatalf("Third request failed: %v", err)
 	}