15five / django-scim2

A SCIM 2.0 Service Provider Implementation (for Django)
http://django-scim2.readthedocs.io/
Other
77 stars 27 forks source link

Sorting based on modified field not supported in get_many method #107

Closed omidraha closed 1 year ago

omidraha commented 1 year ago

Describe the bug

The code sorts the results based on the lookup_field , and there is no way to sort the results based on any other field, especially the modified field.

    def get_many(self, request):
        query = request.GET.get('filter')
        if query:
            return self._search(request, query, *self._page(request))

        extra_filter_kwargs = self.get_extra_filter_kwargs(request)
        extra_exclude_kwargs = self.get_extra_exclude_kwargs(request)
        qs = self.model_cls.objects.filter(
            **extra_filter_kwargs
        ).exclude(
            **extra_exclude_kwargs
        )
        qs = self.get_queryset_post_processor(request, qs)
        qs = qs.order_by(self.lookup_field)
        return self._build_response(request, qs, *self._page(request))

To Reproduce Steps to reproduce the behavior...

Expected behavior We expect to be able to sort the results based on any field, especially the modified field.

Stacktrace

Additional context

By swapping these two lines of code, the ordering of the queryset by lookup_field will be performed before applying any custom filtering or modifications via get_queryset_post_processor(),

So the solution is to replace the following code:

qs = self.get_queryset_post_processor(request, qs)
qs = qs.order_by(self.lookup_field)

with this code:

qs = qs.order_by(self.lookup_field)
qs = self.get_queryset_post_processor(request, qs)
logston commented 1 year ago

Hi @omidraha, thanks for pointing this out. Sort was listed as optional in the SCIM2.0 spec and thus not a major focus when developing this library.

Your fix does, at first glance, appear to fix the issue. I'd be more than happy to review a PR with this change if you are willing to issue it. Otherwise, I'll have to address this as my time allows.

logston commented 1 year ago

Scratch that, I was able to address this now.