Amertz08 / drf_ujson2

JSON parser and renderer using ujson for Django Rest Framework
MIT License
36 stars 4 forks source link

TypeError thrown when serializing UUIDField #9

Open shydefoo opened 4 years ago

shydefoo commented 4 years ago

Issue rendering UUID field as foreign key

Output from shell

>>> p = Photo.objects.all()[0]
>>> p
<Photo: LE123: Photo_L*>
>>> s = PhotoSerializer(p)
>>> s.data
{'id': 4875, 'name': 'LE123 Photo_L.jpg', 'image': '/images/LE123_Photo_L.jpg', 'product': UUID('ffdce82f-f094-499a-b221-2b49ec9c36ff')}
>>> UJSONRenderer().render(s.data)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/local/lib/python3.6/site-packages/drf_ujson/renderers.py", line 42, in render
    indent=indent or 0,
TypeError: �1a-� is not JSON serializable

>>> from rest_framework.renderers import JSONRenderer
>>> JSONRenderer().render(s.data)
b'{"id":4875,"name":"LE123 Photo_L.jpg","image":"/images/LE123_Photo_L.jpg","product":"ffdce82f-f094-499a-b221-2b49ec9c36ff"}'

Models & Serializers


class Category(OrderedModel):
    category = models.CharField(max_length=50)
    order = models.PositiveIntegerField(_("order"), editable=False, db_index=True)

class Product(models.Model):
    id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
    ...
    category = models.ManyToManyField(Category)

class Photo(models.Model):
    name = models.CharField(max_length=500, null=True)
    image = models.ImageField(null=True)
    product = models.ForeignKey(Product, on_delete=models.SET_NULL, null=True)

class PhotoSerializer(serializers.ModelSerializer):
    class Meta:
        model = Photo
        fields = "__all__"

class ProdSerializer(serializers.ModelSerializer):
    category = serializers.StringRelatedField(many=True)
    photo_set = PhotoSerializer(many=True)

    class Meta:
        model = Product
        fields = "__all__"

class CategorySerializer(serializers.ModelSerializer):
    class Meta:
        model = Category
        fields = "__all__"
Amertz08 commented 4 years ago

I've ran into this issue myself. I'd consider explicitly declaring a serialize on ProdSerializer to handle the uuid.

uuid = serializers.CharField()
shydefoo commented 4 years ago

Hmm, this seems to be a temp fix. It would be great if the UUID Fields could be rendered just like how JSONRenderer handles them.

Amertz08 commented 4 years ago

Yeah so it looks like the built in json module supports passing an encoder class which is what the base JSONRenderer class takes advantage of in order to handle UUIDs. It looks like ujson is adding support for it [1]. So I don't think I can do anything particularly clean until then. I did post a ticket to the repo to suggest supporting UUIDs directly but it doesn't look like that will be implemented [2].

[1] https://github.com/ultrajson/ultrajson/issues/124 [2] https://github.com/ultrajson/ultrajson/issues/385

Amertz08 commented 4 years ago

I think the closest thing to a fix at the renderer level would be something like follows.

class UJSONRenderer:

    translate_func = lambda v: v

    def render(self, ...):
        # ...
        ret = ujson.dumps(self.translate_func(data), ...)

Where you can write a function that translates your response into a ujson serializable object. There would be performance implications obviously that should be explored.

Amertz08 commented 4 years ago

I might implement this in an alpha release and see how it goes.

aaronn commented 4 years ago

Would love to see this– I'm using UUIDs for all of my PKs. Doesn't sound like ujson is ever going to prioritize either custom encoders or support uuid serialization directly.

Amertz08 commented 4 years ago

@aaronn The last time I looked (haven't in a while) at ujson they were going to add support for custom encoders. So my thought was wait to see what that interface looks like. I'll have to look into what their timeline is now. Last I checked they basically said if json out of the box doesn't support it then they won't either so encoding UUID objects directly is a no go. The custom encoders were thus their path forward (to my understanding).

aaronn commented 4 years ago

Just checked the issue and they've removed it from the milestone. It doesn't look like something they have any plans to prioritize.

Amertz08 commented 4 years ago

Alright I'll put something out on this. I have had time for this I just haven't made time.

Amertz08 commented 4 years ago

For this to work you'll have to implement a recursive function that will traverse the various structures and types. I think the recursion will really matter around dict/list. This was the reason I was so apprehensive about implementing it. That being said should be available via pip install drf_ujson2==1.7.0a1

Amertz08 commented 4 years ago

I actually think I have the typing wrong on data: Union[dict, None], in the def render call. That might need to be Any or Union[Dict, List, None] not sure what makes sense.

Amertz08 commented 4 years ago

Something like this.

def translate(data: Any):
    if isinstance(data, list):
        return [translate(d) for d in data]
    if isinstance(data, dict):
        return {key: translate(value) for key, value in data.items()}
    if isinstance(data, uuid.UUID):
        return str(data)
    return data

could be wrong though 🤷