Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix serializer multiple inheritance bug #6980

Merged
merged 3 commits into from Dec 12, 2019

Conversation

@rpkilby
Copy link
Member

@rpkilby rpkilby commented Oct 9, 2019

Fixes #5798. As pointed out by @svilendobrev, the current code reverses the field order twice, defeating the point of reversing the first place. Thanks to @mwhansen for providing a minimal test case.

Note that I ran the test locally to verify the failure. Also, even though this is a relatively minor bugfix, I'm adding to the 3.11 milestone since it's technically a change in behavior to the public API.

@carltongibson
Copy link
Collaborator

@carltongibson carltongibson commented Oct 9, 2019

Lets just make sure we triple check this before changing the behaviour here.

This logic has been in place as-is since day 0

@xordoquy
Copy link
Collaborator

@xordoquy xordoquy commented Oct 9, 2019

I have some doubt about how it'll behave if we define a field in the grand parent and override it in a parent.
Will try to write a test case for that.

@xordoquy
Copy link
Collaborator

@xordoquy xordoquy commented Oct 9, 2019

Scratch my previous comment, it seems correct already.
The only side effect I can see now is the order of the fields will be different which may have some impact when inherited serializer is displayed as a form.

@carltongibson
Copy link
Collaborator

@carltongibson carltongibson commented Oct 9, 2019

OK, so it's just exactly what we say in a release note... "...set fields explicitly if you need to retain the previous order..."?

@xordoquy
Copy link
Collaborator

@xordoquy xordoquy commented Oct 9, 2019

@carltongibson that would apply to ModelSerializer yes, but does it also work with regular Serializer ?

- Test declared filter ordering
- Test multiple inheritance
@rpkilby rpkilby force-pushed the serializer-multiple-inheritance branch 3 times, most recently from 9984fa2 to 6d84cb7 Oct 11, 2019
@rpkilby
Copy link
Member Author

@rpkilby rpkilby commented Oct 11, 2019

I added a test for declarative field ordering. The original version of the PR did break this, but the current version respects both mro and the field ordering.

@carltongibson
Copy link
Collaborator

@carltongibson carltongibson commented Oct 11, 2019

Nice. Good effort @rpkilby.

I'm going to have to sit down with a coffee over the weekend and make sure I'm 100% clear on why this works. 😀

@rpkilby
Copy link
Member Author

@rpkilby rpkilby commented Oct 11, 2019

This is a bit longwinded, but as additional context.. let's use:

class Base(serializers.Serializer):
    f1 = serializers.CharField()
    f2 = serializers.CharField()

class A(Base):
    f3 = serializers.CharField()

class B(serializers.Serializer):
    f3 = serializers.CharField()
    f4 = serializers.CharField()

class TestSerializer(A, B):
    f2 = serializers.CharField()
    f5 = serializers.CharField()

Of note, f2 is a parent/child override, and f3 a sibling override. The expected field order is f1 through f5 in sequence. As to how this is processed, what's confusing is that we're not actually processing these in forward or reverse mro. The mro itself is:

  • [TestSerializer, A, Base, B, Serializer, etc...]

And if you process the fields by mro, you would actually get:

  • forward mro: [f2, f5, f3, f1, f4]
  • reverse mro: [f3, f4, f1, f2, f5]

Instead, the bases are just the direct base classes (so, [A, B]). The base classes have already resolved their declared fields, so A already has the correct field order ([f1, f2, f3]).

As to the existing implementation, it made sense to process the bases in reverse order and prepend, since the childmost fields have alread been resolved. Effectively, the code builds fields as:

fields = [f2, f5]  # TestSerializer
fields = [f3, f4, f2, f5]  # B, TestSerializer
fields = [f1, f2, f3, f3, f4, f2, f5]  # A, B, TestSerializer

However, there's no reason to process in reverse order. The bases could have been collected in forwards order, and the TestSerializer appended last.

The existing implementation is mostly correct. When the fields are collapsed into the ordered dict, the correct order is maintained, and the f2 from TestSerializer is used instead of the one from Base. However, this breaks for the sibling f3, since B.f3 is after A.f3*. When initializing a dict with duplicate keys, the last entry is used.

* although processed in reverse order, the base fields are prepended, placing B after A


For the first PR implementation, I effectively unreversed the processing of the bases, so

fields = [f2, f5]  # TestSerializer
fields = [f2, f5, f3, f4]  # TestSerializer, B
fields = [f2, f5, f3, f4, f3, f1, f2]  # TestSerializer, B, A

This had the opposite effect of the above. While A.f3 is correctly after B.f3, the field order is broken, and the Base.f2 is used instead of TestSerializer.f2.


As to the current PR implementation, base_fields are processed separately since the for loop has been combined into a nested comprehension. But in effect, we're building:

fields = [f2, f5]  # TestSerializer
fields = [f1, f2, f3] + [f2, f5]  # A + TestSerializer
fields = [f1, f2, f3, f4] + [f2, f5]  # A, B + TestSerializer

The result is similar to the existing implementation, however the visit/known helper prevents duplicate fields from subsequent bases (B.f3 is skipped above) from being appended to the fields list and overriding base fields that have higher precedence (A.f3).
Note that base_fields contains Base.f2, even though this overridden by TestSerializer.f2. This is to maintain proper field ordering for parent/child overrides. Otherwise, the order would be
[f1, f3, f4, f2, f5].

I guess the best way of explaining the PR is that there are two changes:

  • the bases for loop has been collapsed into a nested comprehension.
  • the field name is now visit()ed and instead of name not in attrs its name not in known (known also includes attrs). This is what actually fixes the bug.

@tomchristie
Copy link
Member

@tomchristie tomchristie commented Dec 12, 2019

Neato bit of fixing, that.
I'mma keep frowning on multiple serializer inheritance, but yup, nice one!

@tomchristie tomchristie merged commit b8c369c into encode:master Dec 12, 2019
1 check passed
@rpkilby rpkilby deleted the serializer-multiple-inheritance branch Dec 12, 2019
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this issue Nov 17, 2020
* Expand declared filtering tests

- Test declared filter ordering
- Test multiple inheritance

* Fix serializer multiple inheritance bug

* Improve field order test to check for field types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants