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

Handle nullable schemas with serialization schema available during JSON Schema generation #10132

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

Viicos
Copy link
Member

@Viicos Viicos commented Aug 14, 2024

As per https://github.com/pydantic/pydantic/pull/10121/files#r1715043570:

It might be that the serialization schema has a specific when_used mode set to unless-none or json-unless-none. In that case, if the base core schema is of type nullable, we need to account for this as the serialization schema might not be used.

This also fixes an issue where unless-none was not taken into account when trying to figure out if the default value should be passed into the plain serializer function to get the JSON Schema default. This was missed in #10121.

Change Summary

Related issue number

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Aug 14, 2024
…g JSON Schema generation

It might be that the `serialization` schema has a specific `when_used`
mode set to `unless-none` or `json-unless-none`. In that case,
if the base core schema is of type `nullable`, we need to account
for this as the `serialization` schema might not be used.

This also fixes an issue where `unless-none` was not taken into
account when trying to figure out if the default value should be
passed into the plain serializer function to get the JSON Schema
default.
Copy link

cloudflare-workers-and-pages bot commented Aug 14, 2024

Deploying pydantic-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 423345b
Status: ✅  Deploy successful!
Preview URL: https://a758d129.pydantic-docs.pages.dev
Branch Preview URL: https://nullable-ser.pydantic-docs.pages.dev

View logs

and ser_schema.get('when_used') in ('unless-none', 'json-unless-none')
and schema_or_field['type'] == 'nullable'
):
json_schema = self.get_flattened_anyof([{'type': 'null'}, json_schema])
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not too happy with how this is done. Ideally, self.nullable_schema should be used but it will use schema['schema'] to generate the inner schema:

def nullable_schema(self, schema: core_schema.NullableSchema) -> JsonSchemaValue:
"""Generates a JSON schema that matches a schema that allows null values.
Args:
schema: The core schema.
Returns:
The generated JSON schema.
"""
null_schema = {'type': 'null'}
inner_json_schema = self.generate_inner(schema['schema'])

While we need schema['serialization'] to be used instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we model the below more closely?

if (
    self.mode == 'serialization' 
    and 'serialization' in schema_or_field
    and (ser_schema := schema_or_field['serialization']
    and (json_schema := self.ser_schema(ser_schema))
    and ser_schema.get('when_used') in ('unless-none', 'json-unless-none')
    and schema_or_field['type'] == 'nullable'
):
    json_schema = self.get_flattened_anyof([{'type': 'null'}, json_schema])

Or something like that

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your concern, but I agree, I think this is a fine patch for now. Maybe add a comment explaining the pattern here - we have so many conditionals, it's a bit hard to understand all together 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we model the below more closely?

I think I prefer the two conditionals, as it shows the logic more clearly imo:

  • if we are in serialization mode and a serialization schema is available:
    • shortcut JSON Schema generation and use ser_schema
    • special case nullable schema depending on the value of when_used

Copy link

codspeed-hq bot commented Aug 14, 2024

CodSpeed Performance Report

Merging #10132 will not alter performance

Comparing nullable-ser (423345b) with main (7abafa4)

Summary

✅ 17 untouched benchmarks

and ser_schema.get('when_used') in ('unless-none', 'json-unless-none')
and schema_or_field['type'] == 'nullable'
):
json_schema = self.get_flattened_anyof([{'type': 'null'}, json_schema])
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we model the below more closely?

if (
    self.mode == 'serialization' 
    and 'serialization' in schema_or_field
    and (ser_schema := schema_or_field['serialization']
    and (json_schema := self.ser_schema(ser_schema))
    and ser_schema.get('when_used') in ('unless-none', 'json-unless-none')
    and schema_or_field['type'] == 'nullable'
):
    json_schema = self.get_flattened_anyof([{'type': 'null'}, json_schema])

Or something like that

and ser_schema.get('when_used') in ('unless-none', 'json-unless-none')
and schema_or_field['type'] == 'nullable'
):
json_schema = self.get_flattened_anyof([{'type': 'null'}, json_schema])
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your concern, but I agree, I think this is a fine patch for now. Maybe add a comment explaining the pattern here - we have so many conditionals, it's a bit hard to understand all together 👍

Copy link
Contributor

github-actions bot commented Aug 15, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

@Viicos Viicos enabled auto-merge (squash) August 15, 2024 08:23
@Viicos Viicos merged commit 4253e14 into main Aug 15, 2024
59 checks passed
@Viicos Viicos deleted the nullable-ser branch August 15, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes-fix Used for bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants