Do not compute JSON Schema default when plain serializers are used with when_used set to 'json-unless-none' and the default value is None

[Viicos]

Fixes #10118

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

[cloudflare-workers-and-pages]

Deploying pydantic-docs with    Cloudflare Pages

Latest commit:	eb1570b
Status:	✅  Deploy successful!
Preview URL:	https://41c39d23.pydantic-docs.pages.dev
Branch Preview URL:	https://10118-plain-json-schema.pydantic-docs.pages.dev

View logs

[codspeed-hq]

CodSpeed Performance Report

Merging #10121 will not alter performance

_{Comparing 10118-plain-json-schema (eb1570b) with main (f0d8f65)}

Summary

✅ 17 untouched benchmarks

[Viicos]

This revealed another issue with the JSON Schema: type should be 'anyOf': [{'type': 'number'}, {'type': 'null'}]}.

When the JSON Schema for our field is created, schema is of type default, and schema['schema'] is of type nullable:

pydantic/pydantic/json_schema.py

Lines 1022 to 1031 in 0c97049

	def default_schema(self, schema: core_schema.WithDefaultSchema) -> JsonSchemaValue:
	"""Generates a JSON schema that matches a schema with a default value.
	Args:
	schema: The core schema.
	Returns:
	The generated JSON schema.
	"""
	json_schema = self.generate_inner(schema['schema'])

However, self.generate_inner(schema['schema']) will not go through nullable_schema as we would expect. As there's a serialization key, ser_schema is being used:

pydantic/pydantic/json_schema.py

Lines 492 to 494 in 0c97049

	if self.mode == 'serialization' and 'serialization' in schema_or_field:
	ser_schema = schema_or_field['serialization'] # type: ignore
	json_schema = self.ser_schema(ser_schema)

This makes sense, however, the issue is ser_schema kind of assumes when_used is always. I'll see if I can fix this in a follow up PR.

[Viicos]

Switching from and ser_schema.get('info_arg') is False releaved an issue in the tests, that I fixed in 0eed384.

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic
config.py
json_schema.py
pydantic/_internal
_config.py
_model_construction.py
Project Total

This report was generated by python-coverage-comment-action

[sydney-runkle]

This patch looks alright to me - I've left one comment that I think might simplify things!

[sydney-runkle]

Could we do something like this?

pydantic/pydantic/json_schema.py

Lines 2017 to 2044 in 0c97049

	def encode_default(self, dft: Any) -> Any:
	"""Encode a default value to a JSON-serializable value.
	This is used to encode default values for fields in the generated JSON schema.
	Args:
	dft: The default value to encode.
	Returns:
	The encoded default value.
	"""
	from .type_adapter import TypeAdapter, _type_has_config
	config = self._config
	try:
	default = (
	dft
	if _type_has_config(type(dft))
	else TypeAdapter(type(dft), config=config.config_dict).dump_python(dft, mode='json')
	)
	except PydanticSchemaGenerationError:
	raise pydantic_core.PydanticSerializationError(f'Unable to encode default value {dft}')
	return pydantic_core.to_jsonable_python(
	default,
	timedelta_mode=config.ser_json_timedelta,
	bytes_mode=config.ser_json_bytes,
	)

I feel like we shouldn't be skipping that logic here, we could maybe call that function?

[Viicos]

I don't think it makes sense to do so, or we may end up with a wrong default value in the JSON Schema (in the related test fixed, '1MB' was set as the default value in the JSON Schema, but this isn't true as the default is 1000000). In my opinion, it's better to have no value than a wrong one.

However, we emit a warning here, so I'm ok to call encode_default.

[sydney-runkle]

That's fair - I wonder if we could use a similar pattern though + attempt to validate the default + include that in the JSON schema. Feels messy to do validation here, I admit.

I'm not opposed to the warning you have in place, I think that's good!

[sydney-runkle]

That's fair - I wonder if we could use a similar pattern though + attempt to validate the default + include that in the JSON schema. Feels messy to do validation here, I admit.

I think we can do this in a separate PR, if someone asks for it. I'm not keen to go ahead and do this now.

Change looks good to me, thanks for including the helpful warning.

[sydney-runkle]

Looks good @Viicos, nice work

[sydney-runkle]

One nit, this isn't always a user defined plain serializer, which might be confusing. We could change the wording of this a bit, but I'm not super worried about it, so will go ahead and merge this.