Ensure that ModelSerializer fields are in same order than in DRF

[antoineauger]

Description of the Change

For serializers inheriting from ModelSerializer that declare additional fields, the order of field names returned by get_field_names can get unpredictable. It is not necessarily following alphabetical order but can also change between several runs of schema generation (tested with drf-spectacular for instance).

This is especially true when a ModelSerializer has fields = "__all__" and inherits from several serializers.

class MyTestModel(DJAModel):
    verified = models.BooleanField(default=False)
    uuid = models.UUIDField()

class AnotherSerializer(serializers.Serializer):
    ref_id = serializers.CharField()
    reference_string = serializers.CharField()

class MyTestModelSerializer(AnotherSerializer, serializers.ModelSerializer):
    an_extra_field = serializers.CharField()

    class Meta:
        model = MyTestModel
        fields = "__all__"
        extra_kwargs = {
            "verified": {"read_only": True},
        }

I believe this PR could lead to more stable OpenAPI specs, which could be nice for users comparing specs using exact diff.
What do you think of this proposal? 🙇🏻

Checklist

• PR only contains one change (considered splitting up PR)
• unit-test added
• documentation updated
• CHANGELOG.md updated (only for user relevant changes)
• author name in AUTHORS

🛠️ with ❤️ by Siemens

[sliverc]

As far as I see this is not related to DJA but rather to Django REST framework? Or is anything specific we do in DJA making the order of fields unpredictable?

If not, it would be something to look into in Django REST framework directly.

[antoineauger]

@sliverc Maybe I'm missing something here but I don't think that this is a Django REST framework upstream issue because django-rest-framework-json-api overrides the get_field_names method from Django REST framework ModelSerializer:

    def get_field_names(self, declared_fields, info):
        """
        We override the parent to omit explicity defined meta fields (such
        as SerializerMethodFields) from the list of declared fields
        """
        meta_fields = getattr(self.Meta, "meta_fields", [])

        declared = {}
        for field_name in set(declared_fields.keys()):
            field = declared_fields[field_name]
            if field_name not in meta_fields:
                declared[field_name] = field
        fields = super().get_field_names(declared, info)
        return list(fields) + list(getattr(self.Meta, "meta_fields", list()))

But, in the override, the use of set(declared_fields.keys()) does not preserve the order in which we iterate over declared_fields.

So the declared dictionary can be different between two calls to super().get_field_names(declared, info), causing the issue described in this PR description.

Also, even if we would have an alphabetically-ordered fields list, how can we be sure that the returned list is ordered here?

        return list(fields) + list(getattr(self.Meta, "meta_fields", list()))

[sliverc]

I assumed this declared fields is only used as a lookup in DRF itself. I dug some deeper into the DRF code base, and noticed that is actually not always the case. So this could be the issue indeed. Instead of sorting, we should fix this for loop. This is actually much better written as a dict comprehension like the following:

    def get_field_names(self, declared_fields, info):
        """
        We override the parent to omit explicity defined meta fields (such
        as SerializerMethodFields) from the list of declared fields
        """
        meta_fields = getattr(self.Meta, "meta_fields", [])

        declared = {
            field_name: field
            for field_name, field in declared_fields.items()
            if field_name not in meta_fields
        }
        fields = super().get_field_names(declared, info)
        return list(fields) + list(getattr(self.Meta, "meta_fields", list()))

as the dict is as per Python 3.7 ordered, declared fields will be in a predictable order.

Can you check whether this works like this?

[antoineauger]

Can you check whether this works like this?

Yes, it works stability-wise meaning that the generated OpenAPI spec is the same over multiple runs 👌🏻

The only thing that I want to point out (don't know if it is a problem) is that the properties might be not alphabetically sorted (on the contrary to the required list). A simple example:

class MyModel(models.Model):
    uuid = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
    verified = models.BooleanField(
        default=False,
        help_text="My help text",
    )

class MySerializer(serializers.ModelSerializer):
    """Description"""
    an_extra_field = serializers.CharField(help_text="Help text")

    class Meta:
        model = MyModel
        resource_name = "myModel"
        fields = "__all__"
        extra_kwargs = {
            "verified": {"read_only": True},
        }

will lead to an OpenAPI spec like:

...
components:
  schemas:
    MyModel:
      type: object
      description: Description
      properties:
        uuid:
          type: string
          format: uuid
          readOnly: true
        an_extra_field:
          type: string
          description: Help text
        verified:
          type: boolean
          readOnly: true
          description: My help text
      required:
      - an_extra_field
      - uuid
      - verified
 ...

[sliverc]

I just took a quick look and could not find where the required fields are getting sorted. Do you see where this should happen?

In any case, as far as I see, OpenAPI does not specify that the properties need to be alphabetically ordered. And DRF returns the fields in the order how they have been defined in the serializer. So we should return in the same order as DRF does.

Could you update the PR to reflect the changes in get_field_names as suggested? It would be great if you could also add a test within tests/test_serializers.py where it checks that the order of the field name is as expected? Best use a use case of a serializer you used to reproduce your issue (e.g. using __all__ as fields).

[antoineauger]

Thanks for the guidance @sliverc. To answer your questions/remarks:

I just took a quick look and could not find where the required fields are getting sorted. Do you see where this should happen?

Nevermind, I think this is specific to the drf-spectacular library that I'm also using (see here).

In any case, as far as I see, OpenAPI does not specify that the properties need to be alphabetically ordered. And DRF returns the fields in the order how they have been defined in the serializer. So we should return in the same order as DRF does.

Fair enough, this also makes sense to me.

Could you update the PR to reflect the changes in get_field_names as suggested? It would be great if you could also add a test within tests/test_serializers.py where it checks that the order of the field name is as expected? Best use a use case of a serializer you used to reproduce your issue (e.g. using all as fields).

Sure thing, I'll ping you when it is ready for review.

[antoineauger]

@sliverc I just updated the description with the latest information and cleaned a bit the commit history.

This is ready for review 🙏🏻

[sliverc]

Thanks a lot for working on this. This looks great.

[antoineauger]

@sliverc Is it planned to release a new version of django-rest-framework-json-api soon so we can benefit from this upstream fix? 😄