Incorrect handling of `sort` field in schema generation

[atkallie]

Description of the Bug Report

I believe that the handling of the sort field in the schema generation is currently incorrect. (Please correct me if I'm wrong.)

The custom AutoSchema class calls self._get_sort_parameters(path, method) for all GET & HEAD requests:

if method in ["GET", "HEAD"]:
    ...
    parameters += self._get_sort_parameters(path, method)
    ...

(Issue 1) The JSON:API spec says, "A server MAY choose to support requests to sort resource collections according to one or more criteria (“sort fields”)." The way that this is currently handled will naively add the sort query parameter irrespective of whether or not rest_framework_json_api.filters.OrderingFilter is enabled. Also, this will add a sort query parameter for requests for single resources as well (though I think DRF also does this).

(Issue 2) The Django REST Framework's OrderingFilter class implements a method for get_schema_operation_parameters that will also add a sort query parameter to the schema for the operation. This leads to duplicate sort query parameters listed for the operation.

def get_schema_operation_parameters(self, view):
        return [
            {
                'name': self.ordering_param,
                'required': False,
                'in': 'query',
                'description': force_str(self.ordering_description),
                'schema': {
                    'type': 'string',
                },
            },
        ]

Current Workaround

I've currently worked around this by overriding the OrderingFilter class:

from rest_framework_json_api.filters import OrderingFilter as DJAOrderingFilter

class OrderingFilter(DJAOrderingFilter):
    def get_schema_operation_parameters(self, view):
        return [{"$ref": "#/components/parameters/sort"}]

...and the AutoSchema class:

from rest_framework_json_api.schemas.openapi import AutoSchema as DJAAutoSchema

class AutoSchema(DJAAutoSchema):
        def _get_sort_parameters(self, path, method):
            return []

...and configuring these in my REST_FRAMEWORK config so that the sort parameter is conditionally added based on whether or not the OrderingFilter is enabled, but still uses the schema provided by this lib. Just thought I'd include this as a potential fix to the problem. However, this patch will still include a sort query parameter for requests for single resources (mentioned above).

[sliverc]

Thanks for your report. I agree this is a bug. The difference between OrderingFilter.get_schema_operation_parameters and #/components/parameters/sort is just the description, it seems. Therefore, I am asking myself, is it worth creating our own OrderingFiltering class with just another description? I think the easiest way would be to simply remove _get_sort_parameters and remove reference #/components/parameters/sort to address this issue. What are your thoughts?

[atkallie]

Of course. Happy to help out. Thanks for the great work on this lib.

Yeah, I think you're right about the two sort specs being more or less the same. DRF allows you to override the description by setting the ordering_description on the filter, so that isn't an issue either.

However, there's already an OrderingFilter class that extends DRF's OrderingFilter class, which is imposing some additional validation logic on top of what DRF does.

That being said, my initial thinking would be to:

1. Remove the _get_sort_parameters method in the SchemaGenerator class (as well as this line)
2. Add the following method to the custom OrderingFilter class to conditionally render the sort field schema operation parameters since I'm not sure if abiding by the spec constraint of only including sort for collection resources will be prioritized (or even acknowledged as a bug) by the DRF contributors:

def get_schema_operation_parameters(self, view):
    # ViewSets have an explicitly defined action, which we can inspect
    if getattr(view, 'action', None) != 'list':
        return []
    else:
        # Return the ordering schema operation parameters for resource collections only
        return super().get_schema_operation_parameters(view)

Note 1: I've used super().get_schema_operation_parameters(view) instead of [{"$ref": "#/components/parameters/sort"}] in the return above given your observation that the two sort specs are pretty much the same. If we do that, then we can also get rid of the corresponding schema definition here.

Note 2: The above if statement naively determines if this is a list view by exploiting the fact that ViewSets are used exclusively in this lib when listing collection resources (I think - but let me know if I'm mistaken). We can't use the DRF is_list_view util (from rest_framework.schemas.utils import is_list_view) because we don't have access to the method or path in get_schema_operation_parameters.

Let me know what you think.

[sliverc]

1. I fully agree with this point. _get_sort_parameters needs to be removed with the scheme definition. Additionally, I would set the ordering_description to what we use today in the corresponding scheme definition. Would you be open to work on a PR for this?
2. This point is a bit more tricky. The reason is that simply removing sort parameter from the schema for single resource endpoints actually does not mean that the sort parameter cannot be used on single resource object endpoints. And when it is used, no error is thrown and the query to the database will actually be different. I haven't had time yet to look deeper into this, but I have a feeling that an error needs to be thrown when query param sort is used on a single resource object. We have to be careful though in terms of regressions.

I recommend we first tackle point 1 and discuss point 2 more deeply afterward.

[atkallie]

RE: Schema/Query Inconsistency - That's a good point.
RE: Pull Request - Of course, no problem. Just created a pull request: #1124. Take a look when you have a minute and let me know if that's what you had in mind.