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

Jump to bottom

Allows searching with SearchFilter in annotated fields #6240

Merged
merged 1 commit into from Feb 25, 2019
Merged

Allows searching with SearchFilter in annotated fields #6240

merged 1 commit into from Feb 25, 2019

Conversation

andrzej3393
Copy link
Contributor

@andrzej3393 andrzej3393 commented Oct 10, 2018 •
edited

Description

Fixes bug with searching in annotated fields.
refs #6094

@oliwarner
Copy link

Argl, why won't somebody review and pull this?

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 7, 2018 •
edited

@oliwarner because it requires time and energy.
@andrzej3393 thanks for the PR. It looks good though I'd like to avoid creating too many specific models.
Do you think you could reuse the SearchFilterModel at the top of the file for that please ?

@andrzej3393
Copy link
Contributor Author

andrzej3393 commented Nov 7, 2018 •
edited

@xordoquy sure, I can reuse it, will push something soon. already done 🙂

@rpkilby rpkilby self-requested a review November 12, 2018 20:04
@rpkilby rpkilby added this to the 3.9.1 Release milestone Nov 12, 2018
@rpkilby
Copy link
Member

rpkilby commented Nov 12, 2018

Thanks for the PR @andrzej3393. Added to the milestone to ensure it's reviewed.

auvipy
auvipy approved these changes Feb 14, 2019
View changes
rpkilby
rpkilby previously requested changes Feb 20, 2019
View changes
rest_framework/filters.py Outdated Show resolved Hide resolved
@tuky
Copy link
Contributor

tuky commented Feb 21, 2019 •
edited

Wouldn't it be simpler to just try and catch the FieldDoesNotExist exception (and continue in that case)? It should not be must_call_distinct's responsibility to fail on missing fields. The actual query that is made later on will make it fail, anyway. Lets go the EAFP route :-) https://docs.python.org/3/glossary.html#term-eafp

carltongibson
carltongibson approved these changes Feb 25, 2019
View changes
Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK, I've pulled in @rpkilby's suggested change: the simple use-case looks right; if we need more complex handling of other query expressions then we can look at that with test cases in hand.

Thanks all. @andrzej3393 Welcome aboard! 🎉

@carltongibson carltongibson merged commit 317174b into encode:master Feb 25, 2019
alk3mist
alk3mist reviewed Dec 25, 2019
View changes
rest_framework/filters.py
@@ -85,6 +85,9 @@ def must_call_distinct(self, queryset, search_fields):
opts = queryset.model._meta
if search_field[0] in self.lookup_prefixes:
search_field = search_field[1:]
# Annotated fields do not need to be distinct
if isinstance(queryset, models.QuerySet) and search_field in queryset.query.annotations:
return False
Copy link

Choose a reason for hiding this comment

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

@andrzej3393 , what if we have several search_fields, and the first is an annotated field, and the others are m2m not annotated?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Opened #7146.

@rpkilby rpkilby mentioned this pull request Jan 16, 2020
Merged
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
@andrzej3393
Avoided calling distinct on annotated fields in SearchFilter. (encode…
c06bffa 
…#6240)

Fixes encode#6094
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
@andrzej3393 @sigvef
Avoided calling distinct on annotated fields in SearchFilter. (encode…
2329909 
…#6240)

Fixes encode#6094
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpkilby rpkilby rpkilby left review comments

@alk3mist alk3mist alk3mist left review comments

@carltongibson carltongibson carltongibson approved these changes

@auvipy auvipy auvipy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.9.2 Release
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@andrzej3393 @oliwarner @xordoquy @rpkilby @tuky @carltongibson @auvipy @alk3mist @tomchristie