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

Support both v1 and v2 of Pydantic #164

Closed
wants to merge 2 commits into from

Conversation

martynsmith
Copy link

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.

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

CLAassistant commented Jul 31, 2023

CLA assistant check
All committers have signed the CLA.

@martynsmith
Copy link
Author

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.

@@ -4,7 +4,7 @@
try:
from pydantic.v1 import BaseModel
except ImportError:
from pydantic import BaseModel
from pydantic import BaseModel # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the #type: ignore's on the v2 imports because mypy, referencing v1 types, would complain about these?

Copy link
Author

Choose a reason for hiding this comment

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

The imports in the except block are actually "v1" imports.

If we can't import from pydantic.v1 (which only exists in v2), then we instead try to import from pydantic (assuming that v1 is installed).

Because in the repo itself we have v2 installed, I've put the ignores on the "v1" imports because when v2 is installed that's actually importing the new v2 API which conflicts with the old v1 version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right pydantic.v1 is the new import 🤦‍♂️

So would this mean users still using Pydantic v1 in their project wouldn't get mypy-assisted type annotations on the methods in this library if they're still on Pydantic v1? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, yeah, I think that's probably right (I have no idea how you'd go about supporting that either) 😬

Copy link
Author

Choose a reason for hiding this comment

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

Actually, depending on how much of the internal pydantic stuff is being exposed, I suspect it probably won't be that much of a degraded experience for those folks?

Copy link
Contributor

Choose a reason for hiding this comment

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

So would this mean users still using Pydantic v1 in their project wouldn't get mypy-assisted type annotations on the methods in this library if they're still on Pydantic v1? 🤔

From what I've seen mypy will also complain about a lot of stuff if you have v2 installed and use the v1 imports, at least if the mypy plugin for pydantic is enabled.

@MasterKale MasterKale mentioned this pull request Aug 1, 2023
@ljodal
Copy link
Contributor

ljodal commented Aug 2, 2023

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

ljodal commented Aug 2, 2023

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

ljodal commented Aug 2, 2023

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

@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
Copy link
Collaborator

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 pydantic/pydantic#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
Copy link
Author

martynsmith commented Aug 7, 2023 via email

@martynsmith
Copy link
Author

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

@MasterKale
Copy link
Collaborator

@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.

@MasterKale MasterKale closed this Aug 15, 2023
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.

4 participants