Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

[feat] Adding a few DB triggers to delete permissions when items are soft deleted #48189

Merged
merged 2 commits into from
Feb 27, 2023

Conversation

kopancek
Copy link
Contributor

These entities are referenced via foreign key in the user_repo_permissions table, and are soft deleted potentially:

  • user
  • user_external_account
  • repo

All of them would leave rows in the user_repo_permissions table, which is not desired. The user_external_account one was the most problematic from the point of view of behavior of authzQuery.

This change only affects the new unified permissions table, old behavior is not affected.

Test plan

Tested locally, directly in the database with the following scenarios:

  1. up and down migration works correctly
  2. user_repo_permissions table is empty -> nothing happens
  3. user_repo_permissions has data from 2 users, 2 repos and 2 external accounts
    1. mark repo as deleted -> only the rows with the same repo_id are deleted
    2. mark user_external_account as deleted -> only the rows with same user_id and user_external_account are deleted
    3. mark user as deleted -> only the rows with the same user_id are deleted

…soft deleted

- user
- user_external_account
- repo

All of these would leave rows in the user_repo_permissions table, which is
not desired. The user_external_account one was the most problematic from
the point of view of behavior of authzQuery.

This only affects the new unified permissions table, old behavior is
not affected.
@kopancek kopancek requested a review from a team February 24, 2023 13:13
@kopancek kopancek self-assigned this Feb 24, 2023
@cla-bot cla-bot bot added the cla-signed label Feb 24, 2023
Copy link
Contributor

@sashaostrikov sashaostrikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kopancek kopancek merged commit ec80653 into main Feb 27, 2023
@kopancek kopancek deleted the milan/trigger_for_soft_deleted_perms_entities branch February 27, 2023 08:29
@mrnugget
Copy link
Contributor

Very cool! Can you also add some high-level "integration-y" test that ensures these triggers work and that fail if someone touches them?

Example: this test here relies on the repo statistics triggers https://github.com/sourcegraph/sourcegraph/blob/8e2430627cdfe42258eeee6bc8873f36c2a478bd/internal/database/repo_statistics_test.go#L18

It uses a DB, inserts data and then checks that the triggers did their work. I think a similar test would also work here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants