Merge branch 'main' into fix/issue-4

Reviewed-on: #7

internal/imgcache/cache.go

@@ -297,19 +297,21 @@ func (c *Cache) CleanExpired(ctx context.Context) error {
 func (c *Cache) Stats(ctx context.Context) (*CacheStats, error) {
 	var stats CacheStats
 
+	// Fetch hit/miss counts from the stats table
 	err := c.db.QueryRowContext(ctx, `
-		SELECT hit_count, miss_count, upstream_fetch_count, upstream_fetch_bytes, transform_count
+		SELECT hit_count, miss_count
 		FROM cache_stats WHERE id = 1
-	`).Scan(&stats.HitCount, &stats.MissCount, &stats.TotalItems, &stats.TotalSizeBytes, &stats.HitRate)
+	`).Scan(&stats.HitCount, &stats.MissCount)
 
 	if err != nil && !errors.Is(err, sql.ErrNoRows) {
 		return nil, fmt.Errorf("failed to get cache stats: %w", err)
 	}
 
-	// Get actual counts
+	// Get actual item count and total size from content tables
 	_ = c.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM request_cache`).Scan(&stats.TotalItems)
 	_ = c.db.QueryRowContext(ctx, `SELECT COALESCE(SUM(size_bytes), 0) FROM output_content`).Scan(&stats.TotalSizeBytes)
 
+	// Compute hit rate as a ratio
 	if stats.HitCount+stats.MissCount > 0 {
 		stats.HitRate = float64(stats.HitCount) / float64(stats.HitCount+stats.MissCount)
 	}

internal/imgcache/divzero_test.go

@@ -0,0 +1,43 @@
+package imgcache
+
+import (
+	"fmt"
+	"math"
+	"testing"
+)
+
+func TestSizePercentSafeWithZeroFetchBytes(t *testing.T) {
+	// Simulate the calculation from processAndStore
+	fetchBytes := int64(0)
+	outputSize := int64(100)
+
+	var sizePercent float64
+	if fetchBytes > 0 {
+		sizePercent = float64(outputSize) / float64(fetchBytes) * 100.0
+	}
+
+	// sizePercent should be 0, not Inf or NaN
+	if math.IsInf(sizePercent, 0) || math.IsNaN(sizePercent) {
+		t.Errorf("sizePercent = %f, should not be Inf/NaN", sizePercent)
+	}
+
+	// Should produce valid log output
+	result := fmt.Sprintf("%.1f%%", sizePercent)
+	if result != "0.0%" {
+		t.Errorf("formatted size ratio = %q, want %q", result, "0.0%")
+	}
+}
+
+func TestSizePercentNormalCase(t *testing.T) {
+	fetchBytes := int64(1000)
+	outputSize := int64(500)
+
+	var sizePercent float64
+	if fetchBytes > 0 {
+		sizePercent = float64(outputSize) / float64(fetchBytes) * 100.0
+	}
+
+	if math.Abs(sizePercent-50.0) > 0.001 {
+		t.Errorf("sizePercent = %f, want 50.0", sizePercent)
+	}
+}

internal/imgcache/negative_cache_test.go

@@ -1,100 +0,0 @@
-package imgcache
-
-import (
-	"context"
-	"errors"
-	"testing"
-	"time"
-)
-
-func TestNegativeCache_StoreAndCheck(t *testing.T) {
-	db := setupTestDB(t)
-	dir := t.TempDir()
-
-	cache, err := NewCache(db, CacheConfig{
-		StateDir:    dir,
-		CacheTTL:    time.Hour,
-		NegativeTTL: 5 * time.Minute,
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	ctx := context.Background()
-	req := &ImageRequest{
-		SourceHost: "example.com",
-		SourcePath: "/missing.jpg",
-	}
-
-	// Initially should not be in negative cache
-	hit, err := cache.checkNegativeCache(ctx, req)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if hit {
-		t.Error("expected no negative cache hit initially")
-	}
-
-	// Store a negative entry
-	err = cache.StoreNegative(ctx, req, 404, "not found")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// Now should be in negative cache
-	hit, err = cache.checkNegativeCache(ctx, req)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if !hit {
-		t.Error("expected negative cache hit after storing")
-	}
-}
-
-func TestNegativeCache_Expired(t *testing.T) {
-	db := setupTestDB(t)
-	dir := t.TempDir()
-
-	cache, err := NewCache(db, CacheConfig{
-		StateDir:    dir,
-		CacheTTL:    time.Hour,
-		NegativeTTL: 1 * time.Millisecond, // very short TTL
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	ctx := context.Background()
-	req := &ImageRequest{
-		SourceHost: "example.com",
-		SourcePath: "/expired.jpg",
-	}
-
-	// Store a negative entry with very short TTL
-	err = cache.StoreNegative(ctx, req, 500, "server error")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// Wait for expiry
-	time.Sleep(10 * time.Millisecond)
-
-	// Should no longer be in negative cache
-	hit, err := cache.checkNegativeCache(ctx, req)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if hit {
-		t.Error("expected expired negative cache entry to be a miss")
-	}
-}
-
-func TestService_Get_ReturnsErrorForNegativeCachedURL(t *testing.T) {
-	// This test verifies that Service.Get() checks the negative cache
-	// We can't easily test the full pipeline without vips, but we can
-	// verify the error type
-	err := ErrNegativeCached
-	if !errors.Is(err, ErrNegativeCached) {
-		t.Error("ErrNegativeCached should be identifiable with errors.Is")
-	}
-}

internal/imgcache/service.go

@@ -78,24 +78,8 @@ func NewService(cfg *ServiceConfig) (*Service, error) {
 	}, nil
 }
 
-// ErrNegativeCached is returned when a URL is in the negative cache (recently failed).
-var ErrNegativeCached = errors.New("request is in negative cache (recently failed)")
-
 // Get retrieves a processed image, fetching and processing if necessary.
 func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, error) {
-	// Check negative cache first - skip fetching for recently-failed URLs
-	negHit, err := s.cache.checkNegativeCache(ctx, req)
-	if err != nil {
-		s.log.Warn("negative cache check failed", "error", err)
-	}
-	if negHit {
-		s.log.Debug("negative cache hit",
-			"host", req.SourceHost,
-			"path", req.SourcePath,
-		)
-		return nil, fmt.Errorf("%w: %w", ErrUpstreamError, ErrNegativeCached)
-	}
-
 	// Check variant cache first (disk only, no DB)
 	result, err := s.cache.Lookup(ctx, req)
 	if err != nil {
@@ -169,8 +153,8 @@ func (s *Service) processFromSourceOrFetch(
 		}
 	}
 
-	// Fetch from upstream if we don't have source data
+	// Fetch from upstream if we don't have source data or it's empty
-	if sourceData == nil {
+	if len(sourceData) == 0 {
 		resp, err := s.fetchAndProcess(ctx, req, cacheKey)
 		if err != nil {
 			return nil, err
@@ -280,7 +264,11 @@ func (s *Service) processAndStore(
 
 	// Log conversion details
 	outputSize := int64(len(processedData))
-	sizePercent := float64(outputSize) / float64(fetchBytes) * 100.0 //nolint:mnd // percentage calculation
+
+	var sizePercent float64
+	if fetchBytes > 0 {
+		sizePercent = float64(outputSize) / float64(fetchBytes) * 100.0 //nolint:mnd // percentage calculation
+	}
 
 	s.log.Info("image converted",
 		"host", req.SourceHost,

internal/imgcache/signature.go

@@ -6,6 +6,7 @@ import (
 	"encoding/base64"
 	"errors"
 	"fmt"
+	"net/url"
 	"strconv"
 	"time"
 )
@@ -96,22 +97,24 @@ func (s *Signer) GenerateSignedURL(req *ImageRequest, ttl time.Duration) (path s
 		sizeStr = fmt.Sprintf("%dx%d", req.Size.Width, req.Size.Height)
 	}
 
-	// Build the path
+	// Build the path.
-	path = fmt.Sprintf("/v1/image/%s%s/%s.%s",
+	// When a source query is present, it is embedded as a path segment
-		req.SourceHost,
+	// (e.g. /host/path?query/size.fmt) so that ParseImagePath can extract
-		req.SourcePath,
+	// it from the last-slash split. The "?" inside a path segment is
-		sizeStr,
+	// percent-encoded by clients but chi delivers it decoded, which is
-		req.Format,
+	// exactly what the URL parser expects.
-	)
-
-	// Append query if present
 	if req.SourceQuery != "" {
-		// The query goes before the size segment in our URL scheme
+		path = fmt.Sprintf("/v1/image/%s%s%%3F%s/%s.%s",
-		// So we need to rebuild the path
+			req.SourceHost,
-		path = fmt.Sprintf("/v1/image/%s%s?%s/%s.%s",
+			req.SourcePath,
+			url.PathEscape(req.SourceQuery),
+			sizeStr,
+			req.Format,
+		)
+	} else {
+		path = fmt.Sprintf("/v1/image/%s%s/%s.%s",
 			req.SourceHost,
 			req.SourcePath,
-			req.SourceQuery,
 			sizeStr,
 			req.Format,
 		)

internal/imgcache/signature_query_test.go

@@ -0,0 +1,55 @@
+package imgcache
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestGenerateSignedURL_WithQueryString(t *testing.T) {
+	signer := NewSigner("test-secret-key-for-testing!")
+
+	req := &ImageRequest{
+		SourceHost:  "cdn.example.com",
+		SourcePath:  "/photos/cat.jpg",
+		SourceQuery: "token=abc&v=2",
+		Size:        Size{Width: 800, Height: 600},
+		Format:      FormatWebP,
+	}
+
+	path, _, _ := signer.GenerateSignedURL(req, time.Hour)
+
+	// The path must NOT contain a bare "?" that would be interpreted as a query string delimiter.
+	// The size segment must appear as the last path component.
+	if strings.Contains(path, "?token=abc") {
+		t.Errorf("GenerateSignedURL() produced bare query string in path: %q", path)
+	}
+
+	// The size segment must be present in the path
+	if !strings.Contains(path, "/800x600.webp") {
+		t.Errorf("GenerateSignedURL() missing size segment in path: %q", path)
+	}
+
+	// Path should end with the size.format, not with query params
+	if !strings.HasSuffix(path, "/800x600.webp") {
+		t.Errorf("GenerateSignedURL() path should end with size.format: %q", path)
+	}
+}
+
+func TestGenerateSignedURL_WithoutQueryString(t *testing.T) {
+	signer := NewSigner("test-secret-key-for-testing!")
+
+	req := &ImageRequest{
+		SourceHost: "cdn.example.com",
+		SourcePath: "/photos/cat.jpg",
+		Size:       Size{Width: 800, Height: 600},
+		Format:     FormatWebP,
+	}
+
+	path, _, _ := signer.GenerateSignedURL(req, time.Hour)
+
+	expected := "/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp"
+	if path != expected {
+		t.Errorf("GenerateSignedURL() path = %q, want %q", path, expected)
+	}
+}

internal/imgcache/stats_test.go

@@ -0,0 +1,90 @@
+package imgcache
+
+import (
+	"context"
+	"database/sql"
+	"math"
+	"testing"
+	"time"
+
+	"sneak.berlin/go/pixa/internal/database"
+)
+
+func setupStatsTestDB(t *testing.T) *sql.DB {
+	t.Helper()
+	db, err := sql.Open("sqlite", ":memory:")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := database.ApplyMigrations(db); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { db.Close() })
+	return db
+}
+
+func TestStats_HitRateIsRatio(t *testing.T) {
+	db := setupStatsTestDB(t)
+	dir := t.TempDir()
+
+	cache, err := NewCache(db, CacheConfig{
+		StateDir:    dir,
+		CacheTTL:    time.Hour,
+		NegativeTTL: 5 * time.Minute,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	ctx := context.Background()
+
+	// Set some hit/miss counts and a transform_count
+	_, err = db.ExecContext(ctx, `
+		UPDATE cache_stats SET hit_count = 75, miss_count = 25, transform_count = 9999 WHERE id = 1
+	`)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	stats, err := cache.Stats(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if stats.HitCount != 75 {
+		t.Errorf("HitCount = %d, want 75", stats.HitCount)
+	}
+	if stats.MissCount != 25 {
+		t.Errorf("MissCount = %d, want 25", stats.MissCount)
+	}
+
+	// HitRate should be 0.75, NOT 9999 (transform_count)
+	expectedRate := 0.75
+	if math.Abs(stats.HitRate-expectedRate) > 0.001 {
+		t.Errorf("HitRate = %f, want %f (was it scanning transform_count?)", stats.HitRate, expectedRate)
+	}
+}
+
+func TestStats_ZeroCounts(t *testing.T) {
+	db := setupStatsTestDB(t)
+	dir := t.TempDir()
+
+	cache, err := NewCache(db, CacheConfig{
+		StateDir:    dir,
+		CacheTTL:    time.Hour,
+		NegativeTTL: 5 * time.Minute,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	stats, err := cache.Stats(context.Background())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// With zero hits and misses, HitRate should be 0, not some garbage value
+	if stats.HitRate != 0.0 {
+		t.Errorf("HitRate = %f, want 0.0 for zero counts", stats.HitRate)
+	}
+}