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: issue#728 pydantic_core._pydantic_core.Url object is not iterable #730

Merged
merged 4 commits into from
Oct 14, 2023

Conversation

tomohirohiratsuka
Copy link
Contributor

Hi,
To fix #728 which causing below errors, I've added Url type in encoder.
I'd appreciate if you check it.
Thank you.

ValueError: [TypeError("'pydantic_core._pydantic_core.Url' object is not iterable"), TypeError('vars() argument must have __dict__ attribute')]

@tomohirohiratsuka tomohirohiratsuka changed the title pre-commit fix: issue#728 pydantic_core._pydantic_core.Url object is not iterable Oct 4, 2023
@gsakkis
Copy link
Contributor

gsakkis commented Oct 4, 2023

@roman-right before fixing this please merge #584. It's been open for 4 months and I already rebased and fixed the conflicts once, I probably won't bother again.

@roman-right
Copy link
Member

Hi @tomohirohiratsuka
Sorry, I've produced a merge conflict here. Please let me know, if you can resolve it, otherwise I'll pick it up.

@tomohirohiratsuka
Copy link
Contributor Author

@roman-right
Hi, I've resolved conflicts, please check it.
Thank you.

@roman-right
Copy link
Member

Hey @tomohirohiratsuka ,
It looks like a few checks failed. Could you please take a look?


import bson
import pydantic
from pydantic_core import Url
Copy link
Member

Choose a reason for hiding this comment

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

I think this one is not available for Pydantic V1. Can you please use a condition with IS_PYDANTIC_V2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make it conditional, changing Mapping to MutableMapping is acceptable ?

DEFAULT_CUSTOM_ENCODERS: MutableMapping[type, SingleArgCallable] = {
    ipaddress.IPv4Address: str,
    ipaddress.IPv4Interface: str,
    ipaddress.IPv4Network: str,
    ipaddress.IPv6Address: str,
    ipaddress.IPv6Interface: str,
    ipaddress.IPv6Network: str,
    pathlib.PurePath: str,
    pydantic.SecretBytes: pydantic.SecretBytes.get_secret_value,
    pydantic.SecretStr: pydantic.SecretStr.get_secret_value,
    datetime.timedelta: operator.methodcaller("total_seconds"),
    enum.Enum: operator.attrgetter("value"),
    Link: operator.attrgetter("ref"),
    bytes: bson.Binary,
    decimal.Decimal: bson.Decimal128,
    uuid.UUID: bson.Binary.from_uuid,
    re.Pattern: bson.Regex.from_native,
}
if IS_PYDANTIC_V2:
    from pydantic_core import Url
    DEFAULT_CUSTOM_ENCODERS[Url] = str

@tomohirohiratsuka
Copy link
Contributor Author

@roman-right
Hi, I've fixed the test case though it is a not perfectly clean since it has if condition inside the test.
Since pydantic v1, v2 url transform logic is changed like below.
I've confirmed beanie can handle both v1 same as before, v2 to fix bug.

Could you check changes?

from pydantic import BaseModel, HttpUrl


class HttpUrlModel(BaseModel):
	url: HttpUrl

pydantic v1

from main import HttpUrlModel


def test_debug():

	model = HttpUrlModel(url="https://www.google.com")
	assert model.url == "https://www.google.com" # True
	assert isinstance(model.url, str) # True

pydantic v2

from pydantic_core import Url

from main import HttpUrlModel


def test_debug():

	model = HttpUrlModel(url="https://www.google.com")
	# assert model.url == "https://www.google.com" # This is not true in v2
	# assert isinstance(model.url, str) # This is not true in v2
	assert isinstance(model.url, Url) # This is true in v2, Url object is a new object in V2.


async def test_url():
# same logic from encoder.py IS_PYDANTIC_V2
is_pydantic_v2 = int(pydantic.VERSION.split(".")[0]) >= 2
Copy link
Member

Choose a reason for hiding this comment

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

It is better to use IS_PYDANTIC_V2 variable here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll use it.

assert new_doc.url_field == doc.url_field
else:
doc = DocumentWithHttpUrlField(url_field="https://example.com")
assert isinstance(doc.url_field, str)
Copy link
Member

Choose a reason for hiding this comment

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

Please check, if Pydantic V1 has a class for URL inherited from str. I'll check it too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roman-right

Yes it has AnyUrl class.
Is it better to assert AnyUrl instead of str ?

def test_debug():

	model = HttpUrlModel(url="https://www.google.com")
	assert model.url == "https://www.google.com"
	assert isinstance(model.url, AnyUrl) # True since HttpUrl(AnyHttpUrl(AnyUrl(str)))

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is better, as it will display, that it was deserialized.
Hm. Will not HttpUrl work for both cases here then?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe I'm missing the point of the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is better, as it will display, that it was deserialized.
Hm. Will not HttpUrl work for both cases here then?

No it won't work since v1 use class inheritance but v2 uses subscripted generics for HttpUrl.

v2 networks.py

AnyUrl = Url
"""A type that will accept any http or https URL."""
HttpUrl = Annotated[Url, UrlConstraints(max_length=2083, allowed_schemes=['http', 'https'])]

v2 _pydantic_core.py

class Url(object):

v1 networks.py

class AnyUrl(str):

But if we use AnyUrl as assertion it will pass both version without if condition though it hides the base class of instance.

async def test_url():
    doc = DocumentWithHttpUrlField(url_field="https://example.com")
    assert isinstance(doc.url_field, AnyUrl)
    await doc.save()

    new_doc = await DocumentWithHttpUrlField.find_one(
        DocumentWithHttpUrlField.id == doc.id
    )

    assert isinstance(new_doc.url_field, AnyUrl)
    assert new_doc.url_field == doc.url_field

I think this depends on beanie's policy what we should assert exactly.

Copy link
Member

Choose a reason for hiding this comment

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

I think we have to be sure that it was encoded correctly. How it was decoded doesn't matter that much.
I'd check how the encoder encodes the document and would check if the output is correct. This way, we will be sure that it was stored in the db correctly. How it will be parsed is more Pydantic's responsibility. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree.
I've added a test for encoding.
Saving/Retrieving test case is remained to confirm it works in practice.

@roman-right
Copy link
Member

@tomohirohiratsuka
Having different tests for different Pydantic versions is ok. I left a few comments, please take a look

@roman-right roman-right merged commit 0db4dd4 into BeanieODM:main Oct 14, 2023
20 of 21 checks passed
@roman-right
Copy link
Member

Merged! Thank you for the PR. It will be published tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] 'pydantic_core._pydantic_core.Url' object is not iterable
3 participants