pydantic / pydantic-core

Core validation logic for pydantic written in rust
MIT License
1.34k stars 207 forks source link

`[Fix(9559)]` - Validation fails for enum field with decimal type #1324

Open mikeleppane opened 4 weeks ago

mikeleppane commented 4 weeks ago

Change Summary

Fixes regression documented in https://github.com/pydantic/pydantic/issues/9559.

Related issue number

pydantic/issues/9559

Checklist

Selected Reviewer: @davidhewitt

codecov[bot] commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review. Files Patch % Lines
src/validators/enum_.rs 90.00% 1 Missing :warning:

:loudspeaker: Thoughts on this report? Let us know!

codspeed-hq[bot] commented 4 weeks ago

CodSpeed Performance Report

Merging #1324 will not alter performance

Comparing mikeleppane:fix(9559)/validation-fails-for-enum-field-with-decimal-type (5d6986c) with main (9507a28)

Summary

✅ 155 untouched benchmarks

mikeleppane commented 4 weeks ago

please review

mikeleppane commented 3 weeks ago

Thanks very much for the PR!

 

Having just reviewed with this, I'd like to see a different solution. As per pydantic/pydantic#9559 (comment) I think the problem is not limited just to Decimal but to many arbitrary types.

 

I think also we don't want to add Decimal coercion to our Literal pipeline as it's never valid to have a Decimal as a Literal.

 

I think instead I would prefer if we added a branch to src/validators/enum_.rs which, as a last resort, attempts to do the equivalent of MyEnum(input). This would add worst-case performance but should restore all the old behaviour.

Hey @davidhewitt,

Thanks for the feedback!

Yes, you're correct. The problem isn't limited to Decimal. I placed the implementation in src/validators/enum_.rs initially. But, I moved it to src/validators/literal.rs.  

I've examined this thoroughly and face uncertain next steps. So I need a little bit of help. Perhaps, I don't understand something. Or, the current machinery can't handle arbitrary types well. The problem that I've been pondering is as follows:


from enum import Enum

from pydantic import BaseModel

class EnumClass(Enum):

    enum_value = 1

    enum_value = 2

class ModelClass(BaseModel):

    enum_field: EnumClass

    enum_field_2: EnumClass

class IntWrappable:

    def __init__(self, value: int):

        self.value = value

    

    def __eq__(self, value: object) -> bool:

        return self.value == value

ModelClass(enum_field=1, enum_field_2=IntWrappable(2))

LiteralLookup { expected_bool: None, expected_int: Some({1: 0, 2: 1}), expected_str: None, expected_py_dict: None, expected_py_list: None, values: [Py(0x7f396c73c950), Py(0x7f396cfe3da0)] }

How can we know which expected_ints to validate? This is in the context of enum validation in src/validators/enum_.rs. I understand handling types that can be converted to int (like Decimal). But what about those custom, artificial IntWrappable kinds of classes (NewType works perfectly)? Do we need to require that those classes implement __float__ instead of __eq__ (or both) in this case? Now, if I understand, to validate IntWrappable we need to loop "blindly" LiteralLookup values to check which one matches?

UPDATE:

Äh, maybe I thought about this too hard. It works now for any type. The types must have a test for equality. But, I feel that there might some corner case that this implementation does not cover.