Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure extra='forbid' is enforced in DotEnvSettingsSource when env_prefix is specified #218

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

kjithin
Copy link
Contributor

@kjithin kjithin commented Jan 25, 2024

Fix #215

The current implementation in DotEnvSettingsSource allows non-prefixed variables even when extra='forbid'(default) is set . This commit addresses the issue by raising a SettingsError when extra is set to 'forbid' and variables are encountered without using the specified prefix.

Selected Reviewer: @dmontagu

…Source when env_prefix is specified

The current implementation in DotEnvSettingsSource allows non-prefixed variables even when extra='forbid'(default) is set . This commit addresses the issue by raising a SettingsError when extra is set to 'forbid' and variables are encountered without using the specified prefix.
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cc6dc25) 97.61% compared to head (82ac72c) 97.64%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #218      +/-   ##
==========================================
+ Coverage   97.61%   97.64%   +0.02%     
==========================================
  Files           5        5              
  Lines         336      339       +3     
  Branches       71       72       +1     
==========================================
+ Hits          328      331       +3     
  Misses          6        6              
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kjithin
Copy link
Contributor Author

kjithin commented Jan 25, 2024

please review

@hramezani
Copy link
Member

Thanks @kjithin for working on this 🙏

I think raising SettingsError is not the right approach. because it is not consistent with the dotenv source without env_prefix.

e.g. if we have a settings model like:

from pydantic_settings import BaseSettings, SettingsConfigDict


class Foo(BaseSettings):
    a: str

    model_config = SettingsConfigDict(env_file="my.env")

with my.env:

a=foo
b=bar

we will get

pydantic_core._pydantic_core.ValidationError: 1 validation error for Foo
b
  Extra inputs are not permitted [type=extra_forbidden, input_value='bar', input_type=str]
    For further information visit https://errors.pydantic.dev/2.5/v/extra_forbidden

So, we want to have the same behavior in the case of the user providing env_prefix as well.
it can probably fixed by touching the next block when we only collect envs that start with env_prefix. we need to collect all of them.

@hramezani
Copy link
Member

please update

@kjithin
Copy link
Contributor Author

kjithin commented Jan 25, 2024

@hramezani . Thanks for the feedback.

Initially, I explored that option. However, when collecting all the environment variables, a challenge arose: it would accept a valid property without a prefix. This occurred because the expected output for this function is a dictionary without prefixes in the keys.

for e.g.

from pydantic_settings import BaseSettings, SettingsConfigDict


class Foo(BaseSettings):
    a: str
    b: str

    model_config = SettingsConfigDict(env_file="my.env", env_prefix="prefix_")

with my.env

prefix_a=foo
b=bar

The identical ValidationError will only be triggered during the final model validation against the generated dictionary, specifically if it includes any additional properties. In the above case, when collecting all the env variables, both a & b properties will be accepted without any issue as they are valid property names.

@hramezani hramezani merged commit a6f6fa4 into pydantic:main Jan 25, 2024
20 checks passed
@hramezani
Copy link
Member

Good point @kjithin
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

env_prefix prevents env_file from working
3 participants