-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(issues): Fallback to case insensitive short id lookup #102103
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: master
Are you sure you want to change the base?
feat(issues): Fallback to case insensitive short id lookup #102103
Conversation
We allowed people to create projects with case insensitive slugs a while ago, its tricky to update them to not be case sensitive. Adds a fallback lookup to case insensitive project slug
src/sentry/models/group.py
Outdated
| Q(project__slug__iexact=slug, short_id__in=short_ids) | ||
| for slug, short_ids in project_short_id_lookup.items() | ||
| ], | ||
| ) |
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: Unused Variable Causes Redundant Database Queries
The missing_short_ids variable is computed but not used when constructing the fallback query. This causes the case-insensitive lookup to re-query for short IDs that were already found in the initial exact-match query, leading to inefficient database operations.
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.
twist my arm
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.
since this is on a pretty important path, should we hide this behind a flag as well?
| for sid in short_ids: | ||
| if sid.short_id in missing_short_ids: | ||
| missing_by_slug[sid.project_slug].append(sid.short_id) |
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 think you hold all the missing short ids already in missing_short_ids?
| for sid in short_ids: | |
| if sid.short_id in missing_short_ids: | |
| missing_by_slug[sid.project_slug].append(sid.short_id) | |
| for missing_short_id in missing_short_ids | |
| missing_by_slug[short_id.project_slug].append(short_id.short_id) |
src/sentry/models/group.py
Outdated
| existing_ids = {g.id for g in groups} | ||
| groups.extend(g for g in fallback_groups if g.id not in existing_ids) | ||
| group_lookup = {group.short_id for group in groups} |
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.
isn't there guaranteed no overlap between fallback_groups and groups? do we need the additional check?
| existing_ids = {g.id for g in groups} | |
| groups.extend(g for g in fallback_groups if g.id not in existing_ids) | |
| group_lookup = {group.short_id for group in groups} | |
| groups.extend(g for g in fallback_groups) | |
| group_lookup = {group.short_id for group in groups} |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #102103 +/- ##
========================================
Coverage 80.95% 80.96%
========================================
Files 8743 8742 -1
Lines 389054 389022 -32
Branches 24693 24685 -8
========================================
+ Hits 314972 314974 +2
+ Misses 73727 73693 -34
Partials 355 355 |
We allowed people to create projects with case insensitive slugs a while ago, its tricky to update them to not be case sensitive.
Adds a fallback lookup to case insensitive project slug. Fixes an issue where the errors dataset could not query by issue short id when the project contained uppercase letters.
issue:PROJECTSLUG-E123Redash query with number of project slugs that contain case insensitive characters
Redash query of projects with insensitive characters with recent issues