SerializerMethodField defaults seem to be treated incorrectly

[rmelick-vida]

Describe the bug
I was trying migrating my project to use drf-spectactular, and ran into an interesting exception when trying to generate the schema for the first time.

I ran

./manage.py spectacular --file schema.yml

and got an exception with the following stacktrace

  Traceback (most recent call last):
  File "/src/webserver/manage.py", line 43, in <module>
    execute_from_command_line(sys.argv)
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/django/core/management/__init__.py", line 381, in execute_from_command_line
    utility.execute()
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/django/core/management/__init__.py", line 375, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/django/core/management/base.py", line 323, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/django/core/management/base.py", line 364, in execute
    output = self.handle(*args, **options)
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/management/commands/spectacular.py", line 50, in handle
    schema = generator.get_schema(request=None, public=True)
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/generators.py", line 257, in get_schema
    paths=self.parse(request, public),
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/generators.py", line 232, in parse
    path, path_regex, path_prefix, method, self.registry
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/openapi.py", line 84, in get_operation
    operation['responses'] = self._get_response_bodies()
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/openapi.py", line 1001, in _get_response_bodies
    return {'200': self._get_response_for_code(response_serializers, '200')}
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/openapi.py", line 1046, in _get_response_for_code
    component = self.resolve_serializer(serializer, 'response')
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/openapi.py", line 1188, in resolve_serializer
    component.schema = self._map_serializer(serializer, direction)
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/openapi.py", line 696, in _map_serializer
    schema = self._map_basic_serializer(serializer, direction)
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/openapi.py", line 760, in _map_basic_serializer
    schema = self._map_serializer_field(field, direction)
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/openapi.py", line 443, in _map_serializer_field
    meta = self._get_serializer_field_meta(field)
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/drf_spectacular/openapi.py", line 739, in _get_serializer_field_meta
    default = field.to_representation(field.default)
  File "/.pyenv/versions/webserver/lib/python3.6/site-packages/rest_framework/fields.py", line 1905, in to_representation
    return method(value)
  File "/src/webserver/food/serializers.py", line 124, in get_serving_measures
    instance.serving_measures if instance.serving_measures is not None else []
AttributeError: 'list' object has no attribute 'serving_measures'

To Reproduce
Here's a stripped down version of the Serializer and Model class I was using (I removed a bunch of other fields since it's from our production codebase)

class RecognizedFoodSerializer(serializers.ModelSerializer):
    food_name = CharField(required=False)
    serving_measures = serializers.SerializerMethodField(read_only=True, default=[])

    class Meta:
        model = RecognizedFood
        fields = (
            "urn",
            "food_name",
            "serving_measures",
        )

    def get_serving_measures(self, instance):
        return (
            instance.serving_measures if instance.serving_measures is not None else []
        )

class RecognizedFood(BaseModel):
    food_name = models.CharField(max_length=255, null=False)
    serving_measures = JSONField(null=True, blank=True)

Expected behavior
It appears that when trying to generate the schema for this serving_measures field, the drf-spectacular code is passing in the default value of the field (an empty list) as the second parameter to the get_serving_measures method. The drf codebase has

if field.default is not None and field.default != empty and not callable(field.default):            
    default = field.to_representation(field.default)

If I read the DRF docs for SerializerMethodField, I believe these functions need to be written to expect the entire object to be passed in as the second argument (a RecognizedFood in my example), not just the value of that field (the empty list).

The serializer method referred to by the method_name argument should accept a single argument (in addition to self), which is the object being serialized.

Am I misunderstanding something?

[tfranzel]

interesting. apparently SerializerMethodField.to_representation behaves differently from the other fields. this has not come up before because its usually not how this is used. a default makes sense for inbound requests, but SerializerMethodField is always read-only, so both parameters are not even necessary.

i would do this:

class RecognizedFoodSerializer(serializers.ModelSerializer):
    food_name = CharField(required=False)
    serving_measures = serializers.SerializerMethodField()

    class Meta:
        model = RecognizedFood
        fields = (
            "urn",
            "food_name",
            "serving_measures",
        )

    def get_serving_measures(self, instance) -> typing.List[str]:
        return (
            instance.serving_measures if instance.serving_measures is not None else []
        )

i have reproduced your issue but i'm not quite sure whether this is actually missing functionality/bug or a "anti-pattern" we do not yet guard against.

[rmelick-vida]

thanks for the quick response @tfranzel , I will try that out :)

[rmelick-vida]

I spent some more time looking into this today. From my reading of the DRF code, it looks like both their docs and our code are incorrect (which feels kind of unlikely). Here's what I see

During a call to Serializer.to_representation, The fields get_attribute() method is called, and then that return value is passed into the fields to_representation() method. source

def to_representation(self, instance):
        ret = OrderedDict()
        fields = self._readable_fields

        for field in fields:
            try:
                attribute = field.get_attribute(instance)
            except SkipField:
                continue

            check_for_none = attribute.pk if isinstance(attribute, PKOnlyObject) else attribute
            if check_for_none is None:
                ret[field.field_name] = None
            else:
                ret[field.field_name] = field.to_representation(attribute)

        return ret

For a SerializerMethodField, the get_attribute() function is inherited from Field.
source

        def get_attribute(self, instance):
        """
        Given the *outgoing* object instance, return the primitive value
        that should be used for this field.
        """
        try:
            return get_attribute(instance, self.source_attrs)
        except BuiltinSignatureError as exc:

and the get_attribute() helper method that's called by Field.get_attribute() will use the python built in getattr to look up the attribute from the object source

def get_attribute(instance, attrs):
    """
    Similar to Python's built in `getattr(instance, attr)`,
    but takes a list of nested attributes, instead of a single attribute.
    Also accepts either attribute lookup on objects or dictionary lookups.
    """
    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

To me, this seems to show that what is passed in to to_representation should not be the entire object (like their docs describe), but instead should be just the value for that field (like drf-spectacular is assuming). I'll have to take some time to test this, and perhaps open a discussion in DRF to get some clarification from their side.

While investigating this, I happened to see that ModelField acts differently than other fields though. It assumes that an instance of the entire object is passed in to to_representation, not just the value for that field. It seems they handle it by returning the entire object from get_attribute though source.

    def get_attribute(self, obj):
        # We pass the object instance onto `to_representation`,
        # not just the field attribute.
        return obj

    def to_representation(self, obj):
        value = self.model_field.value_from_object(obj)
        if is_protected_type(value):
            return value
        return self.model_field.value_to_string(obj)

I wonder if drf-spectacular would also hit an error if someone specified a default on a ModelField. I do think that it's valid to set defaults on fields for both inbound and outbound. I definitely see the point that in our code, it's redundant though, since we can just handle the None case inside of our method.

[rmelick-vida]

I got a test case going (https://github.com/rmelick-vida/drf-troubleshooting), and confirmed that the DRF docs are correct: the value passed into to_representation on a SerializerMethodField is in fact an instance of the object, not an attribute value.

While stepping through the debugger, I saw what was happening. When the get_attribute helper method was called, the second argument (attrs) is an empty list. So, it falls back to that last line where it just does return instance

If you run the example, and start up the server using manage.py, you can see the difference. When calling the api with curl, DRF passes in an object of type User

 curl -H 'Accept: application/json; indent=4' -u admin:password http://127.0.0.1:8000/users/

{
    "count": 1,
    "next": null,
    "previous": null,
    "results": [
        {
            "url": "http://127.0.0.1:8000/users/1/",
            "username": "admin",
            "email": "admin@example.com",
            "groups": [],
            "object_types": [
                "<User: admin>"
            ]
        }
    ]
}%  

But, when generating a schema, it is passed the list

(env) ➜  drf-quickstart git:(master) ✗ ./manage.py spectacular --file schema.yml
Traceback (most recent call last):
  File "./manage.py", line 22, in <module>
    main()
  File "./manage.py", line 18, in main
    execute_from_command_line(sys.argv)
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/django/core/management/__init__.py", line 419, in execute_from_command_line
    utility.execute()
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/django/core/management/__init__.py", line 413, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/django/core/management/base.py", line 354, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/django/core/management/base.py", line 398, in execute
    output = self.handle(*args, **options)
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/management/commands/spectacular.py", line 50, in handle
    schema = generator.get_schema(request=None, public=True)
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/generators.py", line 257, in get_schema
    paths=self.parse(request, public),
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/generators.py", line 232, in parse
    path, path_regex, path_prefix, method, self.registry
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/openapi.py", line 84, in get_operation
    operation['responses'] = self._get_response_bodies()
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/openapi.py", line 1013, in _get_response_bodies
    return {'200': self._get_response_for_code(response_serializers, '200')}
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/openapi.py", line 1058, in _get_response_for_code
    component = self.resolve_serializer(serializer, 'response')
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/openapi.py", line 1204, in resolve_serializer
    component.schema = self._map_serializer(serializer, direction)
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/openapi.py", line 705, in _map_serializer
    schema = self._map_basic_serializer(serializer, direction)
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/openapi.py", line 769, in _map_basic_serializer
    schema = self._map_serializer_field(field, direction)
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/openapi.py", line 452, in _map_serializer_field
    meta = self._get_serializer_field_meta(field)
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/drf_spectacular/openapi.py", line 748, in _get_serializer_field_meta
    default = field.to_representation(field.default)
  File "/Users/russell/src/other/drf-quickstart/env/lib/python3.7/site-packages/rest_framework/fields.py", line 1882, in to_representation
    return method(value)
  File "/Users/russell/src/other/drf-quickstart/tutorial/quickstart/serializers.py", line 16, in get_object_types
    raise Exception("Unexpected type passed <%s: %s>" % (obj.__class__.__name__, obj))
Exception: Unexpected type passed <list: []>

What do you think, should we open a ticket / issue within the DRF project to ask them why they're inconsistent in the parameters that to_representation expects? It seems like it's almost part of their public API, since they say you need to override it if you define a Custom Field. Or should drf-spectacular change how it handles defaults (maybe skip calling to_representation and just return the value directly, perhaps only for weird fields like ModelField and SerializerMethodField)?

[tfranzel]

@rmelick-vida thanks for taking such a detailed look at this. i gave it a quick glance to provide you with a first opinion. however, i have to have a bit more time to look at it in detail and give you a more definite answer.

what i can tell you already is that the likelihood of them changing such a seasoned API is small unless this is a serious bug, which i'm not yet sure about. also we do call to_representation for a reason. i vaguely remember that we added this to mitigate objects slipping through unprocessed (like datetime objects). so i don't think we should remove it altogether. probably the best course of action is us accounting for those cases and do something smarter there.

just a first opinion which may turn out wrong, but i will schedule some time in the next couple of days for this.

[rmelick-vida]

Hi @tfranzel I wonder if you had a chance to think any further about this.

[tfranzel]

@rmelick-vida sry it fell of the wagon and i was busy. I had a look again and came to the conclusion that simply using the plain value is pretty much the only thing we can do with ModelField. Otherwise we would need to produce a model instance out of thin air. imho this is a lot better than bailing even if the solution is not perfect as with all other fields.

does this fix work for you?

[rmelick-vida]

Hi @tfranzel I think this is the right solution, but we also need to add the special case for SerializerMethodField, in addition to ModelField

[tfranzel]

good point! i missed that one. that should do it

[tfranzel]

closing this issue for now. feel free to comment if anything is missing or not working and we will follow-up.