From 80051396d82e40145c5724fee3d4c44fa5ed9e78 Mon Sep 17 00:00:00 2001 From: Alexander Date: Thu, 7 May 2026 17:16:32 +0200 Subject: [PATCH] refactor: split monolithic HTTP client into focused methods --- internal/provider/musicbrainz/client.go | 95 +++++++++++++++---------- 1 file changed, 58 insertions(+), 37 deletions(-) diff --git a/internal/provider/musicbrainz/client.go b/internal/provider/musicbrainz/client.go index 54c563f..a4bd482 100644 --- a/internal/provider/musicbrainz/client.go +++ b/internal/provider/musicbrainz/client.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/url" + "strings" "time" "github.com/rs/zerolog" @@ -35,14 +36,30 @@ func newClient() *client { } func (c *client) get(ctx context.Context, endpoint string, params url.Values) ([]byte, error) { - log := zerolog.Ctx(ctx) - - log.Trace().Str("endpoint", endpoint).Msg("waiting for rate limiter") - if err := c.limiter.Wait(ctx); err != nil { - log.Debug().Err(err).Msg("rate limiter interrupted") - return nil, fmt.Errorf("rate limiter: %w", err) + if err := c.waitForRateLimit(ctx); err != nil { + return nil, err } + req, err := c.buildRequest(ctx, endpoint, params) + if err != nil { + return nil, err + } + + return c.executeAndRead(ctx, req, endpoint) +} + +func (c *client) waitForRateLimit(ctx context.Context) error { + log := zerolog.Ctx(ctx) + log.Trace().Msg("waiting for rate limiter") + + if err := c.limiter.Wait(ctx); err != nil { + log.Debug().Err(err).Msg("rate limiter interrupted") + return fmt.Errorf("rate limiter: %w", err) + } + return nil +} + +func (c *client) buildRequest(ctx context.Context, endpoint string, params url.Values) (*http.Request, error) { if params == nil { params = url.Values{} } @@ -57,13 +74,19 @@ func (c *client) get(ctx context.Context, endpoint string, params url.Values) ([ req.Header.Set("User-Agent", userAgent) req.Header.Set("Accept", "application/json") + return req, nil +} +func (c *client) executeAndRead(ctx context.Context, req *http.Request, endpoint string) ([]byte, error) { + log := zerolog.Ctx(ctx) start := time.Now() - log.Trace().Str("url", reqURL).Msg("sending HTTP request") + + log.Trace().Str("url", req.URL.String()).Msg("sending HTTP request") resp, err := c.http.Do(req) if err != nil { - log.Debug().Err(err).Str("endpoint", endpoint).Dur("duration", time.Since(start)).Msg("HTTP request failed") + duration := time.Since(start) + log.Debug().Err(err).Str("endpoint", endpoint).Dur("duration", duration).Msg("HTTP request failed") metrics.ProviderRequests.WithLabelValues("musicbrainz", endpoint, "error").Inc() return nil, fmt.Errorf("do request: %w", err) } @@ -72,44 +95,42 @@ func (c *client) get(ctx context.Context, endpoint string, params url.Values) ([ duration := time.Since(start) metrics.ProviderLatency.WithLabelValues("musicbrainz", endpoint).Observe(duration.Seconds()) - if resp.StatusCode == http.StatusNotFound { - log.Debug().Str("endpoint", endpoint).Int("status", resp.StatusCode).Dur("duration", duration).Msg("HTTP 404") + return c.readResponse(ctx, resp, endpoint, duration) +} + +func (c *client) readResponse(ctx context.Context, resp *http.Response, endpoint string, duration time.Duration) ([]byte, error) { + log := zerolog.Ctx(ctx) + + switch resp.StatusCode { + case http.StatusOK: + log.Trace().Str("endpoint", endpoint).Dur("duration", duration).Msg("HTTP request succeeded") + metrics.ProviderRequests.WithLabelValues("musicbrainz", endpoint, "ok").Inc() + return io.ReadAll(resp.Body) + + case http.StatusNotFound: + log.Debug().Str("endpoint", endpoint).Dur("duration", duration).Msg("HTTP 404") metrics.ProviderRequests.WithLabelValues("musicbrainz", endpoint, "not_found").Inc() return nil, ErrNotFound - } - if resp.StatusCode == http.StatusServiceUnavailable { + case http.StatusServiceUnavailable: log.Warn().Str("endpoint", endpoint).Dur("duration", duration).Msg("HTTP 503 rate limited by provider") metrics.ProviderRequests.WithLabelValues("musicbrainz", endpoint, "rate_limited").Inc() return nil, ErrRateLimited - } - if resp.StatusCode != http.StatusOK { + default: body, _ := io.ReadAll(resp.Body) log.Warn().Str("endpoint", endpoint).Int("status", resp.StatusCode).Str("body", string(body)).Dur("duration", duration).Msg("unexpected HTTP status") metrics.ProviderRequests.WithLabelValues("musicbrainz", endpoint, fmt.Sprintf("%d", resp.StatusCode)).Inc() return nil, fmt.Errorf("unexpected status %d: %s", resp.StatusCode, string(body)) } - - log.Trace().Str("endpoint", endpoint).Int("status", resp.StatusCode).Dur("duration", duration).Msg("HTTP request succeeded") - metrics.ProviderRequests.WithLabelValues("musicbrainz", endpoint, "ok").Inc() - - return io.ReadAll(resp.Body) } func (c *client) lookup(ctx context.Context, entity, id string, inc []string) ([]byte, error) { zerolog.Ctx(ctx).Debug().Str("entity", entity).Str("id", id).Strs("includes", inc).Msg("provider lookup") params := url.Values{} - if len(inc) > 0 { - incStr := "" - for i, v := range inc { - if i > 0 { - incStr += "+" - } - incStr += v - } - params.Set("inc", incStr) + if joined := joinIncludes(inc); joined != "" { + params.Set("inc", joined) } return c.get(ctx, fmt.Sprintf("%s/%s", entity, id), params) @@ -129,15 +150,8 @@ func (c *client) browse(ctx context.Context, entity, linkedEntity, linkedID stri params.Set("limit", fmt.Sprintf("%d", limit)) params.Set("offset", fmt.Sprintf("%d", offset)) - if len(inc) > 0 { - incStr := "" - for i, v := range inc { - if i > 0 { - incStr += "+" - } - incStr += v - } - params.Set("inc", incStr) + if joined := joinIncludes(inc); joined != "" { + params.Set("inc", joined) } return c.get(ctx, entity, params) @@ -154,6 +168,13 @@ func (c *client) search(ctx context.Context, entity, query string, limit, offset return c.get(ctx, entity, params) } +func joinIncludes(inc []string) string { + if len(inc) == 0 { + return "" + } + return strings.Join(inc, "+") +} + func decode[T any](data []byte) (*T, error) { var result T if err := json.Unmarshal(data, &result); err != nil {