Implement settings validation and boundary checking#181
Conversation
eli-litzelman
commented
Apr 29, 2026
eli-litzelman
commented
- Default optional settings to None with proper validation
- Add boundary validation to catch missing env vars early
- Use new property for computed settings (fastapi_root_path)
- Move validation checks closer to where settings are used
- Add tests for settings validation
- Default optional settings to None with proper validation - Add boundary validation to catch missing env vars early - Use new property for computed settings (fastapi_root_path) - Move validation checks closer to where settings are used - Add comprehensive tests for settings validation
jbkolze
left a comment
jbkolze
left a comment
There was a problem hiding this comment.
Looks good but I would go ahead and wipe out (most of) the existing settings defaults as well -- that way the application won't assume configuration that has potentially been missed during environment setup.
| @@ -13,16 +14,49 @@ class Settings(BaseSettings): | |||
| cda_api_root: str = "http://traefik/cwms-data/" | |||
| default_job_runner: str = "batch" | |||
| dynamodb_host: str = "http://dynamodb:9010" | |||
| pguser: str = "" | |||
| pgpassword: str = "" | |||
| pguser: str | None = None | |||
| pgpassword: str | None = None | |||
| pgdatabase: str = "postgres" | |||
| pghost: str = "db" | |||
There was a problem hiding this comment.
I would probably go ahead and default these fields to None. Ideally everything will be provided to the environment through either the ECS infrastructure in CWBI or locally through the docker compose config. Keeping defaults set within the app source could lead to debugging confusion -- expected config should be provided explicitly through environment setup for consistency.
default_job_runner is an exception and can remain "batch" -- it isn't provided within the ECS infra.
| token, | ||
| key, | ||
| algorithms=["RS256"], | ||
| issuer=ISSUER[settings.auth_environment], |
There was a problem hiding this comment.
Pylance/Pyright shows an error here due to the change to settings.auth_environment. It's not actually an issue since Pyright just isn't smart enough to fully interpret the validator_boundaries function, but I'm a big fan of getting rid of all linting errors so you might consider re-enforcing the check at the function level (here). Up to you.
jbkolze
left a comment
jbkolze
left a comment
There was a problem hiding this comment.
Well it looks like we've opened up a bit of a can of worms here...
After the changes, if you spin up a local dev environment from the docker compose file you'll notice that the dispatcher container errors out. It doesn't have sufficient env var configuration to meet the new requirements.
Due to the way the application is structured, any component using the core app (e.g. the API and the local dispatcher container) inherits all of the settings and the associated validation. The API and dispatcher don't require the same config but use the same validation regardless.
Off of the top of my head there are three main ways to resolve this:
- Abandon the changes and go back to setting defaults within the pydantic settings. This is the easiest fix but could potentially obfuscate environment configuration issues.
- Set "dummy" env vars to avoid errors in the dispatcher container. This would sort of sidestep the issue, but would really only affect a local dev component and nothing used in production so is arguably acceptable.
- Break apart the base
Settingsclass into targeted settings classes, e.g.BaseSettings,DispatcherSettings,ApiSettings, etc. using inheritance as appropriate. This is probably the cleanest approach.
Given that this is a national enterprise application I would ultimately lean toward going the 3 route. That will, however, require:
- Identifying the config dependencies for each component to separate them
- Removing the global
settings = get_settings()and retrieving the appropriate settings as required within each module
So, if you want to go with 1 or 2 I think that'd be acceptable for now. Just depends on how deep you want to dive in. Let me know if you want to discuss.
2 participants
