Deprecate ordering options, rework into filter

[rpkilby]

Fixes #438. The objective is to completely deprecate the ordering features of the FilterSet itself, instead encouraging the use of an explicitly declared ordering filter. In theory, order_by and the refactored get_ordering_filter could be kept, however I think it's better to remove it. The existing behavior of order_by is just not compatible with too many use cases. It would be possible to re-add order_by, but with a simpler, more restricted set of rules.

Details:
This completely reworks ordering into a filter, while retaining some deprecation compatibility.

• works like a ChoiceFilter and a CSVFilter
• a fields argument allows users to alias the underlying field names. eg, account => username
• can customize labels w/ field_labels, which is a map of field names to display texts
• builds default choices from fields, automatically adding descending options
• To bypass the default choices, a user can simply provide their own choices argument. e.g., to disable sorting by a descending option or something.

Changes:

• Add OrderingFilter
• Deprecate Meta.order_by and Meta.order_by_field.
• Refactor get_ordering_field into get_ordering_filter. While now deprecated, the behavior for Meta.order_by is completely backwards compatible with the caveat that it only works for the Meta.fields list syntax.
• Add an assertion error to get_ordering_filter that prevents undefined behavior from occurring when using the Meta.fields dict syntax with Meta.order_by.
• Add assertion errors for get_order_by and get_ordering_field.
• Add deprecation warnings

Notes:

• Deprecations are forwards compatible
• For get_order_by and get_ordering_field, assertions were chosen over deprecation warnings because those changes are forwards incompatible. The best that can be done is to warn the user that the behavior has changed. Otherwise, the behavior would silently change after upgrading, as those methods are no longer invoked.
• The default ordering when not specified is not really easy detect/emulate/warn against. This should be mentioned in the migration docs.

TODO:

• wait for 459, which contains the strict and order_by_field moves.
• filters docs
• document deprecations
  • Meta.order_by deprecation
  • Ordering methods deprecation (get_order_by, get_ordering_field, etc...)
  • Document all of the deprecations, including the lack or default ordering
  • Default qs ordering when no ordering param has been removed. (Not possible to emulate? See removed: test_filterset.FilterSetOrderingTests.test_ordering_when_unbound)

[carltongibson]

What's the filter class called?

[carltongibson]

OK. This looks good.

• There's that AutoOrderingFilter reference, which just looks an error.
• On the naming, OrderingFilter seems simpler that SimpleOrderingFilter — do you think they're better named the other way.
• Yes, docs are important here.

[rpkilby]

So the names were BaseOrderingFilter/OrderingFilter, then OrderingFilter/AutoOrderingFilter, then SimpleOrderingFilter/OrderingFilter. Obviously, couldn't quite settle on the names 😄

I think I'll revert back to BaseOrderingFilter and just... leave it undocumented? It's really only there to support backwards compatibility with the existing order_by api and most likely, users will just need to use OrderingFilter.

[carltongibson]

@rpkilby Sounds good!

I think I'll revert back to BaseOrderingFilter and just... leave it undocumented? It's really only there to support backwards compatibility with the existing order_by api and most likely, users will just need to use OrderingFilter.

I'm happy with that — perhaps a decent doc string — and then a decent entry in the actual docs for OrderingFilter.

[rpkilby]

There's that AutoOrderingFilter reference, which just looks an error.

Oh yeah, was fixing some of those inconsistencies when my laptop crashed - called it an evening. I'll patch it up soon.

[rpkilby]

Hey @carltongibson - I reworked the PR and think it's a bit cleaner. Merged the two filter classes into one as there wasn't any real reason to distinguish between the two and was potentially a point of confusion. Also cleaned up the arguments to the filters to make a little more sense.

[kevin-brown]

Shouldn't this probably be translatable?

[rpkilby]

Yep!

[rpkilby]

Hey - this has been re-rebased.

[carltongibson]

@rpkilby Was your intention to have the "default select widget" in play? (It's not, but should be I think...)

[carltongibson]

I'm thinking BaseCSVField needs to handle that it won't get a list from e.g. a Select widget.

[rpkilby]

Was your intention to have the "default select widget" in play?

The select widget needs to be augmented by the CSVWidget (similar to how the CSV field and filter work). I'm not sure if this can be handled generically or if this needs to be a one off CSVSelect widget.

I'm thinking BaseCSVField needs to handle that it won't get a list from e.g. a Select widget.

Off the cuff I'd agree - no reason the field couldn't handle both values. That said, a different widget class won't know how to render the list of values it will get from the CSV field. At worst, it raises an exception. At best, it will force the list as a text value.

I think the correct thing to do here is have conditional rendering in the base CSV widget. If it has a single value (eg, from a select dropdown), use the main widget to render. If it has multiple values, force a text input.

[carltongibson]

That all makes sense. #501 looks good. I'll review over the weekend. Thanks!

[rubengrill]

Hi, what is the supposed way of setting the initial ordering in OrderingFilter?

In the docs (migration guide) it is stated, that the queryset method order_by should be used, but I find this not appropriate, as I have to set the ordering then in my view (where my queryset gets prepared) or I have to create a subclass of OrderingField. If set in a view, it is very hard to maintain that, because when I decide to change the default ordering, I have to change the order_by invocation in several places. Also, I would have the correct ordering then, but thats not reflected in the ChoiceField that gets rendered (meaning that the select input shows a wrong value).
Same if I choose to override the OrderingFilter method filter, even if source code of initial value is where it belongs, also that initial value would not be reflected in the rendered ChoiceField.

I tried to pass the kwarg initial to the OrderingField, but this initial value gets only applied when the form is not bound. But when passing request.GET as data to the FilterSet, then the form is always bound, and the initial value is not used.

I saw that the FilterMixin uses in get_filterset_kwargs the expression kwargs = {'data': self.request.GET or None}, but I'm also not happy with this approach, as I often have query parameters that are unrelated to the filter view (e.g. edit parameter in Django CMS).

Am I missing sth here?

Should the FilterSet class be extended maybe, to filter data keys by existing filter fields to check if it is bound or unbound?

[rpkilby]

Hi @rubengrill. Trying to distill/understand your comment - it seems the problem is that you want to apply a default ordering to the queryset, but the rendered form is not aware of the default value, instead rendering some other value (probably just the first choice in the option list).

For context, part of the odd behavior is the lack of an 'empty' option for ChoiceFilters (OrderingFilter is a subclass). My guess is that it's selecting the first item in your option list, which is different from the default value. Some of this is discussed in #261. #519 adds an empty option, which would make the form a little more sensible.

I tried to pass the kwarg initial to the OrderingField, but this initial value gets only applied when the form is not bound. But when passing request.GET as data to the FilterSet, then the form is always bound, and the initial value is not used.

Yep - initial values are not default values. The unbound form simply preselects that options during rendering. It's still up to the user to submit a final value.

Anyway, you should be able to do something like the following:

class UserFilter(FilterSet):
    ordering = OrderingFilter(fields=(...), initial='username')

    class Meta:
        model = User
        fields = ['username', 'full_name']

# in your view:
    f = UserFilter(queryset=User.objects.order_by('username'), data=request.GET)
    ...

Filters pass any unrecognized options to its underlying field. This allows you to take full advantage of the required, initial, etc... options from the field.

[rubengrill]

Hi @rpkilby, Thanks for your answer.

it seems the problem is that you want to apply a default ordering to the queryset, but the rendered form is not aware of the default value, instead rendering some other value (probably just the first choice in the option list).

Exactly, it renders the first choice in the option list, although another choice was passed to order_by.

For context, part of the odd behavior is the lack of an 'empty' option for ChoiceFilters (OrderingFilter is a subclass). My guess is that it's selecting the first item in your option list, which is different from the default value. Some of this is discussed in #261. #519 adds an empty option, which would make the form a little more sensible.

The empty option is correct, if my view does not at all use queryset.order_by(), but instead only relies on the OrderingField. Then it would be correct to render the first choice (which is the empty choice), telling the user that no sorting at all was done when rendered (and the OrderingField really didn't sort, because of the empty value.

But in my case, I always want to sort by a default sort field. The user should not be able to choose an empty option, because no sorting at all would not make sense to the user.

I'll show my concrete use case to clarify that:

The user requests search/ without any query params.
The FilterMixin#get_filterset_kwargs method realizes that request.GET is empty, thus returns None for data:
kwargs = {'data': self.request.GET or None}

The FilterSet is unbound and uses the initial value for the OrderingField (e.g. rating).
That value is also selected in the rendered Select input field.

But:
What if the user requests search?edit. edit in this case just tells that the used django cms should be in edit mode, it could be any other query parameter (that is not related to the FilterSet).
The FilterMixin#get_filterset_kwargs method would now return self.request.GET for data.
The FilterSet is bound and does not use the initial value for the OrderingField.

So the problem is that I have a search view only for GET requests, that is sometimes bound, sometimes not, depending if ANY query parameter is given (unlike django Forms which are usually bound only for POST requests).

Shouldn't the FilterSet be bound only if query parameters are given that are actually filter fields?

I wrote a FilterSet, which does exactly this, and it works very well for me:

class ProductFilter(django_filters.FilterSet):
    price__lte = django_filters.NumberFilter(name='price', lookup_expr='lte')
    price__gte = django_filters.NumberFilter(name='price', lookup_expr='gte')

    o = django_filters.OrderingFilter(
        choices=(
            ('-rating', 'Rating'),
            ('price', 'Price (ascending)'),
            ('-price', 'Price (descending)'),
        ),
        initial=['-rating'],
    )

    def __init__(self, data=None, queryset=None, prefix=None, strict=None):
        # Filter data dict by keys that are actually filter fields
        # If resulting dict is empty, pass None to super constructor,
        # so that this filter is unbound
        # Required to get initial value for OrderingFilter working
        #
        # https://github.com/carltongibson/django-filter/pull/472#issuecomment-252511903
        keys = self.base_filters.keys()
        data = data or {}
        data = {key: value for key, value in data.iteritems() if key in keys}
        data = data or None
        super(ProductFilter, self).__init__(data, queryset, prefix, strict)

    class Meta:
        model = models.Product
        fields = []

This way, all initial values would be applied (not just for OrderingField), even when other query params are given (that are not related to the FilterSet).

[rpkilby]

But in my case, I always want to sort by a default sort field. The user should not be able to choose an empty option, because no sorting at all would not make sense to the user.

The user selecting an empty option here should be equivalent to saying "I'm letting the server chose how to sort the results".

This way, all initial values would be applied (not just for OrderingField), even when other query params are given (that are not related to the FilterSet).

Remember - initial is just a preselected value for unbound forms. Do not conflate this with a default value. Forms don't really have a mechanism for defaults.

Also, this approach has a subtle bug. Filters that use MultiWidgets (such as those used with the RangeFilter) will be silently dropped since they expect param_0=value&param_1=value1 instead of param=value. This applies to any widget where the query param does not match the filter name, but this realistically only affects MultiWidget.

---

Either way, the real problem here is that FilterMixin does not play well with unexpected query params. I don't think that mucking with the data dict inside of FilterSet is the way to go. Instead, I'd recommend creating a different view class that works with POST data instead.

class FilterView(django_filters.views.FilterView):
    """
    Use POST data instead of GET query params
    """
    def get_filterset_kwargs(self, filterset_class):
        kwargs = super(FilterView, self).get_filterset_kwargs(filterset_class)
        kwargs['data'] = self.request.POST or None
        return kwargs

Don't forget to update your form method in the template:

<form method="POST">
     ...
</form>

[rpkilby]

As an addendum (since #519 is basically ready) - if you really want to enforce filtering, you should be able to do it via the following:

CHOICES = [...]

class ProductFilter(django_filters.FilterSet):
    o = django_filters.OrderingFilter(
        choices=CHOICES
        required=True,
        empty_label=None,
    )

• Adding required should force the form to complain about empty values (Not sure if you would need to fiddle with the strictness option)
• If the PR is merged, then adding empty_label=None would explicitly disable the empty option, so a user would have to pick a valid choice.