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 #1458 - Allow for custom parsing of environment variables via env_parse #3977

Closed
wants to merge 6 commits into from

Conversation

acmiyaguchi
Copy link
Contributor

@acmiyaguchi acmiyaguchi commented Apr 5, 2022

Change Summary

This PR adds an env_parse extra into Fields to allow for custom parsing of the environment within Settings. This is useful when specifying a custom format in environment strings e.g. lists (1,2,3) or key-value pairs (a=1,b=2,c=3). String-encoded JSON is not ideal in certain situations, such as when the environment is specified inside of a YAML file (leading to string-escaped string-encoded JSON).

Related issue number

fixes #1458

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

acmiyaguchi added a commit to acmiyaguchi/pydantic that referenced this pull request Apr 5, 2022
@acmiyaguchi
Copy link
Contributor Author

please review

@gradybarrett
Copy link

gradybarrett commented Jun 21, 2022

Any chance of this getting reviewed any time soon?

@acmiyaguchi what about the case where I want to pre-process/parse a simple value? Let's say strip and sanitize a value during parsing. One could do that w/ this same mechanism if we extended the parse function to simple types as well, such as this logic branch:

https://github.com/samuelcolvin/pydantic/blob/b01c8f2a6256c711472ad2f6bb5a35325e4d5f12/pydantic/env_settings.py#L177-L179

@acmiyaguchi
Copy link
Contributor Author

I appreciate the bump on the review.

What about the case where I want to pre-process/parse a simple value? Let's say strip and sanitize a value during parsing. One could do that w/ this same mechanism if we extended the parse function to simple types as well [...].

I want to limit the scope of this PR to the related issue. If I understand correctly, you can use a validator to transform a simple value instead of using a custom parsing function (which is only necessary when trying to parse from an external source like the environment). While weird, minor modifications like stripping and sanitizing are idiomatic in Pydantic. I might be misunderstanding, though -- discussing the parent issue or a new issue might be in order.

@gradybarrett
Copy link

gradybarrett commented Jun 22, 2022

Ahhh you're right. I hadn't quite digested how all the components could be composed.

Still looking forward to this or a similar solution to make customizing complex field-level deserialization less hacky. 😄

@samuelcolvin
Copy link
Member

I'll try to get to this soon. I've been working pretty hard on getting pydantic-core ready for the first released, but I'll try to get back to work on 1.10 soon.

@samuelcolvin
Copy link
Member

Looks good, but having reviewed the PR, I think this would be better deal with using suggestion 2 from the original PR: looking for a parse_env_var method on Config.

@acmiyaguchi do you think you could please update the PR?

@acmiyaguchi
Copy link
Contributor Author

Ack, I'll look into implementing the second method this week. Thanks for the review!

@samuelcolvin
Copy link
Member

samuelcolvin commented Aug 19, 2022

Notice

See twitter 🐦, I've you'd like this to be included in V1.10, please fix it and request a review TODAY.

Or if you need this in V1.10 but don't have time to complete it (or aren't the author), please comment here and on #4324.

@acmiyaguchi
Copy link
Contributor Author

Apologies for being last minute with implementation and discussion here, but I'd like to make a case against option 2:

Looks good, but having reviewed the PR, I think this would be better deal with using suggestion 2 from the original PR: looking for a parse_env_var method on Config.

Suppose I had two fields in a setting class: one of them parses using _parse_custom_dict: Callable[[str], Dict[int, str]] and the other _parse_custom_list: Callable[[str], List[str]]. The current implementation in this PR has the following shape:

class Settings(BaseSettings):
    my_list: List[str] = Field(env_parse=_parse_custom_list)
    my_dict: Dict[int, str] = Field(env_parse=_parse_custom_dict)

Whereas suggestion two take on the following look:

class BaseSettings:
    my_list: List[str]
    my_dict: Dict[int, str]

    class Config:
        @classmethod
        def parse_env_var(cls, field, raw_val):
            if field == "my_list":
                return _parse_custom_list(raw_val)
            elif field == "my_dict":
                return _parse_custom_dict(raw_val)
            return cls.json_loads(raw_val)

I personally prefer the former solution in the context of the codebase where I'm currently using pydantic. I want to use a custom parsing function for lists and key-value pairs across several setting classes. Being forced to dispatch the parsing function via parse_env_var in the Config file is less flexible, because it requires knowing the names of the fields up front or doing some actual parsing to determine at runtime whether the environment variable is "listy" or "dicty".

I believe the tests in the PR show a bit of flexibility in how parsing could be done with the option on the Field. An alternative to my first implementation above could be done by passing str as the parsing function and using a validator to transform the str into the appropriate value:

class Settings(BaseSettings):
    my_list: List[str] = Field(env_parse=str)
    my_dict: Dict[int, str] = Field(env_parse=str)
    
    @validator('my_list', pre=True)
    def val_list(cls, v):
        assert isinstance(v, str)
        return _parse_custom_list(v)

    @validator('my_list', pre=True)
    def val_dict(cls, v):
        assert isinstance(v, str)
        return _parse_custom_dict(v)

It's a little verbose, but it seems to fit the style of pydantic.

@acmiyaguchi
Copy link
Contributor Author

I've implemented the alternative method via Config.parse_env_var in #4406. I've been using a workaround using a union type and custom validator, so there's no urgency on my end to make it into the next version of the package.

acmiyaguchi added a commit to acmiyaguchi/pydantic that referenced this pull request Aug 19, 2022
@acmiyaguchi
Copy link
Contributor Author

please review

@samuelcolvin
Copy link
Member

I think your implementation in #4406 is much better, I'm going to close this and accept that PR once a few small things are tweaked.

Apologies for being last minute with implementation and discussion here, but I'd like to make a case against option 2:

Having thought about this I'm afraid I disagree. I get that Field(env_parse=str) could be ergonomic and useful, and I think it's something we should think about in V2 where settings might become a separate package, pydantic-settings.

The reasons I prefer parse_env_var are:

  1. It's closer to the way the rest of pydantic works, in a minor release I want to make as few conceptual changes as possible
  2. I think it's simpler to implement and therefore less likely to introduce bugs and breaking changes
  3. If we do want per-field parsing, I think we should use something like the Json type, or ✨ Add types SpaceSeparated, CommaSeparated, CommaSeparatedStripped #1848, not as a Field argument - unless we move to using Field() more for stuff like this in pydantic V2
  4. With lots of fields on settings, I personally would prefer to have one function that takes care of parsing for multiple fields, rather than have to remember to set/change Field.env_parse on every field

Thanks so much for working on this, and for #4406. I'd love your help more when we get to redesign settings management in V2.

samuelcolvin added a commit that referenced this pull request Aug 22, 2022
…se_env_var in Config object (#4406)

* Fix #1458 - Allow for custom parsing of environment variables via env_parse

* Add docs for env_parse usage

* Add changes file for #3977

* fixup: remove stray print statement

* Revert env_parse property on field

* Add parse_env_var classmethod in nested Config

* Update documentation for parse_env_var

* Update changes file.

* fixup: linting in example

* Rebase and remove quotes around imported example

* fix example

* my suggestions

* remove unnecessary Field(env_parse=_parse_custom_dict)

Co-authored-by: Samuel Colvin <s@muelcolvin.com>
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.

Settings: Custom parsing of environment variables (non-JSON)
5 participants