diff --git a/internal/server/analytics_collect.go b/internal/server/analytics_collect.go index ddb0f44be..b1558b55b 100644 --- a/internal/server/analytics_collect.go +++ b/internal/server/analytics_collect.go @@ -40,16 +40,22 @@ const ( analyticsDDLTimeout = 5 * time.Minute ) -// LoadAnalyticsConfig reads the collector knobs from env, falling back to -// defaults for unset/blank values. Out-of-range or unparseable values are -// preserved as-is here so Validate can reject them with a clear message at -// startup (fail-fast at the boundary) rather than being silently clamped. -func LoadAnalyticsConfig() AnalyticsConfig { +// LoadAnalyticsConfig defaults unset/blank values and rejects malformed booleans. +// Integer validation remains in Validate. +func LoadAnalyticsConfig() (AnalyticsConfig, error) { + enabled := true + if raw := os.Getenv("ANALYTICS_COLLECTION_ENABLED"); raw != "" { + value, err := strconv.ParseBool(raw) + if err != nil { + return AnalyticsConfig{}, fmt.Errorf("invalid ANALYTICS_COLLECTION_ENABLED: %w", err) + } + enabled = value + } return AnalyticsConfig{ - Enabled: getEnvBool("ANALYTICS_COLLECTION_ENABLED", true), + Enabled: enabled, RetentionMonths: loadAnalyticsInt("ANALYTICS_RETENTION_MONTHS", defaultAnalyticsRetentionMonths), PartitionsAhead: loadAnalyticsInt("ANALYTICS_PARTITIONS_AHEAD", defaultAnalyticsPartitionsAhead), - } + }, nil } // loadAnalyticsInt reads an integer collector knob from env. An unset/blank @@ -91,17 +97,6 @@ func withDDLTimeout(ctx context.Context, fn func(context.Context, int) error, ar return fn(stepCtx, arg) } -// getEnvBool parses a boolean env var, returning defaultVal when unset or -// unparseable. Accepts the strconv.ParseBool truth set (1/t/true/...). -func getEnvBool(key string, defaultVal bool) bool { - if val := os.Getenv(key); val != "" { - if result, err := strconv.ParseBool(val); err == nil { - return result - } - } - return defaultVal -} - // newAnalyticsCollector builds the savings-snapshot collector against the live // DB connection. Kept here (not inline in reinitializeAfterConnect) so the // app.go footprint is one localized call (eases the in-flight app.go rebase). diff --git a/internal/server/analytics_collect_test.go b/internal/server/analytics_collect_test.go index ba971e6c8..9cc5b0ce2 100644 --- a/internal/server/analytics_collect_test.go +++ b/internal/server/analytics_collect_test.go @@ -2,11 +2,57 @@ package server import ( "context" + "os" + "strings" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil" ) +func TestLoadAnalyticsConfigEnabled(t *testing.T) { + t.Setenv("ANALYTICS_RETENTION_MONTHS", "") + t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "") + for _, tc := range []struct { + name, value string + wantEnabled bool + wantError bool + }{ + {"unset", "", true, false}, + {"empty", "", true, false}, + {"true", "true", true, false}, + {"TRUE", "TRUE", true, false}, + {"1", "1", true, false}, + {"false", "false", false, false}, + {"FALSE", "FALSE", false, false}, + {"0", "0", false, false}, + {"no", "no", false, true}, + {"off", "off", false, true}, + {"trailing space", "False ", false, true}, + {"disabled", "disabled", false, true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("ANALYTICS_COLLECTION_ENABLED", tc.value) + if tc.name == "unset" { + if err := os.Unsetenv("ANALYTICS_COLLECTION_ENABLED"); err != nil { + t.Fatal(err) + } + } + cfg, err := LoadAnalyticsConfig() + if tc.wantError { + if err == nil || !strings.Contains(err.Error(), "ANALYTICS_COLLECTION_ENABLED") { + t.Fatalf("expected enabled parse error, got %v", err) + } + return + } + if err != nil { + t.Fatal(err) + } + testutil.AssertEqual(t, tc.wantEnabled, cfg.Enabled) + testutil.AssertNoError(t, cfg.Validate()) + }) + } +} + // TestLoadAnalyticsConfig_FailFastOnMalformedInt is the CR #1049 regression: // a set-but-unparseable ANALYTICS_RETENTION_MONTHS / ANALYTICS_PARTITIONS_AHEAD // must produce a config that Validate() rejects (fail-fast at startup) instead @@ -16,7 +62,8 @@ func TestLoadAnalyticsConfig_FailFastOnMalformedInt(t *testing.T) { t.Run("unset uses defaults and validates", func(t *testing.T) { t.Setenv("ANALYTICS_RETENTION_MONTHS", "") t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "") - cfg := LoadAnalyticsConfig() + cfg, err := LoadAnalyticsConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, defaultAnalyticsRetentionMonths, cfg.RetentionMonths) testutil.AssertEqual(t, defaultAnalyticsPartitionsAhead, cfg.PartitionsAhead) testutil.AssertNoError(t, cfg.Validate()) @@ -25,7 +72,8 @@ func TestLoadAnalyticsConfig_FailFastOnMalformedInt(t *testing.T) { t.Run("valid override is parsed", func(t *testing.T) { t.Setenv("ANALYTICS_RETENTION_MONTHS", "12") t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "6") - cfg := LoadAnalyticsConfig() + cfg, err := LoadAnalyticsConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, 12, cfg.RetentionMonths) testutil.AssertEqual(t, 6, cfg.PartitionsAhead) testutil.AssertNoError(t, cfg.Validate()) @@ -34,7 +82,8 @@ func TestLoadAnalyticsConfig_FailFastOnMalformedInt(t *testing.T) { t.Run("malformed retention is rejected by Validate", func(t *testing.T) { t.Setenv("ANALYTICS_RETENTION_MONTHS", "not-a-number") t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "3") - cfg := LoadAnalyticsConfig() + cfg, err := LoadAnalyticsConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, 0, cfg.RetentionMonths) // sentinel testutil.AssertTrue(t, cfg.Validate() != nil, "malformed retention must fail Validate") }) @@ -42,7 +91,8 @@ func TestLoadAnalyticsConfig_FailFastOnMalformedInt(t *testing.T) { t.Run("malformed partitions-ahead is rejected by Validate", func(t *testing.T) { t.Setenv("ANALYTICS_RETENTION_MONTHS", "24") t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "12x") - cfg := LoadAnalyticsConfig() + cfg, err := LoadAnalyticsConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, 0, cfg.PartitionsAhead) // sentinel testutil.AssertTrue(t, cfg.Validate() != nil, "malformed partitions-ahead must fail Validate") }) diff --git a/internal/server/app.go b/internal/server/app.go index d1f6d7cfd..b6a94656a 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -258,18 +258,38 @@ func decorateSenderWithMute(sender email.SenderInterface, mc email.MuteChecker, } // LoadApplicationConfig reads all configuration from environment variables. -func LoadApplicationConfig() ApplicationConfig { +func LoadApplicationConfig() (ApplicationConfig, error) { version := os.Getenv("VERSION") if version == "" { version = "dev" } + term := 3 + if raw := os.Getenv("DEFAULT_TERM"); raw != "" { + value, err := strconv.Atoi(raw) + if err != nil { + return ApplicationConfig{}, fmt.Errorf("invalid DEFAULT_TERM: %w", err) + } + term = value + } + coverage := 80.0 + if raw := os.Getenv("DEFAULT_COVERAGE"); raw != "" { + value, err := strconv.ParseFloat(raw, 64) + if err != nil { + return ApplicationConfig{}, fmt.Errorf("invalid DEFAULT_COVERAGE: %w", err) + } + coverage = value + } + analyticsConfig, err := LoadAnalyticsConfig() + if err != nil { + return ApplicationConfig{}, err + } return ApplicationConfig{ Version: version, NotificationDaysBefore: getEnvInt("NOTIFICATION_DAYS_BEFORE", 3), - DefaultTerm: getEnvInt("DEFAULT_TERM", 3), + DefaultTerm: term, DefaultPaymentOption: os.Getenv("DEFAULT_PAYMENT_OPTION"), - DefaultCoverage: getEnvFloat("DEFAULT_COVERAGE", 80), + DefaultCoverage: coverage, DefaultRampSchedule: os.Getenv("DEFAULT_RAMP_SCHEDULE"), APIKeySecretARN: os.Getenv("API_KEY_SECRET_ARN"), EnableDashboard: os.Getenv("ENABLE_DASHBOARD") == "true", @@ -283,8 +303,8 @@ func LoadApplicationConfig() ApplicationConfig { ScheduledTaskSecret: os.Getenv("SCHEDULED_TASK_SECRET"), ScheduledTaskSecretName: os.Getenv("SCHEDULED_TASK_SECRET_NAME"), IsLambda: runtime.IsLambda(), - Analytics: LoadAnalyticsConfig(), - } + Analytics: analyticsConfig, + }, nil } // validateAppConfigEnvDefaults validates the money-moving env-sourced defaults @@ -532,12 +552,15 @@ func NewApplicationFromDeps(ctx context.Context, cfg ApplicationConfig, deps Ext // can pass the ldflags-stamped value directly instead of round-tripping // through os.Setenv / os.Getenv (04-N1). Pass "" to fall back to the env. func NewApplication(ctx context.Context, version string) (*Application, error) { - cfg := LoadApplicationConfig() + cfg, err := LoadApplicationConfig() + if err != nil { + return nil, err + } if version != "" { cfg.Version = version } - if err := cfg.Analytics.Validate(); err != nil { + if err = cfg.Analytics.Validate(); err != nil { return nil, fmt.Errorf("invalid analytics configuration: %w", err) } @@ -969,23 +992,6 @@ func getEnvInt(key string, defaultVal int) int { return defaultVal } -// getEnvFloat mirrors getEnvInt for float-valued env vars. defaultVal is kept -// parameterized (rather than inlined) to stay symmetric with getEnvInt even -// though every current caller passes the same coverage default. -// -//nolint:unparam // general-purpose env parser; default kept parameterized for symmetry with getEnvInt -func getEnvFloat(key string, defaultVal float64) float64 { - if val := os.Getenv(key); val != "" { - result, err := strconv.ParseFloat(val, 64) - if err != nil { - log.Printf("WARNING: %s=%q is not a valid float; using default %g", key, val, defaultVal) // #nosec G706 -- env var value is operator-controlled configuration; logged for diagnostics - return defaultVal - } - return result - } - return defaultVal -} - // authServiceAdapter adapts auth.Service to api.AuthServiceInterface. type authServiceAdapter struct { service *auth.Service diff --git a/internal/server/app_test.go b/internal/server/app_test.go index 9a7046603..f6e6ff0d9 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -73,22 +73,6 @@ func TestGetEnvInt(t *testing.T) { testutil.AssertEqual(t, 42, getEnvInt(key, 42)) } -func TestGetEnvFloat(t *testing.T) { - key := "TEST_GET_ENV_FLOAT_CUDLY" - defer os.Unsetenv(key) - - // Default value - testutil.AssertEqual(t, 80.0, getEnvFloat(key, 80.0)) - - // Valid float - os.Setenv(key, "95.5") - testutil.AssertEqual(t, 95.5, getEnvFloat(key, 80.0)) - - // Invalid float - returns default - os.Setenv(key, "not-a-float") - testutil.AssertEqual(t, 80.0, getEnvFloat(key, 80.0)) -} - func TestHttpToLambdaRequest_XForwardedFor(t *testing.T) { req := httptest.NewRequestWithContext(context.Background(), "GET", "/api/test", nil) req.Header.Set("X-Forwarded-For", "1.2.3.4, 5.6.7.8") @@ -336,7 +320,8 @@ func TestLoadApplicationConfig(t *testing.T) { testutil.SetEnv(t, key, "") } - cfg := LoadApplicationConfig() + cfg, err := LoadApplicationConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, "dev", cfg.Version) testutil.AssertEqual(t, 3, cfg.NotificationDaysBefore) @@ -362,7 +347,8 @@ func TestLoadApplicationConfig(t *testing.T) { testutil.SetEnv(t, "CORS_ALLOWED_ORIGIN", "https://example.com") testutil.SetEnv(t, "AWS_LAMBDA_RUNTIME_API", "localhost:9001") - cfg := LoadApplicationConfig() + cfg, err := LoadApplicationConfig() + testutil.AssertNoError(t, err) testutil.AssertEqual(t, "1.2.3", cfg.Version) testutil.AssertEqual(t, 7, cfg.NotificationDaysBefore) @@ -379,6 +365,77 @@ func TestLoadApplicationConfig(t *testing.T) { }) } +func TestNewApplicationRejectsMalformedEnvDefaults(t *testing.T) { + for _, key := range []string{"DEFAULT_TERM", "DEFAULT_COVERAGE", "ANALYTICS_COLLECTION_ENABLED", "ANALYTICS_RETENTION_MONTHS", "ANALYTICS_PARTITIONS_AHEAD", "SQS_REPORT_BATCH_ITEM_FAILURES", "DB_HOST"} { + t.Setenv(key, "") + } + for _, tc := range []struct{ key, value string }{ + {"DEFAULT_TERM", "3y"}, + {"DEFAULT_TERM", "999999999999999999999999999"}, + {"DEFAULT_COVERAGE", "80%"}, + {"DEFAULT_COVERAGE", "1e999"}, + {"ANALYTICS_COLLECTION_ENABLED", "no"}, + {"ANALYTICS_COLLECTION_ENABLED", "off"}, + {"ANALYTICS_COLLECTION_ENABLED", "False "}, + {"ANALYTICS_COLLECTION_ENABLED", "disabled"}, + } { + t.Run(tc.key+"="+tc.value, func(t *testing.T) { + t.Setenv(tc.key, tc.value) + if _, err := LoadApplicationConfig(); err == nil || !strings.Contains(err.Error(), tc.key) { + t.Fatalf("expected %s load error, got %v", tc.key, err) + } + app, err := NewApplication(context.Background(), "test") + if app != nil || err == nil || !strings.Contains(err.Error(), tc.key) { + t.Fatalf("expected %s startup error and nil application, got app=%v err=%v", tc.key, app, err) + } + }) + } +} + +func TestLoadApplicationConfigNumericDefaults(t *testing.T) { + t.Setenv("ANALYTICS_COLLECTION_ENABLED", "false") + t.Setenv("ANALYTICS_RETENTION_MONTHS", "") + t.Setenv("ANALYTICS_PARTITIONS_AHEAD", "") + for _, tc := range []struct { + name, term, coverage string + wantTerm int + wantCoverage float64 + }{ + {"unset", "", "", 3, 80}, + {"empty", "", "", 3, 80}, + {"one year", "1", "95.5", 1, 95.5}, + {"zero coverage", "3", "0", 3, 0}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("DEFAULT_TERM", tc.term) + t.Setenv("DEFAULT_COVERAGE", tc.coverage) + if tc.name == "unset" { + if err := os.Unsetenv("DEFAULT_TERM"); err != nil { + t.Fatal(err) + } + if err := os.Unsetenv("DEFAULT_COVERAGE"); err != nil { + t.Fatal(err) + } + } + cfg, err := LoadApplicationConfig() + if err != nil { + t.Fatal(err) + } + t.Setenv("DEFAULT_TERM", "3y") + t.Setenv("DEFAULT_COVERAGE", "80%") + t.Setenv("ANALYTICS_COLLECTION_ENABLED", "off") + testutil.AssertEqual(t, tc.wantTerm, cfg.DefaultTerm) + testutil.AssertEqual(t, tc.wantCoverage, cfg.DefaultCoverage) + testutil.AssertNoError(t, validateAppConfigEnvDefaults(cfg)) + testutil.AssertNoError(t, cfg.Analytics.Validate()) + app := &Application{appConfig: cfg} + result, err := app.handleCollectAnalytics(context.Background()) + testutil.AssertNoError(t, err) + testutil.AssertEqual(t, "disabled", result["status"]) + }) + } +} + func TestNewApplicationFromDeps(t *testing.T) { ctx := testutil.TestContext(t) @@ -575,26 +632,6 @@ func TestGetEnvIntLogsOnBadValue(t *testing.T) { "Expected key name in warning log, got: "+logged) } -// TestGetEnvFloatLogsOnBadValue mirrors TestGetEnvIntLogsOnBadValue for floats. -func TestGetEnvFloatLogsOnBadValue(t *testing.T) { - testutil.SetEnv(t, "TEST_ENV_FLOAT_BAD", "eighty") - - var logged string - orig := log.Writer() - var buf strings.Builder - log.SetOutput(&buf) - t.Cleanup(func() { log.SetOutput(orig) }) - - result := getEnvFloat("TEST_ENV_FLOAT_BAD", 80.0) - logged = buf.String() - - testutil.AssertEqual(t, 80.0, result) - testutil.AssertTrue(t, strings.Contains(logged, "WARNING"), - "Expected WARNING log for bad float env var, got: "+logged) - testutil.AssertTrue(t, strings.Contains(logged, "TEST_ENV_FLOAT_BAD"), - "Expected key name in warning log, got: "+logged) -} - func TestInitConfigStore(t *testing.T) { t.Run("missing DB_HOST returns error", func(t *testing.T) { testutil.SetEnv(t, "DB_HOST", "")