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

Make it so callable JSON schema extra works #6798

Merged
merged 4 commits into from Jul 24, 2023

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Jul 21, 2023

Closes #6647

Note that as mentioned here, the better solution for FastAPI is to use a custom GenerateJsonSchema that generates the JSON schema differently for Optional query params, etc.. But this at least makes it possible to express what you want on the pydantic side via:

from typing import Annotated

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


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

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

Selected Reviewer: @samuelcolvin

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jul 21, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6745019
Status: ✅  Deploy successful!
Preview URL: https://19f679d4.pydantic-docs2.pages.dev
Branch Preview URL: https://field-json-schema-extra-call.pydantic-docs2.pages.dev

View logs

@dmontagu
Copy link
Contributor Author

please review

for field_info in field_infos:
new_kwargs.update(field_info._attributes_set)
for x in field_info.metadata:
if not isinstance(x, FieldInfo):
metadata[type(x)] = x
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into a case where this was necessary to get proper merging of FieldInfos that use gt, lt, etc.

Copy link
Member

Choose a reason for hiding this comment

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

This stuff is getting really annoying. @samuelcolvin not having to do this stuff is one of the big advantages of the Output enum thing we were discussing the other day.

@@ -1480,6 +1462,37 @@ def json_schema_update_func(
return maybe_updated_schema
return original_schema

def apply_single_annotation_json_schema(
Copy link
Contributor Author

@dmontagu dmontagu Jul 21, 2023

Choose a reason for hiding this comment

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

I had to break this out as a separate function in order to ensure the callable JSON schema ran after all modifications to the core schema that influence the json schema are performed. Otherwise, for example, you can't remove the default in the callable.

@@ -1493,6 +1506,7 @@ def _get_wrapped_inner_schema(
def new_handler(source: Any) -> core_schema.CoreSchema:
schema = metadata_get_schema(source, get_inner_schema)
schema = self.apply_single_annotation(schema, annotation)
schema = self.apply_single_annotation_json_schema(schema, annotation)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see here, everything related to JSON schema now runs after everything not related to JSON schema.

Copy link
Member

Choose a reason for hiding this comment

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

Typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No typo as far as I can tell — the self.apply_single_annotation does everything not related to JSON schema, then after that, self.apply_single_annotation_json_schema does everything related to json schema. I think that's what I said in the comment above?

d: Annotated[int, Field(default=4), Field(json_schema_extra=pop_default)]
# TODO: it doesn't work properly to have both annotation and assigned value of FieldInfo:
# e: Annotated[int, Field(json_schema_extra=pop_default)] = Field(default=5)
# f: Annotated[int, Field(default=6)] = Field(json_schema_extra=pop_default)
Copy link
Contributor Author

@dmontagu dmontagu Jul 21, 2023

Choose a reason for hiding this comment

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

I think it would be possible to make this and the previous case work (i.e., e and f), but I tried and it ended up being a significantly bigger change and I wasn't able to see it all the way through and get all tests passing. So rather than wasting time on it for now, I just added a note here. Happy to create an issue for this if there isn't one already. (CC @adriangb)

While it might be tempting to try to make this an error, for various reasons, I think it would be preferable if assigning a FieldInfo was equivalent to adding it as the last annotation on the field (and that does work, as you can see in the handling of attributes c and d).

Copy link
Member

Choose a reason for hiding this comment

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

I do think we should make it work at some point. Is it not working now? I thought it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works for most things, it just doesn't work for json_schema_extra and default specifically, annoyingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, even on main, this doesn't work:

from pydantic import BaseModel, Field

from typing_extensions import Annotated

class Model(BaseModel):
    a: Annotated[int, Field(default=1)] = Field()
Model()
pydantic_core._pydantic_core.ValidationError: 1 validation error for Model
a
  Field required [type=missing, input_value={}, input_type=dict]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to fix this through an appropriate change in FieldInfo.from_annotated_attribute 💪

@adriangb
Copy link
Member

Can’t this go in

json_schema: JsonSchemaValue | None
? To avoid making Field even more complex.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Looks fine, I'm a bit lost on a few things but if you're confident, let's merge.

Comment on lines 760 to 761
if isinstance(field_info.json_schema_extra, dict):
json_schema_updates.update(field_info.json_schema_extra)
Copy link
Member

Choose a reason for hiding this comment

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

why don't we call field_info.json_schema_extra if it's a function here?

Copy link
Contributor Author

@dmontagu dmontagu Jul 24, 2023

Choose a reason for hiding this comment

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

okay so, in the update func (def json_schema_update_func below), we have:

            json_schema = {**handler(schema), **json_schema_updates}

and handler(schema) may result in a different schema than just the updates; json_schema_updates is not the whole json schema. In particular, getting stuff like "type": "object" (or even other types for RootModel subclasses), you probably want that to be accessible in the json_schema_extra call, which is why that needs to happen after the call to handler in the json_schema_update_func below.

Comment on lines 765 to 766
if callable(field_info.json_schema_extra):
field_info.json_schema_extra(json_schema)
Copy link
Member

Choose a reason for hiding this comment

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

again, I'm a bit lost why we don't update do json_schema.update(field_info.json_schema_extra) here if it's a dict?

If you sure these are fine, great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can clean this up, I'll push a commit doing that shortly

@dmontagu dmontagu merged commit 2181d12 into main Jul 24, 2023
48 checks passed
@dmontagu dmontagu deleted the field-json-schema-extra-callable branch July 24, 2023 20:59
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.

anyOf causing problems with Swagger/ReDoc on primitive types
3 participants