Narrow down implicit boolean conversion introduced in 0.5.8 #468
Conversation
george-zubrienko
changed the title
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif _issubclass_safe(field_type, (int, float, str, bool)): | ||
| elif _issubclass_safe(field_type, bool): | ||
| # issubclass(bool, int) -> True | ||
| # thus values >1 will always convert to True in the next clause, unless intercepted |
There was a problem hiding this comment.
I do not see >1 ints being set to true, or am I blind?
There was a problem hiding this comment.
Run code from the issue author with 0.5.14 and you will see the 1234 int field being set to True when running from_dict, if target field has bool type.
There was a problem hiding this comment.
How is that caught by your if-statements below?
There was a problem hiding this comment.
It is not caught, we just do not allow the invocation to happen in the first place, if target type is bool. field_type is the type of a dataclass field.
There was a problem hiding this comment.
As discussed offline, None should become False, otherwise I am good
| res = False | ||
| else: | ||
| raise ValueError(f"Value {field_value} of input type {field_type.__name__} cannot be decoded as boolean") | ||
| elif _issubclass_safe(field_type, int) or _issubclass_safe(field_type, float) or _issubclass_safe(field_type, str): |
There was a problem hiding this comment.
Please keep the tuple notation here
There was a problem hiding this comment.
This is not correct because that method doesn't accept tuple. It was an error in original implementation
There was a problem hiding this comment.
The argument is passed directly to issubclass which accepts a tuple.
https://python-reference.readthedocs.io/en/latest/docs/functions/issubclass.html
There was a problem hiding this comment.
I see, however, there is one twist there: https://docs.python.org/3.11/library/functions.html#issubclass
Changed in version 3.10: classinfo can be a Union Type.
I'll see if this affects anything
|
Closing as this is too risky to integrate, will instead do a separate one for v1 |
Fixes #466 (at least the boolean part of it that is defo a bug)
Enhanced builtin instantiation introduced in 0.5.8 had a side effect of converting any int in json to a
Trueboolean value if the destination field hasbooltype. This PR narrows allowed value range for implicit conversion to boolean