From 2dab0c026262d36d2ce9e55f0c323368d8d8c311 Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Tue, 15 Jul 2025 11:39:46 +0200 Subject: [PATCH 1/4] fix(group): Calculate member count as a whole Signed-off-by: Javier Rodriguez --- app/controlplane/cmd/wire_gen.go | 4 +- app/controlplane/pkg/biz/group.go | 2 + .../pkg/biz/testhelpers/wire_gen.go | 4 +- app/controlplane/pkg/data/group.go | 71 +++++++++++++------ app/controlplane/pkg/data/membership.go | 63 +++++++++++----- 5 files changed, 98 insertions(+), 46 deletions(-) diff --git a/app/controlplane/cmd/wire_gen.go b/app/controlplane/cmd/wire_gen.go index e78d95a63..66d111c2c 100644 --- a/app/controlplane/cmd/wire_gen.go +++ b/app/controlplane/cmd/wire_gen.go @@ -38,7 +38,8 @@ func wireApp(bootstrap *conf.Bootstrap, readerWriter credentials.ReaderWriter, l return nil, nil, err } userRepo := data.NewUserRepo(dataData, logger) - membershipRepo := data.NewMembershipRepo(dataData, logger) + groupRepo := data.NewGroupRepo(dataData, logger) + membershipRepo := data.NewMembershipRepo(dataData, groupRepo, logger) organizationRepo := data.NewOrganizationRepo(dataData, logger) casBackendRepo := data.NewCASBackendRepo(dataData, logger) providers := loader.LoadProviders(readerWriter) @@ -128,7 +129,6 @@ func wireApp(bootstrap *conf.Bootstrap, readerWriter credentials.ReaderWriter, l } workflowContractUseCase := biz.NewWorkflowContractUseCase(workflowContractRepo, registry, auditorUseCase, logger) workflowUseCase := biz.NewWorkflowUsecase(workflowRepo, projectsRepo, workflowContractUseCase, auditorUseCase, membershipUseCase, logger) - groupRepo := data.NewGroupRepo(dataData, logger) orgInvitationRepo := data.NewOrgInvitation(dataData, logger) orgInvitationUseCase, err := biz.NewOrgInvitationUseCase(orgInvitationRepo, membershipRepo, userRepo, auditorUseCase, groupRepo, projectsRepo, logger) if err != nil { diff --git a/app/controlplane/pkg/biz/group.go b/app/controlplane/pkg/biz/group.go index 3be207832..3f506410b 100644 --- a/app/controlplane/pkg/biz/group.go +++ b/app/controlplane/pkg/biz/group.go @@ -55,6 +55,8 @@ type GroupRepo interface { ListPendingInvitationsByGroup(ctx context.Context, orgID uuid.UUID, groupID uuid.UUID, paginationOpts *pagination.OffsetPaginationOpts) ([]*OrgInvitation, int, error) // ListProjectsByGroup retrieves a list of projects that a group is a member of with pagination. ListProjectsByGroup(ctx context.Context, orgID uuid.UUID, groupID uuid.UUID, visibleProjectIDs []uuid.UUID, paginationOpts *pagination.OffsetPaginationOpts) ([]*GroupProjectInfo, int, error) + // UpdateGroupMemberCount updates the member count of a group. + UpdateGroupMemberCount(ctx context.Context, groupID uuid.UUID) error } // GroupMembership represents a membership of a user in a group. diff --git a/app/controlplane/pkg/biz/testhelpers/wire_gen.go b/app/controlplane/pkg/biz/testhelpers/wire_gen.go index 8346da9df..554bfb377 100644 --- a/app/controlplane/pkg/biz/testhelpers/wire_gen.go +++ b/app/controlplane/pkg/biz/testhelpers/wire_gen.go @@ -37,7 +37,8 @@ func WireTestData(testDatabase *TestDatabase, t *testing.T, logger log.Logger, r if err != nil { return nil, nil, err } - membershipRepo := data.NewMembershipRepo(dataData, logger) + groupRepo := data.NewGroupRepo(dataData, logger) + membershipRepo := data.NewMembershipRepo(dataData, groupRepo, logger) organizationRepo := data.NewOrganizationRepo(dataData, logger) casBackendRepo := data.NewCASBackendRepo(dataData, logger) bootstrap_CASServer := NewCASBackendConfig() @@ -118,7 +119,6 @@ func WireTestData(testDatabase *TestDatabase, t *testing.T, logger log.Logger, r casMappingRepo := data.NewCASMappingRepo(dataData, casBackendRepo, logger) casMappingUseCase := biz.NewCASMappingUseCase(casMappingRepo, membershipUseCase, logger) orgInvitationRepo := data.NewOrgInvitation(dataData, logger) - groupRepo := data.NewGroupRepo(dataData, logger) orgInvitationUseCase, err := biz.NewOrgInvitationUseCase(orgInvitationRepo, membershipRepo, userRepo, auditorUseCase, groupRepo, projectsRepo, logger) if err != nil { cleanup() diff --git a/app/controlplane/pkg/data/group.go b/app/controlplane/pkg/data/group.go index 4366724e2..ad0b977e2 100644 --- a/app/controlplane/pkg/data/group.go +++ b/app/controlplane/pkg/data/group.go @@ -20,13 +20,6 @@ import ( "fmt" "time" - "github.com/chainloop-dev/chainloop/app/controlplane/pkg/data/ent/projectversion" - - "github.com/chainloop-dev/chainloop/app/controlplane/pkg/data/ent/project" - - "entgo.io/ent/dialect/sql/sqljson" - - "entgo.io/ent/dialect/sql" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/authz" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/biz" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/data/ent" @@ -36,10 +29,14 @@ import ( "github.com/chainloop-dev/chainloop/app/controlplane/pkg/data/ent/organization" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/data/ent/orginvitation" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/data/ent/predicate" + "github.com/chainloop-dev/chainloop/app/controlplane/pkg/data/ent/project" + "github.com/chainloop-dev/chainloop/app/controlplane/pkg/data/ent/projectversion" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/data/ent/user" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/data/ent/workflow" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/pagination" + "entgo.io/ent/dialect/sql" + "entgo.io/ent/dialect/sql/sqljson" "github.com/go-kratos/kratos/v2/log" "github.com/google/uuid" ) @@ -231,13 +228,12 @@ func (g GroupRepo) Create(ctx context.Context, orgID uuid.UUID, opts *biz.Create SetDescription(opts.Description). SetOrganizationID(orgID) - // Only set the member count and add member if userID is provided + // Set initial member count to 0, we'll update it after transaction + builder = builder.SetMemberCount(0) + + // Add member if userID is provided if opts.UserID != nil { - builder = builder. - AddMemberIDs(*opts.UserID). - SetMemberCount(1) - } else { - builder = builder.SetMemberCount(0) + builder = builder.AddMemberIDs(*opts.UserID) } gr, err := builder.Save(ctx) @@ -287,6 +283,11 @@ func (g GroupRepo) Create(ctx context.Context, orgID uuid.UUID, opts *biz.Create return nil, fmt.Errorf("failed to create group: %w", err) } + // Update the member count based on actual query + if err := g.UpdateGroupMemberCount(ctx, entGroup.ID); err != nil { + g.log.Warnf("failed to update member count for newly created group %s: %v", entGroup.ID, err) + } + return g.FindByOrgAndID(ctx, orgID, entGroup.ID) } @@ -474,16 +475,16 @@ func (g GroupRepo) AddMemberToGroup(ctx context.Context, orgID uuid.UUID, groupI } } - // Increment the member count of the group - if err := tx.Group.UpdateOneID(groupID).AddMemberCount(1).Exec(ctx); err != nil { - return fmt.Errorf("failed to increment group member count: %w", err) - } - return nil }); err != nil { return nil, fmt.Errorf("failed to add member to group: %w", err) } + // Update the member count based on actual query after transaction + if err := g.UpdateGroupMemberCount(ctx, groupID); err != nil { + g.log.Warnf("failed to update member count after adding member to group %s: %v", groupID, err) + } + // Return the newly created membership return g.FindGroupMembershipByGroupAndID(ctx, groupID, userID) } @@ -527,11 +528,6 @@ func (g GroupRepo) RemoveMemberFromGroup(ctx context.Context, orgID uuid.UUID, g } } - // Decrement the member count of the group - if err := tx.Group.UpdateOneID(groupID).AddMemberCount(-1).Exec(ctx); err != nil { - return fmt.Errorf("failed to increment group member count: %w", err) - } - return nil }) @@ -539,6 +535,11 @@ func (g GroupRepo) RemoveMemberFromGroup(ctx context.Context, orgID uuid.UUID, g return err } + // Update the member count based on actual query after transaction + if err := g.UpdateGroupMemberCount(ctx, groupID); err != nil { + g.log.Warnf("failed to update member count after removing member from group %s: %v", groupID, err) + } + return nil } @@ -758,3 +759,27 @@ func entGroupMembershipToBiz(gu *ent.GroupMembership) *biz.GroupMembership { DeletedAt: toTimePtr(gu.DeletedAt), } } + +// UpdateGroupMemberCount updates the member count of a group based on an actual count query +// This should be called after membership changes have been committed. +func (g GroupRepo) UpdateGroupMemberCount(ctx context.Context, groupID uuid.UUID) error { + // Count active members in the group + count, err := g.data.DB.GroupMembership.Query(). + Where( + groupmembership.GroupIDEQ(groupID), + groupmembership.DeletedAtIsNil(), + ). + Count(ctx) + if err != nil { + return fmt.Errorf("failed to count group members: %w", err) + } + + // Update the group's member count to the actual count + if _, err := g.data.DB.Group.UpdateOneID(groupID). + SetMemberCount(count). + Save(ctx); err != nil { + return fmt.Errorf("failed to update group member count: %w", err) + } + + return nil +} diff --git a/app/controlplane/pkg/data/membership.go b/app/controlplane/pkg/data/membership.go index c61b32ece..27407457e 100644 --- a/app/controlplane/pkg/data/membership.go +++ b/app/controlplane/pkg/data/membership.go @@ -34,14 +34,16 @@ import ( ) type MembershipRepo struct { - data *Data - log *log.Helper + data *Data + log *log.Helper + groupRepo biz.GroupRepo } -func NewMembershipRepo(data *Data, logger log.Logger) biz.MembershipRepo { +func NewMembershipRepo(data *Data, groupRepo biz.GroupRepo, logger log.Logger) biz.MembershipRepo { return &MembershipRepo{ - data: data, - log: log.NewHelper(logger), + data: data, + groupRepo: groupRepo, + log: log.NewHelper(logger), } } @@ -253,7 +255,10 @@ func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error { return fmt.Errorf("failed to get membership: %w", err) } - return WithTx(ctx, r.data.DB, func(tx *ent.Tx) error { + // Prepare a slice to hold group IDs that need to be updated + var groupIDs []uuid.UUID + + if trErr := WithTx(ctx, r.data.DB, func(tx *ent.Tx) error { // Delete the specific membership if err := tx.Membership.DeleteOneID(id).Exec(ctx); err != nil { return fmt.Errorf("failed to delete membership: %w", err) @@ -279,6 +284,21 @@ func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error { // Remove the user from all groups in the organization by soft-deleting group memberships now := time.Now() + // Find all group IDs where this user is a member in this organization + groupMemberships, grpMemErr := tx.GroupMembership.Query().Where( + groupmembership.UserID(userID), + groupmembership.DeletedAtIsNil(), + groupmembership.HasGroupWith(group.OrganizationID(orgID)), + ).Select(groupmembership.FieldGroupID).All(ctx) + if grpMemErr != nil { + return fmt.Errorf("failed to fetch group IDs for user %s in organization %s: %w", userID, orgID, err) + } + + // Collect group IDs to update member counts later + for _, gm := range groupMemberships { + groupIDs = append(groupIDs, gm.GroupID) + } + // Soft delete all group memberships for this user in this organization if _, err := tx.GroupMembership.Update().Where( groupmembership.UserID(userID), @@ -287,22 +307,27 @@ func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error { ).SetDeletedAt(now).SetUpdatedAt(now).Save(ctx); err != nil { return fmt.Errorf("failed to delete group memberships for user %s in organization %s: %w", userID, orgID, err) } - - // Decrement the member count of each group this user was a member of - // We don't need a separate check for groups the user was a maintainer of - // because all memberships were already deleted above - if _, err := tx.Group.Update(). - Where( - group.HasMembersWith(user.ID(userID)), - group.HasOrganizationWith(organization.ID(orgID)), - ). - AddMemberCount(-1).Save(ctx); err != nil { - return fmt.Errorf("failed to decrement group member count: %w", err) - } } return nil - }) + }); trErr != nil { + return trErr + } + + // For each affected group, update the member count based on actual query + updated := map[uuid.UUID]struct{}{} + for _, gid := range groupIDs { + if _, seen := updated[gid]; seen { + // deduplicate group IDs + continue + } + updated[gid] = struct{}{} + if err := r.groupRepo.UpdateGroupMemberCount(ctx, gid); err != nil { + return fmt.Errorf("failed to update group member count for group %s: %w", gid, err) + } + } + + return nil } // RBAC methods From a76fb2d5c471af9a4f49060a8b8c9a75da615bc5 Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Tue, 15 Jul 2025 12:21:36 +0200 Subject: [PATCH 2/4] include migration Signed-off-by: Javier Rodriguez --- .../data/ent/migrate/migrations/20250715100956.sql | 13 +++++++++++++ .../pkg/data/ent/migrate/migrations/atlas.sum | 3 ++- app/controlplane/pkg/data/group.go | 3 --- 3 files changed, 15 insertions(+), 4 deletions(-) create mode 100644 app/controlplane/pkg/data/ent/migrate/migrations/20250715100956.sql diff --git a/app/controlplane/pkg/data/ent/migrate/migrations/20250715100956.sql b/app/controlplane/pkg/data/ent/migrate/migrations/20250715100956.sql new file mode 100644 index 000000000..e5235210e --- /dev/null +++ b/app/controlplane/pkg/data/ent/migrate/migrations/20250715100956.sql @@ -0,0 +1,13 @@ +-- Fix group member counts by calculating the correct number of members for each group +-- This migration addresses any inconsistencies in the member_count field of the groups table +-- by counting the actual number of non-deleted memberships for each group. + +-- Update the member_count in groups table based on a count of active memberships +UPDATE "groups" g +SET "member_count" = ( + SELECT COUNT(*) + FROM "group_memberships" gm + WHERE gm."group_id" = g."id" + AND gm."deleted_at" IS NULL +) +WHERE 1=1; -- Apply to all groups diff --git a/app/controlplane/pkg/data/ent/migrate/migrations/atlas.sum b/app/controlplane/pkg/data/ent/migrate/migrations/atlas.sum index 8e22a2ad1..5b387b55e 100644 --- a/app/controlplane/pkg/data/ent/migrate/migrations/atlas.sum +++ b/app/controlplane/pkg/data/ent/migrate/migrations/atlas.sum @@ -1,4 +1,4 @@ -h1:WasPyGuTE05lx75h+qqmoAknSDJPoN50E8hqbhNFVQs= +h1:XoiYtAbqVAQdLw1aRpVIJu6sqZPWEZR3WuocvtoCi2Y= 20230706165452_init-schema.sql h1:VvqbNFEQnCvUVyj2iDYVQQxDM0+sSXqocpt/5H64k8M= 20230710111950-cas-backend.sql h1:A8iBuSzZIEbdsv9ipBtscZQuaBp3V5/VMw7eZH6GX+g= 20230712094107-cas-backends-workflow-runs.sql h1:a5rzxpVGyd56nLRSsKrmCFc9sebg65RWzLghKHh5xvI= @@ -97,3 +97,4 @@ h1:WasPyGuTE05lx75h+qqmoAknSDJPoN50E8hqbhNFVQs= 20250704090359.sql h1:a0ksfjy2dtzviJL16HbC4eT1xBxy2qFH5mNFOpYlUrA= 20250710105502.sql h1:EA6Ta1qsZcrNoOrO5zUNgiweHDtjl0HUlobukRuruko= 20250714172256.sql h1:S0ImNk0sMjWVVZvS6VVHn2h96/nx8GOf4aVxELbJAcg= +20250715100956.sql h1:8gZbbUts4ZcQRqhg/ZmZoIC5ScoonGDKaxJcvaglkoo= diff --git a/app/controlplane/pkg/data/group.go b/app/controlplane/pkg/data/group.go index ad0b977e2..0a06315a2 100644 --- a/app/controlplane/pkg/data/group.go +++ b/app/controlplane/pkg/data/group.go @@ -228,9 +228,6 @@ func (g GroupRepo) Create(ctx context.Context, orgID uuid.UUID, opts *biz.Create SetDescription(opts.Description). SetOrganizationID(orgID) - // Set initial member count to 0, we'll update it after transaction - builder = builder.SetMemberCount(0) - // Add member if userID is provided if opts.UserID != nil { builder = builder.AddMemberIDs(*opts.UserID) From 96d9fa0e8783a1f014383ffebee97bd2340f1051 Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Tue, 15 Jul 2025 12:34:16 +0200 Subject: [PATCH 3/4] revert query Signed-off-by: Javier Rodriguez --- app/controlplane/cmd/wire_gen.go | 4 +- .../pkg/biz/testhelpers/wire_gen.go | 4 +- app/controlplane/pkg/data/membership.go | 63 ++++++------------- 3 files changed, 23 insertions(+), 48 deletions(-) diff --git a/app/controlplane/cmd/wire_gen.go b/app/controlplane/cmd/wire_gen.go index 66d111c2c..e78d95a63 100644 --- a/app/controlplane/cmd/wire_gen.go +++ b/app/controlplane/cmd/wire_gen.go @@ -38,8 +38,7 @@ func wireApp(bootstrap *conf.Bootstrap, readerWriter credentials.ReaderWriter, l return nil, nil, err } userRepo := data.NewUserRepo(dataData, logger) - groupRepo := data.NewGroupRepo(dataData, logger) - membershipRepo := data.NewMembershipRepo(dataData, groupRepo, logger) + membershipRepo := data.NewMembershipRepo(dataData, logger) organizationRepo := data.NewOrganizationRepo(dataData, logger) casBackendRepo := data.NewCASBackendRepo(dataData, logger) providers := loader.LoadProviders(readerWriter) @@ -129,6 +128,7 @@ func wireApp(bootstrap *conf.Bootstrap, readerWriter credentials.ReaderWriter, l } workflowContractUseCase := biz.NewWorkflowContractUseCase(workflowContractRepo, registry, auditorUseCase, logger) workflowUseCase := biz.NewWorkflowUsecase(workflowRepo, projectsRepo, workflowContractUseCase, auditorUseCase, membershipUseCase, logger) + groupRepo := data.NewGroupRepo(dataData, logger) orgInvitationRepo := data.NewOrgInvitation(dataData, logger) orgInvitationUseCase, err := biz.NewOrgInvitationUseCase(orgInvitationRepo, membershipRepo, userRepo, auditorUseCase, groupRepo, projectsRepo, logger) if err != nil { diff --git a/app/controlplane/pkg/biz/testhelpers/wire_gen.go b/app/controlplane/pkg/biz/testhelpers/wire_gen.go index 554bfb377..8346da9df 100644 --- a/app/controlplane/pkg/biz/testhelpers/wire_gen.go +++ b/app/controlplane/pkg/biz/testhelpers/wire_gen.go @@ -37,8 +37,7 @@ func WireTestData(testDatabase *TestDatabase, t *testing.T, logger log.Logger, r if err != nil { return nil, nil, err } - groupRepo := data.NewGroupRepo(dataData, logger) - membershipRepo := data.NewMembershipRepo(dataData, groupRepo, logger) + membershipRepo := data.NewMembershipRepo(dataData, logger) organizationRepo := data.NewOrganizationRepo(dataData, logger) casBackendRepo := data.NewCASBackendRepo(dataData, logger) bootstrap_CASServer := NewCASBackendConfig() @@ -119,6 +118,7 @@ func WireTestData(testDatabase *TestDatabase, t *testing.T, logger log.Logger, r casMappingRepo := data.NewCASMappingRepo(dataData, casBackendRepo, logger) casMappingUseCase := biz.NewCASMappingUseCase(casMappingRepo, membershipUseCase, logger) orgInvitationRepo := data.NewOrgInvitation(dataData, logger) + groupRepo := data.NewGroupRepo(dataData, logger) orgInvitationUseCase, err := biz.NewOrgInvitationUseCase(orgInvitationRepo, membershipRepo, userRepo, auditorUseCase, groupRepo, projectsRepo, logger) if err != nil { cleanup() diff --git a/app/controlplane/pkg/data/membership.go b/app/controlplane/pkg/data/membership.go index 27407457e..0449ac275 100644 --- a/app/controlplane/pkg/data/membership.go +++ b/app/controlplane/pkg/data/membership.go @@ -34,16 +34,14 @@ import ( ) type MembershipRepo struct { - data *Data - log *log.Helper - groupRepo biz.GroupRepo + data *Data + log *log.Helper } -func NewMembershipRepo(data *Data, groupRepo biz.GroupRepo, logger log.Logger) biz.MembershipRepo { +func NewMembershipRepo(data *Data, logger log.Logger) biz.MembershipRepo { return &MembershipRepo{ - data: data, - groupRepo: groupRepo, - log: log.NewHelper(logger), + data: data, + log: log.NewHelper(logger), } } @@ -255,10 +253,7 @@ func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error { return fmt.Errorf("failed to get membership: %w", err) } - // Prepare a slice to hold group IDs that need to be updated - var groupIDs []uuid.UUID - - if trErr := WithTx(ctx, r.data.DB, func(tx *ent.Tx) error { + return WithTx(ctx, r.data.DB, func(tx *ent.Tx) error { // Delete the specific membership if err := tx.Membership.DeleteOneID(id).Exec(ctx); err != nil { return fmt.Errorf("failed to delete membership: %w", err) @@ -284,21 +279,6 @@ func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error { // Remove the user from all groups in the organization by soft-deleting group memberships now := time.Now() - // Find all group IDs where this user is a member in this organization - groupMemberships, grpMemErr := tx.GroupMembership.Query().Where( - groupmembership.UserID(userID), - groupmembership.DeletedAtIsNil(), - groupmembership.HasGroupWith(group.OrganizationID(orgID)), - ).Select(groupmembership.FieldGroupID).All(ctx) - if grpMemErr != nil { - return fmt.Errorf("failed to fetch group IDs for user %s in organization %s: %w", userID, orgID, err) - } - - // Collect group IDs to update member counts later - for _, gm := range groupMemberships { - groupIDs = append(groupIDs, gm.GroupID) - } - // Soft delete all group memberships for this user in this organization if _, err := tx.GroupMembership.Update().Where( groupmembership.UserID(userID), @@ -307,27 +287,22 @@ func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error { ).SetDeletedAt(now).SetUpdatedAt(now).Save(ctx); err != nil { return fmt.Errorf("failed to delete group memberships for user %s in organization %s: %w", userID, orgID, err) } - } - return nil - }); trErr != nil { - return trErr - } - - // For each affected group, update the member count based on actual query - updated := map[uuid.UUID]struct{}{} - for _, gid := range groupIDs { - if _, seen := updated[gid]; seen { - // deduplicate group IDs - continue - } - updated[gid] = struct{}{} - if err := r.groupRepo.UpdateGroupMemberCount(ctx, gid); err != nil { - return fmt.Errorf("failed to update group member count for group %s: %w", gid, err) + // Decrement the member count of each group this user was a member of + // Only decrement if member_count > 0 to avoid negative values + if _, err := tx.Group.Update(). + Where( + group.HasGroupUsersWith(groupmembership.UserID(userID), groupmembership.DeletedAtIsNil()), + group.HasOrganizationWith(organization.ID(orgID)), + group.MemberCountGT(0), + ). + AddMemberCount(-1).Save(ctx); err != nil { + return fmt.Errorf("failed to decrement group member count: %w", err) + } } - } - return nil + return nil + }) } // RBAC methods From 33a05ae87b6e894f3ba0e32ab237827fe0bca105 Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Tue, 15 Jul 2025 13:07:04 +0200 Subject: [PATCH 4/4] update values member count as a whole Signed-off-by: Javier Rodriguez --- app/controlplane/cmd/wire_gen.go | 4 +- .../pkg/biz/testhelpers/wire_gen.go | 4 +- .../ent/migrate/migrations/20250715100956.sql | 3 +- .../pkg/data/ent/migrate/migrations/atlas.sum | 4 +- app/controlplane/pkg/data/membership.go | 63 +++++++++++++------ 5 files changed, 51 insertions(+), 27 deletions(-) diff --git a/app/controlplane/cmd/wire_gen.go b/app/controlplane/cmd/wire_gen.go index e78d95a63..66d111c2c 100644 --- a/app/controlplane/cmd/wire_gen.go +++ b/app/controlplane/cmd/wire_gen.go @@ -38,7 +38,8 @@ func wireApp(bootstrap *conf.Bootstrap, readerWriter credentials.ReaderWriter, l return nil, nil, err } userRepo := data.NewUserRepo(dataData, logger) - membershipRepo := data.NewMembershipRepo(dataData, logger) + groupRepo := data.NewGroupRepo(dataData, logger) + membershipRepo := data.NewMembershipRepo(dataData, groupRepo, logger) organizationRepo := data.NewOrganizationRepo(dataData, logger) casBackendRepo := data.NewCASBackendRepo(dataData, logger) providers := loader.LoadProviders(readerWriter) @@ -128,7 +129,6 @@ func wireApp(bootstrap *conf.Bootstrap, readerWriter credentials.ReaderWriter, l } workflowContractUseCase := biz.NewWorkflowContractUseCase(workflowContractRepo, registry, auditorUseCase, logger) workflowUseCase := biz.NewWorkflowUsecase(workflowRepo, projectsRepo, workflowContractUseCase, auditorUseCase, membershipUseCase, logger) - groupRepo := data.NewGroupRepo(dataData, logger) orgInvitationRepo := data.NewOrgInvitation(dataData, logger) orgInvitationUseCase, err := biz.NewOrgInvitationUseCase(orgInvitationRepo, membershipRepo, userRepo, auditorUseCase, groupRepo, projectsRepo, logger) if err != nil { diff --git a/app/controlplane/pkg/biz/testhelpers/wire_gen.go b/app/controlplane/pkg/biz/testhelpers/wire_gen.go index 8346da9df..554bfb377 100644 --- a/app/controlplane/pkg/biz/testhelpers/wire_gen.go +++ b/app/controlplane/pkg/biz/testhelpers/wire_gen.go @@ -37,7 +37,8 @@ func WireTestData(testDatabase *TestDatabase, t *testing.T, logger log.Logger, r if err != nil { return nil, nil, err } - membershipRepo := data.NewMembershipRepo(dataData, logger) + groupRepo := data.NewGroupRepo(dataData, logger) + membershipRepo := data.NewMembershipRepo(dataData, groupRepo, logger) organizationRepo := data.NewOrganizationRepo(dataData, logger) casBackendRepo := data.NewCASBackendRepo(dataData, logger) bootstrap_CASServer := NewCASBackendConfig() @@ -118,7 +119,6 @@ func WireTestData(testDatabase *TestDatabase, t *testing.T, logger log.Logger, r casMappingRepo := data.NewCASMappingRepo(dataData, casBackendRepo, logger) casMappingUseCase := biz.NewCASMappingUseCase(casMappingRepo, membershipUseCase, logger) orgInvitationRepo := data.NewOrgInvitation(dataData, logger) - groupRepo := data.NewGroupRepo(dataData, logger) orgInvitationUseCase, err := biz.NewOrgInvitationUseCase(orgInvitationRepo, membershipRepo, userRepo, auditorUseCase, groupRepo, projectsRepo, logger) if err != nil { cleanup() diff --git a/app/controlplane/pkg/data/ent/migrate/migrations/20250715100956.sql b/app/controlplane/pkg/data/ent/migrate/migrations/20250715100956.sql index e5235210e..24b0ec154 100644 --- a/app/controlplane/pkg/data/ent/migrate/migrations/20250715100956.sql +++ b/app/controlplane/pkg/data/ent/migrate/migrations/20250715100956.sql @@ -9,5 +9,4 @@ SET "member_count" = ( FROM "group_memberships" gm WHERE gm."group_id" = g."id" AND gm."deleted_at" IS NULL -) -WHERE 1=1; -- Apply to all groups +); -- Apply to all groups diff --git a/app/controlplane/pkg/data/ent/migrate/migrations/atlas.sum b/app/controlplane/pkg/data/ent/migrate/migrations/atlas.sum index 5b387b55e..a9af79d90 100644 --- a/app/controlplane/pkg/data/ent/migrate/migrations/atlas.sum +++ b/app/controlplane/pkg/data/ent/migrate/migrations/atlas.sum @@ -1,4 +1,4 @@ -h1:XoiYtAbqVAQdLw1aRpVIJu6sqZPWEZR3WuocvtoCi2Y= +h1:bxjg7J7Xk0+87+PhS0+F/c4AcHJZ3pkf0FnPMZa39Q0= 20230706165452_init-schema.sql h1:VvqbNFEQnCvUVyj2iDYVQQxDM0+sSXqocpt/5H64k8M= 20230710111950-cas-backend.sql h1:A8iBuSzZIEbdsv9ipBtscZQuaBp3V5/VMw7eZH6GX+g= 20230712094107-cas-backends-workflow-runs.sql h1:a5rzxpVGyd56nLRSsKrmCFc9sebg65RWzLghKHh5xvI= @@ -97,4 +97,4 @@ h1:XoiYtAbqVAQdLw1aRpVIJu6sqZPWEZR3WuocvtoCi2Y= 20250704090359.sql h1:a0ksfjy2dtzviJL16HbC4eT1xBxy2qFH5mNFOpYlUrA= 20250710105502.sql h1:EA6Ta1qsZcrNoOrO5zUNgiweHDtjl0HUlobukRuruko= 20250714172256.sql h1:S0ImNk0sMjWVVZvS6VVHn2h96/nx8GOf4aVxELbJAcg= -20250715100956.sql h1:8gZbbUts4ZcQRqhg/ZmZoIC5ScoonGDKaxJcvaglkoo= +20250715100956.sql h1:y9eOaPMpQTlcJppjaGzeuHBTNDwe6sGbxSVU8e7LL1o= diff --git a/app/controlplane/pkg/data/membership.go b/app/controlplane/pkg/data/membership.go index 0449ac275..27407457e 100644 --- a/app/controlplane/pkg/data/membership.go +++ b/app/controlplane/pkg/data/membership.go @@ -34,14 +34,16 @@ import ( ) type MembershipRepo struct { - data *Data - log *log.Helper + data *Data + log *log.Helper + groupRepo biz.GroupRepo } -func NewMembershipRepo(data *Data, logger log.Logger) biz.MembershipRepo { +func NewMembershipRepo(data *Data, groupRepo biz.GroupRepo, logger log.Logger) biz.MembershipRepo { return &MembershipRepo{ - data: data, - log: log.NewHelper(logger), + data: data, + groupRepo: groupRepo, + log: log.NewHelper(logger), } } @@ -253,7 +255,10 @@ func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error { return fmt.Errorf("failed to get membership: %w", err) } - return WithTx(ctx, r.data.DB, func(tx *ent.Tx) error { + // Prepare a slice to hold group IDs that need to be updated + var groupIDs []uuid.UUID + + if trErr := WithTx(ctx, r.data.DB, func(tx *ent.Tx) error { // Delete the specific membership if err := tx.Membership.DeleteOneID(id).Exec(ctx); err != nil { return fmt.Errorf("failed to delete membership: %w", err) @@ -279,6 +284,21 @@ func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error { // Remove the user from all groups in the organization by soft-deleting group memberships now := time.Now() + // Find all group IDs where this user is a member in this organization + groupMemberships, grpMemErr := tx.GroupMembership.Query().Where( + groupmembership.UserID(userID), + groupmembership.DeletedAtIsNil(), + groupmembership.HasGroupWith(group.OrganizationID(orgID)), + ).Select(groupmembership.FieldGroupID).All(ctx) + if grpMemErr != nil { + return fmt.Errorf("failed to fetch group IDs for user %s in organization %s: %w", userID, orgID, err) + } + + // Collect group IDs to update member counts later + for _, gm := range groupMemberships { + groupIDs = append(groupIDs, gm.GroupID) + } + // Soft delete all group memberships for this user in this organization if _, err := tx.GroupMembership.Update().Where( groupmembership.UserID(userID), @@ -287,22 +307,27 @@ func (r *MembershipRepo) Delete(ctx context.Context, id uuid.UUID) error { ).SetDeletedAt(now).SetUpdatedAt(now).Save(ctx); err != nil { return fmt.Errorf("failed to delete group memberships for user %s in organization %s: %w", userID, orgID, err) } - - // Decrement the member count of each group this user was a member of - // Only decrement if member_count > 0 to avoid negative values - if _, err := tx.Group.Update(). - Where( - group.HasGroupUsersWith(groupmembership.UserID(userID), groupmembership.DeletedAtIsNil()), - group.HasOrganizationWith(organization.ID(orgID)), - group.MemberCountGT(0), - ). - AddMemberCount(-1).Save(ctx); err != nil { - return fmt.Errorf("failed to decrement group member count: %w", err) - } } return nil - }) + }); trErr != nil { + return trErr + } + + // For each affected group, update the member count based on actual query + updated := map[uuid.UUID]struct{}{} + for _, gid := range groupIDs { + if _, seen := updated[gid]; seen { + // deduplicate group IDs + continue + } + updated[gid] = struct{}{} + if err := r.groupRepo.UpdateGroupMemberCount(ctx, gid); err != nil { + return fmt.Errorf("failed to update group member count for group %s: %w", gid, err) + } + } + + return nil } // RBAC methods