Raise error when calling serializer.data after serializer.is_valid fails

[robcharlwood]

Checklist

• I have verified that that issue exists against the master branch of Django REST framework.
• I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• This is not a usage question. (Those should be directed to the discussion group instead.)
• This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
• I have reduced the issue to the simplest possible case.
• I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

I have included the steps I took below via a terminal. This seems to affect both standard serializers and model serializers.

Python 3.6.6 (default, Sep 12 2018, 02:19:14) 
[GCC 6.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from rest_framework import serializers
>>> 
>>> class Foo(serializers.Serializer):
...     foo = serializers.CharField(required=True)
...     bar = serializers.IntegerField(required=True)
... 
>>> data = {}
>>> s = Foo(data=data, many=False)
>>> s.is_valid()
False
>>> s.data
{}
>>> 
>>> data_multi = [{}, {}, {}]
>>> s_multi = Foo(data=data_multi, many=True)
>>> s_multi.is_valid()
False
>>> s_multi.data
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/rest_framework/fields.py", line 454, in get_attribute
    return get_attribute(instance, self.source_attrs)
  File "/usr/local/lib/python3.6/site-packages/rest_framework/fields.py", line 100, in get_attribute
    instance = instance[attr]
KeyError: 'foo'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/local/lib/python3.6/site-packages/rest_framework/serializers.py", line 768, in data
    ret = super(ListSerializer, self).data
  File "/usr/local/lib/python3.6/site-packages/rest_framework/serializers.py", line 266, in data
    self._data = self.get_initial()
  File "/usr/local/lib/python3.6/site-packages/rest_framework/serializers.py", line 603, in get_initial
    return self.to_representation(self.initial_data)
  File "/usr/local/lib/python3.6/site-packages/rest_framework/serializers.py", line 686, in to_representation
    self.child.to_representation(item) for item in iterable
  File "/usr/local/lib/python3.6/site-packages/rest_framework/serializers.py", line 686, in <listcomp>
    self.child.to_representation(item) for item in iterable
  File "/usr/local/lib/python3.6/site-packages/rest_framework/serializers.py", line 517, in to_representation
    attribute = field.get_attribute(instance)
  File "/usr/local/lib/python3.6/site-packages/rest_framework/fields.py", line 475, in get_attribute
    raise type(exc)(msg)
KeyError: "Got KeyError when attempting to get a value for field `foo` on serializer `Foo`.\nThe serializer field might be named incorrectly and not match any attribute or key on the `dict` instance.\nOriginal exception text was: 'foo'."
>>> 

Expected behavior

I would expect that when calling the data attribute on the "many" serializer that the input data would be returned, so in the example above, I would expect the following output from data

[{}, {}, {}]

Actual behavior

Actual behaviour raises the exception included above. It seems that the get_attribute method naively always assumes that an attribute will be present in data.

I would really appreciate your thoughts on this.
Thanks for this amazing framework, big fan!! :)

[xordoquy]

It doesn’t make sense to call .data on an invalid serializer

[tomchristie]

I guess we may want to consider raising an explicit error for this usage.
We could start with a pull request that raises the error when used like this, and ensure that it's not currently being triggered anywhere.

[robcharlwood]

@tomchristie yes, this sounds sensible. If it's not intended practice to access that attribute on an invalid serializer, it should explicitly warn the engineer. Currently, it looks like a bug since accessing the same attribute on a serializer where many=False returns data without any error at all. 👍

[turfaa]

I guess we may want to consider raising an explicit error for this usage.
We could start with a pull request that raises the error when used like this, and ensure that it's not currently being triggered anywhere.

@tomchristie Do you mean it should raise an error when serializer.data is called after failing serializer.is_valid()? If yes, should it be able to call serializer.data before serializer.is_valid() is called?

[xordoquy]

Do you mean it should raise an error when serializer.data is called after failing serializer.is_valid()?

Yes

should it be able to call serializer.data before serializer.is_valid() is called

most probably, otherwise it wouldn't be able to serialize data any more.

serializer.data is the serialized data from the instance. Instance can be provided during serializer's creation (the read action) or after a serializer has been saved (result of the write action).

[turfaa]

Do you mean it should raise an error when serializer.data is called after failing serializer.is_valid()?

Yes

should it be able to call serializer.data before serializer.is_valid() is called

most probably, otherwise it wouldn't be able to serialize data any more.

serializer.data is the serialized data from the instance. Instance can be provided during serializer's creation (the read action) or after a serializer has been saved (result of the write action).

Ok, I am working on it.

What about serializer.validated_data? I assume it should raise an error too if it's called after failing serializer.is_valid()

[xordoquy]

What about serializer.validated_data? I assume it should raise an error too if it's called after failing serializer.is_valid()

Current behavior feels ok to me: validated_data will be {} if is_valid() fails.

[knivets]

Is there an update on this issue? I think this scenario should result in exception because .data will potentially contain invalid data.

[tomchristie]

Closing as per #6528. I think having slightly under defined behaviour isn't necessarily ideal, but isn't awful either.