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.

Sign up for GitHub

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

Jump to bottom

fix for #8512 #8567

Merged
merged 5 commits into from Jan 19, 2024
Merged

fix for #8512 #8567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
202a251
propagate serialization information in PlainValidator schema creation…
Anvil Jan 13, 2024
33dc02c
Test that PlainValidator actually keep serialization information in t…
Anvil Jan 15, 2024
b10e771
ensure correct type is used for serialization in PlainValidator using…
Anvil Jan 16, 2024
dac27ff
put both serializer/validator combinations in test_plain_validator_pl…
Anvil Jan 17, 2024
1c222e9
add bug reference as test_plain_validator_plain_serializer docstring
Anvil Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions pydantic/functional_validators.py
View file
Open in desktop
Expand Up @@ -153,13 +153,17 @@ class Model(BaseModel):
func: core_schema.NoInfoValidatorFunction | core_schema.WithInfoValidatorFunction

def __get_pydantic_core_schema__(self, source_type: Any, handler: _GetCoreSchemaHandler) -> core_schema.CoreSchema:
schema = handler(source_type)
serialization = core_schema.wrap_serializer_function_ser_schema(function=lambda v, h: h(v), schema=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 think we can avoid introducing a wrapper here and just do

Suggested change
serialization = core_schema.wrap_serializer_function_ser_schema(function=lambda v, h: h(v), schema=schema)
serialization = schema.get('serialization')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello.

Thanks for the advice. I originally tried that, but ended up with some type checking errors:

Typecheck................................................................Failed
- hook id: typecheck
- exit code: 1

/home/anvil/git/pydantic/pydantic/functional_validators.py
  /home/anvil/git/pydantic/pydantic/functional_validators.py:163:68 - error: Impossible d’affecter l’argument de type « SerSchema | IncExDictOrElseSerSchema | IncExSeqOrElseSerSchema | None » au paramètre « serialization » de type « SerSchema | None » dans la fonction « with_info_plain_validator_function » (reportGeneralTypeIssues)
  /home/anvil/git/pydantic/pydantic/functional_validators.py:167:85 - error: Impossible d’affecter l’argument de type « SerSchema | IncExDictOrElseSerSchema | IncExSeqOrElseSerSchema | None » au paramètre « serialization » de type « SerSchema | None » dans la fonction « no_info_plain_validator_function » (reportGeneralTypeIssues)
2 errors, 0 warnings, 0 informations

Should I cast the value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ha, pardon my french.

PDM, version 2.11.2
pdm run ruff pydantic tests docs/plugins
pdm run ruff format --check pydantic tests docs/plugins
164 files already formatted
pre-commit 3.6.0
pre-commit run typecheck --all-files
Typecheck................................................................Failed
- hook id: typecheck
- exit code: 1

/home/anvil/git/pydantic/pydantic/functional_validators.py
  /home/anvil/git/pydantic/pydantic/functional_validators.py:163:68 - error: Argument of type "SerSchema | IncExDictOrElseSerSchema | IncExSeqOrElseSerSchema | None" cannot be assigned to parameter "serialization" of type "SerSchema | None" in function "with_info_plain_validator_function" (reportGeneralTypeIssues)
  /home/anvil/git/pydantic/pydantic/functional_validators.py:167:85 - error: Argument of type "SerSchema | IncExDictOrElseSerSchema | IncExSeqOrElseSerSchema | None" cannot be assigned to parameter "serialization" of type "SerSchema | None" in function "no_info_plain_validator_function" (reportGeneralTypeIssues)
2 errors, 0 warnings, 0 informations

Copy link
Member

Choose a reason for hiding this comment

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

with_info_plain_validator_function accepts a SerSchema: https://github.com/pydantic/pydantic-core/blob/4da7192ffc104cd6c424d102bc1c9c5bfdad543e/python/pydantic_core/core_schema.py#L2183

But some schemas, e.g. dict have other serializers: https://github.com/pydantic/pydantic-core/blob/4da7192ffc104cd6c424d102bc1c9c5bfdad543e/python/pydantic_core/core_schema.py#L1724-L1733

@davidhewitt I don't recall what the deal was with the inc/exc stuff but I'd suggest one of two things:

  • They get added to the SerSchema union if this usage is valid
  • In this PR we add some assert schema['type'] in ... or assert schema['type'] not in ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Potentially instead of the assert we can fall back to the slower wrap serializer proposed here just to be safe?

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 dont get all the details (yet?), but if I can be of any use, let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, adding the inc/exc stuff to the SerSchema union isn't easily done - for ex:

IncExSeqOrElseSerSchema = Union[IncExSeqSerSchema, SerSchema]
IncExDictOrElseSerSchema = Union[IncExDictSerSchema, SerSchema]

We would end up having recursive defs in the SerSchema union, as this is how those types are defined. For now, I think it makes sense to do a type check, and perhaps we could add an issue to the pydantic-core board if we want to figure out a way to add those types to the SerSchema Union in the long term.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, a proposed fix:

schema = handler(source_type)

# if the serialization schema is of type 'include-exclude-dict' or 'include-exclude-set',
# we need to wrap it in a serializer function schema
# otherwise, we can leave it as is (which is more efficient)
serialization = schema.get('serialization')
if serialization and serialization['type'] in ('include-exclude-dict', 'include-exclude-set'):
    serialization = core_schema.wrap_serializer_function_ser_schema(function=lambda v, h: h(v), schema=schema)

info_arg = _inspect_validator(self.func, 'plain')
if info_arg:
    func = cast(core_schema.WithInfoValidatorFunction, self.func)
    return core_schema.with_info_plain_validator_function(
        func,
        field_name=handler.field_name,
        serialization=serialization,  # type: ignore
    )
else:
    func = cast(core_schema.NoInfoValidatorFunction, self.func)
    return core_schema.no_info_plain_validator_function(func, serialization=serialization)  # type: ignore

This requires some # type: ignore statements, which isn't ideal. However, it's not possible to check if something is an instance of SerSchema, as that's a union of types. Furthermore, checking against the individual types is difficult bc they're often TypedDict types, which aren't friendly with isinstance checks.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, let's go with:

schema = handler(source_type)

_ser_schema_serialization_types = (
    *get_args(core_schema.ExpectedSerializationTypes),
    'function-plain',
    'function-wrap',
    'format',
    'to-string',
    'function',
    'model',
)

# if the serialization schema is of type 'include-exclude-dict' or 'include-exclude-set',
# we need to wrap it in a serializer function schema
# otherwise, we can leave it as is (which is more efficient)
serialization = schema.get('serialization')
if serialization:
    if serialization['type'] in ('include-exclude-dict', 'include-exclude-set'):
        serialization = core_schema.wrap_serializer_function_ser_schema(
            function=lambda v, h: h(v), schema=schema
        )
    else:
        assert serialization['type'] in _ser_schema_serialization_types

info_arg = _inspect_validator(self.func, 'plain')
if info_arg:
    func = cast(core_schema.WithInfoValidatorFunction, self.func)
    return core_schema.with_info_plain_validator_function(
        func,
        field_name=handler.field_name,
        serialization=serialization,  # type: ignore
    )
else:
    func = cast(core_schema.NoInfoValidatorFunction, self.func)
    return core_schema.no_info_plain_validator_function(func, serialization=serialization)  # type: ignore

Or something along those lines. @Anvil, could you please implement that change + we can get this across the line today?

Just as a side note - this approach still feels a bit unclean to me, so I'm going to mark this as something to come back to and see if we can clean up.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, after discussing a bit more with @adriangb, I think that my above suggestion is ugly enough that we shouldn't use that alternative.

@Anvil, let's go with what you currently have, and I'll create an issue to address the performance of the

serialization = core_schema.wrap_serializer_function_ser_schema(function=lambda v, h: h(v), schema=schema)

line that you've added. I'd rather have the bug fix now and improve the performance down the line.

Thanks for all of your help on this!

info_arg = _inspect_validator(self.func, 'plain')
if info_arg:
func = cast(core_schema.WithInfoValidatorFunction, self.func)
return core_schema.with_info_plain_validator_function(func, field_name=handler.field_name)
return core_schema.with_info_plain_validator_function(
func, field_name=handler.field_name, serialization=serialization
)
else:
func = cast(core_schema.NoInfoValidatorFunction, self.func)
return core_schema.no_info_plain_validator_function(func)
return core_schema.no_info_plain_validator_function(func, serialization=serialization)


@dataclasses.dataclass(frozen=True, **_internal_dataclass.slots_true)
Expand Down
17 changes: 17 additions & 0 deletions tests/test_validators.py
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@
ConfigDict,
Field,
GetCoreSchemaHandler,
PlainSerializer,
PydanticDeprecatedSince20,
PydanticUserError,
TypeAdapter,
Expand Down Expand Up @@ -2810,3 +2811,19 @@ def value_b_validator(cls, value):
'ctx': {'error': IsInstance(AssertionError)},
},
]


def test_plain_validator_plain_serializer() -> None:
"""https://github.com/pydantic/pydantic/issues/8512"""
ser_type = str
serializer = PlainSerializer(lambda x: ser_type(int(x)), return_type=ser_type)
validator = PlainValidator(lambda x: bool(int(x)))

class Blah(BaseModel):
foo: Annotated[bool, validator, serializer]
bar: Annotated[bool, serializer, validator]

blah = Blah(foo='0', bar='1')
data = blah.model_dump()
assert isinstance(data['foo'], ser_type)
assert isinstance(data['bar'], ser_type)