feat(store): group delete keeps the row(soft delete)

core/deleter/service.go

@@ -339,6 +339,9 @@ func (d Service) DeleteOrganization(ctx context.Context, id string) error {
 	}
 	for _, g := range groups {
 		if err = d.DeleteGroup(ctx, g.ID); err != nil {
+			if errors.Is(err, group.ErrNotExist) {
+				continue
+			}
 			return fmt.Errorf("failed to delete org while deleting a group[%s]: %w", g.Name, err)
 		}
 	}

core/deleter/service_test.go

@@ -437,6 +437,31 @@ func TestDeleteOrganization(t *testing.T) {
 		assert.Equal(t, []string{"resource:res-live", "org"}, steps)
 	})
 
+	t.Run("a group that is already gone is skipped and the rest are deleted before the org row", func(t *testing.T) {
+		m := newBareMocks(t)
+		var steps []string
+
+		m.orgSvc.EXPECT().GetRaw(mock.Anything, "org-1").
+			Return(organization.Organization{ID: "org-1"}, nil)
+		m.custSvc.EXPECT().List(mock.Anything, customer.Filter{OrgID: "org-1"}).
+			Return([]customer.Customer{}, nil)
+		m.projSvc.EXPECT().List(mock.Anything, project.Filter{OrgID: "org-1", IncludeDisabled: true}).
+			Return([]project.Project{}, nil)
+		m.resSvc.EXPECT().List(mock.Anything, resource.Filter{OrgID: "org-1"}).
+			Return([]resource.Resource{}, nil)
+		m.grpSvc.EXPECT().List(mock.Anything, group.Filter{OrganizationID: "org-1", IncludeDisabled: true}).
+			Return([]group.Group{{ID: "grp-gone", Name: "gone"}, {ID: "grp-live", Name: "live"}}, nil)
+		m.mbrSvc.EXPECT().OnGroupDeleted(mock.Anything, "grp-gone").Return(group.ErrNotExist)
+		m.mbrSvc.EXPECT().OnGroupDeleted(mock.Anything, "grp-live").Return(nil)
+		m.grpSvc.EXPECT().DeleteModel(mock.Anything, "grp-live").
+			Run(func(_ context.Context, id string) { steps = append(steps, "group:"+id) }).Return(nil)
+		expectEmptyOrgTail(m, &steps)
+
+		err := m.build().DeleteOrganization(context.Background(), "org-1")
+		assert.NoError(t, err)
+		assert.Equal(t, []string{"group:grp-live", "org"}, steps)
+	})
+
 	t.Run("a failed org resource delete stops before the org row", func(t *testing.T) {
 		m := newBareMocks(t)
 

internal/store/postgres/group_repository.go

@@ -304,7 +304,7 @@ func (r GroupRepository) SetState(ctx context.Context, id string, state group.St
 }
 
 func (r GroupRepository) Delete(ctx context.Context, id string) error {
-	query, params, err := dialect.Delete(TABLE_GROUPS).Where(
+	query, params, err := softDelete(TABLE_GROUPS).Where(
 		goqu.Ex{
 			"id": id,
 		},

internal/store/postgres/group_repository_test.go

@@ -580,46 +580,42 @@ func (s *GroupRepositoryTestSuite) TestSetState() {
 }
 
 func (s *GroupRepositoryTestSuite) TestDelete() {
-	type testCase struct {
-		Description   string
-		GroupID       string
-		ExpectedError error
-	}
+	deleted := s.groups[0]
 
-	var testCases = []testCase{
-		{
-			Description:   "should delete group",
-			GroupID:       s.groups[0].ID,
-			ExpectedError: nil,
-		},
-		{
-			Description:   "should return error if group not exist",
-			GroupID:       "52e4dc5d-eb93-4e0d-bc72-4f24a67d679f",
-			ExpectedError: group.ErrNotExist,
-		},
-		{
-			Description:   "should return error if group id is not uuid",
-			GroupID:       "12345",
-			ExpectedError: group.ErrInvalidUUID,
-		},
-		{
-			Description:   "should return error if group id is empty",
-			ExpectedError: group.ErrInvalidUUID,
-		},
-	}
+	s.Run("keeps the row with deleted_at set and hides it", func() {
+		s.Require().NoError(s.repository.Delete(s.ctx, deleted.ID))
 
-	for _, tc := range testCases {
-		s.Run(tc.Description, func() {
-			err := s.repository.Delete(s.ctx, tc.GroupID)
-			if tc.ExpectedError != nil {
-				if !errors.Is(err, tc.ExpectedError) {
-					s.T().Fatalf("got error %v, expected was %v", err, tc.ExpectedError)
-				}
-			} else {
-				s.Assert().NoError(err)
-			}
+		var deletedRows int
+		s.Require().NoError(s.client.QueryRowxContext(s.ctx,
+			"SELECT count(*) FROM groups WHERE id = $1 AND deleted_at IS NOT NULL", deleted.ID).Scan(&deletedRows))
+		s.Assert().Equal(1, deletedRows)
+
+		_, err := s.repository.GetByID(s.ctx, deleted.ID)
+		s.Assert().ErrorIs(err, group.ErrNotExist)
+
+		listed, err := s.repository.List(s.ctx, group.Filter{OrganizationID: deleted.OrganizationID})
+		s.Require().NoError(err)
+		for _, g := range listed {
+			s.Assert().NotEqual(deleted.ID, g.ID)
+		}
+	})
+	s.Run("a second delete returns not found", func() {
+		s.Assert().ErrorIs(s.repository.Delete(s.ctx, deleted.ID), group.ErrNotExist)
+	})
+	s.Run("a group that does not exist returns not found", func() {
+		s.Assert().ErrorIs(s.repository.Delete(s.ctx, uuid.NewString()), group.ErrNotExist)
+	})
+	s.Run("an id that is not a uuid returns invalid uuid", func() {
+		s.Assert().ErrorIs(s.repository.Delete(s.ctx, "12345"), group.ErrInvalidUUID)
+		s.Assert().ErrorIs(s.repository.Delete(s.ctx, ""), group.ErrInvalidUUID)
+	})
+	s.Run("a new group in the same org can take the deleted group's name", func() {
+		_, err := s.repository.Create(s.ctx, group.Group{
+			Name:           deleted.Name,
+			OrganizationID: deleted.OrganizationID,
 		})
-	}
+		s.Assert().NoError(err)
+	})
 }
 
 func TestGroupRepository(t *testing.T) {

test/e2e/regression/api_test.go

@@ -1259,6 +1259,32 @@ func (s *APIRegressionTestSuite) TestGroupAPI() {
 		s.Assert().NoError(err)
 		s.Assert().False(checkUserStatus.Msg.GetStatus())
 
+		var deletedRows int
+		s.Require().NoError(s.dbc.GetContext(context.Background(), &deletedRows,
+			"SELECT count(*) FROM groups WHERE id = $1 AND deleted_at IS NOT NULL", deletedGroupID))
+		s.Assert().Equal(1, deletedRows)
+
+		_, err = s.testBench.Client.GetGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetGroupRequest{Id: deletedGroupID}))
+		s.Require().Error(err)
+		s.Assert().Equal(connect.CodePermissionDenied, connect.CodeOf(err))
+		_, err = s.testBench.Client.DeleteGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteGroupRequest{Id: deletedGroupID}))
+		s.Require().Error(err)
+		s.Assert().Equal(connect.CodePermissionDenied, connect.CodeOf(err))
+
+		recreateResp, err := s.testBench.Client.CreateGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateGroupRequest{
+			OrgId: myOrg.GetId(),
+			Body:  &frontierv1beta1.GroupRequestBody{Name: "group-11"},
+		}))
+		s.Require().NoError(err)
+		listResp, err := s.testBench.Client.ListOrganizationGroups(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListOrganizationGroupsRequest{OrgId: myOrg.GetId()}))
+		s.Require().NoError(err)
+		listedIDs := []string{}
+		for _, g := range listResp.Msg.GetGroups() {
+			listedIDs = append(listedIDs, g.GetId())
+		}
+		s.Assert().Contains(listedIDs, recreateResp.Msg.GetGroup().GetId())
+		s.Assert().NotContains(listedIDs, deletedGroupID)
+
 		_, err = s.testBench.Client.DeleteProject(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteProjectRequest{
 			Id: groupPolicyProjResp.Msg.GetProject().GetId(),
 		}))
@@ -3732,6 +3758,15 @@ func (s *APIRegressionTestSuite) TestOrganizationDeleteAlsoDeletesDisabledProjec
 	s.Require().NotEmpty(disabledProjectPolicyIDs)
 	s.Require().NotEmpty(disabledGroupPolicyIDs)
 
+	_, err = s.testBench.Client.CreatePolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreatePolicyRequest{
+		Body: &frontierv1beta1.PolicyRequestBody{
+			RoleId:    schema.RoleOrganizationViewer,
+			Resource:  schema.JoinNamespaceAndResourceID(schema.OrganizationNamespace, orgID),
+			Principal: schema.JoinNamespaceAndResourceID(schema.GroupPrincipal, disabledGroupID),
+		},
+	}))
+	s.Require().NoError(err)
+
 	enabledProjects, err := s.testBench.AdminClient.ListProjects(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListProjectsRequest{OrgId: orgID}))
 	s.Require().NoError(err)
 	s.Require().Len(enabledProjects.Msg.GetProjects(), 1)
@@ -3760,14 +3795,26 @@ func (s *APIRegressionTestSuite) TestOrganizationDeleteAlsoDeletesDisabledProjec
 		}
 	})
 
-	s.Run("no group row of the org is left, disabled ones included", func() {
+	s.Run("no group of the org is listed, disabled ones included", func() {
 		for _, state := range []string{"", "disabled"} {
 			resp, err := s.testBench.AdminClient.ListGroups(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.ListGroupsRequest{OrgId: orgID, State: state}))
 			s.Require().NoError(err)
 			s.Assert().Empty(resp.Msg.GetGroups(), "state %q", state)
 		}
 	})
 
+	s.Run("both groups stay in the database marked deleted, with no live policy on or naming them", func() {
+		for _, id := range []string{enabledGroupID, disabledGroupID} {
+			var deletedRows, livePolicies int
+			s.Require().NoError(s.dbc.GetContext(context.Background(), &deletedRows,
+				"SELECT count(*) FROM groups WHERE id = $1 AND deleted_at IS NOT NULL", id))
+			s.Require().NoError(s.dbc.GetContext(context.Background(), &livePolicies,
+				"SELECT count(*) FROM policies WHERE (resource_id = $1 OR principal_id = $1) AND deleted_at IS NULL", id))
+			s.Assert().Equal(1, deletedRows, "group %s", id)
+			s.Assert().Equal(0, livePolicies, "group %s", id)
+		}
+	})
+
 	s.Run("no policy of the disabled project or group is left", func() {
 		for _, id := range append(disabledProjectPolicyIDs, disabledGroupPolicyIDs...) {
 			_, err := s.testBench.Client.GetPolicy(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetPolicyRequest{Id: id}))