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

Closes
[coder/internal#477](https://github.com/coder/internal/issues/477)

![Screenshot 2025-03-21 at 11 25
57 AM](https://github.com/user-attachments/assets/50cc03e9-395d-4fc7-8882-18cb66b1fac9)

I'm solving this issue in two parts:

1. Updated the postgres function so that it doesn't omit 0 values in the
error
2. Created a new query to fetch the number of resources associated with
an organization and using that information to provider a cleaner error
message to the frontend

> **_NOTE:_** SQL is not my strong suit, and the code was created with
the help of AI. So I'd take extra time looking over what I wrote there
This commit is contained in:
brettkolodny
2025-03-25 15:31:24 -04:00
committed by GitHub
parent 2c53f7ae7c
commit cf10d98aab
13 changed files with 416 additions and 22 deletions
+29
View File
@@ -1989,6 +1989,35 @@ func (q *querier) GetOrganizationIDsByMemberIDs(ctx context.Context, ids []uuid.
return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetOrganizationIDsByMemberIDs)(ctx, ids)
}
func (q *querier) GetOrganizationResourceCountByID(ctx context.Context, organizationID uuid.UUID) (database.GetOrganizationResourceCountByIDRow, error) {
// Can read org members
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceOrganizationMember.InOrg(organizationID)); err != nil {
return database.GetOrganizationResourceCountByIDRow{}, err
}
// Can read org workspaces
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceWorkspace.InOrg(organizationID)); err != nil {
return database.GetOrganizationResourceCountByIDRow{}, err
}
// Can read org groups
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceGroup.InOrg(organizationID)); err != nil {
return database.GetOrganizationResourceCountByIDRow{}, err
}
// Can read org templates
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceTemplate.InOrg(organizationID)); err != nil {
return database.GetOrganizationResourceCountByIDRow{}, err
}
// Can read org provisioner daemons
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerDaemon.InOrg(organizationID)); err != nil {
return database.GetOrganizationResourceCountByIDRow{}, err
}
return q.db.GetOrganizationResourceCountByID(ctx, organizationID)
}
func (q *querier) GetOrganizations(ctx context.Context, args database.GetOrganizationsParams) ([]database.Organization, error) {
fetch := func(ctx context.Context, _ interface{}) ([]database.Organization, error) {
return q.db.GetOrganizations(ctx, args)
+33
View File
@@ -815,6 +815,39 @@ func (s *MethodTestSuite) TestOrganization() {
o := dbgen.Organization(s.T(), db, database.Organization{})
check.Args(o.ID).Asserts(o, policy.ActionRead).Returns(o)
}))
s.Run("GetOrganizationResourceCountByID", s.Subtest(func(db database.Store, check *expects) {
u := dbgen.User(s.T(), db, database.User{})
o := dbgen.Organization(s.T(), db, database.Organization{})
t := dbgen.Template(s.T(), db, database.Template{
CreatedBy: u.ID,
OrganizationID: o.ID,
})
dbgen.Workspace(s.T(), db, database.WorkspaceTable{
OrganizationID: o.ID,
OwnerID: u.ID,
TemplateID: t.ID,
})
dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{
OrganizationID: o.ID,
UserID: u.ID,
})
check.Args(o.ID).Asserts(
rbac.ResourceOrganizationMember.InOrg(o.ID), policy.ActionRead,
rbac.ResourceWorkspace.InOrg(o.ID), policy.ActionRead,
rbac.ResourceGroup.InOrg(o.ID), policy.ActionRead,
rbac.ResourceTemplate.InOrg(o.ID), policy.ActionRead,
rbac.ResourceProvisionerDaemon.InOrg(o.ID), policy.ActionRead,
).Returns(database.GetOrganizationResourceCountByIDRow{
WorkspaceCount: 1,
GroupCount: 1,
TemplateCount: 1,
MemberCount: 1,
ProvisionerKeyCount: 0,
})
}))
s.Run("GetDefaultOrganization", s.Subtest(func(db database.Store, check *expects) {
o, _ := db.GetDefaultOrganization(context.Background())
check.Args().Asserts(o, policy.ActionRead).Returns(o)
+48
View File
@@ -4008,6 +4008,54 @@ func (q *FakeQuerier) GetOrganizationIDsByMemberIDs(_ context.Context, ids []uui
return getOrganizationIDsByMemberIDRows, nil
}
func (q *FakeQuerier) GetOrganizationResourceCountByID(_ context.Context, organizationID uuid.UUID) (database.GetOrganizationResourceCountByIDRow, error) {
q.mutex.RLock()
defer q.mutex.RUnlock()
workspacesCount := 0
for _, workspace := range q.workspaces {
if workspace.OrganizationID == organizationID {
workspacesCount++
}
}
groupsCount := 0
for _, group := range q.groups {
if group.OrganizationID == organizationID {
groupsCount++
}
}
templatesCount := 0
for _, template := range q.templates {
if template.OrganizationID == organizationID {
templatesCount++
}
}
organizationMembersCount := 0
for _, organizationMember := range q.organizationMembers {
if organizationMember.OrganizationID == organizationID {
organizationMembersCount++
}
}
provKeyCount := 0
for _, provKey := range q.provisionerKeys {
if provKey.OrganizationID == organizationID {
provKeyCount++
}
}
return database.GetOrganizationResourceCountByIDRow{
WorkspaceCount: int64(workspacesCount),
GroupCount: int64(groupsCount),
TemplateCount: int64(templatesCount),
MemberCount: int64(organizationMembersCount),
ProvisionerKeyCount: int64(provKeyCount),
}, nil
}
func (q *FakeQuerier) GetOrganizations(_ context.Context, args database.GetOrganizationsParams) ([]database.Organization, error) {
q.mutex.RLock()
defer q.mutex.RUnlock()
@@ -1012,6 +1012,13 @@ func (m queryMetricsStore) GetOrganizationIDsByMemberIDs(ctx context.Context, id
return organizations, err
}
func (m queryMetricsStore) GetOrganizationResourceCountByID(ctx context.Context, organizationID uuid.UUID) (database.GetOrganizationResourceCountByIDRow, error) {
start := time.Now()
r0, r1 := m.s.GetOrganizationResourceCountByID(ctx, organizationID)
m.queryLatencies.WithLabelValues("GetOrganizationResourceCountByID").Observe(time.Since(start).Seconds())
return r0, r1
}
func (m queryMetricsStore) GetOrganizations(ctx context.Context, args database.GetOrganizationsParams) ([]database.Organization, error) {
start := time.Now()
organizations, err := m.s.GetOrganizations(ctx, args)
+15
View File
@@ -2062,6 +2062,21 @@ func (mr *MockStoreMockRecorder) GetOrganizationIDsByMemberIDs(ctx, ids any) *go
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetOrganizationIDsByMemberIDs", reflect.TypeOf((*MockStore)(nil).GetOrganizationIDsByMemberIDs), ctx, ids)
}
// GetOrganizationResourceCountByID mocks base method.
func (m *MockStore) GetOrganizationResourceCountByID(ctx context.Context, organizationID uuid.UUID) (database.GetOrganizationResourceCountByIDRow, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetOrganizationResourceCountByID", ctx, organizationID)
ret0, _ := ret[0].(database.GetOrganizationResourceCountByIDRow)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// GetOrganizationResourceCountByID indicates an expected call of GetOrganizationResourceCountByID.
func (mr *MockStoreMockRecorder) GetOrganizationResourceCountByID(ctx, organizationID any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetOrganizationResourceCountByID", reflect.TypeOf((*MockStore)(nil).GetOrganizationResourceCountByID), ctx, organizationID)
}
// GetOrganizations mocks base method.
func (m *MockStore) GetOrganizations(ctx context.Context, arg database.GetOrganizationsParams) ([]database.Organization, error) {
m.ctrl.T.Helper()
+38 -19
View File
@@ -450,10 +450,10 @@ CREATE FUNCTION protect_deleting_organizations() 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
@@ -462,50 +462,69 @@ 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
RAISE EXCEPTION 'cannot delete organization: organization has % workspaces, % templates, and % provisioner keys that must be deleted first', workspace_count, template_count, provisioner_keys_count;
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
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;
@@ -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;
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
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();
@@ -0,0 +1,96 @@
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
$$
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();
+1
View File
@@ -217,6 +217,7 @@ type sqlcQuerier interface {
GetOrganizationByID(ctx context.Context, id uuid.UUID) (Organization, error)
GetOrganizationByName(ctx context.Context, arg GetOrganizationByNameParams) (Organization, error)
GetOrganizationIDsByMemberIDs(ctx context.Context, ids []uuid.UUID) ([]GetOrganizationIDsByMemberIDsRow, error)
GetOrganizationResourceCountByID(ctx context.Context, organizationID uuid.UUID) (GetOrganizationResourceCountByIDRow, error)
GetOrganizations(ctx context.Context, arg GetOrganizationsParams) ([]Organization, error)
GetOrganizationsByUserID(ctx context.Context, arg GetOrganizationsByUserIDParams) ([]Organization, error)
GetParameterSchemasByJobID(ctx context.Context, jobID uuid.UUID) ([]ParameterSchema, error)
-1
View File
@@ -3507,7 +3507,6 @@ func TestOrganizationDeleteTrigger(t *testing.T) {
require.Error(t, err)
// cannot delete organization: organization has 0 workspaces and 1 templates that must be deleted first
require.ErrorContains(t, err, "cannot delete organization")
require.ErrorContains(t, err, "has 0 workspaces")
require.ErrorContains(t, err, "1 templates")
})
+30
View File
@@ -5521,6 +5521,36 @@ func (q *sqlQuerier) GetOrganizationByName(ctx context.Context, arg GetOrganizat
return i, err
}
const getOrganizationResourceCountByID = `-- name: GetOrganizationResourceCountByID :one
SELECT
(SELECT COUNT(*) FROM workspaces WHERE workspaces.organization_id = $1 AND workspaces.deleted = false) AS workspace_count,
(SELECT COUNT(*) FROM groups WHERE groups.organization_id = $1) AS group_count,
(SELECT COUNT(*) FROM templates WHERE templates.organization_id = $1 AND templates.deleted = false) AS template_count,
(SELECT COUNT(*) FROM organization_members WHERE organization_members.organization_id = $1) AS member_count,
(SELECT COUNT(*) FROM provisioner_keys WHERE provisioner_keys.organization_id = $1) AS provisioner_key_count
`
type GetOrganizationResourceCountByIDRow struct {
WorkspaceCount int64 `db:"workspace_count" json:"workspace_count"`
GroupCount int64 `db:"group_count" json:"group_count"`
TemplateCount int64 `db:"template_count" json:"template_count"`
MemberCount int64 `db:"member_count" json:"member_count"`
ProvisionerKeyCount int64 `db:"provisioner_key_count" json:"provisioner_key_count"`
}
func (q *sqlQuerier) GetOrganizationResourceCountByID(ctx context.Context, organizationID uuid.UUID) (GetOrganizationResourceCountByIDRow, error) {
row := q.db.QueryRowContext(ctx, getOrganizationResourceCountByID, organizationID)
var i GetOrganizationResourceCountByIDRow
err := row.Scan(
&i.WorkspaceCount,
&i.GroupCount,
&i.TemplateCount,
&i.MemberCount,
&i.ProvisionerKeyCount,
)
return i, err
}
const getOrganizations = `-- name: GetOrganizations :many
SELECT
id, name, description, created_at, updated_at, is_default, display_name, icon, deleted
@@ -66,6 +66,14 @@ WHERE
user_id = $1
);
-- name: GetOrganizationResourceCountByID :one
SELECT
(SELECT COUNT(*) FROM workspaces WHERE workspaces.organization_id = $1 AND workspaces.deleted = false) AS workspace_count,
(SELECT COUNT(*) FROM groups WHERE groups.organization_id = $1) AS group_count,
(SELECT COUNT(*) FROM templates WHERE templates.organization_id = $1 AND templates.deleted = false) AS template_count,
(SELECT COUNT(*) FROM organization_members WHERE organization_members.organization_id = $1) AS member_count,
(SELECT COUNT(*) FROM provisioner_keys WHERE provisioner_keys.organization_id = $1) AS provisioner_key_count;
-- name: InsertOrganization :one
INSERT INTO
organizations (id, "name", display_name, description, icon, created_at, updated_at, is_default)
+34 -2
View File
@@ -4,6 +4,7 @@ import (
"database/sql"
"fmt"
"net/http"
"strings"
"github.com/google/uuid"
"golang.org/x/xerrors"
@@ -161,10 +162,41 @@ func (api *API) deleteOrganization(rw http.ResponseWriter, r *http.Request) {
return nil
}, nil)
if err != nil {
orgResourcesRow, queryErr := api.Database.GetOrganizationResourceCountByID(ctx, organization.ID)
if queryErr != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error deleting organization.",
Detail: fmt.Sprintf("delete organization: %s", err.Error()),
})
return
}
detailParts := make([]string, 0)
addDetailPart := func(resource string, count int64) {
if count == 1 {
detailParts = append(detailParts, fmt.Sprintf("1 %s", resource))
} else if count > 1 {
detailParts = append(detailParts, fmt.Sprintf("%d %ss", count, resource))
}
}
addDetailPart("workspace", orgResourcesRow.WorkspaceCount)
addDetailPart("template", orgResourcesRow.TemplateCount)
// There will always be one member and group so instead we need to check that
// the count is greater than one.
addDetailPart("member", orgResourcesRow.MemberCount-1)
addDetailPart("group", orgResourcesRow.GroupCount-1)
addDetailPart("provisioner key", orgResourcesRow.ProvisionerKeyCount)
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error deleting organization.",
Detail: fmt.Sprintf("delete organization: %s", err.Error()),
Message: "Error deleting organization.",
Detail: fmt.Sprintf("This organization has %s that must be deleted first.", strings.Join(detailParts, ", ")),
})
return
}