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

Support union types and maybe other corner cases #62

Closed
emarbo opened this issue Sep 15, 2022 · 3 comments
Closed

Support union types and maybe other corner cases #62

emarbo opened this issue Sep 15, 2022 · 3 comments

Comments

@emarbo
Copy link

emarbo commented Sep 15, 2022

Hello there!

We have a type alias that makes use of unions, and we would like to map it to a JSONField. This example summarizes the use case (simplified):

import typing as t
from dataclass import dataclass

JsonPrimitive = str | int | float | bool | None
Json = dict[str, "Json"] | list["Json"] | JsonPrimitive

class ExtendedDataclassSerializer(DataclassSerializer):
    serializer_field_mapping = DataclassSerializer.serializer_field_mapping | {
        Json: serializers.JSONField, 
    }


@dataclass
class Event:
    id: UUID

    @classmethod
    def get_serializer(cls):
        """Dynamically creates the serializer"""
        class Meta:
            dataclass = cls
        return type(
            f"{cls.__name__}Serializer", (ExtendedDataclassSerializer,), {"Meta": Meta}
        )

# ... many events inherit ...

@dataclass
class Event123(Event):
    config: Json

The current implementation does not support dynamically mapping fields of type Json to a JSONField. Note that this problem does not exist when declaring the Serializer as you can declare the config field as a JSONField. This problem affects only dynamic fields.

The following two patches made it work. I think the first one is a bug as it detects wrongly int | str | None as an Optional[int].

def is_optional_type_patch(tp: type) -> bool:
    """
    Patch original typing_utils.is_optional_type.

    This patch returns fixes detecting "None | int | str" as optional,
    only stuff like "None | int" should be detected as so.
    """
    origin = typing_utils.get_origin(tp)
    args = list(set(typing_utils.get_args(tp)))
    none_type = type(None)
    return (
        # int | None
        origin in typing_utils.UNION_TYPES
        and len(args) == 2
        and none_type in args
    ) or (
        # Literal["a", "b", None]
        typing_utils.is_literal_type(tp)
        and None in typing_utils.get_literal_choices(tp)
    )

typing_utils.is_optional_type = is_optional_type_patch


_original_looup_type_in_mapping = field_utils.lookup_type_in_mapping
def lookup_type_in_mapping_patch(mapping: dict[type, T], key: type) -> T:
    """
    This patch allows using anything as type annotation
    """
    if key in mapping:
        return mapping[key]
    return _original_looup_type_in_mapping(mapping, key)

field_utils.lookup_type_in_mapping = lookup_type_in_mapping_patch

The second patch allows mapping UnionType aliases to Field serializers but does not support overriding other kinds of aliases. For instance, it does not allow mapping JsonKV to a JSONField:

JsonPrimitive = str | int | float | bool | None
JsonKV = dict[str, JsonPrimitive]

@dataclass
class EventZ(Event):
    config: JsonKV

In this case, the library calls lookup_type_in_mapping using JsonPrimitive rather than JsonKV. To be honest, I don't know which is the best approach to solve this or even if this should be supported. It makes sense for our dynamic serializers though.

@oxan
Copy link
Owner

oxan commented Sep 26, 2022

There's a lot to unpack here.

The following two patches made it work. I think the first one is a bug as it detects wrongly int | str | None as an Optional[int].

That's indeed not correct, though I don't think your patch is a good solution. int | str | None actually is an optional type (optional really just means nullable), but it should be Optional[int | str], not Optional[int].

The second patch allows mapping UnionType aliases to Field serializers but does not support overriding other kinds of aliases.

This part looks correct to me.

In this case, the library calls lookup_type_in_mapping using JsonPrimitive rather than JsonKV. To be honest, I don't know which is the best approach to solve this or even if this should be supported. It makes sense for our dynamic serializers though.

I agree it's not a bad idea to support this, though I'm also not immediately sure of the best way to support this.

@oxan
Copy link
Owner

oxan commented Nov 18, 2022

The following two patches made it work. I think the first one is a bug as it detects wrongly int | str | None as an Optional[int].

That's indeed not correct, though I don't think your patch is a good solution. int | str | None actually is an optional type (optional really just means nullable), but it should be Optional[int | str], not Optional[int].

This has been fixed in e26c57a (and follow-up bc8d79a).

The second patch allows mapping UnionType aliases to Field serializers but does not support overriding other kinds of aliases.

This part looks correct to me.

This has been fixed in be6359b (I forgot to credit you, sorry about that).

I'll leave this issue open for your last point, though I'm not sure right now if and how that can be fixed.

@emarbo
Copy link
Author

emarbo commented Nov 20, 2022

Hi @oxan,

Thank you for taking the time to respond and fix that bugs!!! I very much appreciate it. And, my apologies for putting so much stuff in a single issue; these were unrelated things.

Regarding the last point, we can leave this as it is. If we reencounter this problem, we'll try to solve it ourselves and then post some proposal here. And no worries about the credit; not needed at all, we are happy just helping a bit.

@emarbo emarbo closed this as completed Nov 20, 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

No branches or pull requests

2 participants