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

Fix: model list schema constraints error when using Pydantic V1 #131

Merged
merged 5 commits into from
Sep 15, 2024

Conversation

juantoca
Copy link
Contributor

@juantoca juantoca commented Sep 11, 2024

What's being fixed

As a user of Pydantic V1, there are some models that use constraints on Pydantic List fields that fail on import:

ValueError: On field "annotations" the following field constraints are set but not enforced: max_length. 
For more details see https://docs.pydantic.dev/usage/schema/#unenforced-field-constraints

Reproduction steps

On the root of the project, with the venv populated and activated:

$ pip install "pydantic<2"

$ python
> import githubkit.versions.v2022_11_28.models.group_0932

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/juan/Documentos/OpenSource/githubkit/githubkit/versions/v2022_11_28/models/group_0932.py", line 21, in <module>
    class ReposOwnerRepoCheckRunsPostBodyPropOutput(GitHubModel):
  File "pydantic/main.py", line 197, in pydantic.main.ModelMetaclass.__new__
  File "pydantic/fields.py", line 502, in pydantic.fields.ModelField.infer
  File "pydantic/schema.py", line 1021, in pydantic.schema.get_annotation_from_field_info
ValueError: On field "annotations" the following field constraints are set but not enforced: max_length. 
For more details see https://docs.pydantic.dev/usage/schema/#unenforced-field-constraints

Solution

To palliate this issue, a ternary operator might be injected on the Pydantic Field constructor to disable or enable the constraints depending on the library version:

from githubkit.compat import PYDANTIC_V2
annotations: Missing[
        List[ReposOwnerRepoCheckRunsPostBodyPropOutputPropAnnotationsItems]
    ] = Field(
-        max_length=50,
        default=UNSET,
        description='Adds information from your analysis to specific lines of code. Annotations are visible on GitHub in the **Checks** and **Files changed** tab of the pull request. The Checks API limits the number of annotations to a maximum of 50 per API request. To create more than 50 annotations, you have to make multiple requests to the [Update a check run](https://docs.github.com/rest/checks/runs#update-a-check-run) endpoint. Each time you update the check run, annotations are appended to the list of annotations that already exist for the check run. GitHub Actions are limited to 10 warning annotations and 10 error annotations per step. For details about how you can view annotations on GitHub, see "[About status checks](https://docs.github.com/articles/about-status-checks#checks)".',
    )
from githubkit.compat import PYDANTIC_V2
annotations: Missing[
        List[ReposOwnerRepoCheckRunsPostBodyPropOutputPropAnnotationsItems]
    ] = Field(
+        max_length=50 if PYDANTIC_V2 else None,
        default=UNSET,
        description='Adds information from your analysis to specific lines of code. Annotations are visible on GitHub in the **Checks** and **Files changed** tab of the pull request. The Checks API limits the number of annotations to a maximum of 50 per API request. To create more than 50 annotations, you have to make multiple requests to the [Update a check run](https://docs.github.com/rest/checks/runs#update-a-check-run) endpoint. Each time you update the check run, annotations are appended to the list of annotations that already exist for the check run. GitHub Actions are limited to 10 warning annotations and 10 error annotations per step. For details about how you can view annotations on GitHub, see "[About status checks](https://docs.github.com/articles/about-status-checks#checks)".',
    )

@juantoca juantoca marked this pull request as ready for review September 11, 2024 16:58
@yanyongyu yanyongyu added the bug Something isn't working label Sep 12, 2024
@yanyongyu
Copy link
Owner

yanyongyu commented Sep 12, 2024

It seems pydantic v1 uses min_items, max_items to restrict the list items count and min_length, max_length are used to restrict the str length.

@yanyongyu yanyongyu added the schema schema related label Sep 12, 2024
@yanyongyu
Copy link
Owner

Maybe we need to add a custom Field in the compat module to handle the gap between v1 and v2.

@juantoca
Copy link
Contributor Author

juantoca commented Sep 13, 2024

Thank you @yanyongyu for the fast response and for maintaining this library.

I've tried your proposal of changing the kwarg name to max_items and sadly the issue persists. I think this is related to pydantic/pydantic#3745 linked in the comment at _get_field_args. AFAIK, this was fixed in Pydantic V2 but it wasn't back-ported to V1. That's the reason why I'm proposing to disable completely the constraints on Pydantic V1 through the operator.

I understand that this is sub-optimal as we are effectively not respecting the JSON schema but I see no other option as the models have ForwardReferences that will break the implementation in V1. Fortunatly, V2 will be unaffected.

@yanyongyu
Copy link
Owner

i see the comments in the code. this issue has occured before when using pydantic v1, and was fixed when pydantic v2 released. i will change the comments to describe the problem better.

@yanyongyu
Copy link
Owner

I will add some test cases later.

@yanyongyu yanyongyu changed the title fix: list constrainst on Pydantic V1 Fix: model list schema constraints error when using Pydantic V1 Sep 15, 2024
@yanyongyu yanyongyu merged commit cb085c8 into yanyongyu:master Sep 15, 2024
2 checks passed
@juantoca juantoca deleted the fix-list-contraints-pydantic-v1 branch September 16, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working schema schema related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants