8000 fix: improve error message when deleting organization with resources by brettkolodny · Pull Request #17049 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Mar 25, 2025
Merged

fix: improve error message when deleting organization with resources #17049

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b30a486
feat: update protect_deleting_organization function
brettkolodny Mar 21, 2025
cc1c564
fix: update querier test
brettkolodny Mar 21, 2025
2711ded
feat: add org resources query
brettkolodny Mar 21, 2025
e94e7c9
feat: implement dbauthz and dbmem for new query
brettkolodny Mar 21, 2025
d193edd
feat: add dbauthz test for new query
brettkolodny Mar 21, 2025
5c0fb09
fix: remove unused variable
brettkolodny Mar 21, 2025
9645382
feat: construct and return user fiendly error when org delete fails
brettkolodny Mar 21, 2025
4cfd54c
fix: change permissions
brettkolodny Mar 21, 2025
8fd0740
fix: consistent spacing in sql migrations
brettkolodny Mar 24, 2025
a24364f
Merge branch 'main' into brett-66783/clarify-language-in-server-error…
brettkolodny Mar 24, 2025
b84d70e
fix: migration numbers
brettkolodny Mar 24, 2025
27320c8
fix: dump
brettkolodny Mar 24, 2025
d17e29f
fix: migration numbers
brettkolodny Mar 25, 2025
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
Prev Previous commit
Next Next commit
fix: consistent spacing in sql migrations
  • Loading branch information
@brettkolodny
brettkolodny committed Mar 24, 2025
commit 8fd074043a40a88810f45d61b65b84ff91daa7ec
38 changes: 19 additions & 19 deletions coderd/database/migrations/000306_update_protect_deleting_organization_function.down.sql
View file
Open in desktop
Copy link
Member

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.

  • up migrations should add your new things
  • down migrations should revert them

I think your down migration really only needs to be

drop trigger if exists protect_deleting_organizations on organizations;
drop function if exists protect_deleting_organizations;

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ 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
RETURNS TRIGGER AS
$$
DECLARE
workspace_count int;
template_count int;
group_count int;
member_count int;
provisioner_keys_count int;
template_count int;
group_count int;
member_count int;
provisioner_keys_count int;
BEGIN
workspace_count := (
SELECT count(*) as count FROM workspaces
Expand All @@ -19,50 +19,50 @@ BEGIN
AND workspaces.deleted = false
);

template_count := (
template_count := (
SELECT count(*) as count FROM templates
WHERE
templates.organization_id = OLD.id
AND templates.deleted = false
);

group_count := (
group_count := (
SELECT count(*) as count FROM groups
WHERE
groups.organization_id = OLD.id
);

member_count := (
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
);
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
-- * 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

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
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
IF (member_count) > 1 THEN
RAISE EXCEPTION 'cannot delete organization: organization has % members that must be deleted first', member_count - 1;
END IF;

Expand Down
40 changes: 20 additions & 20 deletions coderd/database/migrations/000306_update_protect_deleting_organization_function.up.sql
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Up @@ -2,14 +2,14 @@ DROP TRIGGER IF EXISTS protect_deleting_organizations ON organizations;

-- Replace the function with the new implementation
CREATE OR REPLACE FUNCTION protect_deleting_organizations()
RETURNS TRIGGER AS
RETURNS TRIGGER AS
$$
DECLARE
workspace_count int;
template_count int;
group_count int;
member_count int;
provisioner_keys_count int;
template_count int;
group_count int;
member_count int;
provisioner_keys_count int;
BEGIN
workspace_count := (
SELECT count(*) as count FROM workspaces
Expand All @@ -18,37 +18,37 @@ BEGIN
AND workspaces.deleted = false
);

template_count := (
template_count := (
SELECT count(*) as count FROM templates
WHERE
templates.organization_id = OLD.id
AND templates.deleted = false
);

group_count := (
group_count := (
SELECT count(*) as count FROM groups
WHERE
groups.organization_id = OLD.id
);

member_count := (
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
);
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
-- * 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
Expand All @@ -73,14 +73,14 @@ BEGIN
END;
END IF;

IF (group_count) > 1 THEN
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
IF (member_count) > 1 THEN
RAISE EXCEPTION 'cannot delete organization: organization has % members that must be deleted first', member_count - 1;
END IF;

Expand All @@ -92,5 +92,5 @@ $$ LANGUAGE plpgsql;
CREATE TRIGGER protect_deleting_organizations
BEFORE UPDATE ON organizations
FOR EACH ROW
WHEN (NEW.deleted = true AND OLD.deleted = false)
WHEN (NEW.deleted = true AND OLD.deleted = false)
EXECUTE FUNCTION protect_deleting_organizations();
Loading
0