pydantic / pydantic-settings

Settings management using pydantic
https://docs.pydantic.dev/latest/usage/pydantic_settings/
MIT License
591 stars 61 forks source link

feat: Enable access to the current state in settings sources #326

Closed VictorColomb closed 2 months ago

VictorColomb commented 3 months ago

Why this PR?

This PR aims at enabling custom settings sources to access the output of the previous sources, through the .previous_state property.

The ultimate use-case is the same as #224, however this PR does not introduce a breaking change.

Fixes #223.

Use-case

My real-life use-case: retrieving settings from AWS Secrets Manager but setting the secret name in environment variables.

Discussion

There are a number of things to be discussed about this PR, and I would very much appreciate the input / guidance of the pydantic-settings maintainers.

  1. I have reversed the order in which the settings sources are discovered (here) in order for the output of the sources with the most priority to be available to the ones of lesser importance. This seems like the most intuitive behavior to me, but this is rather subjective. Any preference?
  2. Currently, settings sources are being handed the raw output from the previous ones. In other words, nothing has yet been validated by Pydantic. Would it be desired behavior to instead pass a validated instance of self.settings_cls? I have not gone this way because imho it adds to much complexity for not that much benefit.
  3. I have defined the __previous_state setter method as "private", in that Python will perform name mangling on the method. This way, we are almost certain to introduce no breaking changes at all (someone could have defined their own set_previous_state method). However, that means calling _PydanticBaseSettingsSource__set_previous_state in the sources discovery code, which is neither elegant nor pythonic. My question therefore is should I remove the double-underscore for cleaner code at the marginal risk of introducing a breaking change for some?
  4. Should I add a bit of documentation, for example as a subsection of the Adding sources section?
hramezani commented 3 months ago

Thanks @VictorColomb for this PR and sorry for the late response.

I have reversed the order in which the settings sources are discovered (here) in order for the output of the sources with the most priority to be available to the ones of lesser importance. This seems like the most intuitive behavior to me, but this is rather subjective. Any preference?

This is good

Currently, settings sources are being handed the raw output from the previous ones. In other words, nothing has yet been validated by Pydantic. Would it be desired behavior to instead pass a validated instance of self.settings_cls? I have not gone this way because imho it adds to much complexity for not that much benefit.

Your approach is fine. don't need to validate beforehand.

I have defined the previous_state setter method as "private", in that Python will perform name mangling on the method. This way, we are almost certain to introduce no breaking changes at all (someone could have defined their own set_previous_state method). However, that means calling _PydanticBaseSettingsSourceset_previous_state in the sources discovery code, which is neither elegant nor pythonic. My question therefore is should I remove the double-underscore for cleaner code at the marginal risk of introducing a breaking change for some?

Yes, let's have it without double underscore. let's have the previous_states as a property and _previous_states and the setter as a public method.

Should I add a bit of documentation, for example as a subsection of the Adding sources section?

sure.

I have a request:

{'source_key': {source_state}}

Please let me know when you are ready for the next round of review.

VictorColomb commented 3 months ago

Hi @hramezani. Thanks for your feedback!

I have implemented your suggestions.

However, I do see a potential pitfall in recording all previous states: if two settings sources have the same name (e.g. two instances of the same class, but with different init parameters), only the latest one would be recorded in previous_states.

hramezani commented 3 months ago

Thanks @VictorColomb for updating the PR.

VictorColomb commented 3 months ago

@hramezani test added! I also updated my branch from main.

hramezani commented 2 months ago

Thanks @VictorColomb