Refactor search controller for consistency and clarity #699
Conversation
openverse-bot
added
the
🚦 status: awaiting triage
|
API Developer Docs Preview: Ready https://wordpress.github.io/openverse-api/_preview/699 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
dhruvkb
added
🟨 priority: medium
| ) -> List[Hit]: | ||
| """ | ||
| After fetching the search results from the back end, iterate through the | ||
| results, perform image validation, and route certain thumbnails through our |
There was a problem hiding this comment.
This is not really in the scope of this PR, but where exactly is this done: route certain thumbnails thumbnails through our proxy in _post_process_results function?
There was a problem hiding this comment.
This code is in api/utils/validate_images.py. It's not exactly what the comment describes though, it checks for broken images but it does not route any images through our proxy. I think we used to do that before for http:// images but I think we stopped doing sometime and didn't update the comment.
| for arg in search_params[serializer_field].split(","): | ||
| filters.append(Q("term", **{es_field: arg})) | ||
| method = getattr(s, behaviour) # can be ``s.filter`` or ``s.exclude`` | ||
| return method("bool", should=filters) |
There was a problem hiding this comment.
From my limited understanding of ES, I think we can replace the should with must here, because we don't need the results that do not contain the filter parameter (i.e., when we are filtering by some value, we don't need the score that should can bring).
This is not a suggestion for this PR, I'm just putting my thoughts in words here :)
There was a problem hiding this comment.
I thought so too, but I don't know enough ES to be confident enough to make the change. I'll update it if you're sure that it should be must instead of should.
This reverts commit 0e442a4.
(cherry picked from commit 0e442a4)
|
@WordPress/openverse can we explore re-merging this work and getting it in an upcoming API deploy? I'd hate to 1. lose this work or 2. have the API drift so far from the changes here that it becomes difficult or impossible to merge. |
|
Fully support that post-v2.5.6! |
Description
This PR refactors the search controller, splitting it up into smaller, easier to understand chunks with proper documentation and type hinting.
This PR
MediaSearchRequestSerializeras the single source of truth for search paramsget_sourcesfunction to use DSL instead of raw query to ESSearchto be idiomatic with Elasticsearch DSL conventionssettings.ESThese refactor PRs I've been making recently is to make the process of adding media types (like 3D models and video) smoother and more intuitive. It also helps to understand what the code does when you can confidently modify it.
Testing Instructions
This is a refactor, passing CI should be sufficient proof that no functionality is impacted.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin