Skip to content

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Jul 13, 2023

This PR solves #6647 on Pydantic side, in the sense that it allows the user to not generate the JSON schema on a type.

Although this is nice to have on Pydantic, it's not ideal for FastAPI users to do T | SkipJsonSchema[None], mainly because of dev experience. That said, @dmontagu proposed that FastAPI would use a FastAPIGeneratorJsonSchema on their side which would skip nullable fields if is not a Body. - I'm going to push a PR in FastAPI to help on this.

Selected Reviewer: @lig

@Kludex
Copy link
Member Author

Kludex commented Jul 13, 2023

please review

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 13, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 36b941a
Status: ✅  Deploy successful!
Preview URL: https://668181d2.pydantic-docs2.pages.dev
Branch Preview URL: https://feat-support-skip-json-schem.pydantic-docs2.pages.dev

View logs

@Kludex Kludex force-pushed the feat/support-skip-json-schema branch 4 times, most recently from c272cb2 to 0705468 Compare July 13, 2023 15:44
@commonism
Copy link
Contributor

commonism commented Jul 13, 2023

I had a look and I think this will cause problems besides

It is RECOMMENDED that a default value be valid against the associated schema.

It creates a difference in the serialization done and communicated (to the consumers of the json document).

It only works by abusing the default value to set the omitted type.

class Model(BaseModel):
x: int | SkipJsonSchema[None] = None
y: int | SkipJsonSchema[None] = 1
z: int | SkipJsonSchema[str] = 'foo'

'properties': {
'x': {'default': None, 'title': 'X', 'type': 'integer'},
'y': {'default': 1, 'title': 'Y', 'type': 'integer'},
'z': {'default': 'foo', 'title': 'Z', 'type': 'integer'},
},

For consumers of the json description document, the resulting difference may be a real problem to overcome, as the description document does not match the protocol.

    class A(BaseModel):
        x: int | SkipJsonSchema[None] = None

    class B(BaseModel):
        x: int | SkipJsonSchema[None] = 5

    class C(BaseModel):
        x: int | SkipJsonSchema[None]

For consumers of the json description document:

  • A(x=None) is invalid
    • {"x":None} is invalid
  • A() will result in x being None anyway.
  • for B it is not possible to set x to None, neither via assignment or constructor due to type mismatch
    • {"x":None} is invalid
  • for C it is not possible to set x to None
    • {"x":None} is invalid

The proposal does not improve anything to the consumers of the description document created by pydantic, but makes the document even incompatible to the datamodel used by pydantic itself.
This degradation of the correctness of the description document is not limited to the consumers which fail at the v3.1 features used by pydantic, but even for clients which can do v3.1 perfectly, all consumers.

I'd prefer to have this compatibility gap bridged otherwise - using type:[…, none], as it does not harm the correctness of the description document used by consumers.
That said - I consider even just waiting for the ecosystem to catch up better.

@Kludex Kludex force-pushed the feat/support-skip-json-schema branch from 0705468 to 36b941a Compare July 18, 2023 09:02
@Kludex
Copy link
Member Author

Kludex commented Jul 18, 2023

please review

@Kludex Kludex requested a review from dmontagu July 18, 2023 14:45
dmontagu
dmontagu previously approved these changes Jul 20, 2023
@dmontagu dmontagu dismissed their stale review July 20, 2023 22:29

commonism makes some good points

@dmontagu
Copy link
Contributor

dmontagu commented Jul 21, 2023

I'm going to approve and merge this, because with #6798 you'll be able to do:

from typing import Annotated

from pydantic import BaseModel, Field
from pydantic.json_schema import SkipJsonSchema


class Model(BaseModel):
    x: Annotated[int | SkipJsonSchema[None], Field(json_schema_extra=lambda x: x.pop('default'))] = None

print(Model.model_json_schema())
#> {'properties': {'x': {'title': 'X', 'type': 'integer'}}, 'title': 'Model', 'type': 'object'}

which gives a way to have a reusable annotation that removes the default, which I hope gives you a way to address your point.

That said, I think everyone currently agrees the better solution to #6647 for FastAPI is on the FastAPI side, with the custom GenerateJsonSchema for non-body parameters that handles optional schemas differently. (I believe @Kludex has opened a PR for that.)

@commonism if you still have issues with this decision, please comment here or create a new issue about it and @-mention me, and we can try to make other improvements.

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.

4 participants