diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index 6d5d4dc..f644fa7 100644 --- a/internal/imgcache/cache.go +++ b/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) } diff --git a/internal/imgcache/divzero_test.go b/internal/imgcache/divzero_test.go new file mode 100644 index 0000000..160d480 --- /dev/null +++ b/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) + } +} diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index 32fe2ba..5c90256 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -169,8 +169,8 @@ func (s *Service) processFromSourceOrFetch( } } - // Fetch from upstream if we don't have source data - if sourceData == nil { + // Fetch from upstream if we don't have source data or it's empty + if len(sourceData) == 0 { resp, err := s.fetchAndProcess(ctx, req, cacheKey) if err != nil { return nil, err @@ -280,7 +280,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, diff --git a/internal/imgcache/signature.go b/internal/imgcache/signature.go index 892c496..1795b1f 100644 --- a/internal/imgcache/signature.go +++ b/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 - path = fmt.Sprintf("/v1/image/%s%s/%s.%s", - req.SourceHost, - req.SourcePath, - sizeStr, - req.Format, - ) - - // Append query if present + // Build the path. + // When a source query is present, it is embedded as a path segment + // (e.g. /host/path?query/size.fmt) so that ParseImagePath can extract + // it from the last-slash split. The "?" inside a path segment is + // percent-encoded by clients but chi delivers it decoded, which is + // exactly what the URL parser expects. if req.SourceQuery != "" { - // The query goes before the size segment in our URL scheme - // So we need to rebuild the path - path = fmt.Sprintf("/v1/image/%s%s?%s/%s.%s", + path = fmt.Sprintf("/v1/image/%s%s%%3F%s/%s.%s", + req.SourceHost, + 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, ) diff --git a/internal/imgcache/signature_query_test.go b/internal/imgcache/signature_query_test.go new file mode 100644 index 0000000..da05d6f --- /dev/null +++ b/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) + } +} diff --git a/internal/imgcache/stats_test.go b/internal/imgcache/stats_test.go new file mode 100644 index 0000000..5b36a3d --- /dev/null +++ b/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) + } +}