refactor: extract httpfetcher package from imgcache #43
Reference in New Issue
Block a user
Delete Branch "refactor/extract-httpfetcher"
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?
Extracts the HTTP fetcher concern out of
internal/imgcache/into its owninternal/httpfetcher/package, per the plan in issue #39.This is one of four planned extractions; only the fetcher is moved here so the diff stays reviewable. The remaining three (signature, magic, urlparser) will land in separate PRs.
What moved
From
internal/imgcache/fetcher.goandinternal/imgcache/mock_fetcher.gointointernal/httpfetcher/:HTTPFetchertype, itsNewconstructor, andConfig(formerlyFetcherConfig) withDefaultConfigvalidateURL,isPrivateIP,isLocalhost,extractHostlimitedReaderisAllowedContentType,detectContentTypeFromPath)ErrSSRFBlocked,ErrUpstreamError,ErrUpstreamTimeout,ErrPayloadTooLarge,ErrDisallowedContentType)DefaultFetchTimeout,DefaultMaxPayloadBytes,DefaultMaxConnectionsPerHost, etc.)Fetcherinterface andFetchResulttype (moved here to keep the import edge one-way:imgcachedepends onhttpfetcher, never the reverse)MockFetchertest helperRenames (no stuttering)
NewHTTPFetcher→httpfetcher.NewFetcherConfig→httpfetcher.ConfigDefaultFetcherConfig→httpfetcher.DefaultConfigNewMockFetcher→httpfetcher.NewMockThe
ServiceConfig.FetcherConfigfield name is retained — it describes what kind of config the field holds (not a stutter).Behavior
Pure refactor. No behavior changes. All existing tests pass; unit tests for the new package are included.
docker build .passes (fmt-check, lint, test, build).refs #39
Review: PR #43 (reviewer:pixa)
Policy divergences
No policy violations found.
Verified:
.golangci.yml,Makefile,Dockerfile, or.gitea/workflows (no cheating).internal/per canonical subdirectory layout.Newconstructor is not stuttering (noNewHTTPFetcher).ServiceConfig.FetcherConfigfield name retained — describes what kind of config the field holds, not a stutter.httpfetcher.go:1-2.Scope of this PR
This PR refs (does not close) issue #39; it is one of four planned extractions (httpfetcher only here; signature/magic/urlparser land in separate PRs per the issue plan).
Itemized requirements checklist (issue #39, httpfetcher only)
internal/httpfetcher/package —internal/httpfetcher/httpfetcher.goHTTPFetcher,FetcherConfig→Config,DefaultFetcherConfig→DefaultConfigssrfSafeDialer,validateURL,isPrivateIP,isLocalhost,extractHost)limitedReaderisAllowedContentType,detectContentTypeFromPath)ErrSSRFBlocked,ErrUpstreamError,ErrUpstreamTimeout,ErrResponseTooLarge,ErrInvalidContentType,ErrInvalidHost,ErrUnsupportedScheme)Fetcherinterface andFetchResulttype intohttpfetcher/(confirmed removed fromimgcache/imgcache.go, removed lines verified via diff)MockFetcherand renameNewMockFetcher→NewMockNew/NewMock, not stutteringinternal/handlers/{handlers,image,imageenc,handlers_test}.go,internal/imgcache/{cache,cache_test,service,testutil_test}.goimgcache.Fetcher/FetchResult/NewHTTPFetcher/DefaultFetcherConfig/FetcherConfig/NewMockFetcher/MockFetcheror relocatedErr*values (grep confirmed zero matches)imgcacheimportshttpfetcher;httpfetcherdoes NOT importimgcache(confirmed by readinghttpfetcher.goimports)docker build .passes — fmt-check, lint, test, build all greenTests
New unit tests in
internal/httpfetcher/httpfetcher_test.gocover:DefaultConfigdefaultsNew(nil)falls back to defaults (constructor happy path)isAllowedContentType(table test, 8 cases)extractHost(table test, 6 cases)isLocalhost(table test, 9 cases)isPrivateIP(table test, 11 cases + nil)validateURL(HTTP reject, AllowHTTP override, localhost, empty host)MockFetcher+NewMock(file fetch, missing file, context cancel)detectContentTypeFromPath(table test, 10 cases)limitedReaderlimit enforcementPre-existing integration coverage of
HTTPFetcher.Fetch(viaimgcacheservice tests andhandlerstests) is preserved. Coverage of internal helpers is strictly improved; no coverage regressed.Diff integrity (pure refactor verification)
Compared
main:internal/imgcache/fetcher.go(445 lines) andmain:internal/imgcache/mock_fetcher.goagainst the new package files. Only changes are:imgcache→httpfetcherNewHTTPFetcher→New,FetcherConfig→Config,DefaultFetcherConfig→DefaultConfig,NewMockFetcher→NewMock)AllowedContentTypescomment onisAllowedContentType)No behavior changes. No removed functionality.
ServiceConfig.FetcherConfigfield retained as stated.Minor (non-blocking) description nits
The PR description lists some error/constant names that don't exist verbatim in the code (e.g.
ErrPayloadTooLarge,ErrDisallowedContentType,DefaultMaxPayloadBytes). The actual names (retained frommain) areErrResponseTooLarge,ErrInvalidContentType,DefaultMaxResponseSize. Code is correct; the PR description is slightly imprecise but this is not a code defect.Build result
PASSED locally (fmt-check, lint, all tests, binary build, runtime image all succeed).
Final verdict
PASS — clean, reviewable pure-refactor extraction. Import edge is correctly one-way, naming follows the project's no-stutter convention, exports are documented, tests are additive (improved coverage for internal helpers), no linter/CI/test integrity violations,
docker build .is green, and the PR branch applies cleanly on top of currentmain(already up-to-date, no rebase needed).Review: PR #43 — extract
httpfetcherfromimgcacheVerdict: Merge-ready.
Independent re-review of head
a853fe7. This is a clean, behavior-preserving package extraction — the first of four planned in issue #39 (httpfetcheronly here;signature/magic/urlparserto follow in separate PRs). Verified againstREPO_POLICIES.mdand the issue's extraction plan by reading the head sources directly.Findings
.golangci.yml,Makefile,Dockerfile, or.gitea/workflows/— the checks are not weakened. The new package lives underinternal/httpfetcher/per the canonical subdirectory layout. Package doc comment present; all exported identifiers carry godoc. No new external dependencies, so the hash-pinning rule is not engaged.internal/httpfetcher/httpfetcher.goandinternal/httpfetcher/mock.goimport only the standard library — neither importsimgcache.imgcacheimportshttpfetcher(cache.go,service.go, and the tests). The edge isimgcache → httpfetcher, never the reverse, exactly as intended.New,NewMock, andDefaultConfig— notNewHTTPFetcher/DefaultFetcherConfig.ServiceConfig.FetcherConfigis correctly retained (it names the kind of config the field holds, not a stutter).FetcherConfig→Config,DefaultFetcherConfig→DefaultConfig,NewHTTPFetcher→New,NewMockFetcher→NewMock), and cosmetic doc-comment tweaks (trailing periods, "whitelist"→"allow list"). TheFetcherinterface andFetchResulttype were relocated out ofimgcache.gointo the new package (removal confirmed in the diff) to keep the edge one-way. All call sites updated:handlers/{handlers,image,imageenc,handlers_test}.goandimgcache/{cache,cache_test,service,testutil_test}.go.internal/httpfetcher/httpfetcher_test.goadds table-driven unit coverage forDefaultConfig,New(nil),isAllowedContentType,extractHost,isLocalhost,isPrivateIP,validateURL,detectContentTypeFromPath,limitedReader, and theMockFetcher/NewMockpath.HTTPFetcher.Fetch's full HTTP path remains covered by the existingimgcache/handlersintegration tests. Coverage of internal helpers is improved; none regressed.check / check (push)is green on heada853fe7(success, 57s). Since theDockerfilerunsmake check(fmt-check, lint, test, build), a passingdocker build .implies all checks pass.mergeable = true; the branch applies on top of currentmain(504afea) with no conflict. Not a draft.Non-blocking note
The PR description lists a few error/constant names that do not appear verbatim in the code (
ErrPayloadTooLarge,ErrDisallowedContentType,DefaultMaxPayloadBytes); the actual retained names areErrResponseTooLarge,ErrInvalidContentType, andDefaultMaxResponseSize. This is a description imprecision only — the code and every call site are internally consistent and correct. Worth tidying the description, but not a merge blocker.Applied label
merge-ready(id 48); assigned tosneak.