Fix AnyUrl not being encodable #1071
Conversation
07pepa
requested review from
staticxterm,
Riverfount,
adeelsohailahmed,
LawMixer,
ayusshsoarav and
CAPITAINMARVEL
|
Looks good, please add some tests to tests/odm/test_encoder.py or elsewhere? |
there already is test that would fail without change ... hiden by using older version upgrading version uncovered this. |
and set compatible pymongo
07pepa
force-pushed
the
fix-bug-of-anyurl
branch
from
433e00f to
9d318f0
Compare
and set compatible pymongo
and set compatible pymongo address newest pydantic changes
Okay, I get this, but I believe there should be a kind of a test that would catch this regression, if possible. But I see you've added the OpenAPI schema generation test. I guess this could suffice for now. |
|
|
||
| if IS_PYDANTIC_V2: | ||
|
|
||
| @classmethod | ||
| def validate(cls, v, _: ValidationInfo): | ||
| def _validate(cls, v: str | PydanticObjectId, *_) -> PydanticObjectId: |
There was a problem hiding this comment.
Why write the signature of the _validate() method with "*_" at the end? Which other arguments can it receive?
I believe the Pydanticv2 no_info_plain_validator_function() signature of the called method is simply (cls, v).
I tested this and it worked for me both under Pydantic v1 and Pydantic v2, using a single cls._validate(cls, v) method.
Also, the v: str | PydanticObjectId part, the v can be "Any" essentially. Due to the if check on line 145, v can also be bytes.
| if IS_PYDANTIC_V2: | ||
| plain_validator = ( | ||
| core_schema.with_info_plain_validator_function | ||
| if hasattr(core_schema, "with_info_plain_validator_function") | ||
| core_schema.no_info_plain_validator_function | ||
| if hasattr(core_schema, "no_info_plain_validator_function") | ||
| else core_schema.general_plain_validator_function | ||
| ) | ||
| else: |
There was a problem hiding this comment.
In my unpublished commit (working on the Link/BackLink serialization bugfix) I simply removed all of this boilerplate code and used the Pydantic v2 no_info_plain_validator_function() and a single cls._validate(cls, v) method, which already had the same validation code running under both major Pydantic versions.
Of course, this required some small but simple refactoring.
|
|
||
| await asyncio.gather(*[insert_find() for _ in range(10)]) | ||
| await SampleModel2.delete_all() | ||
| clients = [] |
There was a problem hiding this comment.
I feel like there's too many unrelated changes now in this PR.
This seems to address this test spitting out some warning when there's multiple AsyncIOMotorClients being created and connecting to the same DB, but without closing the previous one.
Could this be put in another PR, please?
|
|
||
|
|
||
| def test_openapi_schema_generation(): | ||
| get_openapi( |
There was a problem hiding this comment.
A test should ideally have some assertation, but I get why this is here...
| @@ -16,7 +16,7 @@ jobs: | |||
| matrix: | |||
| python-version: [ "3.8", "3.9", "3.10", "3.11", "3.12", "3.13" ] | |||
| mongodb-version: [4.4, 5.0, 6.0, 7.0, 8.0 ] | |||
| pydantic-version: [ "1.10.18", "2.9.2" ] | |||
| pydantic-version: [ "1.10.18", "2.10.1" ] | |||
There was a problem hiding this comment.
Is there any way we can ensure that it is now not also broken on Pydantic v2.9.2, or some version inbetween?
Or should we always support the newer versions, or rather, test against these newer versions?
Because in the pyproject.toml we support any Pydantic v2 version.
| return model_type.parse_obj(data) | ||
| except Exception: | ||
| print(f"Error parsing model {model_type} with data {data}") | ||
| model_type.model_validate(data) |
There was a problem hiding this comment.
If the validation failed for whatever reason, why try and re-call the model_validate() method again, in the Exception case?
If we were using Pydantic v1, this would again re-throw...
Should this line be removed, since it's a no-op?
There was a problem hiding this comment.
Just review the notes of @staticxterm and the Github Action is failing, fix this issues.
|
Looked into this a bit, it seems there's a lot of breakage with Url types in Pydantic v2.10.x. It was still broken for me on current latest 2.10.3. I suggest to reduce this PR to a minimum change in project.toml where we skip the Pydantic minor version range which is causing these issues (works fine on 2.9.2, started breaking with 2.10.x) and lock it down to a known working version (2.9.2) in our CI as well. Hopefully Pydantic team fixes it upstream with 2.11.0+ or some other patch version on 2.10.x (doesn't seem like it). |
On nevest pydantic anyurl is not ecodable