lidatong / dataclasses-json

Easily serialize Data Classes to and from JSON
MIT License
1.34k stars 150 forks source link

fix: support generic dataclass #525

Closed PJCampi closed 2 months ago

PJCampi commented 2 months ago

support generic dataclass

matt035343 commented 2 months ago

@george-zubrienko

@matt035343 wdyt about this? Imo this allows you to squeeze lots of problems in as there are no bounds on T, which means it will always be a string/number/bool, what's the point then?

I am unsure what the current behavior is, crash and burn? In that case anything is better. However, I am interested in seeing some more unit tests for more complex generic types. For example:

PJCampi commented 2 months ago

The current implementation will not decode the generic dataclass. The dataclass attribute will be assigned the json dict value instead. In my unittest, nested is assigned {"value": "value"}

After my proposed change, whether the decoding works is down to whether T itself is supported by dataclass_json. Currently it works if the materialized type of T is supported (extended types, collections of them, dataclass, etc...). One can also register a global decoder for the TypeVar (f.ex. if T is bound or constrained).

It is not a perfect solution at all but the library's coverage improves a little. I can add more tests to better highlight what works and what doesn't.

george-zubrienko commented 2 months ago

It is not a perfect solution at all but the library's coverage improves a little. I can add more tests to better highlight what works and what doesn't.

Please do that - and request review again when ready :)

george-zubrienko commented 2 months ago

After my proposed change, whether the decoding works is down to whether T itself is supported by dataclass_json

Shouldn't we throw in case it is not supported?

PJCampi commented 2 months ago

Maybe I should describe my change in more details because the name of the PR (and the ticket I opened) is a bit misleading.

It's not about improving the decoding of type variables in a generic dataclass. That feature currently not supported by the library. The library sets the raw json node to a dataclass field of type T regarldess of its bound, constraints or annotation via __cls_getitem__. It never fails:

    @dataclass_json
    @dataclass
    class Foo(Generic[T]):
        bar: T

    @dataclass_json
    @dataclass
    class Bar:
        value: int

    assert Foo[Bar].from_json('{"bar": {"value": 1}}') == Foo({"value": 1})  # Bar is not decoded correctly  

I am trying to align the behaviour of the library when a generic dataclass is used as type annotation to the behavior I just described.

In the case where a generic dataclass (like Foo) is used as part of a type annotation, it is wrapped in an generic wrapper (_GenericAlias in python 3.11).

Consider the following example:

    @dataclass_json
    @dataclass
    class Baz(Generic[T]):
        foo: Foo[T]

On master we silently fail to decode the generic dataclass attribute foo and the json node is assigned to the attribute:

    assert Baz[int].from_json('{"foo": {"bar": 1}}') == Baz({"bar": 1})
    assert Foo[int].from_json('{"bar": 1}') == Foo(1)

After my change we extract the concrete dataclass type from the generic wrapper. The type is now decoded as a dataclass with generic type T. The decoding of foo becomes consistent with the decoding of an instance of Foo:

    assert Baz[int].from_json('{"foo": {"bar": 1}}') == Baz(Foo(1))
    assert Foo[int].from_json('{"bar": 1}') == Foo(1)

Throwing an exception instead of silently incorrectly setting a json node to an attribute of complex type would be a good idea but it's orthogonal to my change, and it would be a breaking change to the behaviour of the library.

I added more test cases now. I did not write any test around type bounds or constraints cause that's not supported by the library anyways.

george-zubrienko commented 2 months ago

I am convinced - @matt035343 any additional input or we release?

matt035343 commented 2 months ago

I am convinced - @matt035343 any additional input or we release?

I think it looks good

george-zubrienko commented 2 months ago

To be released Monday :) fyi @PJCampi - and thanks for contributions and thorough explanations!