wimglenn / djangorestframework-queryfields

Allows clients to control which fields will be sent in the API response
http://djangorestframework-queryfields.readthedocs.io/
MIT License
216 stars 16 forks source link

Consider overriding to_representation instead of modifying self.fields #1

Closed fabiosussetto closed 7 years ago

fabiosussetto commented 7 years ago

Hey Wim, nice little useful package this one, I was surprised they don't support this out of the box.

One thing I'd consider is having your mixin override to_representation (http://www.django-rest-framework.org/api-guide/serializers/#baseserializer) instead of manually popping the fields out of the serialiser instance. to_representation already returns a dict, so you could call super(), access self.context['request'] to determine the fields to exclude, and finally use pop on the dict returned from super.

To me this way has these two advantages:

Cheers!

wimglenn commented 7 years ago

Hi Fabio, thanks for your comments!

The disadvantage of your suggested approach is that in creating the dict returned from super() proxy, we have already serialized many of the fields (which will then get excluded from the representation). I'm currently using this package in an API where a few of those fields are very expensive to serialize, and some api clients don't need the expensive fields. If they are excluded on post-processing of to_representation, the damage has already been done so to speak.

You could be right that we may be able to exclude the fields, at a later time though. I was following an example from the official docs where the fields were customized at __init__ time by popping the dict. I'll look into writing some tests with a custom field that blows up when serialized, and see how late we can delay the exclusion action - the unwritten promise, which I didn't really cover in the unit tests and neglected to mention in the docs, is that unneeded fields don't get serialized at all.

You're correct that it's destructive on the serializer instance, but since the instance is bound to the request context, and the fields are only modified if we're within a GET request context, I can't imagine a situation where we could re-use the same serializer instance for an input context? Could you elaborate on that kind of use-case?

fabiosussetto commented 7 years ago

I see, if you don't want the fields to be serialized at all then my suggestion doesn't make much sense.

I can't imagine a situation where we could re-use the same serializer instance for an input context

Yeah you're right, since the serialiser is bound to the request then I can't think of a real practical case where you may want to reuse the instance. I guess that's just me not liking so much the fact serialisers in DRF are objects instead of functions with no state / side-effects.