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

Queryset filtering based on other serializer properties #4426

Closed
stevelacey opened this issue Aug 20, 2016 · 9 comments
Closed

Queryset filtering based on other serializer properties #4426

stevelacey opened this issue Aug 20, 2016 · 9 comments

Comments

@stevelacey
Copy link

I've approached this subject before (#2292), and am interested in seeing if there's a better way around this yet in DRF.

I have a serializer that looks like this:

class InspectionSerializer(ModelSerializer):
    status = serializers.SlugRelatedField(queryset=Status.objects.all(), slug_field='code')

    class Meta:
        model = Inspection
        fields = (
            'vehicle',
        )

And the following relations apply:

  • Vehicle has many Inspection
  • Vehicle has many Status (defines what statuses are allowed for inspections of this vehicle)
  • Inspection has one Status

I need to filter the queryset of the InspectionSerializer's status field so that it only allows a status that belongs to the Vehicle the Inspection also belongs to.

In older versions of DRF, this was pretty difficult, I would filter the queryset in the serializer __init__ using either request data or the instance to determine the vehicle within the context of the current request – necessary to cover all forms of create/update. I am interested in any better way to do this.

@tomchristie
Copy link
Member

Any suggestions?

I wonder if we should consider a filter_<fieldname> method that can be used to provide runtime filtering. Would that make providing for your use-case slightly easier?

@xordoquy
Copy link
Collaborator

Taking advantage of the ability to filter related choices for relation fields as suggested in http://medium.com/django-rest-framework/limit-related-data-choices-with-django-rest-framework-c54e96f5815e along with providing the vehicle in the context

@stevelacey
Copy link
Author

@xordoquy okay so that's neat for filtering by the request/user which is another issue I run into, but the main issue I am discussing here is filtering by data/instance – i.e. I want to filter by a property of the object… which gets complicated when we could be talking about a create/update or partial update

@stevelacey
Copy link
Author

stevelacey commented Aug 22, 2016

@tomchristie I usually end up doing variations of this:

class VehicleSerializer(ModelSerializer):
    location = serializers.SlugRelatedField(queryset=Location.objects.all(), slug_field='code')

    def __init__(self, *args, **kwargs):
        self.filter_location_queryset(self.fields['location'], *args, **kwargs)
        super().__init__(*args, **kwargs)

    def filter_location_queryset(self, f, instance=None, data=serializers.empty, request=None, **kwargs):
        if data is serializers.empty:
            return
        f.queryset = f.queryset.filter(group__sellers=data.get('seller') or instance.seller)

Technically the above leaks data – or at least it used to – I see there's been changes recently about what you yield in OPTIONS requests to combat similar problems.

When I do care about the leaking, I empty the queryset:

    def filter_location_queryset(self, f, instance=None, data=serializers.empty, request=None, **kwargs):
        if data is serializers.empty:
            f.queryset = f.queryset.none()
        f.queryset = f.queryset.filter(group__sellers=data.get('seller') or instance.seller)

@stevelacey
Copy link
Author

stevelacey commented Aug 22, 2016

@tomchristie a less elegant but marginally DRYer approach I've also taken in the past… and maybe regretted, contextualises a custom field in ModelSerializer.__init__ with request and data to achieve the same goal – I would extend ModelSerializer and hide away this class to cope with the shame:

serializers.py

class MagicSerializer(ModelSerializer):
    def __init__(self, *args, **kwargs):
        fkwargs = dict(kwargs, **kwargs.get('context', {}))

        for name, f in self.fields.iteritems():
            self.__init_slug_related_dependent_field__(name, f, *args, **fkwargs)

        super().__init__(*args, **kwargs)

    def __init_slug_related_dependent_field__(self, name, f, instance=None, data=empty, request=None, **kwargs):
        if hasattr(f, 'child_relation') and hasattr(f.child_relation, 'depends_on'):
            assert isinstance(f.child_relation.depends_field, SlugRelatedField)

            # assign instance and data for get_queryset
            f.child_relation.contextualize(instance, data)

            # inject relation values from instance if they were omitted so they are validated regardless
            if data is not empty and instance and name not in data:
                data[name] = [getattr(relation, f.child_relation.slug_field) for relation in getattr(instance, name).all()]

I think I named this class aptly.

relations.py

from django.utils.functional import SimpleLazyObject
from rest_framework.relations import ManyRelatedField as BaseManyRelatedField, MANY_RELATION_KWARGS, SlugRelatedField as BaseSlugRelatedField


class ManyRelatedField(BaseManyRelatedField):
    def to_representation(self, iterable):
        return [
            self.child_relation.to_representation(value)
            for value in iterable if self.read_only or value in self.child_relation.queryset
        ]


class SlugRelatedField(BaseSlugRelatedField):
    @classmethod
    def many_init(cls, *args, **kwargs):
        list_kwargs = {'child_relation': cls(*args, **kwargs)}
        for key in kwargs.keys():
            if key in MANY_RELATION_KWARGS:
                list_kwargs[key] = kwargs[key]
        return ManyRelatedField(**list_kwargs)


class SlugRelatedDependentField(SlugRelatedField):
    def __init__(self, depends_on=None, **kwargs):
        assert depends_on is not None, 'The `depends_on` argument is required.'

        self.depends_on       = depends_on # something__something or something
        self.depends_segments = self.depends_on.split('__')
        self.depends_parent   = self.depends_segments.pop(0)
        self.depends_field    = SimpleLazyObject(lambda: self.parent.parent.fields[self.depends_parent])
        self.depends_queryset = SimpleLazyObject(lambda: self.depends_field.queryset)
        self.depends_model    = SimpleLazyObject(lambda: self.depends_queryset.model)

        super(SlugRelatedDependentField, self).__init__(**kwargs)

    def contextualize(self, instance, data):
        self.data = data
        self.instance = instance

    def get_queryset(self):
        try:
            return self.queryset.filter(**{self.depends_on: reduce(getattr, self.depends_segments, self.get_relation())})
        except self.depends_model.DoesNotExist:
            # if parent was absent or invalid, empty the queryset
            return self.queryset.none()
        except TypeError:
            # if parent was a Page instance, use the full queryset, it's only a list view
            return self.queryset.all()

    def get_relation(self):
        try:
            # if an allowed parent was passed, filter by it
            return self.depends_queryset.get(**{self.depends_field.slug_field: self.data[self.depends_parent]})
        except (KeyError, TypeError):
            # if data was empty or no parent was passed, try and grab it off of the model instance
            if isinstance(self.instance, self.parent.parent.Meta.model):
                return getattr(self.instance, self.depends_parent)
            elif self.instance is None:
                raise self.depends_model.DoesNotExist
            else:
                raise TypeError

TL;DR what this allows you to do is:

class RepositorySerializer(MagicSerializer):
    organization = SlugRelatedField(queryset=Organization.objects.all(), slug_field='code')
    teams = SlugRelatedDependentField(allow_null=True, depends_on='organization', many=True, queryset=Team.objects.all(), required=False, slug_field='slug')

I don't recall if this accounted for every scenario but it accounted for most – I think the edge cases were few.

Validation is always applied to the field because the serializer __init__ stuffs the slug values into data if they're not present i.e. it's a PATCH request – forcing validation to occur for the field regardless – which is important when you consider that when the organization changes (organization having many teams) all assosciated teams would need reconsidering.

I don't recommend this implementation for sure, it worked for me, in older DRF, but maybe it'll help inspire something better – but hopefully as convenient.

@dhm116
Copy link

dhm116 commented Sep 7, 2016

We've been able to solve this issue in a few different ways (depending upon how DRY we needed to be with the particular serializer):

  1. Simply override create and update on the serializer to further restrict the queryset filter based upon the value of the related field

    def create(self, validated_data):
        allowable_statuses = Status.objects.filter(
            code__in=validated_data.get('vehicle').statuses.all().values_list('code', flat=True))
        if validated_data.get('status') not in allowable_statuses:
          raise ValidationError('...')
  2. Add a validate_ method to the serializer

    def validate_status(self, val):
        vehicle = Vehicle.objects.filter(self.root.initial_data.get('vehicle_id').first()
        # validate if the vehicle was found
        if not vehicle.statuses.filter(id=val).count():
           raise ValidationError('...')
  3. Create a custom Field type that performs the validation similarly to the previous validate_ method.

I'm not sure if any of that is helpful at all, but I agree that it would be nice if there were a standard-ish mechanism for validating related fields.

@sergiomb2
Copy link

this isn't a duplicated of #1811 ?

@stevelacey
Copy link
Author

@sergiomb2 yes, I think so

@adwaith-webtrigon
Copy link

@sergiomb2 I don't think so - limit_choices_to doesn't allow us to pass a filter based on other fields in the model.

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

No branches or pull requests

6 participants