Change schema generation for PydanticObjectId #1099

dantetemplar · 2025-01-02T07:35:12Z

dantetemplar · 2025-01-02T07:40:32Z

Okay, tests raised error... I will try to fix it

Required test coverage of 80.0% reached. Total coverage: 89.13%
============================================================================== short test summary info ===============================================================================
FAILED tests/odm/test_encoder.py::test_should_encode_pydantic_v2_url_correctly - ValueError: Cannot encode AnyUrl('https://example.com/')
FAILED tests/odm/test_encoder.py::test_should_be_able_to_save_retrieve_doc_with_url - ValueError: Cannot encode HttpUrl('https://example.com/')
FAILED tests/odm/test_id.py::test_pydantic_object_id_validation_json - AssertionError: assert False
================================================================ 3 failed, 451 passed, 3 skipped in 436.49s (0:07:16) ================================================================

dantetemplar · 2025-01-02T08:14:43Z

Now it should work (some tests failed but they no related to the issue)

staticxterm

Hi @dantetemplar, thank you for the PR.
However, please note that this PR doesn't add much as the OpenAPI schema generation fix e.g. for PydanticObjectId is already merged with #1080. This PR simply adds a new type definition to the OpenAPI schema to not reference "string" everywhere but a custom defined type.
Also, please if you could change the PR title to match this description, "Add custom PydanticObjectId type to JSON schema" or something like that.

staticxterm · 2025-01-02T19:34:41Z

beanie/odm/fields.py

+                        python_schema=core_schema.no_info_plain_validator_function(
+                            cls._validate
+                        ),
+                        json_schema=core_schema.no_info_after_validator_function(


Previously, this was: no_info_plain_validator_function. Why was it now changed to no_info_after_validator_function?
Could someone please explain what is the difference here.

Related topic:
https://docs.pydantic.dev/latest/concepts/validators/

Plain validator works as mode="before" (actually as mode="plain") while after validator as mode="after".

Dunno why it actually better and fixes the error.

Use of no_info_plain_validator_function will fail the test.

============================================================================== short test summary info =============================================================================== FAILED tests/fastapi/test_openapi_schema_generation.py::test_openapi_schema_generation - pydantic.errors.PydanticInvalidForJsonSchema: Cannot generate a JsonSchema for core_schema.PlainValidatorFunctionSchema ({'type': 'no-info', 'function': <bound method PydanticOb... ============================================================================ 1 failed, 6 passed in 2.83s =============================================================================

While with no_info_after_validator_function will pass.

def no_info_plain_validator_function( function: NoInfoValidatorFunction, # < no schema parameter that actually defines the schema *, ref: str | None = None, json_schema_input_schema: CoreSchema | None = None, metadata: Dict[str, Any] | None = None, serialization: SerSchema | None = None, ) -> PlainValidatorFunctionSchema:

def no_info_after_validator_function( function: NoInfoValidatorFunction, schema: CoreSchema, # < and here we have it, and it will passed to the _dict_not_none helper *, ref: str | None = None, json_schema_input_schema: CoreSchema | None = None, metadata: Dict[str, Any] | None = None, serialization: SerSchema | None = None, ) -> AfterValidatorFunctionSchema: return _dict_not_none( type='function-plain', function={'type': 'no-info', 'function': function}, ref=ref, json_schema_input_schema=json_schema_input_schema, metadata=metadata, serialization=serialization, )

Okay, it seems then that the Schema change breaks something.
I am not sure off the top of my head, but this needs to be tested in more detail as the _validate() method here expected a string ObjectId, which it then converts.
I would hold off from merging until this is verified how it behaves.
Unfortunately, I can't test this out as I won't have access to a PC for two weeks.
Would be good if you included that FastAPI route params PydanticObjectId test here...

So, I need to include to that PR more tests?

I'm a little tired of amend & force pushing, can this be done with different commits (as mention here)?

beanie/odm/fields.py

dantetemplar · 2025-01-03T05:33:33Z

Hi @dantetemplar, thank you for the PR. However, please note that this PR doesn't add much as the OpenAPI schema generation fix e.g. for PydanticObjectId is already merged with #1080. This PR simply adds a new type definition to the OpenAPI schema to not reference "string" everywhere but a custom defined type. Also, please if you could change the PR title to match this description, "Add custom PydanticObjectId type to JSON schema" or something like that.

Nope, seems issue with {id} PydanticObjectId in path parameters stays even with this fix (I have suggested pr with test). So, current schema generation is not proper.

staticxterm · 2025-01-03T08:12:40Z

Hi @dantetemplar, thank you for the PR. However, please note that this PR doesn't add much as the OpenAPI schema generation fix e.g. for PydanticObjectId is already merged with #1080. This PR simply adds a new type definition to the OpenAPI schema to not reference "string" everywhere but a custom defined type. Also, please if you could change the PR title to match this description, "Add custom PydanticObjectId type to JSON schema" or something like that.

Nope, seems issue with {id} PydanticObjectId in path parameters stays even with this fix (I have suggested pr with test). So, current schema generation is not proper.

Sorry, but this is simply not the case as you've 'proven' in your other PR #1100 which has a test for this very case and is passing without this commit that has this supposed fix...
The issue remains only in combination FastAPI + Pydantic v1, as I've commented elsewhere.
If this addresses that, then great! Unfortunately I can't test this in detail, to test without the FastAPI ENCODERS_BY_TYPE override that Beanie does in its tests here: ...

beanie/tests/fastapi/routes.py

Line 13 in 687b4a7

ENCODERS_BY_TYPE.update(PYDANTIC_ENCODERS_BY_TYPE)

dantetemplar · 2025-01-03T10:25:45Z

Hi @dantetemplar, thank you for the PR. However, please note that this PR doesn't add much as the OpenAPI schema generation fix e.g. for PydanticObjectId is already merged with #1080. This PR simply adds a new type definition to the OpenAPI schema to not reference "string" everywhere but a custom defined type. Also, please if you could change the PR title to match this description, "Add custom PydanticObjectId type to JSON schema" or something like that.

Nope, seems issue with {id} PydanticObjectId in path parameters stays even with this fix (I have suggested pr with test). So, current schema generation is not proper.

Sorry, but this is simply not the case as you've 'proven' in your other PR #1100 which has a test for this very case and is passing without this commit that has this supposed fix... The issue remains only in combination FastAPI + Pydantic v1, as I've commented elsewhere. If this addresses that, then great! Unfortunately I can't test this in detail, to test without the FastAPI ENCODERS_BY_TYPE override that Beanie does in its tests here: ...

beanie/tests/fastapi/routes.py

Line 13 in 687b4a7

ENCODERS_BY_TYPE.update(PYDANTIC_ENCODERS_BY_TYPE)

PR #1100 fails on my local setup because I've not pulled #1095 changes. Sorry for hassle.

Okay, this PR tried to fix yet not reported issue (but seems to be fixed by #1095) which may had name "[BUG] pydantic.errors.PydanticInvalidForJsonSchema occured when APIRouter's has PydanticObjectId in path parameter" with beanie=1.28 and Pydantic 2.10. Although, people identify the very same issue in existing #1087: #1087 (comment)

dantetemplar · 2025-01-03T10:50:46Z

I've tried to rebase my branch onto #1095, Let's see what happens with CI

dantetemplar · 2025-01-03T10:57:54Z

Damn...

dantetemplar · 2025-01-03T14:35:19Z

Damn...

Interesting, GitHub Runner hits the rate limit during build stage of dockerized action:
supercharge/mongodb-github-action#62

adeelsohailahmed · 2025-01-04T00:45:46Z

All tests now pass, @dantetemplar.

staticxterm · 2025-01-04T14:32:02Z

Sorry, @dantetemplar, I won't be able to test this in detail yet, so can't approve.

dantetemplar force-pushed the objectid-fix branch from e07cb7e to a2605bd Compare January 2, 2025 08:11

dantetemplar force-pushed the objectid-fix branch 2 times, most recently from 0f1ce78 to 7cc114f Compare January 2, 2025 09:45

dantetemplar changed the title ~~Change validation schema for PydanticObjectId to fix #1087~~ Change schema for PydanticObjectId to fix #1087 Jan 2, 2025

staticxterm requested changes Jan 2, 2025

View reviewed changes

dantetemplar changed the title ~~Change schema for PydanticObjectId to fix #1087~~ Change schema generation for PydanticObjectId Jan 3, 2025

dantetemplar force-pushed the objectid-fix branch from 7cc114f to 5992387 Compare January 3, 2025 08:13

Change validation schema for PydanticObjectId

7f96679

dantetemplar force-pushed the objectid-fix branch from 5992387 to 7f96679 Compare January 3, 2025 10:49

dantetemplar requested a review from staticxterm January 4, 2025 14:12

staticxterm requested review from a team and removed request for staticxterm January 4, 2025 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change schema generation for PydanticObjectId #1099

Change schema generation for PydanticObjectId #1099

dantetemplar commented Jan 2, 2025 •

edited

Loading

dantetemplar commented Jan 2, 2025 •

edited

Loading

dantetemplar commented Jan 2, 2025 •

edited

Loading

staticxterm left a comment

staticxterm Jan 2, 2025

dantetemplar Jan 3, 2025 •

edited

Loading

dantetemplar Jan 3, 2025 •

edited

Loading

staticxterm Jan 3, 2025

dantetemplar Jan 3, 2025 •

edited

Loading

dantetemplar commented Jan 3, 2025 •

edited

Loading

staticxterm commented Jan 3, 2025

dantetemplar commented Jan 3, 2025 •

edited

Loading

dantetemplar commented Jan 3, 2025

dantetemplar commented Jan 3, 2025

dantetemplar commented Jan 3, 2025

adeelsohailahmed commented Jan 4, 2025

staticxterm commented Jan 4, 2025

Change schema generation for PydanticObjectId #1099

Are you sure you want to change the base?

Change schema generation for PydanticObjectId #1099

Conversation

dantetemplar commented Jan 2, 2025 • edited Loading

dantetemplar commented Jan 2, 2025 • edited Loading

dantetemplar commented Jan 2, 2025 • edited Loading

staticxterm left a comment

Choose a reason for hiding this comment

staticxterm Jan 2, 2025

Choose a reason for hiding this comment

dantetemplar Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

dantetemplar Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

staticxterm Jan 3, 2025

Choose a reason for hiding this comment

dantetemplar Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

dantetemplar commented Jan 3, 2025 • edited Loading

staticxterm commented Jan 3, 2025

dantetemplar commented Jan 3, 2025 • edited Loading

dantetemplar commented Jan 3, 2025

dantetemplar commented Jan 3, 2025

dantetemplar commented Jan 3, 2025

adeelsohailahmed commented Jan 4, 2025

staticxterm commented Jan 4, 2025

dantetemplar commented Jan 2, 2025 •

edited

Loading

dantetemplar commented Jan 2, 2025 •

edited

Loading

dantetemplar commented Jan 2, 2025 •

edited

Loading

dantetemplar Jan 3, 2025 •

edited

Loading

dantetemplar Jan 3, 2025 •

edited

Loading

dantetemplar Jan 3, 2025 •

edited

Loading

dantetemplar commented Jan 3, 2025 •

edited

Loading

dantetemplar commented Jan 3, 2025 •

edited

Loading