-
Notifications
You must be signed in to change notification settings - Fork 929
fix: improve error message when deleting organization with resources #17049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
b30a486
cc1c564
2711ded
e94e7c9
d193edd
5c0fb09
9645382
4cfd54c
8fd0740
a24364f
b84d70e
27320c8
d17e29f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
-- Drop trigger that uses this function | ||
DROP TRIGGER IF EXISTS protect_deleting_organizations ON organizations; | ||
|
||
-- Revert the function to its original implementation | ||
CREATE OR REPLACE FUNCTION protect_deleting_organizations() | ||
RETURNS TRIGGER AS | ||
$$ | ||
DECLARE | ||
workspace_count int; | ||
template_count int; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. classic tabs vs spaces moment (you've got 4 spaces on one line and a tab on the next) |
||
F438 | group_count int; | |
member_count int; | ||
provisioner_keys_count int; | ||
BEGIN | ||
workspace_count := ( | ||
SELECT count(*) as count FROM workspaces | ||
WHERE | ||
workspaces.organization_id = OLD.id | ||
AND workspaces.deleted = false | ||
); | ||
|
||
template_count := ( | ||
SELECT count(*) as count FROM templates | ||
WHERE | ||
templates.organization_id = OLD.id | ||
AND templates.deleted = false | ||
); | ||
|
||
group_count := ( | ||
SELECT count(*) as count FROM groups | ||
WHERE | ||
groups.organization_id = OLD.id | ||
); | ||
|
||
member_count := ( | ||
SELECT count(*) as count FROM organization_members | ||
WHERE | ||
organization_members.organization_id = OLD.id | ||
); | ||
|
||
provisioner_keys_count := ( | ||
Select count(*) as count FROM provisioner_keys | ||
WHERE | ||
provisioner_keys.organization_id = OLD.id | ||
); | ||
|
||
-- Fail the deletion if one of the following: | ||
-- * the organization has 1 or more workspaces | ||
-- * the organization has 1 or more templates | ||
-- * the organization has 1 or more groups other than "Everyone" group | ||
-- * the organization has 1 or more members other than the organization owner | ||
-- * the organization has 1 or more provisioner keys | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. goes back and forth again here, and actually a lot throughout the file 😅 I usually normalize to tabs, but I don't think we really have a consistent indentation style in sql. it is nice to pick one and stick with it within a file at least. |
||
|
||
IF (workspace_count + template_count + provisioner_keys_count) > 0 THEN | ||
RAISE EXCEPTION 'cannot delete organization: organization has % workspaces, % templates, and % provisioner keys that must be deleted first', workspace_count, template_count, provisioner_keys_count; | ||
END IF; | ||
|
||
IF (group_count) > 1 THEN | ||
RAISE EXCEPTION 'cannot delete organization: organization has % groups that must be deleted first', group_count - 1; | ||
END IF; | ||
|
||
-- Allow 1 member to exist, because you cannot remove yourself. You can | ||
-- remove everyone else. Ideally, we only omit the member that matches | ||
-- the user_id of the caller, however in a trigger, the caller is unknown. | ||
IF (member_count) > 1 THEN | ||
RAISE EXCEPTION 'cannot delete organization: organization has % members that must be deleted first', member_count - 1; | ||
END IF; | ||
|
||
RETURN NEW; | ||
END; | ||
$$ LANGUAGE plpgsql; | ||
|
||
-- Re-create trigger that uses this function | ||
CREATE TRIGGER protect_deleting_organizations | ||
BEFORE DELETE ON organizations | ||
FOR EACH ROW | ||
EXECUTE FUNCTION protect_deleting_organizations(); |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and this file also has inconsistent indentation throughout. we're really not usually picky about sql formatting, but sticking with a consistent indentation character/width is nice. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
DROP TRIGGER IF EXISTS protect_deleting_organizations ON organizations; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems a bit over defensive, but also doesn't really hurt anything. mostly I'm just wondering if you think this needs to stay or if this is just some weird thing that your little buddy dreamt up 😂 |
||
|
||
-- Replace the function with the new implementation | ||
CREATE OR REPLACE FUNCTION protect_deleting_organizations() | ||
RETURNS TRIGGER AS | ||
$$ | ||
DECLARE | ||
workspace_count int; | ||
template_count int; | ||
group_count int; | ||
member_count int; | ||
provisioner_keys_count int; | ||
BEGIN | ||
workspace_count := ( | ||
SELECT count(*) as count FROM workspaces | ||
WHERE | ||
workspaces.organization_id = OLD.id | ||
AND workspaces.deleted = false | ||
); | ||
|
||
template_count := ( | ||
SELECT count(*) as count FROM templates | ||
WHERE | ||
templates.organization_id = OLD.id | ||
AND templates.deleted = false | ||
); | ||
|
||
group_count := ( | ||
SELECT count(*) as count FROM groups | ||
WHERE | ||
groups.organization_id = OLD.id | ||
); | ||
|
||
member_count := ( | ||
SELECT count(*) as count FROM organization_members | ||
WHERE | ||
organization_members.organization_id = OLD.id | ||
); | ||
|
||
provisioner_keys_count := ( | ||
Select count(*) as count FROM provisioner_keys | ||
WHERE | ||
provisioner_keys.organization_id = OLD.id | ||
); | ||
|
||
-- Fail the deletion if one of the following: | ||
-- * the organization has 1 or more workspaces | ||
-- * the organization has 1 or more templates | ||
-- * the organization has 1 or more groups other than "Everyone" group | ||
-- * the organization has 1 or more members other than the organization owner | ||
-- * the organization has 1 or more provisioner keys | ||
|
||
-- Only create error message for resources that actually exist | ||
IF (workspace_count + template_count + provisioner_keys_count) > 0 THEN | ||
DECLARE | ||
error_message text := 'cannot delete organization: organization has '; | ||
error_parts text[] := '{}'; | ||
BEGIN | ||
IF workspace_count > 0 THEN | ||
error_parts := array_append(error_parts, workspace_count || ' workspaces'); | ||
END IF; | ||
|
||
IF template_count > 0 THEN | ||
error_parts := array_append(error_parts, template_count || ' templates'); | ||
END IF; | ||
|
||
IF provisioner_keys_count > 0 THEN | ||
error_parts := array_append(error_parts, provisioner_keys_count || ' provisioner keys'); | ||
END IF; | ||
|
||
error_message := error_message || array_to_string(error_parts, ', ') || ' that must be deleted first'; | ||
RAISE EXCEPTION '%', error_message; | ||
END; | ||
END IF; | ||
|
||
IF (group_count) > 1 THEN | ||
RAISE EXCEPTION 'cannot delete organization: organization has % groups that must be deleted first', group_count - 1; | ||
END IF; | ||
|
||
-- Allow 1 member to exist, because you cannot remove yourself. You can | ||
-- remove everyone else. Ideally, we only omit the member that matches | ||
-- the user_id of the caller, however in a trigger, the caller is unknown. | ||
IF (member_count) > 1 THEN | ||
RAISE EXCEPTION 'cannot delete organization: organization has % members that must be deleted first', member_count - 1; | ||
END IF; | ||
|
||
RETURN NEW; | ||
END; | ||
$$ LANGUAGE plpgsql; | ||
|
||
-- Trigger to protect organizations from being soft deleted with existing resources | ||
CREATE TRIGGER protect_deleting_organizations | ||
BEFORE UPDATE ON organizations | ||
FOR EACH ROW | ||
WHEN (NEW.deleted = true AND OLD.deleted = false) | ||
EXECUTE FUNCTION protect_deleting_organizations(); |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like right now your down migration is just a copy of your up migration.
I think your down migration really only needs to be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think there were some copy paste errors as I changed from another branch to this one (long story). Probably also why there is the tabs vs spaces issue.
The down migration can't just be dropping the function since the function already exists, as far as I understand I'd have to re-declare the original function from a previous migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually taking a closer look now the down migration is correct, or at least what I meant it to be. In it we drop the existing functions and triggers and re-create the old one from migration 296