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

Narrow down implicit boolean conversion introduced in 0.5.8 #468

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion dataclasses_json/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,16 @@ def _support_extended_types(field_type, field_value):
res = (field_value
if isinstance(field_value, UUID)
else UUID(field_value))
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see >1 ints being set to true, or am I blind?

Copy link
Collaborator Author

@george-zubrienko george-zubrienko Aug 12, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How is that caught by your if-statements below?

Copy link
Collaborator Author

@george-zubrienko george-zubrienko Aug 13, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@matt035343 matt035343 Aug 14, 2023

Choose a reason for hiding this comment

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

As discussed offline, None should become False, otherwise I am good

if field_value in ('True', 'true', True, 1):
res = True
elif field_value in ('False', 'false', False, 0):
res = False
matt035343 marked this conversation as resolved.
Show resolved Hide resolved
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):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep the tuple notation here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not correct because that method doesn't accept tuple. It was an error in original implementation

Copy link
Collaborator

Choose a reason for hiding this comment

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

The argument is passed directly to issubclass which accepts a tuple.
https://python-reference.readthedocs.io/en/latest/docs/functions/issubclass.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

res = (field_value
if isinstance(field_value, field_type)
else field_type(field_value))
Expand Down
2 changes: 1 addition & 1 deletion dataclasses_json/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ def _isinstance_safe(o, t):
return result


def _issubclass_safe(cls, classinfo):
def _issubclass_safe(cls: Type[Any], classinfo: Type[Any]) -> bool:
try:
return issubclass(cls, classinfo)
except Exception:
Expand Down
44 changes: 38 additions & 6 deletions tests/test_builtins.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
from dataclasses import dataclass
from decimal import Decimal
from typing import Optional
from typing import Optional, Dict

import pytest
from pytest import mark, param


Expand All @@ -13,22 +14,53 @@ class DataClassWithBuiltins(DataClassJsonMixin):
actually_a_str: str
actually_an_int: int
actually_a_float: float
actually_a_bool: bool


@mark.parametrize(
"model_dict, expected_model",
[
param(
{"actually_a_str": "str", "actually_an_int": 42, "actually_a_float": 42.1},
DataClassWithBuiltins(actually_a_str="str", actually_an_int=42, actually_a_float=42.1),
{"actually_a_str": "str", "actually_an_int": 42, "actually_a_float": 42.1, "actually_a_bool": True},
DataClassWithBuiltins(actually_a_str="str", actually_an_int=42, actually_a_float=42.1, actually_a_bool=True),
id="Happy case"
),
param(
{"actually_a_str": "str", "actually_an_int": Decimal("42.1"), "actually_a_float": Decimal("42.1")},
DataClassWithBuiltins(actually_a_str="str", actually_an_int=42, actually_a_float=42.1),
{"actually_a_str": "str", "actually_an_int": Decimal("42.1"), "actually_a_float": Decimal("42.1"), "actually_a_bool": True },
DataClassWithBuiltins(actually_a_str="str", actually_an_int=42, actually_a_float=42.1, actually_a_bool=True),
id="Decimal as int and float"
),
param(
{"actually_a_str": "str", "actually_an_int": 42, "actually_a_float": 42.1, "actually_a_bool": "False"},
DataClassWithBuiltins(actually_a_str="str", actually_an_int=42, actually_a_float=42.1,
actually_a_bool=False),
id="Bool passed as a valid string"
),
param(
{"actually_a_str": "str", "actually_an_int": 42, "actually_a_float": 42.1, "actually_a_bool": 1},
DataClassWithBuiltins(actually_a_str="str", actually_an_int=42, actually_a_float=42.1,
actually_a_bool=True),
id="Bool passed using valid discrete integer range [0,1]"
),
]
)
def test__DataClassWithBuiltins__from_dict(model_dict, expected_model):
def test_dataclass_with_implicit_builtins(model_dict: Dict, expected_model: DataClassWithBuiltins):
assert DataClassWithBuiltins.from_dict(model_dict) == expected_model


@mark.parametrize(
"model_dict",
[
param(
{"actually_a_str": "str", "actually_an_int": 42, "actually_a_float": 42.1, "actually_a_bool": 1234},
id="Bool passed using invalid integer"
),
param(
{"actually_a_str": "str", "actually_an_int": 42, "actually_a_float": 42.1, "actually_a_bool": "0"},
id="Bool passed using a string rather than an integer or a boolean string"
),
]
)
def test_dataclass_with_implicit_builtins_failed_bool(model_dict: Dict):
with pytest.raises(ValueError):
DataClassWithBuiltins.from_dict(model_dict)