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

Passing the view context to the filterset #857

Closed
antwan opened this issue Jan 17, 2018 · 7 comments
Closed

Passing the view context to the filterset #857

antwan opened this issue Jan 17, 2018 · 7 comments

Comments

@antwan
Copy link

antwan commented Jan 17, 2018

It would be nice to access the view calling the filterset for context purposes.
This would be pretty much the same thing allowed by DRF's context. We already have the request but the view would be a nice addition to do context-based filtering.

For example this can be used to access the DRF view action (list/retrieve/etc), know the parent when doing nested listing (api/object/:id/subobjects/) or access view properties.

It's an easy one to do, and can bring a lot. What do you think?

@carltongibson
Copy link
Owner

My initial thought is that whatever logic you need would be better off in the view, when instantiating the FilterSet. (Either setting attributes or choosing a different FilterSet class or...)

Can you provide a concrete use-case? (It's much easier to think about with an actual example.)

@antwan
Copy link
Author

antwan commented Jan 18, 2018

As for request context, this is pretty much for advanced filtering (involving custom methods).

  • The example concrete example I have in mind is when creating nested endpoints, for e.g api/authors/1/books/?year=2004.
    You may want in this particular case to know what is the parent author when filtering type of books.

  • Another case would be when you want to be aware of other filter backends (e.g DjangoObjectPermissionsFilter in DRF again).

  • Another case would be to know what's the current DRF action (retrieve/list/create) to behave differently.

I appreciate this is for advanced usages, but it's always nice to be able to do things when you need them. Currently these cases are only covered by doing hackish and/or painful things (create custom filterset and backend mixins just for it, instantiate with arguments, etc).
IMHO this is as useful as when you need access to the view from DRF serializers.

@rpkilby
Copy link
Collaborator

rpkilby commented Jan 26, 2018

My two cents - coupling the filterset to the view will most likely lead to relatively brittle code. eg, the filterset is now relying on the view's methods and other internals, which are not necessarily well defined. For example, relying on a DRF viewset method would make the filterset incompatible with Django views. At least, it would be necessary to write some amount of compatibility code.

Additionally, this would only work with CBVs - not FBVs - and writing unit tests would probably be a pain. It would be necessary to duplicate the initialization performed in View.as_view().

In comparison, depending on request is much more acceptable since its API is well defined. There are differences between the DRF Request and Django's HttpRequest, but users are usually relying on common properties, such as the headers and user.

As to the examples, it would make sense to me that filtering by author and permissions would be performed in the View.get_queryset() method, and not in the filterset. The filtered queryset is then what's used to initialized the filterset.


While I strongly recommend against coupling the filterset to the view, the DRF backend could probably provide an easier way to pass custom kwargs to the filterset init. As it currently stands, it's necessary to override the backend on a per-case basis, which is an annoyance to say the least. One possibility - the backend could accept a View.get_filter_kwargs method.

@carltongibson
Copy link
Owner

carltongibson commented Jan 26, 2018

Thanks for the input @rpkilby.

As to the examples, it would make sense to me that filtering by author and permissions would be performed in the View.get_queryset() method, and not in the filterset. The filtered queryset is then what's used to initialized the filterset.

Yes. This is the correct response. (This is exactly where view specific filtering logic is meant to go.)

Also, Django Filter's task is to box-up the generic, repeated bits of filtering. Specific view-related logic is out of scope. Do it in the view!

One possibility - the backend could accept a View.get_filter_kwargs method.

Grrrr.... 🙂 (those watching closely may have noticed) this is the kind of back-injecting API that I don't like. But yes, this would be possible.

  • It would be nice to see an example, but I don't know that I want to actually include it.
  • I always feel here that we should be instantiating Xs — for whatever the X is — with callables, rather than back-injecting API to the calling class. I need to sit and work out how that works with the declarative style, since this pattern comes up from time to time.

@antwan
Copy link
Author

antwan commented Jan 26, 2018

As to the examples, it would make sense to me that filtering by author and permissions would be performed in the View.get_queryset() method, and not in the filterset. The filtered queryset is then what's used to initialized the filterset.

... Unless you also want the Filterset's filters to behave differently depending on author and permissions.
Accessing context is essential IMO to create DRY filtersets, the same way it is for DRF serializers (they have access to the view, request, and format). Currently the only solution is to copy/paste/override Filterset classes them for your different use cases, leading to code duplication and lack of clarity.

While I strongly recommend against coupling the filterset to the view, the DRF backend could probably provide an easier way to pass custom kwargs to the filterset init. As it currently stands, it's necessary to override the backend on a per-case basis, which is an annoyance to say the least. One possibility - the backend could accept a View.get_filter_kwargs method.

👍 That's exactly what the DRF Serializers are doing, there should be a get_filter_context so we can pass what's required, even customize the filter on the view side (for example have a single FilterSet class which can have some filters enabled or not depending how you instantiate it). And such context dict should contain the request.

Anyway I won't push harder just for the sake of my own usage. As much as I do share your point and also think back-injecting is not nice, I have concrete use cases that are much much tidier and more reusable with such architecture. Having everything in the view, as you suggest, is a also trade-off with keeping filtering logic in filtersets.

I guess having access to filterset **kwargs without much hacking is close enough, so thanks for this!

@rpkilby
Copy link
Collaborator

rpkilby commented Jan 26, 2018

The serializer context is something that's always bothered me, and I'd actually like to see it removed. That said, there isn't really a practical way to do this, given that the hyperlinked relations rely on the context.

#865 will actually give you the tools necessary to implement this hook yourself. eg

  • override filterset __init__ to accept additional arguments
  • override backend get_filterset_kwargs to call a method on the view class to get additional kwargs.

Something like..

class MyBackend(filters.DjangoFilterBackend):
    def get_filterset_kwargs(self, request, queryset, view):
        kwargs = super().get_filterset_kwargs(request, queryset, view)

        # merge filterset kwargs provided by view class
        if hasattr(view, 'get_filterset_kwargs'):
            kwargs.update(view.get_filterset_kwargs())

        return kwargs

class BooksFilter(filters.FitlerSet):
    def __init__(self, *args, author=None, **kwargs):
        super().__init__(*args, **kwargs)
        # do something w/ author

class BookViewSet(viewsets.ModelViewSet):
    filter_backends = [MyBackend]
    filter_class = BookFilter

    def get_filteset_kwargs(self):
        return {
            'author': self.get_author(),
        }

@hynrey
Copy link

hynrey commented Oct 5, 2023

def get_filteset_kwargs(self):
    return {
        'author': self.get_author(),
    }

If someone like me didn't immediately notice the error in the name of the class view function. It should be get_filterset_kwargs, r is missing

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