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

package_managers: generic: Use a serializer for URL-typed fields #764

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eskultety
Copy link
Member

Pydantic formats down URL types as "type casts" in the serialized dictionary [1]. Apparently, that's by design and the docs advises to use custom serializers for given fields [2].

This patch is necessary, because pydantic 2.10.2 changed their behavior when it comes to handling URL types [3] and so once we bump the version we'd hit a test failure requiring us to change the unit test to "cast" some fields as AnyUrl rather than Url. Since these have no place in plain dictionaries, this patch fixes the problem in the correct way by the use of pydantic serializers.

[1] pydantic/pydantic#10998
[2] https://docs.pydantic.dev/latest/api/functional_serializers/#pydantic.functional_serializers.PlainSerializer
[3] https://pydantic.dev/articles/pydantic-v2-10-release#migrate-to-subclassing-instead-of-annotated-approach-for-pydantic-url-types

This relates to the changes necessary in order to eventually merge #756 .

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

Pydantic formats down URL types as "type casts" in the serialized
dictionary [1]. Apparently, that's by design and the docs advises to
use custom serializers for given fields [2].

This patch is necessary, because pydantic 2.10.2 changed their behavior
when it comes to handling URL types [3] and so once we bump the version
we'd hit a test failure requiring us to change the unit test to "cast"
some fields as AnyUrl rather than Url. Since these have no place in
plain dictionaries, this patch fixes the problem in the correct way by
the use of pydantic serializers.

[1] pydantic/pydantic#10998
[2] https://docs.pydantic.dev/latest/api/functional_serializers/#pydantic.functional_serializers.PlainSerializer
[3] https://pydantic.dev/articles/pydantic-v2-10-release#migrate-to-subclassing-instead-of-annotated-approach-for-pydantic-url-types

Signed-off-by: Erik Skultety <[email protected]>
@@ -88,7 +96,7 @@
:param download_url: The URL to download the artifact from.
"""

download_url: AnyUrl
download_url: Annotated[AnyUrl, PlainSerializer(lambda url: str(url), return_type=str)]

Check notice

Code scanning / CodeQL

Unnecessary lambda Note

This 'lambda' is just a simple wrapper around a callable object. Use that object directly.
@@ -120,7 +128,7 @@
class LockfileArtifactMavenAttributes(BaseModel):
"""Attributes for a Maven artifact in the lockfile."""

repository_url: AnyUrl
repository_url: Annotated[AnyUrl, PlainSerializer(lambda url: str(url), return_type=str)]

Check notice

Code scanning / CodeQL

Unnecessary lambda Note

This 'lambda' is just a simple wrapper around a callable object. Use that object directly.
@@ -3,7 +3,6 @@
from unittest import mock

import pytest
from pydantic_core import Url
Copy link
Member

@slimreaper35 slimreaper35 Dec 9, 2024

Choose a reason for hiding this comment

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

https://github.com/pydantic/pydantic-core

NOTE: You should not need to use pydantic-core directly; instead, use pydantic, which in turn uses pydantic-core.

:) This looked suspicious to me. We have a couple of other imports, so we should fix them too.

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK, but that's a material for a standalone PR I believe, this one is purely targeting a fix allowing us to in turn bump the pydantic version updates more seamlessly.

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