-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Pr 101615 #101658
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
Pr 101615 #101658
Changes from all commits
f75b64e
e81f0c4
1a9c49a
1ea52bd
564ad72
65d8a30
cafe382
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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( | ||
|
|
@@ -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, | ||
|
|
@@ -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 = {} | ||
|
|
@@ -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 | ||
|
|
||
| # Leave activity_data["version"] as "" | ||
| # and set activity_data["future_release_version"] in process_group_resolution | ||
|
Comment on lines
+414
to
+424
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,
|
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Uninitialized Variable Causes ErrorThe |
||
| 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 | ||
|
|
@@ -452,6 +486,7 @@ def handle_resolve_in_release( | |
| group, | ||
| group_list, | ||
| release, | ||
| future_release_version, | ||
| commit, | ||
| res_type, | ||
| res_status, | ||
|
|
@@ -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, | ||
|
|
@@ -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 | ||
| ) | ||
|
|
@@ -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 | ||
|
|
||
| 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] | ||
|
|
@@ -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." | ||
| ) | ||
|
|
@@ -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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
|
@@ -30,6 +31,7 @@ class GroupResolution(Model): | |
| class Type: | ||
| in_release = 0 | ||
| in_next_release = 1 | ||
| in_future_release = 2 | ||
|
|
||
| class Status: | ||
| pending = 0 | ||
|
|
@@ -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) | ||
|
|
@@ -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") | ||
|
|
@@ -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: | ||
|
|
@@ -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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Inconsistent Error Handling in Version ParsingThe Additional Locations (1) |
||
|
|
||
| # 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 | ||
|
|
||
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: Null Release Causes Database Constraint Violation
Resolving an issue as
resolvedInFutureReleasecan lead to a database constraint violation. If no releases exist for the project,get_release_to_resolve_by()returnsNone, which is then used for the non-nullablereleaseforeign key when creating aGroupResolutionobject.