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

Sideloaded object not have all included fields #176

Closed
dpaluch-rp opened this issue Jun 29, 2017 · 2 comments
Closed

Sideloaded object not have all included fields #176

dpaluch-rp opened this issue Jun 29, 2017 · 2 comments

Comments

@dpaluch-rp
Copy link
Contributor

dpaluch-rp commented Jun 29, 2017

Hello I have a problem with sideloading, when I nest Serializer and include the same object multiple time, but with another fields not all fields are returned.

Location (pk 2) is included in Car (field: short_name) and Part (field: name) but in sideloaded object sometimes short_name is returned and sometimes name never both.

Example:

# models.py

class Location(models.Model):
    name = models.CharField(max_length=60)
    short_name = models.CharField(max_length=30)
    description = models.TextField()

class Car(models.Model):
    name = models.CharField(max_length=60)
    location = models.ForeignKey(Location)


class Part(models.Model):
    car = models.ForeignKey(Car)
    name = models.CharField(max_length=60)
    location = models.ForeignKey(Location)
# serializers.py

class LocationSerializer(DynamicModelSerializer):
    name = serializers.CharField(deferred=True)
    short_name = serializers.CharField(deferred=True)
    description = serializers.CharField(deferred=True)
    location = serializers.DynamicRelationField('LocationSerializer', many=True, deferred=True)


    class Meta:
        model = Location 

class PartSerializer(DynamicModelSerializer):
    name = serializers.CharField(deferred=True)
    description = serializers.CharField(deferred=True)
    location = serializers.DynamicRelationField('LocationSerializer', many=True, deferred=True)

    class Meta:
        model = Car

class CarSerializer(DynamicModelSerializer):
    parts = serializers.DynamicRelationField('PartSerializer', many=True, deferred=True)
    name = serializers.CharField(deferred=True)
    description = serializers.CharField(deferred=True)

    class Meta:
        model = Car
# viewsets.py

class CarViewset(DynamicModelViewset):
    serializer_class = CarSerializer

For query:

localhost:8080/api/cars/1/?include[]=name&include[]=location.short_name&include[]=parts.name&include[]=parts.location.name

I radomly get one of responses:

{
    "cars": [
        {
            "id": 1,
            "name": "Porshe",
            "location": 2
        }
    ],
    "locations": [
        {
            "id": 1
            "name": "China",
        },
        {
            "id": 2
            "short_name": "US",
        }
    ],
    "parts": [
      {
        "id": 2,
        "name": "wheel",
        "location": 2
      },
      {
        "id": 2,
        "name": "tire",
        "location": 1
      }
    ]
}

or

{
    "cars": [
        {
            "id": 1,
            "name": "Porshe",
            "location": 2
        }
    ],
    "locations": [
        {
            "id": 1
            "name": "China",
        },
        {
            "id": 2
            "name": "United States",
        }
    ],
    "parts": [
      {
        "id": 2,
        "name": "wheel",
        "location": 2
      },
      {
        "id": 2,
        "name": "tire",
        "location": 1
      }
    ]
}

But I think correct response is like bellow

{
    "cars": [
        {
            "id": 1,
            "name": "Porshe",
            "location": 2
        }
    ],
    "locations": [
        {
            "id": 1
            "name": "China",
        },
        {
            "id": 2
            "name": "United States",
            "short_name": "Us",
        }
    ],
    "parts": [
      {
        "id": 2,
        "name": "wheel",
        "location": 2
      },
      {
        "id": 2,
        "name": "tire",
        "location": 1
      }
    ]
}

I propose this way for fix that: ( wrapped in # SIDELOADING FIX # comments)

# processors.py

"""This module contains response processors."""

from django.utils import six
from dynamic_rest.processors import SideloadingProcessor as BaseSideloadingProcessor
from rest_framework.utils.serializer_helpers import ReturnDict

from dynamic_rest.conf import settings

class SideloadingProcessor(BaseSideloadingProcessor):
    """A processor that sideloads serializer data.

    Sideloaded records are returned under top-level
    response keys and produces responses that are
    typically smaller than their nested equivalent.
    """
    def process(self, obj, parent=None, parent_key=None, depth=0):
        """Recursively process the data for sideloading.

        Converts the nested representation into a sideloaded representation.
        """
        if isinstance(obj, list):
            for key, o in enumerate(obj):
                # traverse into lists of objects
                self.process(o, parent=obj, parent_key=key, depth=depth)
        elif isinstance(obj, dict):
            dynamic = self.is_dynamic(obj)
            returned = isinstance(obj, ReturnDict)
            if dynamic or returned:
                # recursively check all fields
                for key, o in six.iteritems(obj):
                    if isinstance(o, list) or isinstance(o, dict):
                        # lists or dicts indicate a relation
                        self.process(
                            o,
                            parent=obj,
                            parent_key=key,
                            depth=depth +
                            1
                        )

                if not dynamic or getattr(obj, 'embed', False):
                    return

                serializer = obj.serializer
                name = serializer.get_plural_name()
                instance = getattr(obj, 'instance', serializer.instance)
                instance_pk = instance.pk if instance else None
                pk = getattr(obj, 'pk_value', instance_pk) or instance_pk

                # For polymorphic relations, `pk` can be a dict, so use the
                # string representation (dict isn't hashable).
                pk_key = repr(pk)

                # sideloading
                seen = True
                # if this object has not yet been seen
                if pk_key not in self.seen[name]:
                    seen = False
                    self.seen[name].add(pk_key)

                # prevent sideloading the primary objects
                if depth == 0:
                    return

                # TODO: spec out the exact behavior for secondary instances of
                # the primary resource

                # if the primary resource is embedded, add it to a prefixed key
                if name == self.plural_name:
                    name = '%s%s' % (
                        settings.ADDITIONAL_PRIMARY_RESOURCE_PREFIX,
                        name
                    )

                if not seen:
                    # allocate a top-level key in the data for this resource
                    # type
                    if name not in self.data:
                        self.data[name] = []

                    # move the object into a new top-level bucket
                    # and mark it as seen
                    self.data[name].append(obj)

                # SIDELOADING FIX #
                if seen:
                    ob = [o for o in self.data[name] if o.instance.pk == pk][0]
                    ob.update(obj)
                    self.data[name] = [ob if o.instance.pk == pk else o for o in self.data[name]]
                # SIDELOADING FIX #

                # replace the object with a reference
                if parent is not None and parent_key is not None:
                    parent[parent_key] = pk
@aleontiev
Copy link
Collaborator

aleontiev commented Jun 29, 2017

Hello @dpaluch-rp, thanks for reporting this issue. I agree with your assessment and proposed fix. I would recommend this version, which would alter the data in-place rather than re-creating lists:

if seen:
    for o in self.data[name]:
        if o.pk == pk:
            o.update(obj)
            break

If you submit a PR with these changes and a corresponding test case, I will merge it in. Otherwise, I can get to this myself but probably not for another week or two.

aleontiev added a commit that referenced this issue Jul 17, 2017
#176 Fix: Sideloaded object not have all included fields
@aleontiev
Copy link
Collaborator

Fixed by #187

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

No branches or pull requests

2 participants