diff --git a/src/sentry/discover/models.py b/src/sentry/discover/models.py index 8f13623acab9..310e9b0db252 100644 --- a/src/sentry/discover/models.py +++ b/src/sentry/discover/models.py @@ -15,6 +15,7 @@ from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey from sentry.db.models.manager.base import BaseManager from sentry.models.dashboard_widget import TypesClass +from sentry.models.organization import Organization from sentry.models.projectteam import ProjectTeam from sentry.tasks.relay import schedule_invalidate_project_config @@ -213,6 +214,113 @@ class Meta: unique_together = (("project_team", "transaction"),) +class DiscoverSavedQueryStarredManager(BaseManager["DiscoverSavedQueryStarred"]): + """ + Positions here are not local to this table, being shared with ExploreSavedQueryStarred. + See `explore/utils.py` and `saved_query_starred_order.py` for the implementation details. + """ + + def get_starred_query( + self, organization: Organization, user_id: int, query: DiscoverSavedQuery + ) -> DiscoverSavedQueryStarred | None: + """ + Returns the starred query if it exists, otherwise None. + """ + return self.filter( + organization=organization, user_id=user_id, discover_saved_query=query + ).first() + + def insert_starred_query( + self, + organization: Organization, + user_id: int, + query: DiscoverSavedQuery, + starred: bool = True, + ) -> bool: + """ + Inserts a new starred query at the end of the shared list. + + Args: + organization: The organization the queries belong to + user_id: The ID of the user whose starred queries are being updated + discover_saved_query: The query to insert + + Returns: + True if the query was starred, False if the query was already starred + """ + from sentry.explore.utils import next_starred_position + + with transaction.atomic(using=router.db_for_write(DiscoverSavedQueryStarred)): + if self.get_starred_query(organization, user_id, query): + return False + + self.create( + organization=organization, + user_id=user_id, + discover_saved_query=query, + position=next_starred_position(organization, user_id), + starred=starred, + ) + return True + + def delete_starred_query( + self, organization: Organization, user_id: int, query: DiscoverSavedQuery + ) -> bool: + """ + Deletes a starred query from the list. + Decrements the position of all later queries in both tables to close the gap. + + Args: + organization: The organization the queries belong to + user_id: The ID of the user whose starred queries are being updated + discover_saved_query: The query to delete + + Returns: + True if the query was unstarred, False if the query was already unstarred + """ + from sentry.explore.utils import shift_starred_positions_by_one + + with transaction.atomic(using=router.db_for_write(DiscoverSavedQueryStarred)): + if not (starred_query := self.get_starred_query(organization, user_id, query)): + return False + + deleted_position = starred_query.position + starred_query.delete() + + # A row unstarred via ``updated_starred_query`` holds no position and so left no + # gap to close. Filtering on ``position__gt=None`` would raise, not match nothing. + if deleted_position is not None: + shift_starred_positions_by_one( + organization, user_id, from_position=deleted_position + ) + return True + + def updated_starred_query( + self, + organization: Organization, + user_id: int, + query: DiscoverSavedQuery, + starred: bool, + ) -> bool: + """ + Updates the starred status of a query. + """ + from sentry.explore.utils import next_starred_position + + with transaction.atomic(using=router.db_for_write(DiscoverSavedQueryStarred)): + if not (starred_query := self.get_starred_query(organization, user_id, query)): + return False + + starred_query.starred = starred + if starred: + starred_query.position = next_starred_position(organization, user_id) + else: + starred_query.position = None + + starred_query.save() + return True + + @cell_silo_model class DiscoverSavedQueryStarred(DefaultFieldsModel): __relocation_scope__ = RelocationScope.Excluded @@ -224,6 +332,8 @@ class DiscoverSavedQueryStarred(DefaultFieldsModel): position = models.PositiveSmallIntegerField(null=True, db_default=None) starred = models.BooleanField(db_default=True) + objects: ClassVar[DiscoverSavedQueryStarredManager] = DiscoverSavedQueryStarredManager() + class Meta: app_label = "discover" db_table = "sentry_discoversavedquerystarred" diff --git a/src/sentry/explore/endpoints/saved_query_starred_order.py b/src/sentry/explore/endpoints/saved_query_starred_order.py index 2c4dc348ef28..8ff3c71f844e 100644 --- a/src/sentry/explore/endpoints/saved_query_starred_order.py +++ b/src/sentry/explore/endpoints/saved_query_starred_order.py @@ -1,6 +1,6 @@ from typing import Any -from django.db import router, transaction +from django.db import IntegrityError, router, transaction from rest_framework import serializers, status from rest_framework.exceptions import ParseError from rest_framework.request import Request @@ -84,7 +84,7 @@ def put(self, request: Request, organization: Organization) -> Response: # DiscoverSavedQueryStarred should be in the same db as ExploreSavedQueryStarred. with transaction.atomic(using=router.db_for_write(ExploreSavedQueryStarred)): utils.reorder_starred_queries(organization, request.user.id, refs) - except ValueError: + except (IntegrityError, ValueError): raise ParseError("Mismatch between existing and provided starred queries.") return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/src/sentry/explore/utils.py b/src/sentry/explore/utils.py index 0351ff1a9bce..1ddd35b81fa4 100644 --- a/src/sentry/explore/utils.py +++ b/src/sentry/explore/utils.py @@ -90,50 +90,47 @@ def reorder_starred_queries( starred query the user has, not just those of one product. Raises: - ValueError: if ``refs`` is not exactly the set of the user's starred rows, or - contains a duplicate. + ValueError: if ``refs`` is not exactly the set of the user's starred rows """ - requested = list(refs) - if len(requested) != len(set(requested)): - raise ValueError("Single query cannot take up multiple positions.") + new_query_positions = list(refs) # grab all starred queries in both tables, and map based on SavedQueryRef. discover_starred_queries = DiscoverSavedQueryStarred.objects.filter( - organization=organization, user_id=user_id, position__isnull=False - ).filter(organization=organization, user_id=user_id, position__isnull=False, starred=True) + organization=organization, user_id=user_id, position__isnull=False, starred=True + ) explore_starred_queries = ExploreSavedQueryStarred.objects.filter( organization=organization, user_id=user_id, position__isnull=False, starred=True ) - combined_starred_queries_map: dict[ - SavedQueryRef, DiscoverSavedQueryStarred | ExploreSavedQueryStarred - ] = {} + existing_query_refs: set[SavedQueryRef] = set() for discover_row in discover_starred_queries: - combined_starred_queries_map[ + existing_query_refs.add( SavedQueryRef(SavedQueryType.DISCOVER, discover_row.discover_saved_query_id) - ] = discover_row + ) for explore_row in explore_starred_queries: - combined_starred_queries_map[ + existing_query_refs.add( SavedQueryRef(SavedQueryType.EXPLORE, explore_row.explore_saved_query_id) - ] = explore_row + ) - if combined_starred_queries_map.keys() != set(requested): + if existing_query_refs != set(new_query_positions): raise ValueError("Mismatch between existing and provided starred queries.") # normalize positions to 1...N, then assign them in order of the ref sequence provided - slots = range(1, len(requested) + 1) + position_map = {ref: position for position, ref in enumerate(new_query_positions, start=1)} discover_updates: list[DiscoverSavedQueryStarred] = [] explore_updates: list[ExploreSavedQueryStarred] = [] - for ref, new_position in zip(requested, slots): - row = combined_starred_queries_map[ref] - row.position = new_position - if isinstance(row, ExploreSavedQueryStarred): - explore_updates.append(row) - else: - discover_updates.append(row) + for discover_row in discover_starred_queries: + discover_ref = SavedQueryRef(SavedQueryType.DISCOVER, discover_row.discover_saved_query_id) + discover_row.position = position_map[discover_ref] + discover_updates.append(discover_row) + + for explore_row in explore_starred_queries: + explore_ref = SavedQueryRef(SavedQueryType.EXPLORE, explore_row.explore_saved_query_id) + explore_row.position = position_map[explore_ref] + explore_updates.append(explore_row) ExploreSavedQueryStarred.objects.bulk_update(explore_updates, ["position"]) DiscoverSavedQueryStarred.objects.bulk_update(discover_updates, ["position"]) diff --git a/tests/sentry/explore/test_utils.py b/tests/sentry/explore/test_utils.py index 3fb0e2c40652..eadd4fa6ee44 100644 --- a/tests/sentry/explore/test_utils.py +++ b/tests/sentry/explore/test_utils.py @@ -159,15 +159,6 @@ def test_moves_a_query_from_the_end_to_the_front(self) -> None: assert self.ordered_refs() == refs - def test_rejects_duplicate_ref(self) -> None: - discover = self.discover_star(1) - self.explore_star(2) - - ref = SavedQueryRef(SavedQueryType.DISCOVER, discover.discover_saved_query_id) - - with pytest.raises(ValueError, match="multiple positions"): - utils.reorder_starred_queries(self.org, self.user.id, [ref, ref]) - def test_rejects_missing_refs(self) -> None: # The failure mode this module exists to prevent: a caller that knows about one # product sends only its own queries, and the other product's positions are lost.