Skip to content
Merged
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
2 changes: 1 addition & 1 deletion src/sentry/api/serializers/models/dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ def get_attrs(self, item_list, user, **kwargs):

favorited_dashboard_ids = set(
DashboardFavoriteUser.objects.filter(
user_id=user.id, dashboard_id__in=item_dict.keys()
user_id=user.id, dashboard_id__in=item_dict.keys(), favorited=True
).values_list("dashboard_id", flat=True)
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ def put(
dashboard=dashboard,
)
else:
DashboardFavoriteUser.objects.delete_favorite_dashboard(
DashboardFavoriteUser.objects.unfavorite_dashboard(
organization=organization,
user_id=request.user.id,
dashboard=dashboard,
Expand Down
19 changes: 15 additions & 4 deletions src/sentry/dashboards/endpoints/organization_dashboards.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
IntegerField,
OrderBy,
OuterRef,
Q,
Subquery,
Value,
When,
Expand Down Expand Up @@ -374,9 +375,15 @@ def get(self, request: Request, organization: Organization) -> Response:
dashboards = Dashboard.objects.filter(organization_id=organization.id)
for f in filters:
if f == "onlyFavorites":
dashboards = dashboards.filter(dashboardfavoriteuser__user_id=request.user.id)
dashboards = dashboards.filter(
dashboardfavoriteuser__user_id=request.user.id,
dashboardfavoriteuser__favorited=True,
)
elif f == "excludeFavorites":
dashboards = dashboards.exclude(dashboardfavoriteuser__user_id=request.user.id)
dashboards = dashboards.exclude(
dashboardfavoriteuser__user_id=request.user.id,
dashboardfavoriteuser__favorited=True,
)
Comment thread
sentry[bot] marked this conversation as resolved.
elif f == "owned":
dashboards = dashboards.filter(created_by_id=request.user.id)
elif f == "shared":
Expand Down Expand Up @@ -493,7 +500,11 @@ def get(self, request: Request, organization: Organization) -> Response:
"organizations:dashboards-starred-reordering", organization, actor=request.user
):
dashboards = dashboards.annotate(
favorites_count=Count("dashboardfavoriteuser", distinct=True)
favorites_count=Count(
"dashboardfavoriteuser",
filter=Q(dashboardfavoriteuser__favorited=True),
distinct=True,
)
)
order_by = [
"favorites_count" if desc else "-favorites_count",
Expand All @@ -506,7 +517,7 @@ def get(self, request: Request, organization: Organization) -> Response:
pin_by = request.query_params.get("pin")
if pin_by == "favorites":
favorited_by_subquery = DashboardFavoriteUser.objects.filter(
dashboard=OuterRef("pk"), user_id=request.user.id
dashboard=OuterRef("pk"), user_id=request.user.id, favorited=True
)

order_by_favorites = [
Expand Down
106 changes: 76 additions & 30 deletions src/sentry/models/dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
self.filter(
organization=organization,
user_id=user_id,
favorited=True,
position__isnull=False,
)
.order_by("-position")
Expand All @@ -60,7 +61,7 @@
"""
Returns all favorited dashboards for a user in an organization.
"""
return self.filter(organization=organization, user_id=user_id).order_by(
return self.filter(organization=organization, user_id=user_id, favorited=True).order_by(
"position", "dashboard__title"
)

Expand All @@ -70,7 +71,9 @@
"""
Returns the favorite dashboard if it exists, otherwise None.
"""
return self.filter(organization=organization, user_id=user_id, dashboard=dashboard).first()
return self.filter(
organization=organization, user_id=user_id, dashboard=dashboard, favorited=True
).first()

def reorder_favorite_dashboards(
self, organization: Organization, user_id: int, new_dashboard_positions: list[int]
Expand All @@ -90,6 +93,7 @@
existing_favorite_dashboards = self.filter(
organization=organization,
user_id=user_id,
favorited=True,
)

existing_dashboard_ids = {
Expand Down Expand Up @@ -141,33 +145,52 @@
True if the dashboard was favorited, False if the dashboard was already favorited
"""
with transaction.atomic(using=router.db_for_write(DashboardFavoriteUser)):
if self.get_favorite_dashboard(organization, user_id, dashboard):
# Lock any existing row to prevent concurrent insert races
existing = (
self.filter(
Comment thread
DominikB2014 marked this conversation as resolved.
organization=organization,
user_id=user_id,
dashboard=dashboard,
)
.select_for_update()
.first()
)

if existing and existing.favorited:
return False

if self.count() == 0:
existing_favorites = self.filter(
organization=organization, user_id=user_id, favorited=True
)
if not existing_favorites.exists():
position = 0
else:
position = self.get_last_position(organization, user_id) + 1

self.create(
organization=organization,
user_id=user_id,
dashboard=dashboard,
position=position,
)
if existing:
existing.favorited = True
existing.position = position
existing.save(update_fields=["favorited", "position"])
else:
self.create(
organization=organization,
user_id=user_id,
dashboard=dashboard,
position=position,
)
Comment thread
DominikB2014 marked this conversation as resolved.
return True

def delete_favorite_dashboard(
def unfavorite_dashboard(
self, organization: Organization, user_id: int, dashboard: Dashboard
) -> bool:
"""
Deletes a favorited dashboard from the list.
Decrements the position of all dashboards after the deletion point.
Unfavorites a dashboard by setting favorited=False and clearing its position.
Decrements the position of all remaining favorited dashboards after the removal point.

Args:
organization: The organization the dashboards belong to
user_id: The ID of the user whose favorited dashboards are being updated
dashboard: The dashboard to delete
dashboard: The dashboard to unfavorite

Returns:
True if the dashboard was unfavorited, False if the dashboard was already unfavorited
Expand All @@ -177,10 +200,15 @@
return False

deleted_position = favorite.position
favorite.delete()
favorite.favorited = False
favorite.position = None
favorite.save(update_fields=["favorited", "position"])

self.filter(
organization=organization, user_id=user_id, position__gt=deleted_position
organization=organization,
user_id=user_id,
favorited=True,
position__gt=deleted_position,
).update(position=models.F("position") - 1)
Comment thread
sentry[bot] marked this conversation as resolved.
return True

Expand Down Expand Up @@ -265,7 +293,7 @@
"""
@deprecated Use the DashboardFavoriteUser object manager instead.
"""
user_ids = DashboardFavoriteUser.objects.filter(dashboard=self).values_list(
user_ids = DashboardFavoriteUser.objects.filter(dashboard=self, favorited=True).values_list(
Comment thread
DominikB2014 marked this conversation as resolved.
"user_id", flat=True
Comment thread
DominikB2014 marked this conversation as resolved.
)
return user_ids
Expand All @@ -277,21 +305,39 @@
"""
from django.db import router, transaction

existing_user_ids = DashboardFavoriteUser.objects.filter(dashboard=self).values_list(
"user_id", flat=True
)
existing_favorites = DashboardFavoriteUser.objects.filter(dashboard=self)
existing_user_ids = set(existing_favorites.values_list("user_id", flat=True))
new_user_ids = set(user_ids)

with transaction.atomic(using=router.db_for_write(DashboardFavoriteUser)):
newly_favourited = [
DashboardFavoriteUser(
dashboard=self, user_id=user_id, organization=self.organization
)
for user_id in set(user_ids) - set(existing_user_ids)
]
DashboardFavoriteUser.objects.filter(
dashboard=self, user_id__in=set(existing_user_ids) - set(user_ids)
).delete()
DashboardFavoriteUser.objects.bulk_create(newly_favourited)
existing_favorites.filter(
user_id__in=existing_user_ids - new_user_ids, favorited=True
).update(favorited=False, position=None)

users_to_add = new_user_ids - existing_user_ids
users_to_refavorite = new_user_ids & existing_user_ids

for user_id in users_to_refavorite:
fav = existing_favorites.filter(user_id=user_id, favorited=False).first()
if fav:
last_position = DashboardFavoriteUser.objects.get_last_position(
self.organization, user_id
)
has_any = DashboardFavoriteUser.objects.filter(
organization=self.organization, user_id=user_id, favorited=True
).exists()
fav.favorited = True
fav.position = 0 if not has_any else last_position + 1
fav.save(update_fields=["favorited", "position"])
DashboardFavoriteUser.objects.bulk_create(
[
DashboardFavoriteUser(
dashboard=self, user_id=user_id, organization=self.organization
)
for user_id in users_to_add
]
)
Comment thread
DominikB2014 marked this conversation as resolved.

Check warning on line 340 in src/sentry/models/dashboard.py

View check run for this annotation

@sentry/warden / warden: sentry-backend-bugs

bulk_create can fail with IntegrityError due to race condition

The `existing_favorites` query is executed outside the transaction (line 305), so concurrent requests can both see the same `existing_user_ids` set, compute the same `users_to_add`, and then one of the `bulk_create` calls will fail with `IntegrityError` due to the unique constraint on `(user_id, dashboard)`. Similar to SENTRY-5HSD patterns where concurrent operations on unique-constrained models cause IntegrityError.
@staticmethod
def get_prebuilt_list(organization, user, title_query=None):
query = list(
Expand Down
9 changes: 7 additions & 2 deletions tests/sentry/models/test_dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,18 @@ def test_deletes_and_increments_existing_positions(self) -> None:

assert DashboardFavoriteUser.objects.count() == 2

DashboardFavoriteUser.objects.delete_favorite_dashboard(
DashboardFavoriteUser.objects.unfavorite_dashboard(
self.organization, self.user.id, first_dashboard
)

dashboard = DashboardFavoriteUser.objects.get_favorite_dashboard(
self.organization, self.user.id, second_dashboard
)
assert DashboardFavoriteUser.objects.count() == 1
# Row still exists but with favorited=False
assert DashboardFavoriteUser.objects.count() == 2
assert DashboardFavoriteUser.objects.filter(favorited=True).count() == 1
unfavorited = DashboardFavoriteUser.objects.get(dashboard=first_dashboard)
assert unfavorited.favorited is False
assert unfavorited.position is None
assert dashboard is not None
assert dashboard.position == 0
Loading