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 Pydantic v1 and v2 #166

Merged
merged 9 commits into from
Aug 15, 2023

Conversation

ljodal
Copy link
Contributor

@ljodal ljodal commented Aug 2, 2023

This is another attempt at supporting both Pydantic v1 and v2, but without using the v1 module. Instead we configure WebAuthnBaseModel slightly differently based on the pydantic version.

This contains a slight behavior change in that trailing = in base64 encoded values are not removed, because pydantic v2 doesn't support that. If that is a problem (I'm not familiar with the spec) I can try to see if I can remove them somehow by doing some post-processing of the serialized values provided by pydantic

This is another attempt at supporting both Pydantic v1 and v2, but
without using the v1 module. Instead we configure WebAuthnBaseModel
slightly differently based on the pydantic version.

This contains a slight behavior change in that trailing = in base64
encoded values are not removed, because pydantic v2 doesn't support
that. If that is a problem (I'm not familiar with the spec) I can try to
see if I can remove them somehow by doing some post-processing of the
serialized values provided by pydantic
@MasterKale
Copy link
Collaborator

MasterKale commented Aug 2, 2023

You've put in a tremendous amount of work trying to make this all work, thank you again. 🙇

This contains a slight behavior change in that trailing = in base64 encoded values are not removed, because pydantic v2 doesn't support that.

...so I feel bad asking this, but can you to try to solve the problem of the padding being included in base64url output? Padding is optional in base64url, and I think it's helped avoid the "base64 vs base64url" ambiguity by omitting the = in JSON output.

I tried figuring out how to do it with the current version of Pydantic. Perhaps affected instances of BaseModel get @field_serializer definitions for each of their bytes fields? I guess there could be the same if PYDANTIC_V2 check in models that have these defined so that we don't try to define them when only Pydantic v1 is available? But boy, that's a lot more code to maintain...

@MasterKale
Copy link
Collaborator

I put in a feature request for Pydantic v2 to propose the addition of a "base64url" option for ConfigDict.ser_json_bytes. I wonder what the response will be...

pydantic/pydantic#7000

@ljodal
Copy link
Contributor Author

ljodal commented Aug 3, 2023

You've put in a tremendous amount of work trying to make this all work, thank you again. 🙇

This contains a slight behavior change in that trailing = in base64 encoded values are not removed, because pydantic v2 doesn't support that.

...so I feel bad asking this, but can you to try to solve the problem of the padding being included in base64url output? Padding is optional in base64url, and I think it's helped avoid the "base64 vs base64url" ambiguity by omitting the = in JSON output.

I tried figuring out how to do it with the current version of Pydantic. Perhaps affected instances of BaseModel get @field_serializer definitions for each of their bytes fields? I guess there could be the same if PYDANTIC_V2 check in models that have these defined so that we don't try to define them when only Pydantic v1 is available? But boy, that's a lot more code to maintain...

I can see what's possibly. One option is of course to explicitly define these serializers for every bytes field. That's possible and no big problem, but it'd be a bit of code to maintain as you say. And for any downstream subclasses the developers would have to do the same.

Another alternative is to dynamically add the field serializers. From what I can tell @field_serialzier doesn't support specifying "*" to get a callback for every field the same way it does for @field_validator. So that means at some point, either through a pydantic hook or a Python hook that runs before pydantic initializes the class, we'd need to dynamically inject the serializers for fields declared as bytes. It might be possible to use [__init_subclass__](https://docs.python.org/3/reference/datamodel.html#customizing-class-creation), but I'm not entirely sure.

I'll take a look next week, I'm currently up in the mountains on a little cabin trip without my laptop ⛰️

@ljodal
Copy link
Contributor Author

ljodal commented Aug 3, 2023

I put in a feature request for Pydantic v2 to propose the addition of a "base64url" option for ConfigDict.ser_json_bytes. I wonder what the response will be...

pydantic/pydantic#7000

Nice! An alternative is to ask for support for something like this:

class MyBaseModel(BaseModel):
    @field_serializer("*")
    def bytes_to_base64url(self, value: Any, info: SerializerInfo):
        # Check if field is bytes and if so serialize to base64url

@ljodal ljodal marked this pull request as ready for review August 7, 2023 09:32
@ljodal
Copy link
Contributor Author

ljodal commented Aug 7, 2023

I got it working with removing trailing = @MasterKale. Turns out it wasn't too hard after all. It's done using a model serializer that loops through the fields and strips trailing = from any bytes fields that have been serialized as strings:

@model_serializer(mode="wrap", when_used="json")
def clean_up_base64(
self, serializer: Callable[..., Dict[str, Any]]
) -> Dict[str, Any]:
"""
Remove trailing "=" from bytes fields serialized as base64 encoded strings.
"""
serialized = serializer(self)
for name, field_info in self.model_fields.items(): # type: ignore[attr-defined]
value = serialized.get(name)
if field_info.annotation is bytes and isinstance(value, str):
serialized[name] = value.rstrip("=")
return serialized

I'm fairly certain CI should be green now, the previous failure was due to a mypy version not supported by the pydantic plugin. Locally I have green mypy and tests on both Pydantic v1 and v2 as this branch currently stands

setup.py Show resolved Hide resolved
@MasterKale
Copy link
Collaborator

It was a tough decision, but I'm choosing this PR over #164 because this PR includes the use of V2 when it's present. That should better prepare this library for the eventual refactor to just Pydantic V2 support whenever that becomes viable.

@MasterKale MasterKale merged commit 731ae63 into duo-labs:master Aug 15, 2023
8 checks passed
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.

2 participants