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

Fix #3807: matching characters in nested env delimeter and env prefix #3975

Merged

Conversation

arsenron
Copy link
Contributor

@arsenron arsenron commented Apr 4, 2022

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 and env_nested_delimiter match, it occurs an unexpected behaviour as the initial splitting was wrong in this case.

I added a new env_prefix field to EnvSettingsSource to fix the issue without parsing ModelField which I think can create a little "code mess".

Related issue number

fix #3807

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Copy link
Member

@samuelcolvin samuelcolvin left a 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.

@@ -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) :]
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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).

Copy link
Contributor Author

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

pydantic/env_settings.py Outdated Show resolved Hide resolved
@samuelcolvin
Copy link
Member

please update.

@samuelcolvin
Copy link
Member

please review.

@arsenron please use the workflow explained in the PR template to assign PRs for review in future.

@samuelcolvin samuelcolvin merged commit 6650edf into pydantic:master Aug 11, 2022
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.

Unexpected behaviour if the prefix contains the env_nested_delimiter
4 participants