search

nymous / pydantic-vault

A simple extension to Pydantic BaseSettings that can retrieve secrets from Hashicorp Vault
MIT License
52 stars 12 forks source link

fix: `TypeError` breaking Pydantic defaults when Vault returns `None` #13

Closed ingvaldlorentzen closed 1 year ago

ingvaldlorentzen commented 1 year ago

This PR fixes an issue where Vault would return None if the path referenced doesn't exist. This would raise a TypeError as line 263 in vault_settings.py would try to access data from None.

It's been solved by catching TypeError in line 264, where VaultError was already being caught if the secret key is not found.

Test has been updated to catch this. Since the fake vault catches KeyErrors, a key that just returns None has been added to the fake vault data.

To get the tests to run on Python 3.11 I had to update the Python requirement to ^3.7.0 and run poetry update. So the lockfile has also been updated.

This means that support for Python 3.6 is dropped, I hope that's alright. Python 3.6 is EOL after all.

ingvaldlorentzen commented 1 year ago

@nymous do you have time to merge this and cut a new release? Or give feedback if you want changes?

nymous commented 1 year ago

Hello! Thank you for your contribution, I am very sorry for the delay, life and work and stuff ^^' I will try to look at it this weekend.

nymous commented 1 year ago

Hi again! Do you think it would be better to test explicitly if the Vault response is None before accessing ["data"], or to catch any TypeError that can happen?

I will see if it's possible to keep the Python 3.6 compatibility ^^

ingvaldlorentzen commented 1 year ago

I am very sorry for the delay, life and work and stuff

Nothing to apologize for! I'm pretty delayed in my response now, for the same reasons, haha

Do you think it would be better to test explicitly if the Vault response is None before accessing ["data"], or to catch any TypeError that can happen?

I think catching TypeError like I've done is a nice way to do it, especially since VaultError is being caught there already. But if you think checking for None is better I can update the PR to do that. Doesn't really matter much to me, the result will be the same from my perspective.

I will see if it's possible to keep the Python 3.6 compatibility ^^

I honestly wouldn't bother, a lot of packages these days are dropping Python 3.8 support, and as mentioned Python 3.6 is EOL, it's not even getting security patches anymore, nobody should be using it.

nymous commented 1 year ago

I mostly fear that TypeError might catch too much, whereas testing None seems more precise, but maybe my fears are not warranted ^^ For the 3.6 release I know, but I thought that I could do a 0.7.2 with just this fix, and right after a 0.8.0 that drops 3.6 compatibility.

ingvaldlorentzen commented 1 year ago

I updated the PR now, so it checks for None and raises a VaultError if it does. So the behaviour is pretty much the same, except that it won't catch TypeError as you mentioned.

As for Python 3.6, yeah, it's a good idea to not drop a Python version in a patch release. Would it work for you if I just update this PR to be 0.8.0 instead?

ingvaldlorentzen commented 1 year ago

Had time to think about the Python 3.6 thing?

nymous commented 1 year ago

I finally decided to revert the changes to the Python version, and release just this fix as 0.7.2. The package is installable on Python 3.11, it just cannot be tested on it yet, but I trust :crossed_fingers: ^^

(I had to upgrade Poetry lockfile and fix the CI and stuff, hope you don't mind my incursion on your PR :sweat_smile:)

Thank you for your contribution!