should required option be valid for output?

[Catstyle]

i had upgraded from 2.4.4 to 3.0, and fix some incompatable things, but still i encountered some troubles
assumed we have a Serializer like this:

class Foo(serializers.Serializer):

    items = serializer.ListField(child=XXX)
    bar = serializers.IntegerField(required=False)

in my views i called

resp = service.get_data(...)
# resp = {'items': [...], 'foo': 'xxx'}
serializer = self.get_serializer(resp)
return Response(serializer.data)

then, i got errors
1, TypeError: 'builtin_function_or_method' object is not iterable
after some tracing, i found that fields.py:get_attribute handling the dict after getattr, but unfortunately i got key name the same with dict.items method name, so i think we should check __getitem__ value first then the getattr value
fixed 81d0b74
2, AttributeError: 'dict' object has no attribute 'bar'
after i fixed above error, i ran into another error, it seems that required=False doesn't work
it started from serializer.data -> serializer.to_representation, and iterate all field.get_attribute(instance)
so should we pass field.required to get_attribute and return None if required is False?

[xordoquy]

There's an explanation in the release notes about how required now works. You probably miss the allow_null=True

[Catstyle]

@xordoquy
thanks, but you can see that i pass resp = {'items': [...], 'foo': 'xxx'} to Foo, it has no bar in resp
so with required=False, i assume it will not complain if it cannot find bar in resp, which it does
AttributeError: 'dict' object has no attribute 'bar'

[xordoquy]

@Catstyle Indeed, my bad. Would it be possible to have the stack traces for both points ?

[Catstyle]

sure, this time we do it simply

In [20]: class Foo(serializers.Serializer):

    x = serializers.IntegerField(required=False)
   ....:     

In [21]: s = Foo({'x': 1})

In [22]: s.data
Out[22]: ReturnDict([('x', 1)])

In [23]: s = Foo({'y': 1})

In [24]: s.data
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-24-b0eddb6f5f12> in <module>()
----> 1 s.data

.../rest_framework/serializers.pyc in data(self)
    420     @property
    421     def data(self):
--> 422         ret = super(Serializer, self).data
    423         return ReturnDict(ret, serializer=self)
    424 

.../rest_framework/serializers.pyc in data(self)
    175         if not hasattr(self, '_data'):
    176             if self.instance is not None and not getattr(self, '_errors', None):
--> 177                 self._data = self.to_representation(self.instance)
    178             elif hasattr(self, '_validated_data') and not getattr(self, '_errors', None):
    179                 self._data = self.to_representation(self.validated_data)

.../rest_framework/serializers.pyc in to_representation(self, instance)
    385 
    386         for field in fields:
--> 387             attribute = field.get_attribute(instance)
    388             if attribute is None:
    389                 ret[field.field_name] = None

.../rest_framework/fields.pyc in get_attribute(self, instance)
    276         that should be used for this field.
    277         """
--> 278         return get_attribute(instance, self.source_attrs)
    279 
    280     def get_default(self):

.../rest_framework/fields.pyc in get_attribute(instance, attrs)
     68                 return instance[attr]
     69             except (KeyError, TypeError, AttributeError):
---> 70                 raise exc
     71         if is_simple_callable(instance):
     72             instance = instance()

AttributeError: 'dict' object has no attribute 'x'

[Catstyle]

it seems that Serializer(instance) and Serializer(data=dict) is different
then i need add more logic to check whether the resp is instance or dict

---

updated
even if i pass foo instance, still this error 2

In [50]: class FooObject(object):
   ....:     
   ....:     def __init__(self, y):
   ....:         self.y = y
   ....:         

In [51]: foo = FooObject(1)

In [52]: s = Foo(foo)

In [53]: s.data
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-53-b0eddb6f5f12> in <module>()
----> 1 s.data

.../rest_framework/serializers.pyc in data(self)
    420     @property
    421     def data(self):
--> 422         ret = super(Serializer, self).data
    423         return ReturnDict(ret, serializer=self)
    424 

.../rest_framework/serializers.pyc in data(self)
    175         if not hasattr(self, '_data'):
    176             if self.instance is not None and not getattr(self, '_errors', None):
--> 177                 self._data = self.to_representation(self.instance)
    178             elif hasattr(self, '_validated_data') and not getattr(self, '_errors', None):
    179                 self._data = self.to_representation(self.validated_data)

.../rest_framework/serializers.pyc in to_representation(self, instance)
    385 
    386         for field in fields:
--> 387             attribute = field.get_attribute(instance)
    388             if attribute is None:
    389                 ret[field.field_name] = None

.../rest_framework/fields.pyc in get_attribute(self, instance)
    276         that should be used for this field.
    277         """
--> 278         return get_attribute(instance, self.source_attrs)
    279 
    280     def get_default(self):

.../rest_framework/fields.pyc in get_attribute(instance, attrs)
     68                 return instance[attr]
     69             except (KeyError, TypeError, AttributeError):
---> 70                 raise exc
     71         if is_simple_callable(instance):
     72             instance = instance()

AttributeError: 'FooObject' object has no attribute 'x'

[tomchristie]

s = Foo({'y': 1})
s.data

The 'required' option indicates if the input is required. Not which attributes are expected to exist on outputting the representation.
What output were you expecting to see here?...

{'x': None} or {}?

[tomchristie]

AttributeError: 'FooObject' object has no attribute 'x'

Indeed. It doesn't.
Again, the 'required' option indicates if it is required input.
Not sure exactly what the 2.x behavior was (what output would you see then, what output were you hoping to see? x field not present, or x field set to None?)

It's possible that we could decide that simply returning None in this case is better, but for many cases that'd actually just end up masking user errors (eg mistype a field and just silently get None on the output)

[Catstyle]

i'd prefer {}, since it will reduce the output :)
yeah, i got what you mean, but sometimes these error still will happen
thought if resp is from mongodb...bar field is not required but bar is not exist in resp
or resp is a object but accidentally has no bar attribute
other people in other language may not want to write logic based on if a field is exist in json response or not, so i take it still need to be {'x': None}

[tomchristie]

So I'd probably recommend a new base serializer class with an overridden to_representation to support this style.

https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/serializers.py#L379-L393

For most of our users having a field that doesn't correspond to an attribute in the object is due to an application error. The softer mongodb style where fields may simply not exist isn't our standard case and it's easy enough to deal with by using a slightly modified base case (eg catch those attribute errors).

However it would still be helpful to know what the 2.x behavior was here. Ie was it {'x': None}, or was it an error?

Could still be open to making some changes here but probably not what we want for the general case.

[Catstyle]

pip install djangorestframework==2.4.4 -U

In [1]: from rest_framework import serializers

In [2]: class Foo(serializers.Serializer):

    x = serializers.IntegerField(required=False)
   ...:     

In [3]: s = Foo({'y': 1})

In [4]: s.data
Out[4]: {'x': None}

sure, i can do some compatible changes, just like you recommend
i just want to ensure if this situation is reasonable

[tomchristie]

Seems more by accident of implementation than by design.

2.x will use None for dictionaries and will error for objects: https://github.com/tomchristie/django-rest-framework/blob/version-2.4.x/rest_framework/fields.py#L56-59

[tomchristie]

Okay, so if you need None or missing keys on output we're going to treat that as a custom case for now. Override .to_representation and catch either KeyError or AttributeError as appropriate.

[tomchristie]

We can re-assess our guidance on this and/or our error messaging for this if it comes up again.