feat(discover): add starred manager and endpoint for discover saved queries

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"

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)

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"])

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.