Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions src/sentry/models/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,41 @@ def by_qualified_short_id_bulk(
).filter(short_id_lookup, project__organization=organization_id)
)
group_lookup: set[int] = {group.short_id for group in groups}

# If any requested short_ids are missing after the exact slug match,
# fallback to a case-insensitive slug lookup to handle legacy/mixed-case slugs.
missing_short_ids = [sid.short_id for sid in short_ids if sid.short_id not in group_lookup]
if missing_short_ids:
# Build a lookup only for the missing short_ids per slug
missing_by_slug = defaultdict(list)
for sid in short_ids:
if sid.short_id in missing_short_ids:
missing_by_slug[sid.project_slug].append(sid.short_id)
Comment on lines 382 to 384
Copy link
Contributor

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?

Suggested change
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)


ci_short_id_lookup = reduce(
or_,
[
Q(project__slug__iexact=slug, short_id__in=sids)
for slug, sids in missing_by_slug.items()
],
)
Copy link
Contributor

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.

Fix in Cursor Fix in Web

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

twist my arm


fallback_groups = list(
self.exclude(
status__in=[
GroupStatus.PENDING_DELETION,
GroupStatus.DELETION_IN_PROGRESS,
GroupStatus.PENDING_MERGE,
]
).filter(ci_short_id_lookup, project__organization=organization_id)
)

if fallback_groups:
# Merge unique groups from fallback
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}
Copy link
Contributor

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?

Suggested change
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}


for short_id in short_ids:
if short_id.short_id not in group_lookup:
raise Group.DoesNotExist()
Expand Down
16 changes: 16 additions & 0 deletions tests/sentry/models/test_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from sentry.models.groupredirect import GroupRedirect
from sentry.models.grouprelease import GroupRelease
from sentry.models.groupsnooze import GroupSnooze
from sentry.models.project import Project
from sentry.models.release import Release, _get_cache_key
from sentry.replays.testutils import mock_replay
from sentry.testutils.cases import ReplaysSnubaTestCase, SnubaTestCase, TestCase
Expand Down Expand Up @@ -169,6 +170,21 @@ def test_qualified_share_id_bulk(self) -> None:
group.organization.id, [group_short_id, group_2_short_id]
)

def test_by_qualified_short_id_bulk_case_insensitive_project_slug(self) -> None:
project = self.create_project(slug="mixedcaseslug")
group = self.create_group(project=project, short_id=project.next_short_id())

Project.objects.filter(id=project.id).update(slug="MixedCaseSlug")
assert Project.objects.get(id=project.id).slug == "MixedCaseSlug"

# Re-fetch to ensure updated relation is used when computing qualified_short_id
group = Group.objects.get(id=group.id)
short_id = group.qualified_short_id

# Should resolve via case-insensitive slug fallback
resolved = Group.objects.by_qualified_short_id_bulk(group.organization.id, [short_id])
assert resolved == [group]

def test_first_last_release(self) -> None:
project = self.create_project()
release = Release.objects.create(version="a", organization_id=project.organization_id)
Expand Down
Loading