-
Notifications
You must be signed in to change notification settings - Fork 10
feat: Add repo name and service id to django admin repo search #591
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #591 +/- ##
=======================================
Coverage 93.87% 93.87%
=======================================
Files 1284 1285 +1
Lines 46667 46675 +8
Branches 1522 1522
=======================================
+ Hits 43809 43817 +8
Misses 2548 2548
Partials 310 310
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Should double-check that those fields are indexed or it's going to be slow as hell
Is service_id not just github, gitlab, etc?
2350489 to
ac9554d
Compare
| # Also search by repoid if the search term is numeric | ||
| try: | ||
| search_term_as_int = int(search_term) | ||
| except ValueError: | ||
| pass | ||
| else: | ||
| queryset |= self.model.objects.filter(repoid=search_term_as_int) | ||
| # avoid N+1 queries for foreign key author | ||
| queryset = queryset.select_related("author") |
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.
Bug: Applying select_related("author") after union() for a queryset added without prior optimization causes N+1 queries for the author relationship.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The code applies select_related("author") after performing a union() operation on two querysets. One of the querysets, self.model.objects.filter(repoid=search_term_as_int), is added without select_related(). This violates Django best practices, which require select_related() to be applied before union(). This will cause N+1 queries when accessing the author relationship for repositories matched by repoid in the Django admin, leading to significant performance degradation.
💡 Suggested Fix
Apply select_related("author") to the new queryset self.model.objects.filter(repoid=search_term_as_int) before the union() operation. The final select_related call can remain or be removed.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/codecov-api/core/admin.py#L188-L196
Potential issue: The code applies `select_related("author")` after performing a
`union()` operation on two querysets. One of the querysets,
`self.model.objects.filter(repoid=search_term_as_int)`, is added without
`select_related()`. This violates Django best practices, which require
`select_related()` to be applied before `union()`. This will cause N+1 queries when
accessing the `author` relationship for repositories matched by `repoid` in the Django
admin, leading to significant performance degradation.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5359759
CodSpeed Performance ReportMerging #591 will not alter performanceComparing Summary
Footnotes |
Closes https://linear.app/getsentry/issue/CCMRG-1906/allow-django-admin-repository-table-to-be-searchable-across-more
Note
Expands Django admin repository search (name, service_id, numeric repoid) and adds a GIN trigram index on UPPER(name) for faster lookups.
RepositoryAdmin.search_fieldsto includenameandservice_id__exact.get_search_resultsto match numericrepoidandselect_related("author")to avoid N+1.GinIndex(OpClass(Upper("name"), name="gin_trgm_ops"), name="repos_name_trgm_idx")toRepository.Meta.indexesand new migrationcore/0077_repository_repos_name_trgm_idx.py.Written by Cursor Bugbot for commit 4db8f39. This will update automatically on new commits. Configure here.