Skip to content
Closed

Pr 101615 #101658

Show file tree
Hide file tree
Changes from all 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
50 changes: 48 additions & 2 deletions src/sentry/api/helpers/group_index/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class ResolutionParams(TypedDict):
status: int | None
actor_id: int | None
current_release_version: NotRequired[str]
future_release_version: NotRequired[str]


def handle_discard(
Expand Down Expand Up @@ -221,7 +222,7 @@ def update_groups(
acting_user=acting_user,
project_lookup=project_lookup,
)
if status in ("resolved", "resolvedInNextRelease"):
if status in ("resolved", "resolvedInNextRelease", "resolvedInFutureRelease"):
try:
result, res_type = handle_resolve_in_release(
status,
Expand Down Expand Up @@ -356,6 +357,7 @@ def handle_resolve_in_release(
) -> tuple[dict[str, Any], int | None]:
res_type = None
release = None
future_release_version = None
commit = None
self_assign_issue = "0"
new_status_details = {}
Expand Down Expand Up @@ -389,6 +391,38 @@ def handle_resolve_in_release(
res_type = GroupResolution.Type.in_next_release
res_type_str = "in_next_release"
res_status = GroupResolution.Status.pending
elif status == "resolvedInFutureRelease" or status_details.get("inFutureRelease"):
if len(projects) > 1:
raise MultipleProjectsError()

release = status_details.get("inFutureRelease")
# Release to resolve by may not exist yet, so just use a random placeholder
# TODO: THIS CAN NEVER BE NONE -- what if no releases exist yet?
release_placeholder = release or get_release_to_resolve_by(projects[0])
# Get the original version string stored by the validator
future_release_version = status_details.get("_future_release_version")

activity_type = ActivityType.SET_RESOLVED_IN_RELEASE.value

if release:
# Release exists, so just resolve in_release
new_status_details["inRelease"] = release.version
res_type = GroupResolution.Type.in_release
res_type_str = "in_release"
res_status = GroupResolution.Status.resolved
activity_data = {"version": release.version}
else:
new_status_details["inFutureRelease"] = future_release_version
res_type = GroupResolution.Type.in_future_release
res_type_str = "in_future_release"
res_status = GroupResolution.Status.pending

# Pass placeholder release to process_group_resolution
release = release_placeholder
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Null Release Causes Database Constraint Violation

Resolving an issue as resolvedInFutureRelease can lead to a database constraint violation. If no releases exist for the project, get_release_to_resolve_by() returns None, which is then used for the non-nullable release foreign key when creating a GroupResolution object.

Fix in Cursor Fix in Web


# Leave activity_data["version"] as ""
# and set activity_data["future_release_version"] in process_group_resolution
Comment on lines +414 to +424
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential bug: When resolving an issue in a future release that doesn't exist, activity_data is not initialized, causing a NameError in process_group_resolution.
  • Description: When resolving an issue with a status of resolvedInFutureRelease for a release that does not yet exist, the release variable is None. This directs control flow to an else block where the activity_data variable is never initialized. Later, this uninitialized variable is passed as an argument to the process_group_resolution function, which will raise a NameError. This failure occurs during a primary use case for the feature, which is resolving an issue against a future, non-existent release version.

  • Suggested fix: Initialize activity_data to an empty dictionary within the else block that handles future releases that do not yet exist. For example, add activity_data = {} at the beginning of the block, similar to how other resolution paths handle this variable.
    severity: 0.9, confidence: 0.98

Did we get this right? 👍 / 👎 to inform future reviews.


Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Uninitialized Variable Causes Error

The activity_data variable is not initialized in handle_resolve_in_release when handling resolvedInFutureRelease if the target release doesn't exist yet. This causes a NameError or UnboundLocalError when activity_data is later accessed or passed to process_group_resolution.

Fix in Cursor Fix in Web

elif status_details.get("inRelease"):
# TODO(jess): We could update validation to check if release
# applies to multiple projects, but I think we agreed to punt
Expand Down Expand Up @@ -452,6 +486,7 @@ def handle_resolve_in_release(
group,
group_list,
release,
future_release_version,
commit,
res_type,
res_status,
Expand Down Expand Up @@ -484,6 +519,7 @@ def process_group_resolution(
group: Group,
group_list: Sequence[Group],
release: Release | None,
future_release_version: str | None,
commit: Commit | None,
res_type: int | None,
res_status: int | None,
Expand Down Expand Up @@ -592,6 +628,12 @@ def process_group_resolution(
# fall back to our current model
...

elif res_type == GroupResolution.Type.in_future_release and future_release_version:
resolution_params.update({"future_release_version": future_release_version})

# Activity status should look like: "... resolved in version >future_release_version"
activity_data.update({"future_release_version": future_release_version})

resolution, created = GroupResolution.objects.get_or_create(
group=group, defaults=resolution_params
)
Expand Down Expand Up @@ -754,7 +796,11 @@ def prepare_response(
# what performance impact this might have & this possibly should be moved else where
try:
if len(group_list) == 1:
if res_type in (GroupResolution.Type.in_next_release, GroupResolution.Type.in_release):
if res_type in (
GroupResolution.Type.in_next_release,
GroupResolution.Type.in_release,
GroupResolution.Type.in_future_release,
):
result["activity"] = serialize(
Activity.objects.get_activities_for_group(
group=group_list[0], num=ACTIVITIES_COUNT
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
from typing import NotRequired, TypedDict
from typing import Any, NotRequired, TypedDict

from drf_spectacular.utils import extend_schema_serializer
from rest_framework import serializers

from sentry import features
from sentry.api.helpers.group_index.validators.in_commit import InCommitResult, InCommitValidator
from sentry.models.release import Release


class StatusDetailsResult(TypedDict):
inFutureRelease: NotRequired[bool]
inNextRelease: NotRequired[bool]
inRelease: NotRequired[str]
inCommit: NotRequired[InCommitResult]
Expand All @@ -20,6 +22,12 @@ class StatusDetailsResult(TypedDict):

@extend_schema_serializer()
class StatusDetailsValidator(serializers.Serializer[StatusDetailsResult]):
inFutureRelease = serializers.CharField(
help_text=(
"The version of the semver release that the issue should be resolved in."
"This release can be a future release that doesn't exist yet."
)
)
inNextRelease = serializers.BooleanField(
help_text="If true, marks the issue as resolved in the next release."
)
Expand Down Expand Up @@ -87,3 +95,56 @@ def validate_inNextRelease(self, value: bool) -> "Release":
raise serializers.ValidationError(
"No release data present in the system to form a basis for 'Next Release'"
)

def validate_inFutureRelease(self, value: str) -> "Release | None":
project = self.context["project"]

if not features.has("organizations:resolve-in-future-release", project.organization):
raise serializers.ValidationError(
"Your organization does not have access to this feature."
)

if not Release.is_valid_version(value):
raise serializers.ValidationError(
"Invalid release version format. Please use semver format: [email protected][-prerelease][+build]."
)

try:
# release doesn't have to be semver if it exists
# because we'll just use the resolveInRelease logic
release = Release.objects.get(
projects=project, organization_id=project.organization_id, version=value
)
except Release.DoesNotExist:
# release must be semver if it doesn't exist
if not Release.is_semver_version(value):
raise serializers.ValidationError(
"Invalid semver format. Please use format: [email protected][-prerelease][+build]"
)
release = None

return release

def validate(self, attrs: dict[str, Any]) -> dict[str, Any]:
"""
Cross-field validation hook called by DRF after individual field validation.
"""
return self._preserve_future_release_version(attrs)

def _preserve_future_release_version(self, attrs: dict[str, Any]) -> dict[str, Any]:
"""
Store the original future release version string for inFutureRelease since the validator
transforms it to a Release object or None, but we need the version string for
process_group_resolution.
"""
if "inFutureRelease" in attrs:
initial_data = getattr(self, "initial_data", {})
# If this is a nested serializer, try to get the data from the parent's initial_data
if not initial_data and hasattr(self, "parent"):
parent_initial_data = getattr(self.parent, "initial_data", {})
initial_data = parent_initial_data.get("statusDetails", {})

future_release_version = initial_data.get("inFutureRelease")
if future_release_version:
attrs["_future_release_version"] = future_release_version
return attrs
7 changes: 5 additions & 2 deletions src/sentry/api/serializers/models/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ class GroupStatusDetailsResponseOptional(TypedDict, total=False):
actor: UserSerializerResponse
inNextRelease: bool
inRelease: str
inFutureRelease: str
inCommit: str
pendingEvents: int
info: Any
Expand Down Expand Up @@ -472,11 +473,13 @@ def _get_status(self, attrs: Mapping[str, Any], obj: Group):
if status == GroupStatus.RESOLVED:
status_label = "resolved"
if attrs["resolution_type"] == "release":
res_type, res_version, _ = attrs["resolution"]
res_type, res_version, future_release_version, _ = attrs["resolution"]
if res_type in (GroupResolution.Type.in_next_release, None):
status_details["inNextRelease"] = True
elif res_type == GroupResolution.Type.in_release:
status_details["inRelease"] = res_version
elif res_type == GroupResolution.Type.in_future_release:
status_details["inFutureRelease"] = future_release_version
status_details["actor"] = attrs["resolution_actor"]
elif attrs["resolution_type"] == "commit":
status_details["inCommit"] = attrs["resolution"]
Expand Down Expand Up @@ -659,7 +662,7 @@ def _resolve_resolutions(
_release_resolutions = {
i[0]: i[1:]
for i in GroupResolution.objects.filter(group__in=resolved_groups).values_list(
"group", "type", "release__version", "actor_id"
"group", "type", "release__version", "future_release_version", "actor_id"
)
}

Expand Down
2 changes: 2 additions & 0 deletions src/sentry/features/temporary.py
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,8 @@ def register_temporary_features(manager: FeatureManager) -> None:
manager.add("organizations:replay-ai-summaries-rpc", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE)
# Enable version 2 of release serializer
manager.add("organizations:releases-serializer-v2", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True)
# Enable resolve in future, possibly nonexistent release
manager.add("organizations:resolve-in-future-release", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True)
# Enable version 2 of reprocessing (completely distinct from v1)
manager.add("organizations:reprocessing-v2", OrganizationFeature, FeatureHandlerStrategy.INTERNAL, api_expose=False)
# Enable issue resolve in current semver release
Expand Down
1 change: 1 addition & 0 deletions src/sentry/models/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ class GroupStatus:
"unresolved": GroupStatus.UNRESOLVED,
"ignored": GroupStatus.IGNORED,
"resolvedInNextRelease": GroupStatus.UNRESOLVED,
"resolvedInFutureRelease": GroupStatus.UNRESOLVED,
# TODO(dcramer): remove in 9.0
"muted": GroupStatus.IGNORED,
}
Expand Down
28 changes: 26 additions & 2 deletions src/sentry/models/groupresolution.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from sentry_relay.processing import compare_version as compare_version_relay
from sentry_relay.processing import parse_release

from sentry import features
from sentry.backup.scopes import RelocationScope
from sentry.db.models import (
BoundedPositiveIntegerField,
Expand All @@ -30,6 +31,7 @@ class GroupResolution(Model):
class Type:
in_release = 0
in_next_release = 1
in_future_release = 2

class Status:
pending = 0
Expand All @@ -46,7 +48,11 @@ class Status:
# user chooses "resolve in future release"
future_release_version = models.CharField(max_length=DB_VERSION_LENGTH, null=True, blank=True)
type = BoundedPositiveIntegerField(
choices=((Type.in_next_release, "in_next_release"), (Type.in_release, "in_release")),
choices=(
(Type.in_next_release, "in_next_release"),
(Type.in_release, "in_release"),
(Type.in_future_release, "in_future_release"),
),
null=True,
)
actor_id = BoundedPositiveIntegerField(null=True)
Expand Down Expand Up @@ -90,6 +96,7 @@ def compare_release_dates_for_in_next_release(res_release, res_release_datetime,
res_release_version,
res_release_datetime,
current_release_version,
future_release_version,
) = (
cls.objects.filter(group=group)
.select_related("release")
Expand All @@ -99,6 +106,7 @@ def compare_release_dates_for_in_next_release(res_release, res_release_datetime,
"release__version",
"release__date_added",
"current_release_version",
"future_release_version",
)[0]
)
except IndexError:
Expand Down Expand Up @@ -149,10 +157,26 @@ def compare_release_dates_for_in_next_release(res_release, res_release_datetime,
except Release.DoesNotExist:
...

elif (
future_release_version
and Release.is_semver_version(release.version)
and Release.is_semver_version(future_release_version)
and features.has("organizations:resolve-in-future-release", group.organization)
):
# we have a regression if future_release_version <= given_release.version
# if future_release_version == given_release.version => 0 # regression
# if future_release_version < given_release.version => -1 # regression
# if future_release_version > given_release.version => 1
future_release_raw = parse_release(future_release_version, json_loads=orjson.loads).get(
"version_raw"
)
release_raw = parse_release(release.version, json_loads=orjson.loads).get("version_raw")
return compare_version_relay(future_release_raw, release_raw) > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Inconsistent Error Handling in Version Parsing

The clear_future_release_resolutions function calls parse_release on release.version without handling RelayError. While is_semver_version is checked, parse_release can still fail for malformed semver strings, potentially crashing the task. This contrasts with the future_release_version parsing in the same function, which includes error handling.

Additional Locations (1)

Fix in Cursor Fix in Web


# We still fallback to the older model if either current_release_version was not set (
# i.e. In all resolved cases except for Resolved in Next Release) or if for whatever
# reason the semver/date checks fail (which should not happen!)
if res_type in (None, cls.Type.in_next_release):
if res_type in (None, cls.Type.in_next_release, cls.Type.in_future_release):
# Add metric here to ensure that this code branch ever runs given that
# clear_expired_resolutions changes the type to `in_release` once a Release instance
# is created
Expand Down
Loading
Loading