Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions app/controlplane/cmd/wire_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions app/controlplane/pkg/biz/group.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions app/controlplane/pkg/biz/testhelpers/wire_gen.go

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,12 @@
-- 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
); -- Apply to all groups
3 changes: 2 additions & 1 deletion app/controlplane/pkg/data/ent/migrate/migrations/atlas.sum
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
h1:WasPyGuTE05lx75h+qqmoAknSDJPoN50E8hqbhNFVQs=
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=
Expand Down Expand Up @@ -97,3 +97,4 @@ h1:WasPyGuTE05lx75h+qqmoAknSDJPoN50E8hqbhNFVQs=
20250704090359.sql h1:a0ksfjy2dtzviJL16HbC4eT1xBxy2qFH5mNFOpYlUrA=
20250710105502.sql h1:EA6Ta1qsZcrNoOrO5zUNgiweHDtjl0HUlobukRuruko=
20250714172256.sql h1:S0ImNk0sMjWVVZvS6VVHn2h96/nx8GOf4aVxELbJAcg=
20250715100956.sql h1:y9eOaPMpQTlcJppjaGzeuHBTNDwe6sGbxSVU8e7LL1o=
68 changes: 45 additions & 23 deletions app/controlplane/pkg/data/group.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
)
Expand Down Expand Up @@ -231,13 +228,9 @@ 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
// 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)
Expand Down Expand Up @@ -287,6 +280,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)
}

Expand Down Expand Up @@ -474,16 +472,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)
}
Expand Down Expand Up @@ -527,18 +525,18 @@ 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
})

if err != nil {
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
}

Expand Down Expand Up @@ -758,3 +756,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
}
63 changes: 44 additions & 19 deletions app/controlplane/pkg/data/membership.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
}

Expand Down Expand Up @@ -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)
Expand All @@ -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(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this change about?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a user is removed from an organization, we need to update the member count for all groups they were part of. This change handles that by retrieving all associated groups and recalculating their member counts

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I don't understand who changing from setting -1 to recalculating adds this amount of logic. Or is it that we didn't do it properly before?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous logic was missing a condition where it was decrementing all memberships even if it was marked as deleted. I've reverted the changes and used the old logic to take into account that part.

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),
Expand All @@ -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
Expand Down
Loading