sneak/pixa
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Refactor: split internal/imgcache into focused packages #39

New Issue
Open
opened 2026-03-18 04:34:17 +01:00 by clawbot · 1 comment
clawbot commented 2026-03-18 04:34:17 +01:00
Collaborator
Copy Link

Per PR #37 discussion, internal/imgcache/ currently bundles many distinct concerns into a single package. This issue tracks splitting it into focused, independently testable packages.

Already Done

  • imageprocessor → internal/imageprocessor/ — Image format conversion and resizing (extracted in PR #37)
  • allowlist → internal/allowlist/ — Host allow list (extracted in PR #41, renamed from whitelist per sneak's feedback)

Easily Separable

These have minimal coupling to the core cache/service layer and can be extracted with straightforward interface boundaries:

  • fetcher (fetcher.go) → internal/httpfetcher/

    • HTTPFetcher, FetcherConfig, DefaultFetcherConfig, SSRF-safe dialer, rate limiting, content-type validation
    • Only coupled via the Fetcher interface and FetchResult type in imgcache.go
    • mock_fetcher.go can move with it or stay as a test helper

  • signature (signature.go) → internal/signature/

    • Signer, NewSigner, Sign, Verify, GenerateSignedURL, ParseSignatureParams
    • Only uses ImageRequest for building the HMAC payload

  • magic (magic.go) → internal/magic/

    • MIMEType, DetectFormat, ValidateMagicBytes, PeekAndValidate, IsSupportedMIMEType, MIMEToImageFormat, ImageFormatToMIME
    • Only uses ImageFormat type constants

  • urlparser (urlparser.go) → internal/urlparser/

    • ParsedURL, ParseImagePath, ParseImageURL, path traversal validation, size/format parsing
    • Uses ImageRequest, Size, ImageFormat, FitMode — would need to import shared types

Tightly Coupled (keep together)

These share significant state and data flow with the service orchestrator. Splitting them further would require significant interface surgery and may not be worth it:

  • cache (cache.go) — Cache lookup, variant storage, negative cache, stats. Tightly coupled with service.go and storage.go.
  • storage (storage.go) — Content-addressed ContentStorage and MetadataStorage. Created and managed by Cache.
  • service (service.go) — The core orchestrator that wires cache, fetcher, and processor together.
  • types/interfaces (imgcache.go) — ImageRequest, ImageResponse, ImageFormat, FitMode, Size, etc. These are the shared vocabulary used across all subpackages.

Approach

Each extraction should:

  1. Create the new package under internal/
  2. Move types and functions, defining standalone types where needed to avoid circular imports
  3. Update all import sites
  4. Ensure docker build . passes (fmt, lint, test, build)
  5. One PR per package extraction to keep reviews manageable

Naming Conventions

Per sneak's feedback on PR #41:

  • No stuttering names — constructors should be New(), not NewTypeName()
  • Use inclusive terminology (allowlist not whitelist)
Per [PR #37 discussion](https://git.eeqj.de/sneak/pixa/pulls/37#issuecomment-1173), `internal/imgcache/` currently bundles many distinct concerns into a single package. This issue tracks splitting it into focused, independently testable packages. ## Already Done - [x] **imageprocessor** → `internal/imageprocessor/` — Image format conversion and resizing (extracted in [PR #37](https://git.eeqj.de/sneak/pixa/pulls/37)) - [x] **allowlist** → `internal/allowlist/` — Host allow list (extracted in [PR #41](https://git.eeqj.de/sneak/pixa/pulls/41), renamed from whitelist per sneak's feedback) ## Easily Separable These have minimal coupling to the core cache/service layer and can be extracted with straightforward interface boundaries: - [ ] **fetcher** (`fetcher.go`) → `internal/httpfetcher/` - `HTTPFetcher`, `FetcherConfig`, `DefaultFetcherConfig`, SSRF-safe dialer, rate limiting, content-type validation - Only coupled via the `Fetcher` interface and `FetchResult` type in `imgcache.go` - `mock_fetcher.go` can move with it or stay as a test helper - [ ] **signature** (`signature.go`) → `internal/signature/` - `Signer`, `NewSigner`, `Sign`, `Verify`, `GenerateSignedURL`, `ParseSignatureParams` - Only uses `ImageRequest` for building the HMAC payload - [ ] **magic** (`magic.go`) → `internal/magic/` - `MIMEType`, `DetectFormat`, `ValidateMagicBytes`, `PeekAndValidate`, `IsSupportedMIMEType`, `MIMEToImageFormat`, `ImageFormatToMIME` - Only uses `ImageFormat` type constants - [ ] **urlparser** (`urlparser.go`) → `internal/urlparser/` - `ParsedURL`, `ParseImagePath`, `ParseImageURL`, path traversal validation, size/format parsing - Uses `ImageRequest`, `Size`, `ImageFormat`, `FitMode` — would need to import shared types ## Tightly Coupled (keep together) These share significant state and data flow with the service orchestrator. Splitting them further would require significant interface surgery and may not be worth it: - **cache** (`cache.go`) — Cache lookup, variant storage, negative cache, stats. Tightly coupled with `service.go` and `storage.go`. - **storage** (`storage.go`) — Content-addressed `ContentStorage` and `MetadataStorage`. Created and managed by `Cache`. - **service** (`service.go`) — The core orchestrator that wires cache, fetcher, and processor together. - **types/interfaces** (`imgcache.go`) — `ImageRequest`, `ImageResponse`, `ImageFormat`, `FitMode`, `Size`, etc. These are the shared vocabulary used across all subpackages. ## Approach Each extraction should: 1. Create the new package under `internal/` 2. Move types and functions, defining standalone types where needed to avoid circular imports 3. Update all import sites 4. Ensure `docker build .` passes (fmt, lint, test, build) 5. One PR per package extraction to keep reviews manageable ## Naming Conventions Per sneak's feedback on PR #41: - **No stuttering names** — constructors should be `New()`, not `NewTypeName()` - Use inclusive terminology (allowlist not whitelist)
clawbot was assigned by sneak 2026-03-25 02:17:50 +01:00
clawbot removed their assignment 2026-07-25 12:10:11 +02:00
clawbot commented 2026-07-25 12:51:45 +02:00
Author
Collaborator
Copy Link

Progress update on the internal/imgcache split.

Done and merged into main:

  • imageprocessor -> internal/imageprocessor/ (PR #37)
  • allowlist -> internal/allowlist/ (PR #41)
  • magic -> internal/magic/ (PR #42)
  • httpfetcher -> internal/httpfetcher/ (PR #43, merged 2026-07-25)

This pass:

  • Opened PR #46 (refactor/extract-signature) extracting signature -> internal/signature/, based on current main. It is independent of the httpfetcher move, so no dependency on other in-flight branches.
  • Moved Signer (New, Sign, Verify, GenerateSignedURL), ParseParams, and the error sentinels. To keep the import edge one-way, the package defines a standalone signature.Request; imgcache projects its ImageRequest onto it via a small helper (same approach magic used for ImageFormat).
  • No-stutter renames: NewSigner -> New, ParseSignatureParams -> ParseParams, ErrSignature{Required,Invalid,Expired} -> Err{Required,Invalid,Expired}. HMAC input bytes are unchanged, so existing signatures still verify.
  • docker build . passes (fmt-check, lint, test, build). Labelled needs-review, awaiting merge into protected main.

Remaining "easily separable" extraction:

  • urlparser (urlparser.go) -> internal/urlparser/. Note it is consumed outside imgcache (by internal/handlers/image.go via ParseImagePath/ParsedURL) and couples to ImageRequest, Size, ImageFormat, FitMode, so it needs standalone shared types on the extracted side. This is the natural next PR once #46 lands.

The "tightly coupled" group (cache, storage, service, and the shared types in imgcache.go) stays together as planned.

Progress update on the `internal/imgcache` split. Done and merged into `main`: - `imageprocessor` -> `internal/imageprocessor/` (PR #37) - `allowlist` -> `internal/allowlist/` (PR #41) - `magic` -> `internal/magic/` (PR #42) - `httpfetcher` -> `internal/httpfetcher/` (PR #43, merged 2026-07-25) This pass: - Opened PR #46 (`refactor/extract-signature`) extracting `signature` -> `internal/signature/`, based on current `main`. It is independent of the `httpfetcher` move, so no dependency on other in-flight branches. - Moved `Signer` (`New`, `Sign`, `Verify`, `GenerateSignedURL`), `ParseParams`, and the error sentinels. To keep the import edge one-way, the package defines a standalone `signature.Request`; `imgcache` projects its `ImageRequest` onto it via a small helper (same approach `magic` used for `ImageFormat`). - No-stutter renames: `NewSigner` -> `New`, `ParseSignatureParams` -> `ParseParams`, `ErrSignature{Required,Invalid,Expired}` -> `Err{Required,Invalid,Expired}`. HMAC input bytes are unchanged, so existing signatures still verify. - `docker build .` passes (fmt-check, lint, test, build). Labelled `needs-review`, awaiting merge into protected `main`. Remaining "easily separable" extraction: - `urlparser` (`urlparser.go`) -> `internal/urlparser/`. Note it is consumed outside `imgcache` (by `internal/handlers/image.go` via `ParseImagePath`/`ParsedURL`) and couples to `ImageRequest`, `Size`, `ImageFormat`, `FitMode`, so it needs standalone shared types on the extracted side. This is the natural next PR once #46 lands. The "tightly coupled" group (`cache`, `storage`, `service`, and the shared types in `imgcache.go`) stays together as planned.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#39
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?