Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to extend serializer context using kwargs of get_serializer method #6956

Closed
wants to merge 1 commit into from
Closed

Allow to extend serializer context using kwargs of get_serializer method #6956

wants to merge 1 commit into from

Conversation

Olerdrive
Copy link

@Olerdrive Olerdrive commented Oct 1, 2019

Fixes #6955

Description

As I mentioned in the #6955 it is impossible to pass any extra context to the serializer from the generic Viewset as it will be overwritten with the default value in the get_serializer method.

My pull request fixes this allowing the default context to be extended with the provided one.

@Olerdrive Olerdrive changed the title Allow to extend serializer context using kwargs of Allow to extend serializer context using kwargs of get_serializer method Oct 1, 2019
@rpkilby
Copy link
Member

rpkilby commented Oct 1, 2019

Hi @Olerdrive. I'd argue that you shouldn't leverage the context here. Instead, context_key should be an argument for initializing the serializer. If you do want to use the context though, the correct thing to do would be to override the get_serializer_context method.

def get_serializer_context(self):
    context = super().get_serializer_context()
    context['context_key'] = 'value'
    return context

@rpkilby rpkilby closed this Oct 1, 2019
@Pithikos
Copy link
Contributor

@rpkilby there are cases where actually this would make things much simpler and make code more consistent.

In my case for example I need to pass some metadata when uploading a file (e.g. reading the headers of an MPEG), but I still want to keep all other views to return just 'request' and 'view'.

The solution you mention, does not work in my case since the context comes from a specific view - create. Overiding get_serializer_context would require from me to refactor all other existing views like 'delete' where that context doesn't make much sense.

@rpkilby
Copy link
Member

rpkilby commented Apr 28, 2020

@Pithikos you could inspect the viewset properties, such as action, to determine if you want to add additional context. e.g.,

def get_serializer_context(self):
    context = super().get_serializer_context()
    if self.action == 'list':
        context['context_key'] = 'value'
    return context

@rpkilby
Copy link
Member

rpkilby commented Apr 28, 2020

I'd agree that it would be preferable to provide the context explicitly in the call to get_serializer, as opposed to introspecting details about the view in get_serializer_context, however the original PR proposed merging the contents of the default context with the provided context, which is what I objected to. It might be reasonable to change get_serializer to do:

    def get_serializer(self, *args, **kwargs):
        """
        Return the serializer instance that should be used for validating and
        deserializing input, and for serializing output.
        """
        serializer_class = self.get_serializer_class()
        kwargs.setdefault('context', self.get_serializer_context())
        return serializer_class(*args, **kwargs)

The above allows the user to override the context, without merging them.

@rpkilby
Copy link
Member

rpkilby commented Apr 29, 2020

@Pithikos see #7298. You would still need to pass the full context as necessary (view, request).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generic Viewset overrides passed serializer's context with the default value
3 participants