diff --git a/docs/config.md b/docs/config.md index 12b889a..2ab6500 100644 --- a/docs/config.md +++ b/docs/config.md @@ -45,7 +45,7 @@ All omitted fields use their built-in defaults. | Field | Default | Rules | | --- | --- | --- | -| `base_url` | empty | Absolute Weather API URL. Required for collection and generation. | +| `base_url` | empty | Absolute HTTP(S) Weather API URL. Required for collection and generation. | | `timeout` | `10s` | Must be greater than zero. | | `precision` | `0` | Must be zero or greater. Sent as the Weather API precision query value. | | `units` | `us` | Required Weather API units query value; `--units` overrides it for one command. | diff --git a/docs/integrations/weatherapi.md b/docs/integrations/weatherapi.md index 2a84a49..9da4faa 100644 --- a/docs/integrations/weatherapi.md +++ b/docs/integrations/weatherapi.md @@ -9,9 +9,9 @@ are documented in [Weather data internals](../internal/weather-data.md) and ## Base URL And Requests -`weather_api.base_url` must be an absolute URL. Weatherreporter joins each -endpoint path to the configured base URL path, so a service hosted under a path -prefix must keep that prefix available. Requests use `GET` and carry the +`weather_api.base_url` must be an absolute HTTP(S) URL. Weatherreporter joins +each endpoint path to the configured base URL path, so a service hosted under a +path prefix must keep that prefix available. Requests use `GET` and carry the configured timeout on every HTTP attempt. Every request sends `format` and, except where noted below, `units`. The diff --git a/docs/internal/collect.md b/docs/internal/collect.md index 566d186..dce2ccc 100644 --- a/docs/internal/collect.md +++ b/docs/internal/collect.md @@ -11,10 +11,10 @@ semantics belong in [weather-data internals](weather-data.md). `config.Config`. It constructs the Weather API adapter from that configuration, calls `FetchBundle`, and returns `Result{Bundle: *weatherdata.Bundle}`. -The package wraps adapter construction failures as weather-collection setup -errors and fetch failures as bundle-collection errors. It does not retry, -persist, select reports, derive facts, build modules, invoke Promptkit, or -notify Distributor. +The package wraps adapter construction failures, including an invalid Weather +API base URL, as weather-collection setup errors and fetch failures as +bundle-collection errors. It does not retry, persist, select reports, derive +facts, build modules, invoke Promptkit, or notify Distributor. ## Application Composition diff --git a/internal/adapters/weatherapi/client.go b/internal/adapters/weatherapi/client.go index 8286641..1ad8a69 100644 --- a/internal/adapters/weatherapi/client.go +++ b/internal/adapters/weatherapi/client.go @@ -75,6 +75,9 @@ func New(cfg config.Config, opts ...Option) (*Client, error) { if err != nil || baseURL.Scheme == "" || baseURL.Host == "" { return nil, fmt.Errorf("weather_api.base_url must be an absolute URL") } + if !strings.EqualFold(baseURL.Scheme, "http") && !strings.EqualFold(baseURL.Scheme, "https") { + return nil, fmt.Errorf("weather_api.base_url must use http or https") + } timeout := cfg.WeatherAPI.Timeout if timeout <= 0 { @@ -453,22 +456,27 @@ func (c *Client) warmup(ctx context.Context) error { } attempts := positiveAttemptCount(c.warmupAttempts) var lastErr error + var lastRetryable bool for attempt := 1; attempt <= attempts; attempt++ { if err := ctx.Err(); err != nil { return fmt.Errorf("warm up weather API via %s: %w", endpoint, err) } if err := c.warmupOnce(ctx, endpoint); err != nil { lastErr = err + lastRetryable = isRetryableRequestError(err) } else { return nil } - if attempt == attempts { + if !lastRetryable || attempt == attempts { break } if err := waitForRetry(ctx, c.warmupDelay); err != nil { return fmt.Errorf("warm up weather API via %s after %d attempt(s): %w", endpoint, attempt, err) } } + if !lastRetryable { + return lastErr + } return fmt.Errorf("warm up weather API via %s failed after %d attempts: %w", endpoint, attempts, lastErr) } @@ -481,16 +489,28 @@ func (c *Client) warmupOnce(ctx context.Context, endpoint string) error { resp, err := c.httpClient.Do(req) if err != nil { - return fmt.Errorf("fetch %s: %w", endpoint, err) + err = fmt.Errorf("fetch %s: %w", endpoint, err) + if ctx.Err() != nil { + return err + } + return retryableRequestError{err: err} } defer resp.Body.Close() body, err := io.ReadAll(io.LimitReader(resp.Body, 10<<20)) if err != nil { - return fmt.Errorf("read %s response: %w", endpoint, err) + err = fmt.Errorf("read %s response: %w", endpoint, err) + if ctx.Err() != nil { + return err + } + return retryableRequestError{err: err} } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return fmt.Errorf("fetch %s: unexpected HTTP status %d: %s", endpoint, resp.StatusCode, strings.TrimSpace(string(body))) + err := fmt.Errorf("fetch %s: unexpected HTTP status %d: %s", endpoint, resp.StatusCode, strings.TrimSpace(string(body))) + if isRetryableHTTPStatus(resp.StatusCode) { + return retryableRequestError{err: err} + } + return err } return nil } diff --git a/internal/adapters/weatherapi/client_test.go b/internal/adapters/weatherapi/client_test.go index 21741bc..9be004e 100644 --- a/internal/adapters/weatherapi/client_test.go +++ b/internal/adapters/weatherapi/client_test.go @@ -3,6 +3,7 @@ package weatherapi import ( "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "os" @@ -15,6 +16,12 @@ import ( "gitea.maximumdirect.net/eric/weatherreporter/internal/weatherdata" ) +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + func TestFetchBundleFromFixtures(t *testing.T) { var requested []string server := fixtureServer(t, nil, &requested) @@ -265,6 +272,25 @@ func TestWarmupFailureStopsBeforeSourceFetches(t *testing.T) { } } +func TestWarmupDoesNotRetryPermanentStatus(t *testing.T) { + var requested []string + server := fixtureServer(t, map[string]handlerOverride{ + defaultWarmupEndpoint: {status: http.StatusNotFound, body: `not found`}, + }, &requested) + client := newTestClient(t, server.URL+"/", nil) + + _, err := client.FetchBundle(context.Background()) + if err == nil || !strings.Contains(err.Error(), "404") { + t.Fatalf("FetchBundle() error = %v, want non-retryable warmup status", err) + } + if got := countPath(requested, defaultWarmupEndpoint); got != 1 { + t.Fatalf("warmup requests = %d, want 1; all requests = %v", got, requested) + } + if containsPath(requested, "/observations") { + t.Fatalf("requested paths = %v, want warmup failure before source fetches", requested) + } +} + func TestFetchRetriesRetryableStatus(t *testing.T) { var hourlyCalls int server := fixtureServer(t, map[string]handlerOverride{ @@ -312,6 +338,40 @@ func TestFetchDoesNotRetryNonRetryableStatus(t *testing.T) { } } +func TestNewValidatesWeatherAPIBaseURLSchemeWithoutRequests(t *testing.T) { + requests := 0 + httpClient := &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + requests++ + return nil, errors.New("unexpected request") + })} + tests := []struct { + name string + baseURL string + wantErr string + }{ + {name: "local HTTP", baseURL: "http://127.0.0.1:8080/weather/"}, + {name: "local HTTPS", baseURL: "https://127.0.0.1:8443/weather/"}, + {name: "unsupported scheme", baseURL: "ftp://weather.example.test/", wantErr: "weather_api.base_url must use http or https"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := testConfig(tt.baseURL) + _, err := New(cfg, WithHTTPClient(httpClient)) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("New() error = %v", err) + } + } else if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("New() error = %v, want %q", err, tt.wantErr) + } + }) + } + if requests != 0 { + t.Fatalf("HTTP requests = %d, want none", requests) + } +} + func TestFetchDoesNotRetryMalformedEnvelope(t *testing.T) { var hourlyCalls int server := fixtureServer(t, map[string]handlerOverride{ diff --git a/internal/config/config_test.go b/internal/config/config_test.go index a1397c5..a861af9 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -82,6 +82,36 @@ func TestDefaults(t *testing.T) { } } +func TestWeatherAPIBaseURLValidation(t *testing.T) { + tests := []struct { + name string + baseURL string + wantErr string + }{ + {name: "local HTTP", baseURL: "http://127.0.0.1:8080/weather/"}, + {name: "local HTTPS", baseURL: "https://127.0.0.1:8443/weather/"}, + {name: "unsupported scheme", baseURL: "ftp://weather.example.test/", wantErr: "weather_api.base_url must use http or https"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := Defaults() + cfg.WeatherAPI.BaseURL = tt.baseURL + + err := Validate(cfg) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("Validate() error = %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("Validate() error = %v, want %q", err, tt.wantErr) + } + }) + } +} + func TestOutputDirectoryLoading(t *testing.T) { tests := []struct { name string diff --git a/internal/config/validate.go b/internal/config/validate.go index c1ef356..65bbb94 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -23,6 +23,9 @@ func Validate(cfg Config) error { if err != nil || parsed.Scheme == "" || parsed.Host == "" { return fmt.Errorf("weather_api.base_url must be an absolute URL") } + if !strings.EqualFold(parsed.Scheme, "http") && !strings.EqualFold(parsed.Scheme, "https") { + return fmt.Errorf("weather_api.base_url must use http or https") + } } if cfg.WeatherAPI.Timeout <= 0 { return fmt.Errorf("weather_api.timeout must be greater than zero")