Merge 7a824dd4c0d64f816a102d979ac31b7a6e51b1c9 into 3ab8ae5807775a81069005a5abbd20bacfa0a15f

models/perm/access/access.go

@@ -91,7 +91,6 @@ func updateUserAccess(accessMap map[int64]*userAccess, user *user_model.User, mo
 	}
 }
 
-// FIXME: do cross-comparison so reduce deletions and additions to the minimum?
 func refreshAccesses(ctx context.Context, repo *repo_model.Repository, accessMap map[int64]*userAccess) (err error) {
 	minMode := perm.AccessModeRead
 	if err := repo.LoadOwner(ctx); err != nil {
@@ -104,30 +103,76 @@ func refreshAccesses(ctx context.Context, repo *repo_model.Repository, accessMap
 		minMode = perm.AccessModeWrite
 	}
 
-	newAccesses := make([]Access, 0, len(accessMap))
+	// Query existing accesses for cross-comparison
+	var existingAccesses []Access
+	if err := db.GetEngine(ctx).Where(builder.Eq{"repo_id": repo.ID}).Find(&existingAccesses); err != nil {
+		return fmt.Errorf("find existing accesses: %w", err)
+	}
+	existingMap := make(map[int64]perm.AccessMode, len(existingAccesses))
+	for _, a := range existingAccesses {
+		existingMap[a.UserID] = a.Mode
+	}
+
+	var toDelete []int64
+	var toInsert []Access
+	var toUpdate []struct {
+		UserID int64
+		Mode   perm.AccessMode
+	}
+
+	// Determine changes
 	for userID, ua := range accessMap {
 		if ua.Mode < minMode && !ua.User.IsRestricted {
-			continue
+			// Should not have access
+			if _, exists := existingMap[userID]; exists {
+				toDelete = append(toDelete, userID)
+			}
+		} else {
+			desiredMode := ua.Mode
+			if existingMode, exists := existingMap[userID]; exists {
+				if existingMode != desiredMode {
+					toUpdate = append(toUpdate, struct {
+						UserID int64
+						Mode   perm.AccessMode
+					}{UserID: userID, Mode: desiredMode})
+				}
+			} else {
+				toInsert = append(toInsert, Access{
+					UserID: userID,
+					RepoID: repo.ID,
+					Mode:   desiredMode,
+				})
+			}
 		}
-
-		newAccesses = append(newAccesses, Access{
-			UserID: userID,
-			RepoID: repo.ID,
-			Mode:   ua.Mode,
-		})
+		delete(existingMap, userID)
 	}
 
-	// Delete old accesses and insert new ones for repository.
-	if _, err = db.DeleteByBean(ctx, &Access{RepoID: repo.ID}); err != nil {
-		return fmt.Errorf("delete old accesses: %w", err)
-	}
-	if len(newAccesses) == 0 {
-		return nil
+	// Remaining in existingMap should be deleted
+	for userID := range existingMap {
+		toDelete = append(toDelete, userID)
 	}
 
-	if err = db.Insert(ctx, newAccesses); err != nil {
-		return fmt.Errorf("insert new accesses: %w", err)
+	// Execute deletions
+	if len(toDelete) > 0 {
+		if _, err = db.GetEngine(ctx).In("user_id", toDelete).And("repo_id = ?", repo.ID).Delete(&Access{}); err != nil {
+			return fmt.Errorf("delete accesses: %w", err)
+		}
 	}
+
+	// Execute updates
+	for _, u := range toUpdate {
+		if _, err = db.GetEngine(ctx).Where("user_id = ? AND repo_id = ?", u.UserID, repo.ID).Cols("mode").Update(&Access{Mode: u.Mode}); err != nil {
+			return fmt.Errorf("update access for user %d: %w", u.UserID, err)
+		}
+	}
+
+	// Execute insertions
+	if len(toInsert) > 0 {
+		if err = db.Insert(ctx, toInsert); err != nil {
+			return fmt.Errorf("insert new accesses: %w", err)
+		}
+	}
+
 	return nil
 }
 

models/perm/access/access_test.go

@@ -132,3 +132,38 @@ func TestRepository_RecalculateAccesses2(t *testing.T) {
 	assert.NoError(t, err)
 	assert.False(t, has)
 }
+
+func TestRepository_RecalculateAccesses_UpdateMode(t *testing.T) {
+	// Test the update path in refreshAccesses optimization
+	// Scenario: User's access mode changes from Read to Write
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
+	assert.NoError(t, repo.LoadOwner(t.Context()))
+
+	// Verify initial access mode
+	initialAccess := &access_model.Access{UserID: 4, RepoID: 4}
+	has, err := db.GetEngine(t.Context()).Get(initialAccess)
+	assert.NoError(t, err)
+	assert.True(t, has)
+	initialMode := initialAccess.Mode
+
+	// Change collaboration mode to trigger update path
+	newMode := perm_model.AccessModeAdmin
+	assert.NotEqual(t, initialMode, newMode, "New mode should differ from initial mode")
+
+	_, err = db.GetEngine(t.Context()).
+		Where("user_id = ? AND repo_id = ?", 4, 4).
+		Cols("mode").
+		Update(&repo_model.Collaboration{Mode: newMode})
+	assert.NoError(t, err)
+
+	// Recalculate accesses - should UPDATE existing access, not delete+insert
+	assert.NoError(t, access_model.RecalculateAccesses(t.Context(), repo))
+
+	// Verify access was updated, not deleted and re-inserted
+	updatedAccess := &access_model.Access{UserID: 4, RepoID: 4}
+	has, err = db.GetEngine(t.Context()).Get(updatedAccess)
+	assert.NoError(t, err)
+	assert.True(t, has, "Access should still exist")
+	assert.Equal(t, newMode, updatedAccess.Mode, "Access mode should be updated to new collaboration mode")
+}