konradhalas / dacite

Simple creation of data classes from dictionaries.
MIT License
1.76k stars 106 forks source link

Union type field somehow affected by Optional nested type #106

Closed panzer closed 4 years ago

panzer commented 4 years ago

I have run up against an issue which seems pretty strange to me. Really the only way I know how to explain it is with an example. I expect the assert at the end to pass (be True) since h should be identically recreated from the dict of g

Minimal example to reproduce on dacite 1.5.0:

@dataclass
class Leaf:
    x: Optional[int]  # changing to `int` fixes the issue

@dataclass
class Branch:
    y: List[Union[Leaf, Branch]]

@dataclass
class Graph:
    z: Union[Leaf, Branch]

g = Graph(z=Branch(y=[]))

g_dict = asdict(g)

h = dacite.from_dict(data_class=Graph, data=g_dict)

print(f"g= {g}")            # g= Graph(z=Branch(y=[]))
print(f"g_dict= {g_dict}")  # g_dict= {'z': {'y': []}}
print(f"h= {h}")            # h= Graph(z=Leaf(x=None))

assert g == h  # fails

Any idea what is going on here?

panzer commented 4 years ago

Probably related. When using strict_unions_match=True an exception is thrown:

Traceback (most recent call last):
    h = dacite.from_dict(data_class=Graph, data=g_dict, config=dacite.Config(strict_unions_match=True))
  File "[removed]/dacite/core.py", line 60, in from_dict
    value = _build_value(type_=field.type, data=transformed_value, config=config)
  File "[removed]/dacite/core.py", line 83, in _build_value
    return _build_value_for_union(union=type_, data=data, config=config)
  File "[removed]/dacite/core.py", line 115, in _build_value_for_union
    raise StrictUnionMatchError(union_matches)
dacite.exceptions.StrictUnionMatchError: can not choose between possible Union matches for field "z": Leaf, Branch
panzer commented 4 years ago

This all seems to be fixed by strict_mode=True. I recommend strict_mode be enabled by default and writing better documentation on strict mode. Either way, (strict mode enabled or not) the example above should always work. There is no ambiguity as to whether {'y': []} is Leaf or Branch.

konradhalas commented 4 years ago

Hi @panzer - thank you for reporting this problem.

As you figured out you have to use Config(strict=True). From dacite point of view {'y': []} looks like a Leaf with some additional data and missing x value (which is OK because it's Optional). Because Leaf is at the beginning of your Union types list dacite will build instance of this class.

I don't want to change strict to True because of: A) backward compatibility, B) you don't want strict mode in most cases.

If you have any idea how I can improve documentation of strict mode I'm open for PR :)