yezyilomo / django-restql

Turn your API made with Django REST Framework(DRF) into a GraphQL like API.
https://yezyilomo.github.io/django-restql
MIT License
620 stars 43 forks source link

does not work with SerializerMethodField #57

Closed oguzhancelikarslan closed 5 years ago

oguzhancelikarslan commented 5 years ago

In case when i use one to many fields i need to pass serializer's id into different serializer.

def get_a_list(self, obj):
    child = b.objects.all()
    serializer = BListSerializer(instance=child, context={"obj_id": obj.id}, many=True)
    return serializer.data
yezyilomo commented 5 years ago

Thanks for the feedback, I would like to know what exactly does not work?, assuming you want to do something like

class PhoneSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    class Meta:
        model = Phone
        fields = ['id', 'number', 'type']

class StudentSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    phones = serializers.SerializerMethodField()

    def get_phones(self, obj):
        phones = obj.phone_numbers.all()
        serializer = PhoneSerializer(phones, many=True)
        return serializer.data

    class Meta:
        model = Student
        fields = ['id', 'name',  'phones']

Were you trying to do something like query={name, phones{number, type}} on students route?. Am trying to understand what you wanted to accomplish and what went wrong.

oguzhancelikarslan commented 5 years ago

In addition to what you type, I pass context to PhoneSerializer. Otherwise everything is same.

yezyilomo commented 5 years ago

Are you getting an error or?

oguzhancelikarslan commented 5 years ago

yes getting error like that "'xyz' is not a nested field"

yezyilomo commented 5 years ago

Okay, if that's the message then it's right cuz you can return any thing on SerializerMethodField, so it's true that phones is not a nested field but it's a calculated field.

oguzhancelikarslan commented 5 years ago

In this case can’t I use this feature? Or can you suggest me any way to go? How can I use this feature?

15 Eyl 2019 Paz, saat 23:08 tarihinde Yezy Ilomo notifications@github.com şunu yazdı:

Okay, if that's the message then it's right cuz you can return any thing on SerializerMethodField, so it's true that phones is not a nested field but it's a calculated field.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yezyilomo/django-restql/issues/57?email_source=notifications&email_token=AGBUCQCEZ2FA6YBWIGCS5Z3QJ2I3FA5CNFSM4IW2Y7C2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6XX74Y#issuecomment-531595251, or mute the thread https://github.com/notifications/unsubscribe-auth/AGBUCQHVU73FGDMGB3DHJ3DQJ2I3FANCNFSM4IW2Y7CQ .

yezyilomo commented 5 years ago

You can do this

class PhoneSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    class Meta:
        model = Phone
        fields = ['id', 'number', 'type']

class StudentSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    phones = PhoneSerializer(many=True)

    class Meta:
        model = Student
        fields = ['id', 'name',  'phones']
yezyilomo commented 5 years ago

I don't see the point of using SerializerMethodField if you are not changing anything on PhoneSeriallizer.

oguzhancelikarslan commented 5 years ago

But inside the PhoneSerializer I need the student id. Because according to student id I make some calculations for every phone number.

yezyilomo commented 5 years ago

But you can get student_id from PhoneSerializer without passing it through context if they are related as you claim, You can do this by writing a custom Serializer which inherit PhoneSerializer and override to_representation method

class CustomPhoneSerializer(PhoneSerializer):
    class Meta(PhoneSerializer.Meta):
        pass

    def to_representation(self, instance):
        if isinstance(self.parent, serializers.ListSerializer):
            parent = self.parent.parent
        elif isinstance(self.parent, serializers.Serializer):
            parent = self.parent
        else:
            pass
        if parent.instance is not None:
            student_id = parent.instance.pk
            # Then do whatever you want with student_id
        super().to_representation(instance)

class StudentSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    phones = CustomPhoneSerializer(many=True)

    class Meta:
        model = Student
        fields = ['id', 'name',  'phones']
yezyilomo commented 5 years ago

But inside the PhoneSerializer I need the student id. Because according to student id I make some calculations for every phone number.

And it looks like you want to do filtering of phones based on student_id, right?

ashleyredzko commented 5 years ago

Thoughts on a possible nested field wrapper for this? It could provide context to the methodfield that the dynamic mixin could use by providing the needed context from the provided serializer class, and thus allow a serializer method field. I'm imagining something like this:

class StudentSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    phones = FieldContext(serializers.SerializerMethodField(), PhoneSerializer))

Also interested in a possible method field implementation that allows a context to be passed in from the package itself:

class StudentSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    phones = SerializerMethodWithContextField(serializer=PhoneSerializer)
yezyilomo commented 5 years ago

It would still be imposible to dynamically filter out fields from what's returned by SerializerMethodField, because as said on the doc "SerializerMethodField is a read-only field. It gets its value by calling a method on the serializer class it is attached to. It can be used to add any sort of data to the serialized representation of your object", so what's returned by SerializerMethodField is already serialized(not a serializer object) but the library depends on a serializer object to do the filtering, also SerializerMethodField could return anything so it's kinda risky to expect it to return a serializer object when it's capable of returning anything, and even if you return a serializer object it would throw an error because what's returned by SerializerMethodField should be JSON serializable and serializer object is not.

ashleyredzko commented 5 years ago

@yezyilomo You'd have access to the context passed into the parent, so as long as you provide either a serializer class (to pull the fields from the Meta) or a list of field names, you can pass along the context in the method itself while still giving context back to the parser.

yezyilomo commented 5 years ago

Okay, so it's usage will be something like this?

class StudentSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    phones = SerializerMethodWithContextField(serializer=PhoneSerializer)

    def get_phones(self, obj):
        phones = obj.phone_numbers.all()
        serializer = PhoneSerializer(phones, many=True)
        return serializer.data
ashleyredzko commented 5 years ago

That's roughly what I'm suggesting, yeah, but making sure we pass the context to the serializer in the method field.

yezyilomo commented 5 years ago

Okay, I think passing the context is not a problem, the is problem is how to get a query that is specific to what is retrieved in the method field, I was experimenting it and I think it could be possible to achieve that without even using a nested field wrapper only if we add SerializerMethodField to nested_class here https://github.com/yezyilomo/django-restql/blob/7a90bd6302e7eabe7953e988917b9d887a0377b7/django_restql/mixins.py#L138.

yezyilomo commented 5 years ago

The downside is that all SerializerMethodFields will be treated as nested field.

yezyilomo commented 5 years ago

That's roughly what I'm suggesting, yeah, but making sure we pass the context to the serializer in the method field.

Anyway can you try to implement that and see if it works?..

yezyilomo commented 5 years ago

The downside is that all SerializerMethodFields will be treated as nested field.

Ooh I've got a way around this, I've defined a class which inherit SerializerMethodField and I've added it to nested_class so that only field which use this class instead of SerializerMethodField are termed as nested fields, it works fine now. I've called this new class DynamicSerializerMethodField meaning a SerializerMethodField which allows fields filtering.

yezyilomo commented 5 years ago

Here is how to use it

class StudentSerializer(DynamicFieldsMixin, serializers.ModelSerializer):
    phones = DynamicSerializerMethodField()
    class Meta:
        model = Student
        fields = ['id', 'name',  'phones']

    def get_phones(self, obj):
        request = self.context.get("request")
        context = {"request": request}
        phones = obj.phone_numbers.all()

        # get child query from parent
        query = self.nested_fields_queries["phones"]
        serializer = PhoneSerializer(phones, query=query, many=True, read_only=True, context=context)
        return serializer.data
ashleyredzko commented 5 years ago

I think the request and context reassignment might be unneeded (we can just pass self.context along`), but this is very exciting!

yezyilomo commented 5 years ago

I think the request and context reassignment might be unneeded (we can just pass self.context along`), but this is very exciting!

Yeah, that's a good way to do it..