Serializer does not respect Python's C3 MRO

[iamahuman]

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. (Fix serializer multiple inheritance bug #6980 and Inheriting multiple Serializers gives unexpected order of overloading of fields #5798 are related.)
• 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

from rest_framework import serializers

# dummy fields for demonstration
class AllProductsField(serializers.Field): pass
class SupportedProductsField(serializers.Field): pass

class CommonSerializer(serializers.Serializer):
    product = AllProductsField()

class CustomerAccess(CommonSerializer):
    def validate_product(self, value):
        return do_something(value)

class TicketSerializer(CommonSerializer):
    product = SupportedProductsField()

class CustomerTicketSerializer(CustomerAccess, TicketSerializer):
    pass

print('[' + ' > '.join(x.__name__ for x in CustomerTicketSerializer.mro()[:4]) + ']')
print(str(CustomerTicketSerializer()))

Expected behavior

[CustomerTicketSerializer > CustomerAccess > TicketSerializer > CommonSerializer]
CustomerTicketSerializer():
    product = SupportedProductsField()

Actual behavior

[CustomerTicketSerializer > CustomerAccess > TicketSerializer > CommonSerializer]
CustomerTicketSerializer():
    product = AllProductsField()

Remarks

For compatibility I think this behavior shall be kept for a while, but not respecting Python MRO may turn out to be astonishing for some users and even lead to potential security problems (e.g. choice-limiting queryset not properly overridden).

[tomchristie]

When I was working through this, my expected behaviour was product = AllProductsField(), because CustomerAccess > TicketSerializer.

So I'd suggest it's a usage to be avoided, given that it's unclear which behaviour you'd expect to see.

[iamahuman]

When I was working through this, my expected behaviour was product = AllProductsField(), because CustomerAccess > TicketSerializer.

Thanks for sharing your thoughts! I respect your idea on this.

So I'd suggest it's a usage to be avoided,

Are you open to having this usage documented as a pattern to be avoided?

If this is the case, I'd be happy to make a pull request on it. Perhaps it might be a good idea to also detect and explicitly warn the user about this at runtime.

given that it's unclear which behaviour you'd expect to see.

IMHO some folks used to how Python resolves multiple base classes may find the expected behavior stated above more "intuitive."

For anyone not familiar with MRO, suppose the serializers.Serializer inheritance is removed:

from rest_framework import serializers

# dummy fields for demonstration
class AllProductsField(serializers.Field): pass
class SupportedProductsField(serializers.Field): pass

class CommonSerializer:
    product = AllProductsField()

class CustomerAccess(CommonSerializer):
    def validate_product(self, value):
        return do_something(value)

class TicketSerializer(CommonSerializer):
    product = SupportedProductsField()

class CustomerTicketSerializer(CustomerAccess, TicketSerializer):
    pass

print('product =', CustomerTicketSerializer().product)

This code would produce product = SupportedProductsField().

Although this case may be solved by omitting the base class from the CustomerAccess mixin, I'm afraid some users might get tripped by this edge case. Plus there may be other instances that may not be immediately obvious.

[tomchristie]

Personally I think this kind of usage invites confusion either ways around, and is best avoided.

[iamahuman]

Personally I think this kind of usage invites confusion either ways around, and is best avoided.

Do you think if this (anti-)pattern is worth being documented?

If this is the case, I'd be happy to make a pull request on it. Perhaps it might be a good idea to also detect and explicitly warn the user about this at runtime.

[tomchristie]

Not particularly, no. I think it's just one of those things that comes with the territory.
Python's a dynamic language that lets you do all sorts of weirdness if you want too.