feat(config): validate boundary values at load time#16
Merged
Conversation
Close a QA gap flagged in the v0.2.0 release review. Previously Load() accepted any integer, including negative values like MAX_REQUEST_BYTES=-100 or REQUEST_TIMEOUT=-1s, which are never valid in production and cause undefined downstream behavior. Add an explicit Validate() method and call it at the end of Load() so bad env configuration fails loudly at startup rather than producing weird runtime behavior under load. Rules applied: - Duration fields (REQUEST_TIMEOUT, SHUTDOWN_TIMEOUT) must be strictly positive. Zero or negative is never a useful timeout. - Count and byte fields (MAX_REQUEST_BYTES, RATE_LIMIT_RPM, RATE_LIMIT_BURST, MAX_CONCURRENT_REQUESTS, MAX_CHANGED_FILES, MAX_DIFF_BYTES, MAX_RESPONSE_BYTES) must be non-negative. Zero is still accepted because body_limit, rate_limit, and concurrency_limit middleware document it as the "disable this guard" convention; rejecting zero here would break that contract. Error messages name the offending env var and include the raw value so operators can fix misconfiguration without guesswork. Tests: - TestLoad_RejectsOutOfRangeValues (11 table-driven cases) covers every field and both the zero-duration and negative- count paths. - TestLoad_ZeroIsValidForCountFields confirms the disable-path semantics still work for all seven count/byte fields. - TestConfig_Validate_ValidDefault is a sanity check that the default-loaded Config (no env vars set) passes Validate. Existing TestLoad_Defaults / TestLoad_OverrideFromEnv / TestLoad_InvalidDurationReturnsError / TestLoad_InvalidIntReturnsError still pass unchanged.
Apply the nits surfaced in the code review:
1. Validate PORT as integer in [0, 65535] so PORT=abcxyz or
PORT=99999 fail at startup instead of at ListenAndServe time.
Zero is kept valid so ephemeral-port tests and dev setups
still work.
2. Validate LOG_LEVEL against the exact set
{debug, info, warn, warning, error}, case-insensitive. This
closes the QA review's P0 finding that logging.parseLevel
silently falls back to info on unknown values; now an
operator who typos the level gets an actionable startup
error instead of mystified info-only logs in production.
The parseLevel default branch stays as defense in depth.
3. Return aggregated errors via errors.Join so an operator who
has multiple misconfigured values at once sees every
problem in a single Load() error rather than discovering
them one restart at a time.
4. Drop TestConfig_Validate_ValidDefault. It was redundant:
Load() internally calls Validate(), so if the default
Config ever stopped validating, TestLoad_Defaults would
catch it first. Removing reduces noise.
New tests:
- PORT: three reject cases (non-numeric, negative, out of
range) added to TestLoad_RejectsOutOfRangeValues; accept
cases (0, 1, 8080, 65535) in TestLoad_PortAcceptsValidRange.
- LOG_LEVEL: two reject cases (unknown, numeric) added to
TestLoad_RejectsOutOfRangeValues; six accept cases (all
case variants of debug/info/warn/warning/error) in
TestLoad_LogLevelIsCaseInsensitive.
- TestLoad_ReportsAllErrorsAtOnce confirms errors.Join
surfaces multiple violations in one error.
Design note: validLogLevels is hardcoded in config.go rather
than imported from internal/logging to avoid a config→logging
import cycle (logging already depends on config). The set is
five entries and stable, so the duplication cost is minimal
and is flagged as a comment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First of the v0.2.0 quality cleanup PRs. Closes a QA gap flagged in the release review: `Load()` previously accepted any integer, so `MAX_REQUEST_BYTES=-100` or `REQUEST_TIMEOUT=-1s` would load silently and cause undefined downstream behavior.
Adds an explicit `Config.Validate()` method called at the end of `Load()`. Misconfiguration now fails loudly at startup with an actionable error message naming the offending env var and the raw value.
Rules
Error message format:
```
REQUEST_TIMEOUT must be positive, got -1s
MAX_REQUEST_BYTES must be non-negative, got -100
```
Test plan
Three new tests added to `config_test.go`:
Local checks
v0.2.0 release cleanup context
This is PR 1/4 of the v0.2.0 cleanup pass before tagging. The sequence is:
Intended to be squash-merged.