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

Fix UncheckedTuples not failing for wrong amount of arguments #513

Closed

Conversation

Germandrummer92
Copy link

@Germandrummer92 Germandrummer92 commented Dec 7, 2022

Currently apischema fails to correctly serialize a correctly typed dataclass with a "conflicting" union.

E.g.

from dataclasses import dataclass
from typing import Union, Tuple

import apischema


@dataclass(frozen=True)
class SomeTupleClass:
    bar: Union[Tuple[int, int], Tuple[int, int, int]]


def test_correct_serialization() -> None:
    serialized_dict = {
        "bar": [0, 0, 0]
    }

    as_python_object = apischema.deserialize(type=SomeTupleClass, data=serialized_dict)

    assert as_python_object == SomeTupleClass(bar=(0, 0, 0))

    assert apischema.serialize(as_python_object) == serialized_dict

currently fails as the serialization doesn't fail on trying to serialize into the first tuple.

It can be fixed by setting check_type=True, but I am not sure if this is really a "type" check or just the union serialization behaving wrongly.

For now we worked around it with check_type, but as your documentation says https://wyfo.github.io/apischema/0.18/de_serialization/#type-checking this has futher performance impacts, I would like to discuss if the tuple method shouldn't also be able to handle this conflicting union, as the typing in the dataclass itself seems correct

@Germandrummer92 Germandrummer92 changed the title fix it the "other" way Fix UncheckedTuples not failing for wrong amount of arguments Dec 7, 2022
@Germandrummer92
Copy link
Author

I guess python3.6 is not working anymore?

@wyfo
Copy link
Owner

wyfo commented Dec 12, 2022

I've just one night to release 0.18, so I'm a bit in a hurry, that's why I close your PR in favor of #521
(your name will appear in changelog, though)

@wyfo wyfo closed this Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants