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 Pydantic v2 #38

Closed
dosisod opened this issue Jul 14, 2023 · 6 comments · Fixed by #45
Closed

Support Pydantic v2 #38

dosisod opened this issue Jul 14, 2023 · 6 comments · Fixed by #45
Labels
enhancement New feature or request

Comments

@dosisod
Copy link
Contributor

dosisod commented Jul 14, 2023

Pydantic v2 just came out, so it would be nice if githubkit could support the newer, faster version of Pydantic. I attempted to upgrade it myself, but there are a few bugs and non-trivial design decisions that would need to be made to support Pydantic v2, so I thought I'd bring this up now.

The migration guide does a good job of explaining what's changed, and how to upgrade your code. I'm just showing what I've encountered thus far, though there is probably additional work that needs to be done.

First up, regex is being renamed to pattern in Field() objects. Easy enough to fix, just change it in the codegen.

Second thing I ran into is that parse_raw_as has been removed, and instead you need to use a TypeAdapter:

    @property
    def parsed_data(self) -> RT:
        # old
        return parse_raw_as(self._data_model, self._response.content)

        # new
        return TypeAdapter(self._data_model).validate_python(self._response.content)

Lastly (or at least, where I stopped) is with Missing[]. This is a bug, as Pydantic v2 does not seem to like literal values like Literal[UNSET]. For example, the following code is throwing an error:

class Dependency(GitHubRestModel):
    """Dependency"""

    package_url: Missing[str] = Field(
        description="Package-url (PURL) of dependency. See https://github.com/package-url/purl-spec for more details.",
        pattern="^pkg",
        default=UNSET,
    )

This produces:

...

  File "githubkit/rest/models.py", line 7876, in <module>
    class Dependency(GitHubRestModel):
  File ".venv/lib/python3.11/site-packages/pydantic/_internal/_model_construction.py", line 172, in __new__
    complete_model_class(
  File ".venv/lib/python3.11/site-packages/pydantic/_internal/_model_construction.py", line 438, in complete_model_class
    cls.__pydantic_validator__ = SchemaValidator(simplified_core_schema, core_config)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
pydantic_core._pydantic_core.SchemaError: Invalid Schema:
model.schema.model-fields.fields.package_url.schema.default.schema.union.pattern
  Extra inputs are not permitted [type=extra_forbidden, input_value='^pkg', input_type=str]
    For further information visit https://errors.pydantic.dev/2.1.2/v/extra_forbidden

Replacing Missing[str] with Union[Literal["xyz"], str] still fails, but Optional[str] works fine.

This seems to be tracked in pydantic/pydantic#6601. The error looks slightly different, though it still is an issue with Literal values in Unions.

I'm happy to work on a PR for this, though I thought I would get your input on a few things before I go ahead:

  • From what I can tell, this is a backwards-incompatible change. If you upgrade to Pydantic v2, you probably won't be able to support v1 as well
  • If the Literal issue doesn't get fixed soon (though it probably should), do you see any potential workarounds? We could just use Optional[str], though None doesn't convey the same thing as UNSET.

Let me know what your thoughts are on this, thanks!

@yanyongyu yanyongyu added the enhancement New feature or request label Jul 14, 2023
@yanyongyu
Copy link
Owner

yanyongyu commented Jul 14, 2023

The error reported with MISSING[str] is that the Union type does not allow to declare a pattern in the Field info. This seems different from pydantic/pydantic#6601, and may be a new issue. Here is a minimum repreduce code:

class Test(BaseModel):
    field: Union[int, str] = Field(default=1, pattern="^pkg")

This is not related to the UNSET and Literal, and can be repreduced using any types union. This field info is allowed in pydantic v1. May be we should report this to pydantic v2.

@yanyongyu
Copy link
Owner

yanyongyu commented Jul 14, 2023

After some experiment...

It works with the Annotated expression:

from typing import Union, Annotated
from pydantic import BaseModel, Field, ConfigDict

class Test(BaseModel):
    field: Union[int, Annotated[str, Field(pattern="^pkg")]] = Field(default=1)

    model_config = ConfigDict(strict=True)

I have reported this problem to pydantic issue.

@yanyongyu
Copy link
Owner

The same issue as pydantic/pydantic#6577.

@dosisod
Copy link
Contributor Author

dosisod commented Jul 14, 2023

I guess that makes sense, though I'd argue any object with a __str__() method should be usable with pattern. Might yield weird results, but feels more intuitive to me (and better than a vague error).

How trivial would it be to add codegen support for this Annotated fields? I imagine that there are params other than pattern that will need to be handled differently as well.

@yanyongyu
Copy link
Owner

yanyongyu commented Jul 15, 2023

Since the new constrant logic of pydantic v2, we may need to bind the constrants directly on the type instead of the field, and generate the type using Annotated. We should have a constrant-type mapping according to the json schema to do this bind operation.

@yanyongyu
Copy link
Owner

Thanks to @sudosubin, githubkit now supports pydantic v2. But, before this feature can be released, i need to investigate how to make it compatible with pydantic v1 and v2. I will refactor the codegen for future improvements including lazy loading #11 after another project i'm currently working on finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants