-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix #1650 Replace search field with generated vector field #2192
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
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: docs/management/commands/update_docs.py
Did you find this useful? React with a 👍 or 👎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR and cleaning out the unused code/docs! I am enjoying learning more about GeneratedFields.
I don't have experience with full-text search nor generated fields, so review from someone else would be great if possible.
If not, or if it's easier to deploy and test this on the preview server, I'm okay with that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request replaces the manually updated search field with a database-generated vector field for Document models to address issue #1650. The change eliminates the delay between document updates and search index updates by leveraging Django's GeneratedField to automatically compute search vectors in the database.
- Replaces
SearchVectorFieldnamedsearchwith aGeneratedFieldnamedvectorthat automatically updates when documents change - Removes manual search index update commands and related management infrastructure
- Updates search queries to use the new
vectorfield instead ofsearch
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/models.py | Replaces manual search field with auto-generated vector field using GeneratedField with language-specific search vector expressions |
| docs/search.py | Refactors search vector definition into a function to support the generated field approach |
| docs/tests/test_models.py | Removes tests for manual search update methods and updates test setup to work with auto-generated vectors |
| docs/tests/test_views.py | Removes manual search update call from test setup |
| docs/management/commands/update_index.py | Deletes the entire management command file as manual indexing is no longer needed |
| docs/management/commands/update_docs.py | Removes search index update logic and command-line options |
| docs/migrations/0007_add_vector_search.py | Database migration to transition from search to vector field |
| docker-entrypoint.dev.sh | Removes commented manual index update command |
| README.rst | Updates documentation to reflect removal of manual indexing commands |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
96b5060 to
e5a1ff3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
e5a1ff3 to
2937691
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks the updates.
Note to self (or @bmispelon, or whoever ends up deploying this): We'll need to remove --update-index from the Ansible repo at the same time. I'll make a companion PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a quick look through, a few comments on the models change, but otherwise it seems perfectly cromulent to me!
2937691 to
638cd7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
docs/models.py:1
- The test creates a document but no longer explicitly tests that the search_vector field is populated automatically. Consider adding an assertion to verify the generated field is working correctly after document creation.
import datetime
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Looks like you got unlucky with a Coveralls failure in the test run there: |
Coveralls has been just removed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's gooooo
@tobiasmcnulty Do you think there's something that needs to be done on the Ops side before merging this PR? If so, please proceed when you can. (I've no intention of pushing from my side) :) In fact, let me know if I can make this easier for you in any way. Thanks again for your help. |
Overview
This pull request addresses issue #1650, which proposed migrating the search vector field for Document models to a Generated Field. The goal is to leverage the database's capabilities to automatically update search-related data, eliminating the need for manual index updates.
Key Changes
SearchVectorFieldwith a database-generated vector field (using Django'sGeneratedField).Why This Matters
Previously, the search index was updated via a management command after document changes. This process introduced a delay between the update of a document and the update of the search index. During this window, users searching for recently updated documentation often encountered blank results, leading to confusion and frequent new issues being opened by contributors and users who could not find expected documentation.
With the Generated Field:
Review Notes
Impact
This change will significantly improve the reliability and responsiveness of documentation search on djangoproject.com. It removes a longstanding source of user confusion and contributor frustration, and leverages modern Django/database features for maintainability and performance.
Thank you for reviewing! Please raise any questions or concerns about edge cases, performance, or deployment.