myyang / django-pb-model

Protobuf mixin for django model
Other
112 stars 22 forks source link

Custom field serializers disable Datetime field serializer #21

Closed davilaerte closed 4 years ago

davilaerte commented 4 years ago

When I try to call the to_pb() method of a class that has custom field serializers, I get the following error when trying to serialize a field of type Datetime:

DjangoPBModelError: Can't serialize Model(datetime_field)'s field: <class 'test.ModelTest'>. Err: Assignment not allowed to field "datetime_field" in protocol message object.

When I remove the custom field serializers the error does not occur and the method to_pb() works correctly. Looking at the code I realized that the error occurs, since the standard serializers are replaced by the customized serializers when there are customized serializers.

myyang commented 4 years ago

The default custom serializer formodels.DateTimeField is mapping to protobuf google/protobuf/timestamp.proto type and tested.

Do you add your custom serializer for models.DateTimeField or the datetime field?

If you want simple conversion between datetime and protobuf payload, yeap, remove default custom serializers as you do.

davilaerte commented 4 years ago

Sorry, I didn't explain it correctly. I didn't add a custom serializer for DateTimeField, I added a custom serializer for JSONField and when I added this custom serializer the default serializer for DateTimeField stopped working. Is this expected behavior? Or shouldn't that happen?

myyang commented 4 years ago

Sorry for late response, I hadn't use python for a while, rebuilding the env take some time.

I re-produce the error with following code, is that correct ?

from django.db import models

from pb_model.models import ProtoBufMixin
from pb_model.fields import JSONField

from . import models_pb2

def json_serializer(pb_obj, pb_field, dj_value):
    setattr(pb_obj, pb_field.name, json.dumps(dj_value))

def json_deserializer(instance, dj_field_name, pb_field, pb_value):
    setattr(instance, dj_field_name, json.loads(pb_value))

class Issue21(ProtoBufMixin, models.Model):
    pb_model = models_pb2.Issue21;

    pb_2_dj_field_serializers = { 
        JSONField: (json_serializer, json_deserializer),
    }   

    dt_field = models.DateTimeField(auto_now=True)
    cus_field = JSONField(null=True)

The problem would be the pb_2_dj_field_serializers is overwrited by child class, default includes uuid and datetime field serializers.

Could be fixed by adding original default fields

    pb_2_dj_field_serializers = {
        fields.JSONField: (json_serializer, json_deserializer),
        models.DateTimeField: (fields._datetimefield_to_pb,
                               fields._datetimefield_from_pb),
        models.UUIDField: (fields._uuid_to_pb,
                           fields._uuid_from_pb),
    }

I think always fallback to default unless child overwrite the same column or type would be better. This may easier for use, do you agree? If so, I'll patch this by modifying the _get_serialization when I have free time in these days.

Any PR is welcome if you could help 😄

davilaerte commented 4 years ago

Hi @myyang, sorry for the late response and thanks for answering my questions.

I re-produce the error with following code, is that correct ?

Yes, this is exactly the error that occurred

I think always fallback to default unless child overwrite the same column or type would be better. This may easier for use, do you agree?

I agree with you, I think that this way is easier to understand.

Any PR is welcome if you could help

When I have free time, I will try to send a PR for this :smile: