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

Filter method and lookup expressions #1199

Closed
Subaku opened this issue Apr 10, 2020 · 4 comments
Closed

Filter method and lookup expressions #1199

Subaku opened this issue Apr 10, 2020 · 4 comments

Comments

@Subaku
Copy link

Subaku commented Apr 10, 2020

Hi,

Seems like a potential bug. Declaring a method on a field in my FilterSet seems to not do anything under a couple of scenarios.

  1. If the field is a DateTimeField my method is not called
  2. If I add any lookup expression to my query it seems to not get called.

Am I just expecting too much of the method?

For reference I had the filter working before without a declared method by defining the following in my Meta:

class Meta:
   model = MyModel
   fields = {
      when_seen: ['gt', 'gte', 'lt', 'lte'],
      ...
   }

Which worked great. I now need a method so I can customize what happens if the user filters by __gt or __gte. So I figured I could simple remove the when_seen from my meta and declare:

when_seen = rest_filters.DateTimeFilter(field_name='when_seen', method='by_when_seen')

Yet no matter what I do my method does not get called. Any ideas?

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 11, 2020

Hi @Subaku. This isn't actually related to the method argument, but the more general case that you declared filters cannot be used as a base for generating filters for additional lookups. In general, the following is invalid:

class F(FilterSet):
    name = CharFilter(field_name='username')

    class Meta:
        model = User
        fields = {'name': ['exact', 'gt', 'lt']}

However, there is a bug and filter generation doesn't warn you that the additional filters aren't generated. This has been fixed in #1061, but the change hasn't been released yet. That said, the fix is just raising a warning instead of silently failing. This kind of filter generation won't work, and you'll have to declare these filters manually.

Additionally, you might be interested in #1150. One deficiency with the current filter method implementation is that you don't get additional arguments like lookup_expr. By passing the filter instance to the filter method, you can then access any argument, including the lookup. The PR also describes current workarounds.

@rpkilby rpkilby closed this as completed Apr 11, 2020
@Subaku
Copy link
Author

Subaku commented Apr 11, 2020

Thanks for pointing out #1150. I'll try that workaround.

Also sorry for not being clearer in my post. I'm aware you can't do what you show. I'm not trying to declare both a manual filter field AND the same field in the Meta. I'm doing either or. That said my proposed, possible bug still stands. I.e.

class F(FilterSet):
    dt = DateTimeFilter(field_name='when_seen', method='get_dt_now')

    class Meta:
        model = User

    def get_dt_now(...):
         ...

For whatever reason get_dt_now never gets called... But if I instead change it to a CharField it works.

Thanks again.

@rpkilby
Copy link
Collaborator

rpkilby commented Apr 13, 2020

I'm aware you can't do what you show. I'm not trying to declare both a manual filter field AND the same field in the Meta. I'm doing either or.

Right, it's not the declared filter that's an issue, it's generating the filters for the other lookups. In the example I posted above, it's the name__lt, name__gt filters that wouldn't be created.

For whatever reason get_dt_now never gets called... But if I instead change it to a CharField it works.

You most likely have a validation error. Here's an example test case that I used to validate that DateTimeFilter is compatible with Filter.method.

    def test_datetime_filter(self):
        class F(FilterSet):
            dt = DateTimeFilter(field_name='published', method='get_dt_now')

            class Meta:
                model = Article
                fields = ['dt']

            def get_dt_now(self, qs, name, value):
                raise Exception('!!')

        f = F({'dt': '2020-01-01 00:00:00'})
        self.assertTrue(f.is_valid())
        with self.assertRaisesMessage(Exception, ('!!')):
            f.qs

If you check your filter's errors attribute, I'd bet that you'll see a validation error there.

@Subaku
Copy link
Author

Subaku commented Apr 13, 2020

Yup! It was a validation error. Thanks for your response :)
I got it working using the example in #1150.

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

2 participants