-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix #3807: matching characters in nested env delimeter and env prefix #3975
Fix #3807: matching characters in nested env delimeter and env prefix #3975
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks so much.
Please add a change description as explained in the PR template.
pydantic/env_settings.py
Outdated
@@ -228,7 +234,8 @@ def explode_env_vars(self, field: ModelField, env_vars: Mapping[str, Optional[st | |||
for env_name, env_val in env_vars.items(): | |||
if not any(env_name.startswith(prefix) for prefix in prefixes): | |||
continue | |||
_, *keys, last_key = env_name.split(self.env_nested_delimiter) | |||
env_name_without_prefix = env_name[len(self.env_prefix) :] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment here explaining why we're doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. We are splitting an environment variable key into keys
and last_key
by self.env_nested_delimiter
and in case of matching characters in both self.env_nested_delimiter
and self.env_prefix
, we were adding to keys
an environment variable prefix. This was before, but now we just strip a prefix and can safely split by a delimeter because there will be no any more conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, when I say "please add a comment", I mean add a comment in the code. I know why we're doing this, but we comment the code to make it easier for someone reading this in future (including me when I've forgotten this conversation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, sorry for misunderstanding
please update. |
please review. @arsenron please use the workflow explained in the PR template to assign PRs for review in future. |
Change Summary
Hi! It is my first pull request to the project which I really, really appreciate :) The problem is when characters in
env_prefix
andenv_nested_delimiter
match, it occurs an unexpected behaviour as the initial splitting was wrong in this case.I added a new
env_prefix
field toEnvSettingsSource
to fix the issue without parsingModelField
which I think can create a little "code mess".Related issue number
fix #3807
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)