From 622827ccb4c23915aeccaa39d3e8152dd2955afb Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Tue, 19 May 2026 14:48:16 +0300 Subject: Require explicit http.Client in podcast service (DIP) Remove the silent nil-fallback in NewPodcastService / NewPodcastServiceWithLogger that fabricated a default http.Client when callers passed nil. The service now panics on nil, forcing callers to inject their own client and own the HTTP timeout / transport policy explicitly. DefaultHTTPClientTimeout remains exported so production wiring (cmd/player/main.go) keeps a sensible default at the composition root, not buried in the service. Updated TestPodcastService_NilHTTPClient_Defaults to TestPodcastService_NilHTTPClient_Panics to document the new contract. All production call sites already pass a non-nil client. Refs task 8a. Co-Authored-By: Claude Opus 4.7 --- player-server/internal/service/podcast.go | 12 ++++++++++-- player-server/internal/service/podcast_test.go | 19 +++++++++++-------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/player-server/internal/service/podcast.go b/player-server/internal/service/podcast.go index 5adc0dc..bb3f0fd 100644 --- a/player-server/internal/service/podcast.go +++ b/player-server/internal/service/podcast.go @@ -79,16 +79,24 @@ type podcastService struct { *podcastFeedChecker } -// DefaultHTTPClientTimeout is the fallback timeout used when no http.Client is injected. +// DefaultHTTPClientTimeout is the recommended timeout for production HTTP clients +// passed into the podcast service. The service no longer constructs a fallback +// client — callers must inject an explicit *http.Client (dependency inversion); +// this constant is exported so production wiring can use a sensible default. const DefaultHTTPClientTimeout = 30 * time.Second // NewPodcastService creates a PodcastService with the given dependencies. // checkInterval should be the number of minutes between background feed checks. +// httpClient is required and must not be nil; the service does not construct +// a default client so callers explicitly own the HTTP timeout / transport policy. func NewPodcastService(store PodcastServiceStore, clk clock.Clock, mediaRoot string, helper *accessHelper, prober probe.Prober, thumbGen thumb.Generator, httpClient *http.Client, checkInterval int) *podcastService { return NewPodcastServiceWithLogger(store, clk, mediaRoot, helper, prober, thumbGen, httpClient, checkInterval, slog.Default()) } // NewPodcastServiceWithLogger creates a PodcastService with an injected logger. +// httpClient is required and must not be nil — passing nil will panic on first +// use. This is intentional: the service depends on an injected client per DIP +// and refuses to silently fabricate one. func NewPodcastServiceWithLogger(store PodcastServiceStore, clk clock.Clock, mediaRoot string, helper *accessHelper, prober probe.Prober, thumbGen thumb.Generator, httpClient *http.Client, checkInterval int, logger *slog.Logger) *podcastService { if checkInterval <= 0 { checkInterval = 60 @@ -97,7 +105,7 @@ func NewPodcastServiceWithLogger(store PodcastServiceStore, clk clock.Clock, med logger = slog.Default() } if httpClient == nil { - httpClient = &http.Client{Timeout: DefaultHTTPClientTimeout} + panic("service.NewPodcastService: httpClient must not be nil") } s := &podcastService{ store: store, diff --git a/player-server/internal/service/podcast_test.go b/player-server/internal/service/podcast_test.go index b3c622e..2c86e99 100644 --- a/player-server/internal/service/podcast_test.go +++ b/player-server/internal/service/podcast_test.go @@ -40,17 +40,20 @@ func TestPodcastService_CustomHTTPClient(t *testing.T) { } } -func TestPodcastService_NilHTTPClient_Defaults(t *testing.T) { +// TestPodcastService_NilHTTPClient_Panics verifies the constructor refuses to +// fabricate a default http.Client. Callers must inject one explicitly (DIP); +// passing nil is a programmer error and must panic loudly, not silently fall +// back to a hidden default. +func TestPodcastService_NilHTTPClient_Panics(t *testing.T) { store := repository.NewMockStore() clk := &clock.MockClock{T: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)} logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - svc := NewPodcastServiceWithLogger(store, clk, t.TempDir(), nil, nil, nil, nil, 60, logger) - if svc.httpClient == nil { - t.Fatal("expected non-nil httpClient when nil passed to constructor") - } - if svc.httpClient.Timeout != DefaultHTTPClientTimeout { - t.Fatalf("expected default timeout %v, got %v", DefaultHTTPClientTimeout, svc.httpClient.Timeout) - } + defer func() { + if r := recover(); r == nil { + t.Fatal("expected panic when nil httpClient passed to constructor, got none") + } + }() + _ = NewPodcastServiceWithLogger(store, clk, t.TempDir(), nil, nil, nil, nil, 60, logger) } func TestPodcastService_SubscribeFeed_Ok(t *testing.T) { -- cgit v1.2.3