search

duo-labs / py_webauthn

Pythonic WebAuthn 🐍
https://duo-labs.github.io/py_webauthn
BSD 3-Clause "New" or "Revised" License
856 stars 171 forks source link

Support both v1 and v2 of Pydantic #164

Closed martynsmith closed 1 year ago

martynsmith commented 1 year ago

One of the nice parts about pydantic v2 is that they provide backward compability by importing from pydantic.v1. This adds support so that regardless of which version of pydantic the project is using, py_webauthn will work.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

martynsmith commented 1 year ago

I do see there's a branch to upgrade to Pydantic V2, this feels like a nice stepping stone for people using the library where both will work.

ljodal commented 1 year ago

This seems like a nice stepping stone for unblocking downstream projects from upgrading to Pydantic v2, but in my experience using the v1 imports is a bit clumsy in a project where you mainly use v2 features. For example when you have the pydantic.mypy plugin installed it will complain about a bunch of things related to the v1 models. Not entirely sure why.

And if this is the approach we go for it should probably be well documented, as any subclasses of WebAuthnBaseModel will also be limited to the v1 api and users would have to be aware of that

ljodal commented 1 year ago

Here's a real-life example from our codebase where this approach would cause very confusing behavior. The first model would be a pydantic v1 model while the second one a v2 model:

Screenshot 2023-08-02 at 10 38 44

We also have a decorator we use to automatically generate OpenAPI schemas for our APIs, which relies heavily on Pydantic and I was able to simplify the logic a from the improvements in Pydantic v2. However, that's going to crash if it encounters a v1.BaseModel because the TypeAdapter class in Pydantic v2 doesn't support that 🤔

ljodal commented 1 year ago

I've attempted an alternative approach in #166, turns out Pydantic v2 has native support for serializing bytes to base64 encoded string, and it appears to be using the URL safe variant, so that might be a viable path

martynsmith commented 1 year ago

@MasterKale - Are you just going to do the split-version thing (i.e. not use this MR) or still undecided?

I'm just sort of hanging out to upgrade to pydantic2 in my own project and trying to figure out how I'm going to proceed (looking for some idea of a timeline on this if you have any idea what that might be)?

MasterKale commented 1 year ago

PR #166 caught my attention and I thought that was going to end up being the way to support v1 and v2, but after coming back to this based on your nudge I think this will probably be the intermediate step. Then I can spend some time working on updating to v2 (though I'm hoping https://github.com/pydantic/pydantic/issues/7000 gets some support because it'll make the migration a lot smoother.)

I'll take a look again in the morning, it's till Sunday evening for me. The fact that tests are passing in this branch gives me hope that I can quickly merge and release as v1.10.0 tomorrow, or Tuesday at the latest to give me some wiggle room.

martynsmith commented 1 year ago

Awesome, thanks heaps for that 👍

On Mon, 7 Aug 2023 at 4:16 PM, Matthew Miller @.***> wrote:

PR #166 https://github.com/duo-labs/py_webauthn/pull/166 caught my attention and I thought that was going to end up being the way to support v1 and v2, but after coming back to this based on your nudge I think this will probably be the intermediate step. Then I can spend some time working on updating to v2 (though I'm hoping pydantic/pydantic#7000 https://github.com/pydantic/pydantic/issues/7000 gets some support because it'll make the migration a lot smoother.)

I'll take a look again in the morning, it's till Sunday evening for me. The fact that tests are passing in this branch gives me hope that I can quickly merge and release as v1.10.0 tomorrow, or Tuesday at the latest to give me some wiggle room.

— Reply to this email directly, view it on GitHub https://github.com/duo-labs/py_webauthn/pull/164#issuecomment-1667158956, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDQFPXFETWEGTPRX6BNDDXUBT2VANCNFSM6AAAAAA2656M6I . You are receiving this because you authored the thread.Message ID: @.***>

martynsmith commented 1 year ago

Hi @MasterKale , any luck having a look at making a v1.10.0?

MasterKale commented 1 year ago

@martynsmith Thank you for creating this PR. I've opted for PR #166 to add intermediate Pydantic V2 support so I'm going to close this one out.