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

Implement proper support for nested complex env values #4216

Conversation

robertaistleitner
Copy link

@robertaistleitner robertaistleitner commented Jul 6, 2022

Change Summary

Pydantic didn't support setting fields containing "complex values" like dicts or lists from environment variables using nesting. This PR contains an implementation addressing this issue by checking which field the nested environment variable targets to, checking if the field is a complex value and if so, trying to parse the environment value as a JSON.

This already worked if explicitly using env=... keyword arg for a field but not for nested environment variables.

Related issue number

There is no issue created yet.

fix #2304 fix #4389

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • 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

@robertaistleitner
Copy link
Author

Hey there, please review.

@robertaistleitner
Copy link
Author

robertaistleitner commented Jul 6, 2022

Oops I missed the failing coverage check, will fix this.

@robertaistleitner
Copy link
Author

robertaistleitner commented Jul 15, 2022

Please note this PR fixed the same missing feature described in https://github.com/samuelcolvin/pydantic/issues/3971: #4235

What this PR also includes is checking the target field (is it really a complex field) and some more test cases.

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

Thanks @robertaistleitner for the patch 👍

I left some comments.

Also, I think we can improve the Parsing environment variable values doc and improve the example there with some examples from your change.

@hramezani
Copy link
Member

please update

robertaistleitner and others added 6 commits July 19, 2022 17:06
Simplify complex change description

Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
Improve typing

Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
Merge conditions with same outcome

Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
Improve typing

Co-authored-by: Hasan Ramezani <hasan.r67@gmail.com>
@robertaistleitner
Copy link
Author

I left some comments.

I worked the comments in as they were because they were all valid as is.

Also, I think we can improve the Parsing environment variable values doc and improve the example there with some examples from your change.

I added a simple example showing the implemented feature.

Please review again.

Copy link
Member

@hramezani hramezani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @robertaistleitner for updating. LGTM 👍

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.

Thanks so much for this, I think it looks like a great addition.

My only concern is whether it should be included in V1.10 or wait for V2? (We've got a bit of heat recently for introducing breaking changes in minor and patch updates, hence my paranoia)

There are two potential risks:

  • known unknowns: someone has a very weird schema for their settings and a field (e.g. Union[str, List[str]]) get's a different value after this PR vs. before
  • unknown unknowns: something else unintended changes

I'll think about it, but any input from @robertaistleitner @AngellusMortis @hramezani @PrettyWood on whether this is a suitable change for a minor release would be very welcome.

Comment on lines +221 to +222
def next_field(field: Optional[ModelField], key: str) -> Optional[ModelField]:
if not field or field.sub_fields:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is only called in one place, are you sure it needs to be a separate function?

If so, please add a docstring explaining what we're doing here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, you're right, my mistake.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there are collisions with existing setting schemas due to the fact that setting complex values from an environment variable never really worked.

Nevertheless, it's up to you - for now I'm using a monkeypatch that does the job pretty well for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand where you're coming from, I'm just a bit paranoid at the moment about "breaking" changes, hence deferring this.

@samuelcolvin samuelcolvin mentioned this pull request Aug 9, 2022
14 tasks
@hramezani
Copy link
Member

My only concern is whether it should be included in V1.10 or wait for V2?

V2 makes more sense to me. The feature itself is great but the implementation is complex and it may introduce breaking changes that is not visible now.

@PrettyWood
Copy link
Member

I would also differ this for v2

@samuelcolvin
Copy link
Member

Closing since:

  1. this is deferred to V2
  2. settings logic has moved to pydantic-settings
  3. We need to resolve [WIP] Improved source management pydantic-settings#10 which might solve this.

@samuelcolvin
Copy link
Member

Thanks so much @robertaistleitner for the proposed change, I'd love you to contribute to pydantic-settings or indeed pydantic in the future.

Sorry we couldn't accept this this time.

@hramezani
Copy link
Member

@robertaistleitner I've created a new PR in pydantic-settings based on your idea here.

@robertaistleitner
Copy link
Author

@hramezani that's awesome, thanks for your effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deferred deferred until future release or until something else gets done
Projects
None yet
4 participants