possible bug with ordering (throws exception)

[sehmaschine]

When I define ordering fields with my ModelViewSet ...

ordering_fields = ("id", "group",)

And my serializer has ...

group = serializers.CharField(source="category", read_only=True)

I expect this ordering field to either work or be ignored. Currently, an exception is being returned ...

django.core.exceptions.FieldError
...
FieldError: Cannot resolve keyword u'group' into field. Choices are: ...

I tried to track this down and had to stop at filters.py, line 158. It seems that remove_invalid_fields always compares the defined ordering_fields to itself (maybe that´s intentional).
https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/filters.py#L158

[tomchristie]

Duplicate of #3324

Currently resolved in master.

[sehmaschine]

Sorry, but I don't think that issue is resolved.

a) IMO there is something missing, because you check for valid_fields is None and valid_fields is "all", but you don't check for the actual defined fields.
b) the defined ordering fields should be compared with both field_name and field.source ... and one of the two should be used. that functionality is missing altogether. please correct me if that is not the intended behaviour.
a) remove_invalid_fields uses getattr(view, 'serializer_class') and it does not work if get_serializer_class is defined with the viewset.

I'm happy to send a PR if you agree with the mentioned issues. If you don't agree, could you please explain how this is supposed to work?

[tomchristie]

Reopening to re-review. First: Can you confirm that you see the same behaviour against master.

[sehmaschine]

I'll do some more tests and get back to you. I'm already having a patch which works well with our environment, but need to do a bit of cleanup.

Just one question about the intended behaviour. Referring to the example above, I should be able to use group as an ordering_field, right? So we need to compare ordering fields with both field names and field sources. Do you agree with that?

[sehmaschine]

Note: I'm able to reproduce the behaviour with master. Added a PR.

[tomchristie]

Oaky I've had a chance to review this.

I expect this ordering field to either work or be ignored.

The expectation here is incorrect. Ordering fields don't get mapped via the serializer source attributes, nor do I want them too - the coupling there would be too strong (If anything the 'all' decision was also misguided)

If you want custom behavior here you'll need to implement this independently - this is not the behavior we want in core.

[tkremmel]

As an API user I would expect to be able to order the result based upon the fields provided in the result set. This means if I receive a result set similar to this one:

"results": [
        {
            "id": 19,
            "status": "1",
            "group": "2",
       },
       {
            "id": 20,
            "status": "1",
            "group": "1",
       }
]

I would expect to be able to order via `ìd,status``, and``group``.

As in this case the group field is a mapping to the category field, this would not be possible with the current DRF implementation:

group = serializers.CharField(source="category", read_only=True)

The API creator would have to explain the API user that the group field is a mapping to the category field and the user should use ?ordering=category instead. This would be highly confusing for me as an API user. The fact that group is a mapping to category is of no interest to the user, and he should not be forced to bother with it.

Therefore, I would ask you to re-open this issue.

[tomchristie]

There's a strong case to make for the Ordering filter to allow explicit mappings between the incoming query parameter and the resulting filter that's applied, but implicit mappings between the filter and the serializer are not the answer.

[tkremmel]

Can you please explain this strong case for explicit mappings between ordering field and model field from the perspective of a user. The user has no clue that the field category even exists.

[xordoquy]

@mr-stateradio the point is that the user would not have to deal with this. It'll just be the developer's work to create this mapping.
DRF doesn't want to be to tied to models things because there are a couple of API that don't use Django's Models

[tomchristie]

^ Yes, although I'd say it more strongly than that - tight coupling between the ordering filter and serializer class is bad for more reasons than just Django's Models (eg. makes alternative third party serializers less viable)

[sehmaschine]

@xordoquy agreed, we don't wanna be tied to model things (but that's exactly what happens right now).

[tkremmel]

@tomchristie The coupling between the ordering filter and the serializer class does not have to be tight or even exclusively. But it should allow the api developer to create an ordering filter for the serializer fields as well as for the model fields. Leave it to the developer to decide what he/she thinks works best for the api users.

[tomchristie]

I'd far rather see us consider gradually deprecating 'all' behavior - having a coupled-to-the-serializer variation avail as third party package but not core.