Document that explicitly declared fields will not have their attributes overridden by `read_only_fields` shortcut.

[TheProdigyFilippo]

class PersonSerializer(serializers.ModelSerializer):
    info = serializers.Field(source='info')

    class Meta:
        model = Person
        read_only_fields = ('info',)

In this case _read_onlyfields is not working (info field is not read only) and we have to add _readonly=True to the field. It's very confusing.

[jpadilla]

@TheProdigyFilippo I'm pretty sure you'll also need to set fields = ('info', ) as the example in the docs. Also, please note that the discussion group is the best place to take this discussion and other usage questions. Thanks!

[TheProdigyFilippo]

@jpadilla Thank for your interest, but you're wrong. Firstly, in the documentation there's no written anywhere that read_only_fields should work only in conjuction with fields. And it shouldn't. Example is EXAMPLE only. If there would be additionaly some crappy global variable in the docs does it mean, that read_only_fields works only when you also use this global variable?

read_only_fields is not working when you add field explicitly to the serializer class and it is a bug. Even adding this field to fields, what you proposed (but what is stupid) is not working. Real life example:

class UserTransferSerializer(serializers.ModelSerializer):
    date = serializers.DateTimeField(format='%s')

    class Meta:
        model = Transfer
        fields = ('date', 'transfer_cost')
        read_only_fields = ('transfer_cost', 'date')

And the result:

s = UserTransferSerializer()
>>> s.fields
{'date': DateTimeField(format='%s'), 'transfer_cost': DecimalField(decimal_places=8, max_digits=18, read_only=True)}

But why the necessity of adding explicitly created Field to the field tuple is stupid? Because you can use exclude which is mutually exclusive with fields and read_only_fields would be useless.

I hope that now you understand the bug. If you have any more questions, I'll do my best to answer, give few more examples and so on. Of course you can also use the discussion group.

[xordoquy]

@TheProdigyFilippo please refrain from using aggressive tone. A few things to keep in mind:

1. This is no paid support and we can't afford to spend hours on each issue.
2. We do see on regular basis examples of DRF bugs that turns out to be user's bug.
3. If you want to speed up this issue, feel free to provide a failing test case we can work on out of the box.

[TheProdigyFilippo]

Dear @xordoquy @jpadilla, excuse me for my aggresive tone. It was response for treating me like a ingoramus and sending away to the docs (I've read it), discussion group and superfast closing this issue by Jose. I'm very sorry again for that. I wanted to help you guys, DRS still is great and your work also is great :)

[xordoquy]

The fact that we close pretty fast issues is linked with our bandwidth. Examples help but still require a lot of our time which is limited. A failing test case definitively helps understanding the issue and figure whether or not it's a DRF bug and whether or not it needs to be fixed.

[tomchristie]

In this case read_only_fields is not working

Correct - if you've specified a field explicitly it doesn't make sense to have it in read_only_fields. We could as an enhancement error loudly?

[TheProdigyFilippo]

if you've specified a field explicitly it doesn't make sense to have it in read_only_fields

Can't agree with you. It would be definitely better (for a coding style) to take advantage of configuration in one place. In my example I'm forced to set read_only property in two places (in field and read_only_fields) or redefine all model fields in serializer and not use read_only_fields to achieve consistency. But it would be great to set read_only property for all wished fields in read_only_fields.

[xordoquy]

I think it'd be more consistent if read_only_fields would also apply to explicitly defined fields. After all, we do need to list them to the fields list so they are taken into account. The major issue I'd see to this change would be the following case:

class PersonSerializer(serializers.ModelSerializer):
    info = serializers.Field(source='info', read_only=False)

    class Meta:
        model = Person
        read_only_fields = ('info',)

[carltongibson]

We had this conversation when the API for 3.0 was being discussed. The summary of it was this:

1. All these convenience flags would be removed, in order to promote a cleaner API.
2. We'd make a special exception for read_only_fields — because it was such a common use-case.

The use-case is when you have a ModelSerializer where you are not explicitly defining fields but quickly want to mark a field as read-only: the obvious example of this is a user field, where you set the user based on request.user and not on submitted data.

We decided that once users stepped onto the path of explicitly defining fields they would be responsible for moving the read-only declaration there, in line with 1 above.

[tomchristie]

It would be definitely better (for a coding style) to take advantage of configuration in one place.

Agree. And if the field has been defined explicitly - that's where.

1. Drop read_only_fields entirely, because it's such an ugly little shortcut.
2. Complain if an explicit field is in read_only_fields.
3. Do nothing.

But having an explicitly defined field that does not match the definition its been given isn't going to happen.

[carltongibson]

Drop read_only_fields entirely, because it's such an ugly little shortcut.

Just on this. It is (an ugly little shortcut) — but it's handy for newcomers to DRF: you can get up and running with ModelSerializer, and get some great results without learning too much. Then you realise need to make your user reference read-only and, without the shortcut, there's instantly a whole load of unfamiliar API that you need to take-in. With the shortcut you can keep progressing and loop back to the Fields API, a good while later.

[tomchristie]

Indeed, so we're not taking (1), and had really already made a decision on that. From my POV that leaves (2) and (3) as valid options.

[twbagustin]

So.. This ain't fixed yet right? Came here through Google.. I don't agree with dropping the shortcut, it's as it was pointed out a "common scenario". If you have a bunch of fields customized say 15 it feels kinda silly to place a read_only flag on each one vs using a shortcut that was actually created for this purpose... Specific use-case: I have a django live production model with a lot of fields, you need the api to throw camelCase field names, so you have to explicitly set each field to rename it, and mark them read_only.. (this points out a feature request maybe, labels dirs on Meta)

For me either go with (1) drop the shortcut if it's not going to work as a shorcut, or (2) properly support the use-case when you have a bunch of explicitly defined fields on a ModelSerializer

[tomchristie]

This ain't fixed yet

It's not that it "ain't fixed", it's that it's not something I'd want to support.

If there's a field explicitly declared on the serializer, then that's what it'll use. You don't need additional shortcuts if you're explicitly declaring it.

If it's the case that you want a subclass of that serializer where you modify a field to be read only then add the field explicitly on the subclass and mark it read only. Job done.

[twbagustin]

There are tons of workarounds for the use-case I'm sure about it, the point is placing a flag that works as a shortcut but it turns out it doesn't on all cases.. See the point? If you ask me.. either one do it right, or one don't, I would drop the flag if I'm not going to support all cases or do it right, just a small grain of salt! Project is awesome tom ;), on my free time I'll try to see the effort involved to support the use case

[xordoquy]

There's not a perfect answer here, both have side effects and will backstab the developer at some point. Either we void parts of the explicitly declared behavior and alter it with some of the Meta's special fields. Or we consider that the explicit behavior take precedence over the Meta.

We have chosen the one that - we feel - demonstrates the most explicit behavior.

[wasinski]

It should be clearly pointed out in the docs that extra_kwargs for fields that were explicitly declared are simply omitted without any warning - even experienced developers make this error.

[konradperzyna]

Correct me if I'm wrong, but it looks like it is enough to explicitly define field in any of the base classes, so any use of extra_kwargs (or read_only_fields) was silently ignored on that field. I guess explicit error (or at least warning) would be a better choice in that case. It might be a nightmare from a maintainer point of view if:

1. setting extra_kwargs requires traversal of the whole inheritance tree in search of explicitly defined fields
2. defining field on the serializer requires a check of extra_kwargs of every class that inherits from it.

Not to forgot that most developers will not be even aware of this silent-ignore behaviour. It took me quite a while to find out why my field is not set as read_only. When you know that even redundancy triggers an error (eg. source set equal to field name), you don't expect silent-ignores.

[carltongibson]

Right, good: the logic of this issue is slowly migrating from "lets drop read_only_fields" to "lets drop extra_kwargs" 🙂

We could take a Note to the serialiser docs below the Additional keyword arguments section saying that both these attributes are superseded by an explicit declaration. (The declaration is canonical. The shortcuts are just helpers, mainly just for quick prototyping.)

There are two general design points I adhere to (from experience) here:

1. Try to favour explicitly declaring fields over the shortcuts.
2. Avoid serialiser inheritance.

Both of these cost time up front but save massively in maintaining your project later. Of the two "Avoid serialiser inheritance" is the most important. Maybe have a project-level base serialiser class (for customising field generation or such) but don't inherit serialisers to avoid typing out fields. That way lies all kinds of issues. (Again, just my opinion from experience.)

[abotte]

Let me step into the discussion and provide an example of when this behavior (Meta.read_only_fields) not apply to explicitly defined field hurts.

I have two classes and one is inherited from another. They both have a same field, which needs to be explicitly specified in the parent one, but read_only status needs to be different for them.

The only way to fix it - violate DRY, which I don't enjoy doing.

[xordoquy]

This should be worked around by overriding serializer's init. read_only_fields or extra_kwargs are an easy way to get this done but not the only way to do so. I second @carltongibson on that issue.

[carltongibson]

... violate DRY ...

I need to write up my views on this.

Yes DRY is great when you're eliminating duplicate logic but for the declarative parts of Django repeating yourself is actually a virtue.

It takes longer to write, but you save time on maintenance later. (Worth observing: This is the exact same case as is made for DRY.)

An occasional quick serialiser subclass can be a good win but, I've seen multiple cases where this has got out of hand.

On one project, serialiser inheritance had become X layers deep such that it was impossible to see where particular fields were coming from. The whole serialiser layer was untouchable — it had essentially become "Append Only" — changes were impossible. It was entirely unmaintainable. The solution was to slowly replace the serialisers (one-by-one) with free-standing versions.

"😱 But DRY", you say.

Serialisers are adjusted on an individual basis. ("But I want to alter all X-type fields": project-level custom field class — yes, DRY there!) This is a fundamental difference in the maintenance profile of this kind of declarative code vs actual code (for want of a better phrase).

For declarative code you want to open the file and see it all there in front of you. You don't want to go looking for the declaration across files. This applies to models and admins and forms and filters (etc) just as much as to serialisers.

IMO 🙂

("😱 But DRY", you say. )

[konradperzyna]

@carltongibson I have to agree with you on the DRY issue. Following it blindly might hurt you. Nevertheless, I still think that silent ignore of extra_kwargs is harmful and I would love to see an error or warning in that case. It might save you from the ghost-hunt debugging session.

@xordoquy

This should be worked around by overriding serializer's init.

You mean setting read_only flag on self.fields?

[carltongibson]

@konradperzyna That's Tom's Option 2 from above. We'd happily review a PR adding an invariant check via a loud assert. (But short of a PR we're going with Option 3 ourselves.)

[jsmedmar]

@carltongibson, thanks for your comments. If I'd like to implement option 2 myself, is there a way to access the explicitly declared fields on a SerializerMetaclass object?

[bwesen]

Sorry to necro this, but I got here by Google, also being hit (now in 2020) of the exact same issue - I have a bunch of read_only_fields and I discovered (to my horror) that they weren't read only!

The bad thing here is that you might start with the read_only_fields list, and then much later add an explicit field override and then suddenly the read_only_fields part stop working, with no warnings.

Would it be that bad to warn about this at startup? All the info is there.

[xordoquy]

Would it be that bad to warn about this at startup

My feeling is that would be a nice thing to have.

[bwesen]

Would it be that bad to warn about this at startup

My feeling is that would be a nice thing to have.

Yeah because this affects security, not as directly as in a security bug in the framework, but indirectly in that people might miss this and think a field is read-only (and not all projects have 100% test coverage..)

[xordoquy]

As Carlton stated before, we're looking forward a PR about that topic.

[Jorl17]

At the very least, this behaviour should be made much more clear in the docs! I think many people assume that adding a field to read_only_fields makes that field read-only, regardless of it being explicitly declared or not !

[Jorl17]

I have created PR https://github.com/encode/django-rest-framework/pull/8228 to help with this.

[nftchance]

Here in 2022, issue still exists.

[samuelchen]

same here, even the field is not explicitly declared.

e.g. I can update fields in read_only_fields, such as username, is_superuser and etc.

class UserSerializer(serializers.ModelSerializer):
    class Meta:
        model = UserModel
        fields = ['id', 'username', 'first_name', 'last_name', 'password',
                  'is_active', 'is_superuser', 'is_staff', 'date_joined',
                  'groups', 'user_permissions']
        read_only_fields = ['username' 'is_superuser', 'is_staff', 'groups',
                            'user_permissions', 'date_joined']
        depth = 1
        extra_kwargs = {'password': {'write_only': True}}

class UserViewSet(mixins.ListModelMixin, mixins.CreateModelMixin,
                  mixins.RetrieveModelMixin, mixins.UpdateModelMixin,
                  mixins.DestroyModelMixin,
                  viewsets.GenericViewSet):
    queryset = UserModel.objects.all()
    serializer_class = UserSerializer
    # permission_classes = [IsAuthenticated]

    def get_queryset(self):
        if self.request.user.is_staff:
            return super().get_queryset()
        else:
            return super().get_queryset().filter(id=self.request.user.id)

    def get_permissions(self):
        if self.action in ['create']:
            permission_classes = [IsAdminUser]
            return [perm() for perm in permission_classes]

        # ['list', 'retrieve', 'update', 'partial_udpate', 'destroy']
        return super().get_permissions()

[auvipy]

I have created PR #8228 to help with this.

I have requested a change on the PR. please try to address that. @samuelchen can you also check the related PR and suggest any additional edge case/test case there as well?

[samuelchen]

I have created PR #8228 to help with this.

I have requested a change on the PR. please try to address that. @samuelchen can you also check the related PR and suggest any additional edge case/test case there as well?

I added the case to #8228 .