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

Add search_fields attribute to SearchFilter #8834

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ad-m-ss
Copy link

@ad-m-ss ad-m-ss commented Jan 10, 2023

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.

@auvipy auvipy self-requested a review January 10, 2023 09:08
rest_framework/filters.py Show resolved Hide resolved
@auvipy auvipy added this to the 3.15 milestone Feb 7, 2023
@ad-m-ss ad-m-ss requested a review from auvipy February 7, 2023 11:40
@auvipy
Copy link
Member

auvipy commented Feb 23, 2023

for design decision I will refer this to @tomchristie

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ad-m-ss
Copy link
Author

ad-m-ss commented Mar 17, 2023

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.

@auvipy auvipy requested a review from a team March 28, 2023 14:46
@stale
Copy link

stale bot commented Jun 11, 2023

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 stale bot added the stale label Jun 11, 2023
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@stale stale bot removed the stale label Jun 13, 2023
@ad-m-ss
Copy link
Author

ad-m-ss commented Jun 21, 2023

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.

@kevin-brown
Copy link
Member

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.

@auvipy auvipy removed this from the 3.15 milestone Jun 21, 2023
@stale
Copy link

stale bot commented Sep 17, 2023

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 stale bot added the stale label Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants