feat(issue-view-sharing): Add migration to create new groupsearchviewstarred table #86065

MichaelSun48 · 2025-02-27T21:24:25Z

This PR creates a new table, groupsearchviewstarred. More details on why we are doing this can be found in this tech spec (internal only).

TL;DR — Previously we could make the assumption that all views in the original groupsearchview table were visible (starred) in the UI, so we could assign each row a position column. Now, that isn't the case, AND you can have a view in your UI that doesn't belong to you. Hence why we need a new table to keep track of which views are visible for each member, and what position they occupy.

The position column in the original groupsearchview table will be dropped once the data is migrated over and endpoint logic is reconfigured to use this new table.

github-actions · 2025-02-27T21:27:29Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0836_create_groupsearchviewstarred_table.py ()

--
-- Create model GroupSearchViewStarred
--
CREATE TABLE "sentry_groupsearchviewstarred" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "date_updated" timestamp with time zone NOT NULL, "date_added" timestamp with time zone NOT NULL, "user_id" bigint NOT NULL, "position" smallint NOT NULL CHECK ("position" >= 0), "group_search_view_id" bigint NOT NULL, "organization_id" bigint NOT NULL, CONSTRAINT "sentry_groupsearchviewstarred_unique_view_position_per_org_user" UNIQUE ("user_id", "organization_id", "position") DEFERRABLE INITIALLY DEFERRED);
ALTER TABLE "sentry_groupsearchviewstarred" ADD CONSTRAINT "sentry_groupsearchvi_group_search_view_id_9bd8a20c_fk_sentry_gr" FOREIGN KEY ("group_search_view_id") REFERENCES "sentry_groupsearchview" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupsearchviewstarred" VALIDATE CONSTRAINT "sentry_groupsearchvi_group_search_view_id_9bd8a20c_fk_sentry_gr";
ALTER TABLE "sentry_groupsearchviewstarred" ADD CONSTRAINT "sentry_groupsearchvi_organization_id_130afeb8_fk_sentry_or" FOREIGN KEY ("organization_id") REFERENCES "sentry_organization" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupsearchviewstarred" VALIDATE CONSTRAINT "sentry_groupsearchvi_organization_id_130afeb8_fk_sentry_or";
CREATE INDEX CONCURRENTLY "sentry_groupsearchviewstarred_user_id_d8550c0c" ON "sentry_groupsearchviewstarred" ("user_id");
CREATE INDEX CONCURRENTLY "sentry_groupsearchviewstarred_group_search_view_id_9bd8a20c" ON "sentry_groupsearchviewstarred" ("group_search_view_id");
CREATE INDEX CONCURRENTLY "sentry_groupsearchviewstarred_organization_id_130afeb8" ON "sentry_groupsearchviewstarred" ("organization_id");

codecov · 2025-02-27T21:34:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #86065       +/-   ##
===========================================
+ Coverage   42.23%   87.89%   +45.65%     
===========================================
  Files        9675     9706       +31     
  Lines      548727   550387     +1660     
  Branches    21419    21419               
===========================================
+ Hits       231770   483753   +251983     
+ Misses     316576    66253   -250323     
  Partials      381      381

MichaelSun48 · 2025-02-27T21:51:54Z

src/sentry/models/groupsearchviewstarred.py

+
+    user_id = HybridCloudForeignKey("sentry.User", on_delete="CASCADE")
+    organization = FlexibleForeignKey("sentry.Organization")
+    group_search_view = FlexibleForeignKey("sentry.GroupSearchView")


Question: There shouldn't be any challenges for adding an index to this column in the future, right? It's not within the scope of this project, but I could see a future where we want to know who/how many people have starred a shared view.

no, super easy

Foreign keys make indexes as part of the constraint creation.

https://stackoverflow.com/a/970605

i don't think group_search_view is unique, so i didn't think the foreign key created an index?

JoshFerge · 2025-02-27T23:48:56Z

src/sentry/models/groupsearchviewstarred.py

+    user_id = HybridCloudForeignKey("sentry.User", on_delete="CASCADE")
+    organization = FlexibleForeignKey("sentry.Organization")


why can't we use OrganizationMember instead here?

Consistency with the original groupsearchview table. I don't like it either, but otherwise we're going to have to convert between them when trying to sync the tables.

what happens to the view when the user leaves the organization?

also -- curious what this behavior today is too

Currently, I supposed their views just stay put, and if they ever rejoin then they'll still be there. Not great.

This makes me want to eventually replace the user_id/org columns with a member column

can also imagine the case where someone creates a critical view that everyone at the company uses. then when that person leaves, that view gets nuked and everyone is sad. may be worth making the user / member column nullable as well, and when a user leaves an organization, allowing for views with no owner.

Currently, I supposed their views just stay put, and if they ever rejoin then they'll still be there. Not great.

This seems like a good thing though. If you have a member leave/return they get to keep their stuff.

Its definitely convenient, but we'd slowly build up dead records (though probably to any damaging scale)

markstory

Schema looks good to me.

markstory · 2025-02-28T16:06:45Z

src/sentry/models/groupsearchviewstarred.py

+    user_id = HybridCloudForeignKey("sentry.User", on_delete="CASCADE")
+    organization = FlexibleForeignKey("sentry.Organization")


Currently, I supposed their views just stay put, and if they ever rejoin then they'll still be there. Not great.

This seems like a good thing though. If you have a member leave/return they get to keep their stuff.

markstory · 2025-02-28T16:07:19Z

src/sentry/models/groupsearchviewstarred.py

+
+    user_id = HybridCloudForeignKey("sentry.User", on_delete="CASCADE")
+    organization = FlexibleForeignKey("sentry.Organization")
+    group_search_view = FlexibleForeignKey("sentry.GroupSearchView")


Foreign keys make indexes as part of the constraint creation.

#86153) This PR updates the `PUT` `/group-search-views/` endpoint to double write the positions parameter to the new table created in [this PR](#86065). This is necessary to ensure the tables stay in sync before backfilling.

Add migration to create new groupsearchviewstarred table

5c0b5d1

vercel bot deployed to Preview February 27, 2025 21:24 View deployment

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 27, 2025

MichaelSun48 marked this pull request as ready for review February 27, 2025 21:26

MichaelSun48 requested a review from a team as a code owner February 27, 2025 21:26

MichaelSun48 requested a review from a team February 27, 2025 21:26

MichaelSun48 commented Feb 27, 2025

View reviewed changes

Use default fields model

6181cb6

vercel bot deployed to Preview February 27, 2025 23:13 View deployment

Update migration file

d7cd14b

vercel bot deployed to Preview February 27, 2025 23:26 View deployment

Fix comparator thing

3ad5600

JoshFerge reviewed Feb 27, 2025

View reviewed changes

vercel bot deployed to Preview February 27, 2025 23:50 View deployment

Fix test user thing

a998967

vercel bot deployed to Preview February 28, 2025 01:29 View deployment

markstory approved these changes Feb 28, 2025

View reviewed changes

Fix the thing again

dd3191f

vercel bot deployed to Preview February 28, 2025 17:11 View deployment

MichaelSun48 merged commit e68102b into master Feb 28, 2025
49 checks passed

MichaelSun48 deleted the msun/sharedViews/createSharedTable branch February 28, 2025 20:00

MichaelSun48 mentioned this pull request Feb 28, 2025

feat(view-sharing): Double write position to new table in PUT endpoint #86153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(issue-view-sharing): Add migration to create new groupsearchviewstarred table #86065

feat(issue-view-sharing): Add migration to create new groupsearchviewstarred table #86065

MichaelSun48 commented Feb 27, 2025

github-actions bot commented Feb 27, 2025 •

edited

Loading

codecov bot commented Feb 27, 2025 •

edited

Loading

MichaelSun48 Feb 27, 2025

JoshFerge Feb 27, 2025

markstory Feb 28, 2025

JoshFerge Feb 28, 2025

JoshFerge Feb 27, 2025

MichaelSun48 Feb 27, 2025

JoshFerge Feb 27, 2025

JoshFerge Feb 27, 2025

MichaelSun48 Feb 27, 2025 •

edited

Loading

JoshFerge Feb 28, 2025

markstory Feb 28, 2025

MichaelSun48 Feb 28, 2025

markstory left a comment

markstory Feb 28, 2025

markstory Feb 28, 2025

		user_id = HybridCloudForeignKey("sentry.User", on_delete="CASCADE")
		organization = FlexibleForeignKey("sentry.Organization")

feat(issue-view-sharing): Add migration to create new groupsearchviewstarred table #86065

feat(issue-view-sharing): Add migration to create new groupsearchviewstarred table #86065

Conversation

MichaelSun48 commented Feb 27, 2025

github-actions bot commented Feb 27, 2025 • edited Loading

codecov bot commented Feb 27, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSun48 Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markstory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 27, 2025 •

edited

Loading

codecov bot commented Feb 27, 2025 •

edited

Loading

MichaelSun48 Feb 27, 2025 •

edited

Loading