-
-
Notifications
You must be signed in to change notification settings - Fork 795
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
Feature: Generalize Search to Other Models #2472
Feature: Generalize Search to Other Models #2472
Conversation
This is awesome! And the test refactor is super appreciated. Trigram matching can be very slow on columns without an GIN index, so like you said it’s probably a good idea to index anything that will be searched. |
Thanks! I pretty much just copy pasted what you worked on and reorganized things a bit. And yeah, my thought process here is to add a quick win for expanding the search, then we can normalize/index anything we use on the frontend. Unless we want to do that upfront now, I just wasn't sure |
What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
(REQUIRED)
This generalizes the recipe search from #2351 to work for any model. After these changes, adding search support to a model is as easy as:
search
param into the model'sget_all
routeWhich issue(s) this PR fixes:
(REQUIRED)
N/A
Special notes for your reviewer:
(fill-in or delete this section)
This PR does not add normalized fields to the newly-supported models, so diacritics and symbols will decrease search accuracy. In the future I want to re-work how the recipe parser finds food/unit matches (using this search API) and that PR should add normalization. I figure it's okay to not normalize in this PR since often it's a less common issue, and fuzzy matching will work around it anyway (if using postgres). We'll probably also want to index those fields (similar to recipes).
If you think this PR should normalize search fields, I'll happily add them, I just didn't want this PR to become too much of a headache (since normalized fields will add migrations) for little benefit (the frontend isn't using this API yet).
Testing
(fill-in or delete this section)
I refactored the recipe search tests to be parametrized (instead of one large test) and added a generic set of tests for units (since recipes use a more complex filter, including ingredients). They probably don't need to both be there, but they're good tests, so I think it's fine.
I also tweaked the tests to allow extra results (e.g. if only one recipe is expected to match, and two match, that's fine, as long as the first one is the one we wanted) to avoid those annoying test failures we've seen in other PRs. I also sandboxed them using their own user/group to avoid other tests leaking in.
Release Notes
(REQUIRED)