django-rest-framework icon indicating copy to clipboard operation
django-rest-framework copied to clipboard

Add search_fields attribute to SearchFilter

Open ad-m-ss opened this issue 2 years ago • 6 comments

Note: Before submitting this pull request, please review our contributing guidelines.

Description

I suggest a small change in the API (backwards compatible) that will simplify some popular notation to express it as:

class CatOwnerNameSearchFilter(filters.SearchFilter):
    search_param = "owner"
    search_fields = ["first_name", "second_name"]


class CatNameSearchFilter(filters.SearchFilter):
    search_param = "name"
    search_fields = ["name"]

class CatViewSet(
    viewsets.ModelViewSet
):
    queryset = Cat.objects.order_by("pk")
    filter_backends = [
        CatOwnerNameSearchFilter,
        CatNameSearchFilter,
    ]

I can add the tests after I get preliminary approval for such a change in the API.

ad-m-ss avatar Jan 10 '23 08:01 ad-m-ss

for design decision I will refer this to @tomchristie

auvipy avatar Feb 23 '23 08:02 auvipy

sorry for mistakenly approving. the tests are failing and we still need design decision confirmed.

Thanks for raising the issue of tests. I will take look into that next week.

ad-m-ss avatar Mar 17 '23 01:03 ad-m-ss

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jun 11 '23 14:06 stale[bot]

Can't we handle this with existing model fields? what additional benefits this change will provide?

In order for the example to work properly without changing code it is required to format it as:

class CatOwnerNameSearchFilter(filters.SearchFilter):
    search_param = "owner"
    def get_search_fields(self, view, request):
        return ["first_name", "second_name"]


class CatNameSearchFilter(filters.SearchFilter):
    search_param = "name"
    def get_search_fields(self, view, request):
        return ["name"]

class CatViewSet(
    viewsets.ModelViewSet
):
    queryset = Cat.objects.order_by("pk")
    filter_backends = [
        CatOwnerNameSearchFilter,
        CatNameSearchFilter,
    ]

I don't like that these attributes are defined in function body, because it's hard to perform dynamic introspection of them eg. generic testing if all fields are valid one.

ad-m-ss avatar Jun 21 '23 00:06 ad-m-ss

I'm -0 on these, largely torn between this being a change which logically makes sense and this being something which should likely just be using django-filters instead. I'm not sure how much mileage we intended for the search filter class itself to get vs using the view attributes and it being used internally behind the scenes.

kevin-brown avatar Jun 21 '23 13:06 kevin-brown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Sep 17 '23 23:09 stale[bot]