Open Sanavesa opened 5 months ago
Hi @Sanavesa
Deserialization of a dictionary to an object should be successful through any class in the hierarchy (including abstract classes) as long as the dictionary represents a concrete subclass that implements all abstract methods. This is expected to work because of the specified discriminator field that indicates the concrete subclass type.
Since you mentioned abstract classes in the context of inclusion, I assume you expect a concrete subclass to be used to deserialize any of its subclasses? What would you be expecting in the following case? An error or a Bar
instance? And if it's a second option, then how should we be able to validate that the incoming dictionary contains exactly the "foo" type?
from dataclasses import dataclass
from typing import Literal
from mashumaro import DataClassDictMixin
from mashumaro.config import BaseConfig
from mashumaro.types import Discriminator
@dataclass
class Base(DataClassDictMixin):
class Config(BaseConfig):
discriminator = Discriminator(field="type", include_subtypes=True)
@dataclass
class Foo(Base):
type: Literal["foo"] = "foo"
@dataclass
class Bar(Foo):
type: Literal["bar"] = "bar"
obj = Foo.from_dict({"type": "bar"}) # what to expect here?
In the code example provided, I noticed that type
is a Literal
. This raises a question: should validation or discrimination take precedence? I lean towards validation (and so does the current implementation), which means an error like mashumaro.exceptions.InvalidFieldValue: Field "type" of type Literal['foo'] in Foo has invalid value 'bar'
should occur. However, if type
were a str
(specifically, ClassVar[str]
), I'd then expect the output to be a Bar
instance instead.
Regarding class hierarchy and their subtype variants, each class seems to have its own lookup map (__mashumaro_subtype_variants__
) including itself (unless it's abstract) and its descendants. Given the hierarchy:
Base
/ \
Foo AbstractFoo
| |
Bar ConcreteBar
The expected lookup maps would be:
Base.__mashumaro_subtype_variants__ = {"foo": Foo, "bar": Bar, "concrete_bar": ConcreteBar}
Foo.__mashumaro_subtype_variants__ = {"foo": Foo, "bar": Bar}
Bar.__mashumaro_subtype_variants__ = {"bar": Bar}
AbstractFoo.__mashumaro_subtype_variants__ = {"concrete_bar": ConcreteBar}
ConcreteBar.__mashumaro_subtype_variants__ = {"concrete_bar": ConcreteBar}
In the from_dict()
method, it should first identify the discriminator field, check its value in the data dictionary, and then look it up in the __mashumaro_subtype_variants__
. If a matching class is found (and all fields are present), it should return data as that class. If not, it should raise a mashumaro.exceptions.SuitableVariantNotFoundError
.
What are your thoughts on this approach? Thank you for considering my feedback.
Sorry for being silent for a long time and thank you for the detailed answer. I like your proposal, however, its implementation may require heavy improvements.
Regarding class hierarchy and their subtype variants, each class seems to have its own lookup map (
__mashumaro_subtype_variants__
) including itself (unless it's abstract) and its descendants.
Unfortunately, this is not true in the current implementation. When a dataclass has a discriminator in its config, a map __mashumaro_subtype_variants__
is being created only for that dataclass. I dropped a note in README about this limitation:
The only difference is that you can't use
include_supertypes=True
because it would lead to a recursion error.
I see the following way to resolve this issue:
1) Allow using include_supertypes=True
along with include_subtypes=True
in class level discriminators. This can be done by generating two methods for the same class — the first one will search for a suitable class in __mashumaro_subtype_variants__
and the second one will be used for deserialization of the current class itself.
2) Add an optional flag to Discriminator
that will enable propagation of discriminator settings to the subclasses.
With an existing workaround, I think this issue will have a low priority. However, if you or someone else can handle it, PR welcome.
Description
Issue Summary: When serializing and deserializing a hierarchy of dataclasses involving abstract classes using mashumaro's
to_dict()
andfrom_dict()
methods, attempting to deserialize a dictionary back to an object through an abstract parent class results in aTypeError
due to the inability to instantiate the abstract class, even when the dictionary represents a concrete subclass.Expected Behavior: Deserialization of a dictionary to an object should be successful through any class in the hierarchy (including abstract classes) as long as the dictionary represents a concrete subclass that implements all abstract methods. This is expected to work because of the specified discriminator field that indicates the concrete subclass type.
Actual Behavior: Deserializing a dictionary to an object through an abstract class using the
from_dict()
method throws aTypeError
, indicating an attempt to instantiate an abstract class with unimplemented abstract methods.What I Did
Superclass
that inherits fromDataClassDictMixin
. This class includes aConfig
subclass with aDiscriminator
configured on fieldtype
and includes all subtypes.AbstractSubclass
inheriting fromSuperclass
with an abstract methodfoo()
.ConcreteSubclass
inheriting fromAbstractSubclass
, implementing thefoo()
method and setting the discriminator fieldtype
to"concrete_subclass"
.ConcreteSubclass
to a dictionary usingto_dict()
.from_dict()
on all classes in the hierarchy.Error Message:
Temporary Workaround: Adding the same
Config
subclass with aDiscriminator
configuration to theAbstractSubclass
resolves the issue. This suggests a potential inconsistency in how discriminator configurations are inherited or recognized in the class hierarchy.