From 3a478159292f00c1a93c28dc9d3c483fa4aee65f Mon Sep 17 00:00:00 2001
From: Abhishek Sah <abhi.sah.97@gmail.com>
Date: Fri, 10 Apr 2026 14:26:29 +0530
Subject: [PATCH] fix: skip redundant policy delete+create when role is
 unchanged

SetProjectMemberRole and SetOrganizationMemberRole now return early
if the principal already has exactly the requested role, avoiding
unnecessary SpiceDB writes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 core/organization/service.go      |  5 +++++
 core/organization/service_test.go | 19 +++++++++++++++++++
 core/project/service.go           |  5 +++++
 core/project/service_test.go      | 21 +++++++++++++++++++++
 4 files changed, 50 insertions(+)

diff --git a/core/organization/service.go b/core/organization/service.go
index 4345f9cb0..de6c6180f 100644
--- a/core/organization/service.go
+++ b/core/organization/service.go
@@ -393,6 +393,11 @@ func (s Service) SetMemberRole(ctx context.Context, orgID, userID, newRoleID str
 		return ErrNotMember
 	}
 
+	// skip if the user already has exactly this role
+	if len(existingPolicies) == 1 && existingPolicies[0].RoleID == newRoleID {
+		return nil
+	}
+
 	// check minimum owner constraint
 	err = s.validateMinOwnerConstraint(ctx, orgID, newRoleID, existingPolicies)
 	if err != nil {
diff --git a/core/organization/service_test.go b/core/organization/service_test.go
index 0c564e0eb..870edce2e 100644
--- a/core/organization/service_test.go
+++ b/core/organization/service_test.go
@@ -598,6 +598,25 @@ func TestService_SetMemberRole(t *testing.T) {
 			newRoleID: ownerRoleID,
 			wantErr:   nil,
 		},
+		{
+			name: "should skip delete+create when role is unchanged",
+			setup: func(repo *mocks.Repository, userSvc *mocks.UserService, roleSvc *mocks.RoleService, policySvc *mocks.PolicyService, _ *mocks.AuditRecordRepository) {
+				repo.EXPECT().GetByID(ctx, orgID).Return(organization.Organization{ID: orgID, State: organization.Enabled}, nil)
+				userSvc.EXPECT().GetByID(ctx, userID).Return(user.User{ID: userID}, nil)
+				roleSvc.EXPECT().Get(ctx, memberRoleID).Return(role.Role{ID: memberRoleID, Name: "member", Scopes: []string{schema.OrganizationNamespace}}, nil)
+				// user already has the same role
+				policySvc.EXPECT().List(ctx, policy.Filter{
+					OrgID:         orgID,
+					PrincipalID:   userID,
+					PrincipalType: schema.UserPrincipal,
+				}).Return([]policy.Policy{{ID: "policy-1", RoleID: memberRoleID}}, nil)
+				// no Delete or Create should be called — early return
+			},
+			orgID:     orgID,
+			userID:    userID,
+			newRoleID: memberRoleID,
+			wantErr:   nil,
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/core/project/service.go b/core/project/service.go
index 6334938eb..abd00c480 100644
--- a/core/project/service.go
+++ b/core/project/service.go
@@ -385,6 +385,11 @@ func (s Service) SetMemberRole(ctx context.Context, projectID, principalID, prin
 		return err
 	}
 
+	// skip if the principal already has exactly this role
+	if len(existingPolicies) == 1 && existingPolicies[0].RoleID == newRoleID {
+		return nil
+	}
+
 	for _, p := range existingPolicies {
 		if err := s.policyService.Delete(ctx, p.ID); err != nil {
 			return err
diff --git a/core/project/service_test.go b/core/project/service_test.go
index 7e5195b37..921902ad8 100644
--- a/core/project/service_test.go
+++ b/core/project/service_test.go
@@ -1215,6 +1215,27 @@ func TestService_SetMemberRole(t *testing.T) {
 			},
 			wantErr: nil,
 		},
+		{
+			name:          "should skip delete+create when role is unchanged",
+			projectID:     projectID,
+			principalID:   userID,
+			principalType: schema.UserPrincipal,
+			roleID:        roleID,
+			setup: func(repo *mocks.Repository, userSvc *mocks.UserService, suserSvc *mocks.ServiceuserService, groupSvc *mocks.GroupService, policySvc *mocks.PolicyService, roleSvc *mocks.RoleService) {
+				repo.EXPECT().GetByID(ctx, projectID).Return(project.Project{ID: projectID, Organization: organization.Organization{ID: orgID}}, nil)
+				userSvc.EXPECT().GetByID(ctx, userID).Return(user.User{ID: userID}, nil)
+				policySvc.EXPECT().List(ctx, policy.Filter{
+					OrgID: orgID, PrincipalID: userID, PrincipalType: schema.UserPrincipal,
+				}).Return([]policy.Policy{{ID: "org-p1"}}, nil)
+				roleSvc.EXPECT().Get(ctx, roleID).Return(role.Role{ID: roleID, Scopes: []string{schema.ProjectNamespace}}, nil)
+				// user already has the same role on this project
+				policySvc.EXPECT().List(ctx, policy.Filter{
+					ProjectID: projectID, PrincipalID: userID, PrincipalType: schema.UserPrincipal,
+				}).Return([]policy.Policy{{ID: "existing-p1", RoleID: roleID}}, nil)
+				// no Delete or Create should be called — early return
+			},
+			wantErr: nil,
+		},
 	}
 
 	for _, tt := range tests {