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

Support for depth limiting #12

Closed
Soviut opened this issue May 25, 2016 · 7 comments
Closed

Support for depth limiting #12

Soviut opened this issue May 25, 2016 · 7 comments

Comments

@Soviut
Copy link

Soviut commented May 25, 2016

It would be nice if the field honoured the Meta.depth attribute. This would help avoid issues with circular relationships as well as keep payloads lighter.

More documentation on depth can be found here http://www.django-rest-framework.org/api-guide/serializers/#specifying-nested-serialization

By default, the ModelSerializer uses PrimaryKeyFields so maybe that can be the fallback when depth is reached. That, or perhaps specify what field type to fallback.

@heywbj
Copy link
Owner

heywbj commented May 26, 2016

Someone else asked for this a couple weeks ago. I'll give it some thought.

Or submit you own pull request if you feel so inclined :)

@Soviut
Copy link
Author

Soviut commented May 27, 2016

Here's a working set of code someone posted on Stack Overflow that allows for recursion levels.

http://stackoverflow.com/questions/13376894/django-rest-framework-nested-self-referential-objects

from rest_framework.reverse import reverse
from rest_framework import serializers

class RecursiveField(serializers.Serializer):
    """
    Can be used as a field within another serializer,
    to produce nested-recursive relationships. Works with
    through models, and limited and/or arbitrarily deep trees.
    """
    def __init__(self, **kwargs):
        self._recurse_through = kwargs.pop('through_serializer', None)
        self._recurse_max = kwargs.pop('max_depth', None)
        self._recurse_view = kwargs.pop('reverse_name', None)
        self._recurse_attr = kwargs.pop('reverse_attr', None)
        self._recurse_many = kwargs.pop('many', False)

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

    def to_representation(self, value):
        parent = self.parent
        if isinstance(parent, serializers.ListSerializer):
            parent = parent.parent

        lvl = getattr(parent, '_recurse_lvl', 1)
        max_lvl = self._recurse_max or getattr(parent, '_recurse_max', None)

        # Defined within RecursiveField(through_serializer=A)
        serializer_class = self._recurse_through
        is_through = has_through = True

        # Informed by previous serializer (for through m2m)
        if not serializer_class:
            is_through = False
            serializer_class = getattr(parent, '_recurse_next', None)

        # Introspected for cases without through models.
        if not serializer_class:
            has_through = False
            serializer_class = parent.__class__

        if is_through or not max_lvl or lvl <= max_lvl: 
            serializer = serializer_class(
                value, many=self._recurse_many, context=self.context)

            # Propagate hereditary attributes.
            serializer._recurse_lvl = lvl + is_through or not has_through
            serializer._recurse_max = max_lvl

            if is_through:
                # Delay using parent serializer till next lvl.
                serializer._recurse_next = parent.__class__

            return serializer.data
        else:
            view = self._recurse_view or self.context['request'].resolver_match.url_name
            attr = self._recurse_attr or 'id'
            return reverse(view, args=[getattr(value, attr)],
                           request=self.context['request'])

@heywbj
Copy link
Owner

heywbj commented Aug 4, 2017

It's been a year and I don't personally see this as a pressing use case. In general, I'd advocate that one limit the size of the serialized object before trying to serialize it, by limiting the scope of the database query, or otherwise preprocessing the data.

Doing it within the serializer is expensive (due to repeated database hits). If you were to reach the 'maximum depth', I think the proper behavior would be to throw an error, which is what you'll get anyway when you reach the bottom of the call stack. Specifying some alternative behavior could be useful in some cases, but it could lead to errors which are rather difficult to debug, since simply terminating the serialization at a certain depth is fairly heavy-handed.

I'm not saying it isn't a valid solution in certain use cases, but I think that tailoring the data before it is passed to this package is probably better -- you have a lot more sophisticated tools for directly manipulating python objects and database queries than simply what the 'max depth' of a tree should be.

@heywbj heywbj closed this as completed Aug 4, 2017
@aavkvard
Copy link

Just to add a datapoint to this:

I am using django_treebeard to store a tree of several thousand categories and have an API endpoint that feeds node data to JSTree so it can dynamically construct a tree from it. Treebeard provides get_children and get_descendants methods on a node which retrieve the immediate children and all the descendants respectively, so in my models I have easy access to querysets of these. In my tree, I have several levels of depth, so while no node has more than ~70 children, many of my nodes have hundreds of descendants, all of which get included in the serialized representation of a single object using RecursiveField, making performance unacceptable. As I'm already serializing a single object, I can't really see a way to ask for less data as you suggest in your comment.

I appreciate the answer to this may well be that my use case is not a good fit for django-rest-framework-recursive. I am new to DRF and found the need to represent recursive objects literally 10 minutes after starting to use it, so there may be other ways to go about this, but they certainly are not obvious and having seen multiple requests among the issues for limiting recursion depth it looks like I'm not the only one with this problem.

@heywbj
Copy link
Owner

heywbj commented Jul 14, 2018

In what sense is performance unacceptable? Is the serialization taking too long? Is the serialized object too large for a single request? Is the database too slow?

I feel like what you are asking for, depth limiting, is a clumsy solution to a complicated problem, but I need to understand what your problem is to give you better advice

@aavkvard
Copy link

Thanks for the quick response.

It is unacceptable in the sense that it is not fit for interactive use. For a node with over 2300 descendants it takes ~6 seconds to load the fully recursed serialized data and another almost 2s to render the tree. The issue is made even worse by the fact that this is a navigation menu, so the user will start at the top level, pretty much guaranteeing they will hit an item with many descendants and hitting these response times. Serializing a node one level down with 260 descendants takes less than 800ms, that is not good, but acceptable.

What I ended up doing is that I've added a helper serializer that only serializes a single item and use it in a method to serialize a list of items I get from MP_Node.get_children and call it in the actual serializer using SerializerMethodField:

class ChildrenSerializer(serializers.ModelSerializer):
    class Meta:
        model = Item
        fields = ('id', ...)

class ItemSerializer(serializers.ModelSerializer):
    children = serializers.SerializerMethodField()
    class Meta:
        model = Item
        fields = ('id', ... , 'children')
    def get_children(self, obj):
        data = [ChildrenSerializer(x).data for x in obj.get_children()]
        return data or None

This only goes to the next level down and takes less than 150ms for the node with 2300 descendants as it only has 6 children. But this method feels hacky and it would be cleaner and more readable to just be able to write something like:
children = serializers.ListField(child=RecursiveField(depth=1))

@heywbj
Copy link
Owner

heywbj commented Jul 14, 2018

If it's a tree of depth one, then I think the method you proposed is reasonable. Why is it a hack? You have a fixed structure for the data you want to serialize, so just build the serializers to match.

children = serializers.ListField(child=ChildrenSerializer())

The person who proposed this originally wanted to avoid circular relationships, which in my view is pointless - if you have a circular relationship then you have bigger problems. He also wanted to keep payloads lighter, but again my view it that you should limit the amount of data you query rather than limit it in the serializer.

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

3 participants