diff --git a/models/perm/access/access.go b/models/perm/access/access.go index 6433c4675c..e7a90f3d41 100644 --- a/models/perm/access/access.go +++ b/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 } diff --git a/models/perm/access/access_test.go b/models/perm/access/access_test.go index 15d18b368c..fb7cf5ca58 100644 --- a/models/perm/access/access_test.go +++ b/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") +}