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 frozenset deserialization #341

Closed
pchanial opened this issue Feb 1, 2022 · 2 comments · Fixed by #342
Closed

Fix frozenset deserialization #341

pchanial opened this issue Feb 1, 2022 · 2 comments · Fixed by #342

Comments

@pchanial
Copy link
Contributor

pchanial commented Feb 1, 2022

AbstractSet and FrozenSet are not deserialized in frozensets.

from typing import AbstractSet, FrozenSet
from apischema import deserialize
assert type(deserialize(AbstractSet[int], [1, 2, 3])) is set
assert type(deserialize(FrozenSet[int], [1, 2, 3])) is set

PR to follow

@wyfo
Copy link
Owner

wyfo commented Feb 2, 2022

Hi, thank you for spotting this bug!

Actually, the true bug is the lack of proper support of typing.FrozenSet/ frozenset. However, having AbstractSet/collections.abc.Set being deserialized as set is not really an issue, as the type constraint is respected.

To give you some context, as you have dug into apischema tests, you have noticed this test case: (AbstractSet, frozenset([0, SimpleDataclass(0)])),; there was indeed a time where AbstractSet was deserialized as frozenset, but not only: Sequence was also deserialized as tuple, and even Maping was deserialized as MappingProxyType.
However, as mentioned above, using set for AbstractSet is still correct, as well as using list for Sequence. And it's not only correct, but also faster to deserialize — immutable types requires to be deserialized as list before to be converted — and can be more intuitive for some people.

That's why, if there is indeed a bug with frozenset, I don't want AbstractSet behavior to be changed for now, because of the performance impact; I will create an other issue about this specific behavior (abstract container deserialization as immutable instance).

P.S. I've checked the history and indeed, AbstractSet deserialization change to set instead of frozenset was indeed about performance. Also, in the PR (#191), I'd fixed Sequence deserialization to list but forgot to modify AbstractSet.

@pchanial
Copy link
Contributor Author

pchanial commented Feb 2, 2022

Interesting discussion. But now I wonder if it makes sense to use an abstract type as a target type for deserialization. They are not enough constrained by definition. What would be the consequences of only supporting concrete classes ? It looks like a source of confusion that may bite again.

@wyfo wyfo closed this as completed in #342 Feb 4, 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 a pull request may close this issue.

2 participants