Django-rest-framework: Changes to the get_attribute method under fields.py after version 3.6.x

Created on 18 Feb 2020 · 6Comments · Source: encode/django-rest-framework

When retrieving field information from source, if the object retrieved by a chain operation is None, it should be returned directly.

This operation was removed after version 3.6.x, if so I may need to make additional judgment, I think this action is friendly

What do you think？

Expected behavior

def get_attribute(instance, attrs):
    for attr in attrs:

        # Break out early if we get `None` at any point in a nested lookup.
        if instance is None:
            return None

        try:
            if isinstance(instance, Mapping):
                instance = instance[attr]
            else:
                instance = getattr(instance, attr)
        except ObjectDoesNotExist:
            return None
        if is_simple_callable(instance):
            try:
                instance = instance()
            except (AttributeError, KeyError) as exc:
                # If we raised an Attribute or KeyError here it'd get treated
                # as an omitted field in `Field.get_attribute()`. Instead we
                # raise a ValueError to ensure the exception is not masked.
                raise ValueError('Exception raised in callable attribute "{}"; original exception was: {}'.format(attr, exc))

    return instance

Actual behavior

def get_attribute(instance, attrs):
    for attr in attrs:
        try:
            if isinstance(instance, Mapping):
                instance = instance[attr]
            else:
                instance = getattr(instance, attr)
        except ObjectDoesNotExist:
            return None
        if is_simple_callable(instance):
            try:
                instance = instance()
            except (AttributeError, KeyError) as exc:
                # If we raised an Attribute or KeyError here it'd get treated
                # as an omitted field in `Field.get_attribute()`. Instead we
                # raise a ValueError to ensure the exception is not masked.
                raise ValueError('Exception raised in callable attribute "{}"; original exception was: {}'.format(attr, exc))

    return instance

Example

drf version：3.10.x

# models.py

from django.db import models

class B(models.Model):
    name = models.CharField(max_length=32)

class A(models.Model):
    field1 = models.ForeignKey("B", on_delete=models.CASCADE, null=True)

# serializer.py

class ASerializer(serializers.ModelSerializer):
    name = serializers.CharField(source="field1.name")
    class Meta:
        fields = ("name", )

>>> from . import models
>>> from . import serializer
>>> ins = models.A.objects.create()
>>> ser = serializer.A(ins)
>>> ser.data
...
AttributeError: Got AttributeError when attempting to get a value for field `name` on serializer `ASerializer`.
The serializer field might be named incorrectly and not match any attribute or key on the `B` instance.
Original exception text was: 'NoneType' object has no attribute 'name'.

I think the expected behavior is friendly and I will submit a PR if you approve.

Thanks.

Source

csrftoken

All 6 comments

It'd be more robust, so yup seems reasonable.

tomchristie on 19 Feb 2020

This was removed in #5375 to fix behavior around using default values. In this case, I think what we might want to do here is raise a more informative error message. Something that indicates that the relation was null, and the serializer should provide a default value.