One image request type instead of five (closes #307)
check / check (push) Canceled after 0s

The new package internal/imagerequest holds the request type, its two
parts Source and Transform, the Format and Fit types, the dimension and
fit checks, the /v1/image/ path parsing and the variant key; it imports
no other package of pixa. imgcache.ParsedURL, imgcache.ImageRequest,
signature.Request, imageprocessor.Request, the second Size, format and
fit types and the conversions between them are gone. The image
processor takes a Transform only. encurl keeps a private struct with the
token's short CBOR field names. VariantKey moves with the function that
makes it. Tests follow the moved and renamed code; the tests of the
removed conversions go with them.

Model: opus-5-5
This commit was merged in pull request #323.
This commit is contained in:
2026-10-21 14:41:07 +02:00
parent 385c0ca76f
commit 00d8ba26c5
68 changed files with 2217 additions and 1978 deletions
+38 -64
View File
@@ -19,6 +19,7 @@ import (
"sneak.berlin/go/pixa/internal/allowlist"
"sneak.berlin/go/pixa/internal/httpfetcher"
"sneak.berlin/go/pixa/internal/imageprocessor"
"sneak.berlin/go/pixa/internal/imagerequest"
"sneak.berlin/go/pixa/internal/magic"
"sneak.berlin/go/pixa/internal/signature"
)
@@ -139,7 +140,9 @@ var ErrProcessingTimedOut = errors.New(
// While its source URL is in the negative cache, a cached variant is still
// served and a variant not in the cache is still made from a cached source;
// only a request that needs the source fetched is refused.
func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, error) {
func (s *Service) Get(
ctx context.Context, req *imagerequest.Request,
) (*ImageResponse, error) {
// Propagate AllowHTTP setting to the request
req.AllowHTTP = s.allowHTTP
@@ -186,7 +189,7 @@ func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, e
}
// Warm pre-fetches and caches an image without returning it.
func (s *Service) Warm(ctx context.Context, req *ImageRequest) error {
func (s *Service) Warm(ctx context.Context, req *imagerequest.Request) error {
_, err := s.Get(ctx, req)
return err
@@ -204,7 +207,7 @@ func (s *Service) WaitForProcessing(ctx context.Context) int {
}
// ValidateRequest validates the request signature if required.
func (s *Service) ValidateRequest(req *ImageRequest) error {
func (s *Service) ValidateRequest(req *imagerequest.Request) error {
// Check if host is allowed (no signature required)
sourceURL := req.SourceURL()
@@ -227,7 +230,7 @@ func (s *Service) ValidateRequest(req *ImageRequest) error {
}
// Signature required for non-allowed hosts
return s.signer.Verify(signatureRequest(req))
return s.signer.Verify(req)
}
// GenerateSignedURL generates a signed URL for the given request. The URL
@@ -236,26 +239,22 @@ func (s *Service) ValidateRequest(req *ImageRequest) error {
// set to 85 or cover, the values the route uses when a URL has no q or fit.
func (s *Service) GenerateSignedURL(
baseURL string,
req *ImageRequest,
req *imagerequest.Request,
ttl time.Duration,
) (string, error) {
if req.Quality == 0 {
req.Quality = 85
if req.Transform.Quality == 0 {
req.Transform.Quality = 85
}
if req.FitMode == "" {
req.FitMode = FitCover
if req.Transform.FitMode == "" {
req.Transform.FitMode = imagerequest.FitCover
}
sigReq := signatureRequest(req)
path, sig, exp := s.signer.GenerateSignedURL(sigReq, ttl)
// Propagate the generated signature and expiration back onto the request.
req.Expires = sigReq.Expires
req.Signature = sigReq.Signature
// Sets the request's Expires and Signature
path, sig, exp := s.signer.GenerateSignedURL(req, ttl)
return fmt.Sprintf("%s%s?sig=%s&exp=%d&q=%d&fit=%s",
baseURL, path, sig, exp, req.Quality, req.FitMode), nil
baseURL, path, sig, exp, req.Transform.Quality, req.Transform.FitMode), nil
}
// errPanicked is returned when processing a variant panicked.
@@ -268,14 +267,14 @@ var errPanicked = errors.New("panic while processing image")
// the first request's cancellation, so the others are still served if that
// client goes away, but keeps its deadline.
func (s *Service) processOrWait(
ctx context.Context, req *ImageRequest,
ctx context.Context, req *imagerequest.Request,
) (*ImageResponse, error) {
// A request that has already ended starts no processing
if ctx.Err() != nil {
return nil, ctx.Err()
}
cacheKey := CacheKey(req)
cacheKey := imagerequest.CacheKey(req)
// Closed when this request's own function runs, which singleflight does
// only when no other request is processing the variant
@@ -293,7 +292,7 @@ func (s *Service) processOrWait(
recovered := recover()
if recovered != nil {
s.log.Error("panic while processing image",
"host", req.SourceHost, "path", req.SourcePath,
"host", req.Source.Host, "path", req.Source.Path,
"panic", recovered, "stack", string(debug.Stack()))
if hub := sentry.GetHubFromContext(ctx); hub != nil {
@@ -393,8 +392,8 @@ func (s *Service) loadCachedSource(
// if available.
func (s *Service) processFromSourceOrFetch(
ctx context.Context,
req *ImageRequest,
cacheKey VariantKey,
req *imagerequest.Request,
cacheKey imagerequest.VariantKey,
) (*processedVariant, error) {
// Check if we have cached source content
contentHash, _, err := s.cache.LookupSource(ctx, req)
@@ -431,8 +430,8 @@ func (s *Service) processFromSourceOrFetch(
// source URL is in the negative cache.
func (s *Service) fetchAndProcess(
ctx context.Context,
req *ImageRequest,
cacheKey VariantKey,
req *imagerequest.Request,
cacheKey imagerequest.VariantKey,
) (*processedVariant, error) {
// Check negative cache - skip fetching for recently-failed URLs
negHit, err := s.cache.checkNegativeCache(ctx, req)
@@ -442,8 +441,8 @@ func (s *Service) fetchAndProcess(
if negHit {
s.log.Debug("negative cache hit",
"host", req.SourceHost,
"path", req.SourcePath,
"host", req.Source.Host,
"path", req.Source.Path,
)
return nil, fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamError, ErrNegativeCached)
@@ -493,8 +492,8 @@ func (s *Service) fetchAndProcess(
// Log upstream fetch details
s.log.Info("upstream fetched",
"request_id", middleware.GetReqID(ctx),
"host", req.SourceHost,
"path", req.SourcePath,
"host", req.Source.Host,
"path", req.Source.Path,
"bytes", fetchBytes,
"fetch_ms", fetchResult.FetchDurationMs,
"rate", downloadRate,
@@ -510,7 +509,7 @@ func (s *Service) fetchAndProcess(
err = magic.ValidateMagicBytes(sourceData, fetchResult.ContentType)
if err != nil {
s.log.Warn("source refused", "request_id", middleware.GetReqID(ctx),
"host", req.SourceHost, "path", req.SourcePath,
"host", req.Source.Host, "path", req.Source.Path,
"content_type", fetchResult.ContentType, "error", err)
s.storeNegative(ctx, req, httpStatusUnsupportedMediaType, err)
@@ -533,12 +532,12 @@ func (s *Service) fetchAndProcess(
// failed with err, so the source is not fetched again until the entry
// expires. A failure to store the entry is only logged.
func (s *Service) storeNegative(
ctx context.Context, req *ImageRequest, statusCode int, err error,
ctx context.Context, req *imagerequest.Request, statusCode int, err error,
) {
storeErr := s.cache.StoreNegative(ctx, req, statusCode, err.Error())
if storeErr != nil {
s.log.Warn("failed to store negative cache entry",
"host", req.SourceHost, "path", req.SourcePath, "error", storeErr)
"host", req.Source.Host, "path", req.Source.Path, "error", storeErr)
}
}
@@ -554,24 +553,17 @@ type processedVariant struct {
// result.
func (s *Service) processAndStore(
ctx context.Context,
req *ImageRequest,
cacheKey VariantKey,
req *imagerequest.Request,
cacheKey imagerequest.VariantKey,
source io.Reader,
fetchBytes int64,
) (*processedVariant, error) {
// Process the image
processStart := time.Now()
processReq := &imageprocessor.Request{
Size: imageprocessor.Size{Width: req.Size.Width, Height: req.Size.Height},
Format: imageprocessor.Format(req.Format),
Quality: req.Quality,
FitMode: imageprocessor.FitMode(req.FitMode),
}
// ctx has the deadline of the request doing the processing, which can
// end while a request waiting for its image has time left
processResult, err := s.processor.Process(ctx, source, processReq)
processResult, err := s.processor.Process(ctx, source, &req.Transform)
if errors.Is(err, context.DeadlineExceeded) {
return nil, fmt.Errorf("%w: %w", ErrProcessingTimedOut, err)
}
@@ -604,10 +596,10 @@ func (s *Service) processAndStore(
s.log.Info("image converted",
"request_id", middleware.GetReqID(ctx),
"host", req.SourceHost,
"path", req.SourcePath,
"host", req.Source.Host,
"path", req.Source.Path,
"src_format", processResult.InputFormat,
"dst_format", req.Format,
"dst_format", req.Transform.Format,
"src_bytes", fetchBytes,
"dst_bytes", outputSize,
"src_dimensions", fmt.Sprintf("%dx%d",
@@ -616,8 +608,8 @@ func (s *Service) processAndStore(
processResult.Width, processResult.Height),
"size_ratio", fmt.Sprintf("%.1f%%", sizePercent),
"convert_ms", processDuration.Milliseconds(),
"quality", req.Quality,
"fit", req.FitMode,
"quality", req.Transform.Quality,
"fit", req.Transform.FitMode,
)
// Store variant to cache
@@ -636,24 +628,6 @@ func (s *Service) processAndStore(
}, nil
}
// signatureRequest projects an ImageRequest onto the standalone
// signature.Request type used by the signature package. This keeps the
// import edge one-way: imgcache depends on signature, never the reverse.
func signatureRequest(req *ImageRequest) *signature.Request {
return &signature.Request{
SourceHost: req.SourceHost,
SourcePath: req.SourcePath,
SourceQuery: req.SourceQuery,
Width: req.Size.Width,
Height: req.Size.Height,
Format: string(req.Format),
Quality: req.Quality,
FitMode: string(req.FitMode),
Signature: req.Signature,
Expires: req.Expires,
}
}
// HTTP status codes for error responses.
const (
httpStatusBadGateway = 502
@@ -680,7 +654,7 @@ func extractStatusCode(err error) int {
const etagHashLength = 16
// formatETag formats a VariantKey as a quoted ETag value.
func formatETag(key VariantKey) string {
func formatETag(key imagerequest.VariantKey) string {
hash := string(key)
// Use first 16 characters of hash for a shorter but still unique ETag
if len(hash) > etagHashLength {