Closed emarbo closed 2 years ago
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 anOptional[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.
The following two patches made it work. I think the first one is a bug as it detects wrongly
int | str | None
as anOptional[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 beOptional[int | str]
, notOptional[int]
.
This has been fixed in e26c57a29e (and follow-up bc8d79adc7).
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 be6359b49d (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.
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.
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):
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 theconfig
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 anOptional[int]
.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:
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.