Skip to content

fix(releases): Normalize both release values to equal one another #101184

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 9, 2025
+119 −8
Merged

fix(releases): Normalize both release values to equal one another #101184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b1aa352
Normalize both release values to equal one another
cmanallen Oct 8, 2025
ada33b9
Fix types
cmanallen Oct 8, 2025
51b2c66
Add coverage
cmanallen Oct 8, 2025
c9a1533
Add helper function
cmanallen Oct 9, 2025
3b71bc5
Query as subquery
cmanallen Oct 9, 2025
fbb12f7
Filter to semver
cmanallen Oct 9, 2025
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
36 changes: 29 additions & 7 deletions src/sentry/api/helpers/group_index/update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
from sentry.models.project import Project
from sentry.models.release import Release, follows_semver_versioning_scheme
from sentry.notifications.types import SUBSCRIPTION_REASON_MAP, GroupSubscriptionReason
from sentry.releases.use_cases.release import fetch_semver_packages_for_group
from sentry.signals import issue_resolved
from sentry.types.activity import ActivityType
from sentry.types.actor import Actor, ActorType
Expand Down Expand Up @@ -154,7 +155,14 @@ def get_current_release_version_of_group(group: Group, follows_semver: bool = Fa
"""
current_release_version = None
if follows_semver:
release = greatest_semver_release(group.project)
# Fetch all the release-packages associated with the group. We'll find the largest semver
# version for one of these packages.
group_packages = fetch_semver_packages_for_group(
organization_id=group.project.organization_id,
project_id=group.project_id,
group_id=group.id,
)
release = greatest_semver_release(group.project, packages=group_packages)
if release is not None:
current_release_version = release.version
else:
Expand Down Expand Up @@ -537,6 +545,10 @@ def process_group_resolution(
# in release
resolution_params.update(
{
"release": Release.objects.filter(
organization_id=release.organization_id,
version=current_release_version,
).get(),
Copy link
Member

Choose a reason for hiding this comment

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

PR looks good to me. One small question about the extra query we have here to get the release that matches current_release_version--can/should we remove the need for this? Disadvantages are a little bit of extra latency and edge cases where get() raises Release.DoesNotExist.

We could do an easy fix in the semver path by returning current_release alongside current_release_version in get_current_release_version_of_group, then doing current_release or Release.objects.filter( here.

The non-semver path also queries the release and returns only the version (ugh), but the logic is buried fairly deep and uses caching so I understand if we don't want to change anything in this path. But it would be cleaner and save us a query if we change get_current_release_version_of_group to get_current_release_of_group altogether. If we want to merge this PR as is, I'm happy to investigate more and potentially write a followup ref PR that does this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. Its not ideal. Definitely something we could address in a follow-up. For this PR, we have enough to worry about so we'll limit the blast radius.

On performance, yes its another query but its indexed so it will be fast. These things do add up so its best to not add queries too cavalierly. My long term goal is to refactor this code path entirely. The code, as is, is incomprehensible. An intermediate step might be the change you suggested or it might be something deeper. It just depends on the performance we measure in production and if its causing issues or not. If its not causing issues then we might focus our efforts on more substantial improvements.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Was gonna ask how I can follow up on measuring perf in prod but I think your DD walkthrough is coming up anyway!

Copy link
Member Author

Choose a reason for hiding this comment

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

Sentry's tracing product would be better! You can filter to the PUT request and find this span in there. You can see how the system performs in aggregate and determine if you need to make a perf change or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Unhandled Exception When Fetching Nonexistent Release

During issue resolution in semver projects, the code fetches a Release object using .get() with current_release_version. If current_release_version doesn't correspond to an existing Release in the database, this raises Release.DoesNotExist, causing an unhandled exception.

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.

True but since the current_release_version has already been shown to exist in the db the only way this could happen is if the release was deleted (which we don't allow). Technically this could raise but I'm not sure what I'd do with the exception anyway. Raising seems to be appropriate for now.

"type": GroupResolution.Type.in_release,
"status": GroupResolution.Status.resolved,
}
Expand Down Expand Up @@ -819,7 +831,11 @@ def get_release_to_resolve_by(project: Project) -> Release | None:
follows_semver = follows_semver_versioning_scheme(
org_id=project.organization_id, project_id=project.id
)
return greatest_semver_release(project) if follows_semver else most_recent_release(project)
return (
greatest_semver_release(project, packages=[])
if follows_semver
else most_recent_release(project)
)


def most_recent_release(project: Project) -> Release | None:
Expand All @@ -841,14 +857,20 @@ def most_recent_release_matching_commit(
)


def greatest_semver_release(project: Project) -> Release | None:
return get_semver_releases(project).first()
def greatest_semver_release(project: Project, packages: list[str]) -> Release | None:
return get_semver_releases(project, packages).first()


def get_semver_releases(project: Project, packages: list[str]) -> QuerySet[Release]:
query = Release.objects.filter(projects=project, organization_id=project.organization_id)

# Multiple packages may exist for a single project. If we were able to infer the packages
# associated with an issue we'll include them.
if packages:
query = query.filter(package__in=packages)

def get_semver_releases(project: Project) -> QuerySet[Release]:
return (
Release.objects.filter(projects=project, organization_id=project.organization_id)
.filter_to_semver() # type: ignore[attr-defined]
query.filter_to_semver() # type: ignore[attr-defined]
.annotate_prerelease_column()
.order_by(*[f"-{col}" for col in Release.SEMVER_COLS])
)
Expand Down
31 changes: 30 additions & 1 deletion src/sentry/releases/use_cases/release.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from collections import defaultdict
from collections.abc import Callable, Iterable, Mapping
from datetime import datetime, timezone
from typing import Any
from typing import Any, cast

import sentry_sdk
from django.contrib.auth.models import AnonymousUser
Expand All @@ -16,6 +16,7 @@
from sentry.models.commit import Commit
from sentry.models.commitauthor import CommitAuthor
from sentry.models.deploy import Deploy
from sentry.models.grouprelease import GroupRelease
from sentry.models.project import Project
from sentry.models.projectplatform import ProjectPlatform
from sentry.models.release import Release
Expand Down Expand Up @@ -435,3 +436,31 @@ def fetch_project_platforms(project_ids: Iterable[int]) -> list[tuple[int, str]]
"project_id", "platform"
)
)


def fetch_semver_packages_for_group(
organization_id: int, project_id: int, group_id: int
) -> list[str]:
"""Fetch a unique list of semver release packages associated with the group."""
release_ids = (
GroupRelease.objects.filter(
group_id=group_id,
project_id=project_id,
)
.distinct()
.values_list("release_id", flat=True)
)
Comment on lines +445 to +452
Copy link

Choose a reason for hiding this comment

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

High severity vulnerability may affect your project—review required:
Line 445 lists a dependency (django) with a known High severity vulnerability.

ℹ️ Why this matters

Affected versions of django are vulnerable to Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection'). SQL injection in Django's ORM column aliases: when using QuerySet.annotate(), QuerySet.alias(), QuerySet.aggregate(), or QuerySet.extra() with dictionary expansion (**kwargs), the dictionary keys are used unescaped as SQL column aliases. On MySQL and MariaDB backends, an attacker who can influence those keys (for example, by passing a crafted dict of annotations) can inject arbitrary SQL into the generated query.

References: GHSA, CVE

To resolve this comment:
Check if you are using Django with MySQL or MariaDB.

  • If you're affected, upgrade this dependency to at least version 5.2.7 at uv.lock.
  • If you're not affected, comment /fp we don't use this [condition]
💬 Ignore this finding

To ignore this, reply with:

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

You can view more details on this finding in the Semgrep AppSec Platform here.


return cast(
list[str],
list(
Release.objects.filter_to_semver()
.filter(
organization_id=organization_id,
id__in=release_ids,
package__isnull=False,
)
.distinct()
.values_list("package", flat=True)
),
)
60 changes: 60 additions & 0 deletions tests/sentry/issues/endpoints/test_organization_group_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3588,6 +3588,66 @@ def test_set_resolved_in_next_release(self) -> None:
)
assert activity.data["version"] == ""

def test_set_resolved_in_next_semver_release(self) -> None:
release = Release.objects.create(
organization_id=self.project.organization_id, version="[email protected]"
)
release.add_project(self.project)

# Smaller than 1.0.0 but more recent.
release2 = Release.objects.create(
organization_id=self.project.organization_id, version="[email protected]"
)
release2.add_project(self.project)

# Bigger than 1.0.0 and more recent but different package.
release3 = Release.objects.create(
organization_id=self.project.organization_id, version="[email protected]"
)
release3.add_project(self.project)

# Smaller than 1.0.0, a different package, and associated to the group. This package's
# release should be in contention for largest semver but not selected.
release4 = Release.objects.create(
organization_id=self.project.organization_id, version="[email protected]"
)
release4.add_project(self.project)

group = self.create_group(status=GroupStatus.UNRESOLVED)

# Record the release as a group release so the group is scoped to at least one package.
self.create_group_release(self.project, group=group, release=release)
self.create_group_release(self.project, group=group, release=release4)

self.login_as(user=self.user)

response = self.get_success_response(
qs_params={"id": group.id}, status="resolved", statusDetails={"inNextRelease": True}
)
assert response.data["status"] == "resolved"
assert response.data["statusDetails"]["inNextRelease"]
assert response.data["statusDetails"]["actor"]["id"] == str(self.user.id)
assert "activity" in response.data

group = Group.objects.get(id=group.id)
assert group.status == GroupStatus.RESOLVED

resolution = GroupResolution.objects.get(group=group)
assert resolution.release == release
assert resolution.type == GroupResolution.Type.in_release
assert resolution.status == GroupResolution.Status.resolved
assert resolution.actor_id == self.user.id

assert GroupSubscription.objects.filter(
user_id=self.user.id, group=group, is_active=True
).exists()

activity = Activity.objects.get(
group=group, type=ActivityType.SET_RESOLVED_IN_RELEASE.value
)
assert activity.data["version"] == ""
assert activity.data["current_release_version"] == "[email protected]"

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