Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alias names for filter and multiple comperators #774

Closed
anx-ckreuzberger opened this issue Sep 5, 2017 · 7 comments
Closed

Alias names for filter and multiple comperators #774

anx-ckreuzberger opened this issue Sep 5, 2017 · 7 comments

Comments

@anx-ckreuzberger
Copy link

First of all: Apologies if this topic is covered already in another issue, PR or SO post. I didn't manage to find anything that got remotely close.

Second: I am using Django Rest Framework and django-filter 1.0.4 with the Django ORM.

My Problem:

I have a Meeting model which (for legacy reasons) has the fields date_time_start and date_time_end. I want to create a Filter Class MeetingFilter with two DateTimeFilters with the following comparators: DATE_COMPERATORS = ('lt', 'lte', 'gt', 'gte', 'exact', ).

class MeetingFilter(BaseFilter):
    class Meta:
        model = Meeting
        fields = {
            'date_time_start': DATE_COMPERATORS,
            'date_time_end': DATE_COMPERATORS
        }

This allows me to call the REST API endpoint with the following options:
/api/meetings/?date_time_start__gte=2017-01-01....&date_time_end__lte=2017-01-31...
For me, this is a very good and intuitive API.

However, I do not want to expose the names date_time_start and date_time_end via query parameters. Instead I would like them to be called start_date and end_date, so they match all my other APIs that are already built with those names. (I know I could just write a migration or "hack" myself into the request and change the paramters, etc..., but this is beyond the scope of what I am trying to achieve here.)

So what I tried was this (note: using name= works as an alias for the ORM field):

...
    end_date__lt = django_filters.DateTimeFilter(name='date_time_end', lookup_expr='lt')
    end_date__gt = django_filters.DateTimeFilter(name='date_time_end', lookup_expr='gt')
    end_date__lte = django_filters.DateTimeFilter(name='date_time_end', lookup_expr='lte')
    end_date__gte = django_filters.DateTimeFilter(name='date_time_end', lookup_expr='gte')
    end_date__exact = django_filters.DateTimeFilter(name='date_time_end', lookup_expr='exact')

    start_date__lt = django_filters.DateTimeFilter(name='date_time_start', lookup_expr='lt')
    start_date__gt = django_filters.DateTimeFilter(name='date_time_start', lookup_expr='gt')
    start_date__lte = django_filters.DateTimeFilter(name='date_time_start', lookup_expr='lte')
    start_date__gte = django_filters.DateTimeFilter(name='date_time_start', lookup_expr='gte')
    start_date__exact = django_filters.DateTimeFilter(name='date_time_start', lookup_expr='exact')

and everything is fine. Except for: it's ugly, and it's certainly not DRY.

Now I thought: it must be possible to do this DRY, mapping the alias and lookup expressions:

    end_date = django_filters.DateTimeFilter(name='date_time_end', lookup_expr=DATE_COMPERATORS)
    start_date = django_filters.DateTimeFilter(name='date_time_start', lookup_expr=DATE_COMPERATORS)

And indeed, this is possible. However, now the API has changed and I am no longer able to do
/api/meetings/?date_time_start__gte=2017-01-01....&date_time_end__lte=2017-01-31...
instead it is
/api/meetings/?start_date_0=2017-01-01....&start_date_1=gte&end_date_0=2017-01-31...&end_date_1=lte

I believe that while the code is now much nicer, the API is no longer intuitive. Now the big question is:
Can I define the filters DRY and have the intuitive API I want?

I've been looking through the code of django-filter already, and I found that the problem is here:
django_filters/filterset.py BaseFilterSet get_filters

            for lookup_expr in lookups:
                filter_name = get_filter_name(field_name, lookup_expr)

                # If the filter is explicitly declared on the class, skip generation
                if filter_name in cls.declared_filters:
                    filters[filter_name] = cls.declared_filters[filter_name]
                    continue

                if field is not None:
                    filters[filter_name] = cls.filter_for_field(field, field_name, lookup_expr)

Basically here it says "for every lookup expression: create a new filter for the field (unless the filter has been declared already)". However, if we use a declared filter and the lookup expressions list is provided in this declared filter, we get the _0 and _1 appended to the URL parameters.

I believe it should be possible to achieve the intuitive API with the following code:

  1. Define end_date and start_date in the meta fields, and declare the filters with an alias name.
class MeetingFilter(BaseFilter):
    class Meta:
        model = Meeting
        fields = {
            'end_date': DATE_COMPERATORS,
            'start_date': DATE_COMPERATORS
        }

    end_date = django_filters.DateTimeFilter(name='date_time_end')
    start_date = django_filters.DateTimeFilter(name='date_time_start')

However, the django-filter code would have to apply DATE_COMPERATORS to the declared filters somehow. Am I out of luck, or is there a way to achieve this?

@anx-ckreuzberger
Copy link
Author

Quick update:

I've digged a little and I found that the function get_filter_name does some magic that could help me here. If I monkey patch this method as follows, my problem is solved (temporarily):

from django_filters import filterset
from django.db.models.constants import LOOKUP_SEP

def my_get_filter_name(field_name, lookup_expr):
    """
    Combine a field name and lookup expression into a usable filter name.
    Exact lookups are the implicit default, so "exact" is stripped from the
    end of the filter name.
    """
    if field_name == 'date_time_start':
        field_name = 'start_date'
    elif field_name == 'date_time_end':
        field_name = 'end_date'

    filter_name = LOOKUP_SEP.join([field_name, lookup_expr])

    # This also works with transformed exact lookups, such as 'date__exact'
    _exact = LOOKUP_SEP + 'exact'
    if filter_name.endswith(_exact):
        filter_name = filter_name[:-len(_exact)]

    return filter_name

filterset.get_filter_name = my_get_filter_name

The filter would look like this:

class MeetingFilter(BaseFilter):
    class Meta:
        model = Meeting
        fields = {
            'date_time_end': BaseFilter.DATE_COMPERATORS,
            'date_time_start': BaseFilter.DATE_COMPERATORS
        }

However, I hate having to monkey patch this... Therefore I would like to propose the following two options:

Option 1
Make get_filter_name a class method, so I could override it in MeetingFilter.

Option 2
Introduce a Meta option field_mappings which would look like this:

        fields = {
            'date_time_end': BaseFilter.DATE_COMPERATORS,
            'date_time_start': BaseFilter.DATE_COMPERATORS
        }
        field_mappings = {
            'date_time_end': 'end_date',
            'date_time_start': 'start_date'
        }

and update get_filter_name or the respective calling method get_filters to use the new field_mappings.

Option 1 would probably be super easy to do, and I would be happy to provide a PR for it.

@carltongibson
Copy link
Owner

carltongibson commented Sep 5, 2017

Hmmm. Interesting.

Given that you definitely want end_date__lt style rather than the separate parameter to specify the lookup type (which we could adjust the names on) , it seems the objection is to typing. 😀

That's not a problem. But the way you're doing it is right. Declaring the filters explicitly is the recommended way.

I won't introduce more meta options.

I can see the case for get_filter_name living on the class.

Of course, I guess you could just use a for loop to declare the filters in __init__... 😀

@anx-ckreuzberger
Copy link
Author

Thanks for your reply! I think the for loop can not live in __init__, as there are several things about the filter that are class based, and not instance based (but correct me if I am wrong here).
I've tried doing it in the class __new__ method, but I failed to create the filters there (they require a QuerySet etc... to create a new instance).

@carltongibson
Copy link
Owner

You'd have to mungle base_filters I guess — but other than that you should be OK. (In general building filter sets at runtime should be available, so if there are issues you run into doing that I'd be happy to look at them.)

More to your point I don't see a problem in moving get_filter_name

@carltongibson
Copy link
Owner

You'd have to mungle base_filters I guess...

But even that... I don't think you'd have an issue.

@ChristianKreuzberger
Copy link
Contributor

Sorry for not trying this earlier. You are absolutely right, I can do this in the __init__ method.
Here are my findings:

class MeetingFilter(BaseFilter):
    def __init__(self, *args, **kwargs):
        super(MeetingFilter, self).__init__(*args, **kwargs)
        # add extra filters for date_time_end as end_date__{comperator}
        for comperator in DATE_COMPERATORS:
            filter_name = "end_date__{}".format(comperator)
            filter = django_filters.DateTimeFilter(name='date_time_end', lookup_expr=comperator)
            self.base_filters[filter_name] = filter

        # add extra filters for date_time_start as start_date__{comperator}
        for comperator in DATE_COMPERATORS:
            filter_name = "start_date__{}".format(comperator)
            filter = django_filters.DateTimeFilter(name='date_time_start', lookup_expr=comperator)
            self.base_filters[filter_name] = filter

The above code works just fine. However, the extra code required here isn't really pretty (and with pretty I mean understandable) compared to overriding the get_filter_name method (Note: This code example isn't working yet, it's an example of what I am trying to do and will be covered with my PR):

class MeetingFilter(BaseFilter):
    class Meta:
        model = Meeting
        fields = {
            'date_time_end': DATE_COMPERATORS,
            'date_time_start': DATE_COMPERATORS
        }

    @classmethod
    def get_filter_name(cls, field_name, lookup_expr):
        """
        Rename the date_time_end and date_time_start fields of meeting to end_date and start_date
        :param field_name:
        :param lookup_expr:
        :return:
        """
        if field_name == 'date_time_start':
            field_name = 'start_date'
        elif field_name == 'date_time_end':
            field_name = 'end_date'

        return BaseFilter.get_filter_name(field_name, lookup_expr)

Anyway, I hope my findings will help others! I will add a PR for this to work :)

@rpkilby
Copy link
Collaborator

rpkilby commented Sep 6, 2017

Making get_filter_name a classmethod seems sensible to me.

ChristianKreuzberger pushed a commit to ChristianKreuzberger/django-filter that referenced this issue Sep 6, 2017
carltongibson pushed a commit that referenced this issue Oct 17, 2017
carltongibson pushed a commit that referenced this issue Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants