Django-rest-framework: [regression] Serializers not returning read_only_fields in response

[bacongobbler]

Hi!

I'm in the middle of migrating us over to 3.x (yay!) and I've hit a bump in the road with our migration. In DRF 2.x, we defined a Serializer for Django's User object like so:

class UserSerializer(serializers.ModelSerializer):

    class Meta:
        model = User
        read_only_fields = ('is_superuser', 'is_staff', 'groups',
                            'user_permissions', 'last_login', 'date_joined')

    @property
    def data(self):
        """Custom data property that removes secure user fields"""
        d = super(UserSerializer, self).data
        for f in ('password',):
            if f in d:
                del d[f]
        return d

However, given the following UserSerializer, I'm finding that our test suite is catching that the read_only_fields are not being sent in the response:

class UserSerializer(serializers.ModelSerializer):
    class Meta:
        model = User
        fields = ('email', 'username', 'password', 'first_name', 'last_name')
        read_only_fields = ('is_superuser', 'is_staff', 'groups',
                            'user_permissions', 'last_login', 'date_joined', 'is_active')
        extra_kwargs = {'password': {'write_only': True}}

    def create(self, validated_data):
        now = timezone.now()
        user = User(
            email=validated_data['email'],
            username=validated_data['username'],
            first_name=validated_data['first_name'],
            last_name=validated_data['last_name'],
            last_login=now,
            date_joined=now,
            is_active=True
        )
        user.set_password(validated_data['password'])
        # Make the first signup an admin / superuser
        if User.objects.all().count() == 0:
            user.is_superuser = user.is_staff = True
        else:
            user.is_superuser = user.is_staff = False
        user.save()
        return user

Which resulted in a failure in our test suite:

><> venv/bin/python manage.py test api.AuthTest
WARNING Cannot synchronize with etcd cluster
Creating test database for alias 'default'...
F...
======================================================================
FAIL: test_auth (api.tests.test_auth.AuthTest)
----------------------------------------------------------------------
Traceback (most recent call last):
    self.assertEqual(response.data.get('is_active'), True, response.data)
AssertionError: ReturnDict([(u'email', u'[email protected]'), (u'username', u'newuser'), (u'first_name', u'Otto'), (u'last_name', u'Test')])

Is this intended behaviour? If not, I'd be happy to take a crack at a PR and fix it up!

[carltongibson]

I think you should have the read only fields also listed in fields.

[carltongibson]

Have the read only fields also in fields follows the example in the docs so I'm going to close this on that basis.

Please follow up if you still think something is missing and we can re-assess.

[tomchristie]

At some point we could improve this by raising an error if a field is listed in read_only_fields that _isn't_ actually a field on the serializer.

[bacongobbler]

Sounds good. I'd be more than happy to contribute a fix so others do not fall into the same issue.

[kylebebak]

I just fell on this issue as well, and yeah, it definitely came as a surprise. Raising an exception for including any field in read_only_fields that's not part of fields as well would be excellent.

[xordoquy]

Imho, this is a good showcase of why we should remove read_only_fields in favor of extra_args with the read_only property

[kylebebak]

That makes sense to me, based on what's in the docs. It would make it impossible to get caught by this particular error.

[tomchristie]

I'd be happy enough with that, tho wouldn't be too surprised to see some complaints if we did. Perhaps if someone feels strongly enough about it to issue a PR that puts read_only_fields on the deprecation path then we could consider it.

[carltongibson]

I've argued numerous times for read_only_fields. This was on the basis that it's such a common use-case that having the extra API greatly reduces the total surface area a beginner (in particular) needs to take-in to be productive.

Requiring an awareness of the kwargs available in the Field API, in order to achieve such a universal requirement, steps up the learning curve significantly.

The base requirement is include all your fields in fields!. Emphasising that again in the tutorial and serialiser docs would be worthwhile. Adding a check to raise an exception, yes too. Removing read_only_fields — whilst I see the argument — not so sure.

Aside: Since serialisers have been printable I essentially never use extra_args: it's less work — especially over time — to print the serialiser in the REPL, copy the relevant definition to the class and adjust it as needed there. read_only_fields is still handy for quick prototyping, but it wouldn't be a loss personally if it vanished. I'm half of the mind to drop all these conveniences if we start getting rid of them.