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

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

Merged
merged 16 commits into from
Jul 7, 2024
Merged

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

merged 16 commits into from
Jul 7, 2024

Conversation

VictorColomb
Copy link
Contributor

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
Copy link
Member

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 _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?

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:

  • The current implementation passes only the previous source state to the next one, I would suggest to pass all the previous states to the next one. so, the states should be something like
{'source_key': {source_state}}

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

@VictorColomb
Copy link
Contributor Author

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
Copy link
Member

Thanks @VictorColomb for updating the PR.

  • Please add some tests related to previous_states

@VictorColomb
Copy link
Contributor Author

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

self._previous_states = states

@property
def previous_states(self) -> dict[str, dict[str, Any]]:
Copy link
Member

Choose a reason for hiding this comment

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

I think previous_states and previous_state may make user confused. what about replacing previous_states with settings_sources_data and previous_state with current_state?

If you have any suggestions please let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have current_settings_state in mind, but it's uselessly verbose.

I don't find settings_sources_data particularly explicit, but I cannot find a better name. Maybe previous_settings_sources_output, or previous_sources_output (a little shorter)?

In any case, I've implemented your suggestions. Let me know what you think!

@hramezani
Copy link
Member

Thanks @VictorColomb

@hramezani hramezani merged commit 8b8803d into pydantic:main Jul 7, 2024
18 checks passed
mr-kamran-ali referenced this pull request in robert-koch-institut/mex-common Aug 8, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [pydantic-settings](https://github.com/pydantic/pydantic-settings)
([changelog](https://github.com/pydantic/pydantic-settings/releases))
| project.dependencies | minor | `==2.3.4` -> `==2.4.0` |

---

### Release Notes

<details>
<summary>pydantic/pydantic-settings (pydantic-settings)</summary>

###
[`v2.4.0`](https://github.com/pydantic/pydantic-settings/releases/tag/v2.4.0)

[Compare
Source](https://github.com/pydantic/pydantic-settings/compare/v2.3.4...v2.4.0)

#### What's Changed

- Fix regex flags accidentally passed as count by
[@&#8203;musicinmybrain](https://github.com/musicinmybrain) in
[https://github.com/pydantic/pydantic-settings/pull/328](https://github.com/pydantic/pydantic-settings/pull/328)
- Deprecate `read_env_file` and move it to `DotEnvSettingsSource` by
[@&#8203;WarpedPixel](https://github.com/WarpedPixel) in
[https://github.com/pydantic/pydantic-settings/pull/318](https://github.com/pydantic/pydantic-settings/pull/318)
- Fix a bug when loading empty yaml file by
[@&#8203;hramezani](https://github.com/hramezani) in
[https://github.com/pydantic/pydantic-settings/pull/330](https://github.com/pydantic/pydantic-settings/pull/330)
- feat: Enable access to the current state in settings sources by
[@&#8203;VictorColomb](https://github.com/VictorColomb) in
[https://github.com/pydantic/pydantic-settings/pull/326](https://github.com/pydantic/pydantic-settings/pull/326)
- Add support for short options. by
[@&#8203;kschwab](https://github.com/kschwab) in
[https://github.com/pydantic/pydantic-settings/pull/339](https://github.com/pydantic/pydantic-settings/pull/339)
- Add Azure Key Vault settings source by
[@&#8203;AndreuCodina](https://github.com/AndreuCodina) in
[https://github.com/pydantic/pydantic-settings/pull/272](https://github.com/pydantic/pydantic-settings/pull/272)
- Add cli_exit_on_error config option by
[@&#8203;kschwab](https://github.com/kschwab) in
[https://github.com/pydantic/pydantic-settings/pull/340](https://github.com/pydantic/pydantic-settings/pull/340)

#### New Contributors

- [@&#8203;musicinmybrain](https://github.com/musicinmybrain) made
their first contribution in
[https://github.com/pydantic/pydantic-settings/pull/328](https://github.com/pydantic/pydantic-settings/pull/328)
- [@&#8203;WarpedPixel](https://github.com/WarpedPixel) made their
first contribution in
[https://github.com/pydantic/pydantic-settings/pull/318](https://github.com/pydantic/pydantic-settings/pull/318)
- [@&#8203;VictorColomb](https://github.com/VictorColomb) made their
first contribution in
[https://github.com/pydantic/pydantic-settings/pull/326](https://github.com/pydantic/pydantic-settings/pull/326)
- [@&#8203;AndreuCodina](https://github.com/AndreuCodina) made their
first contribution in
[https://github.com/pydantic/pydantic-settings/pull/272](https://github.com/pydantic/pydantic-settings/pull/272)

**Full Changelog**:
pydantic/pydantic-settings@v2.3.4...v2.4.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yMC4xIiwidXBkYXRlZEluVmVyIjoiMzguMjEuNCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to build a cascading settings resolver
2 participants