fix: roles seed

2026-05-18 01:28:58 +00:00 · 2026-01-03 02:09:03 +02:00
parent 64a618f54a
commit 8848ba0304
3 changed files with 189 additions and 12 deletions
--- a/lib/Migration/SeedHelper.php
+++ b/lib/Migration/SeedHelper.php
@@ -247,7 +247,8 @@ class SeedHelper {
 			$existingRoles = $result->fetchAll();
 			$result->closeCursor();

-			$existingTypes = array_map(fn ($role) => $role['role_type'], $existingRoles);
+			// Use array_unique to handle duplicates (shouldn't happen after cleanup migration, but be defensive)
+			$existingTypes = array_unique(array_map(fn ($role) => $role['role_type'], $existingRoles));

 			if (count($existingTypes) === 4) {
 				$logger->info('Forum seeding: Default roles already exist, skipping');
@@ -327,7 +328,8 @@ class SeedHelper {
 			$db->commit();

 			// Validate that critical roles can be found by role_type after creation
-			$roleMapper = \OC::$server->get(\OCA\Forum\Db\RoleMapper::class);
+			// Note: We query directly instead of using RoleMapper to avoid MultipleObjectsReturnedException
+			// if duplicates somehow exist (the cleanup migration should have removed them, but be defensive)
 			$criticalRoles = [
 				\OCA\Forum\Db\Role::ROLE_TYPE_GUEST => 'Guest',
 				\OCA\Forum\Db\Role::ROLE_TYPE_DEFAULT => 'Default User',
@@ -335,10 +337,18 @@ class SeedHelper {
 			];

 			foreach ($criticalRoles as $roleType => $roleName) {
-				try {
-					$role = $roleMapper->findByRoleType($roleType);
-					$logger->info("Forum seeding: Validated $roleName role (ID {$role->getId()}, type: $roleType)");
-				} catch (\OCP\AppFramework\Db\DoesNotExistException $e) {
+				$qb = $db->getQueryBuilder();
+				$qb->select('id')
+					->from('forum_roles')
+					->where($qb->expr()->eq('role_type', $qb->createNamedParameter($roleType, \OCP\DB\QueryBuilder\IQueryBuilder::PARAM_STR)))
+					->setMaxResults(1);
+				$result = $qb->executeQuery();
+				$role = $result->fetch();
+				$result->closeCursor();
+
+				if ($role) {
+					$logger->info("Forum seeding: Validated $roleName role (ID {$role['id']}, type: $roleType)");
+				} else {
 					$logger->error("Forum seeding: CRITICAL - $roleName role not found after creation. This will break functionality.");
 					if ($output) {
 						$output->warning("  ✗ CRITICAL: $roleName role not found - forum may not function correctly");
--- a/lib/Migration/Version14Date20260101000000.php
+++ b/lib/Migration/Version14Date20260101000000.php
@@ -14,11 +14,10 @@ use OCP\Migration\SimpleMigrationStep;

 /**
 * Version 14 Migration:
- * - Run seed to ensure all required data exists
+ * - Originally ran seed to ensure all required data exists
+ * - Seeding moved to Version15 which first cleans up duplicate roles
 *
- * This migration fixes an issue from Version 13 where seeding used hardcoded role IDs
- * that may not match the actual role IDs in the database during upgrades.
- * The fix ensures roles are looked up by role_type instead of by ID.
+ * This migration is now a no-op but kept for migration history.
 */
 class Version14Date20260101000000 extends SimpleMigrationStep {
 	/**
@@ -27,7 +26,6 @@ class Version14Date20260101000000 extends SimpleMigrationStep {
 	 * @param array $options
 	 */
 	public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
-		// SeedHelper now uses role_type instead of hardcoded IDs to find roles
-		SeedHelper::seedAll($output);
+		// No-op: Seeding moved to Version15 which first cleans up duplicate roles
 	}
 }
--- a/lib/Migration/Version15Date20260103000000.php
+++ b/lib/Migration/Version15Date20260103000000.php
@@ -0,0 +1,169 @@
+<?php
+
+declare(strict_types=1);
+
+// SPDX-FileCopyrightText: Chen Asraf <contact@casraf.dev>
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+namespace OCA\Forum\Migration;
+
+use Closure;
+use OCP\DB\ISchemaWrapper;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\IDBConnection;
+use OCP\Migration\IOutput;
+use OCP\Migration\SimpleMigrationStep;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Version 15 Migration:
+ * - Clean up duplicate roles that may exist from partial installations
+ * - Add unique constraint on role_type to prevent future duplicates
+ * - Re-run seeding to ensure all required data exists
+ */
+class Version15Date20260103000000 extends SimpleMigrationStep {
+	public function __construct(
+		private IDBConnection $db,
+		private LoggerInterface $logger,
+	) {
+	}
+
+	/**
+	 * Clean up duplicate roles before schema changes
+	 */
+	public function preSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
+		$output->info('Forum: Checking for duplicate roles...');
+		$this->cleanupDuplicateRoles($output);
+	}
+
+	/**
+	 * @param IOutput $output
+	 * @param Closure(): ISchemaWrapper $schemaClosure
+	 * @param array $options
+	 * @return ISchemaWrapper|null
+	 */
+	public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
+		/** @var ISchemaWrapper $schema */
+		$schema = $schemaClosure();
+
+		// Add unique index on role_type to prevent future duplicates
+		if ($schema->hasTable('forum_roles')) {
+			$table = $schema->getTable('forum_roles');
+
+			// Check if the unique index already exists
+			$hasUniqueIndex = false;
+			foreach ($table->getIndexes() as $index) {
+				if ($index->getColumns() === ['role_type'] && $index->isUnique()) {
+					$hasUniqueIndex = true;
+					break;
+				}
+			}
+
+			if (!$hasUniqueIndex) {
+				$output->info('Forum: Adding unique constraint on role_type...');
+				$table->addUniqueIndex(['role_type'], 'forum_roles_role_type_uniq');
+				return $schema;
+			}
+		}
+
+		return null;
+	}
+
+	/**
+	 * @param IOutput $output
+	 * @param Closure(): ISchemaWrapper $schemaClosure
+	 * @param array $options
+	 */
+	public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
+		// Re-run seeding to ensure all required data exists
+		SeedHelper::seedAll($output);
+	}
+
+	/**
+	 * Remove duplicate roles, keeping only the first one of each type
+	 */
+	private function cleanupDuplicateRoles(IOutput $output): void {
+		$roleTypes = ['admin', 'moderator', 'default', 'guest'];
+		$duplicatesRemoved = 0;
+
+		foreach ($roleTypes as $roleType) {
+			// Find all roles of this type, ordered by ID
+			$qb = $this->db->getQueryBuilder();
+			$qb->select('id')
+				->from('forum_roles')
+				->where($qb->expr()->eq('role_type', $qb->createNamedParameter($roleType, IQueryBuilder::PARAM_STR)))
+				->orderBy('id', 'ASC');
+
+			$result = $qb->executeQuery();
+			$roles = $result->fetchAll();
+			$result->closeCursor();
+
+			if (count($roles) <= 1) {
+				continue;
+			}
+
+			// Keep the first one, delete the rest
+			$keepId = (int)$roles[0]['id'];
+			$deleteIds = array_map(fn ($r) => (int)$r['id'], array_slice($roles, 1));
+
+			$this->logger->info('Forum migration: Found ' . count($deleteIds) . " duplicate '$roleType' roles, keeping ID $keepId");
+			$output->info('  Found ' . count($deleteIds) . " duplicate '$roleType' roles, keeping ID $keepId");
+
+			// Update user_roles to point to the kept role before deleting duplicates
+			foreach ($deleteIds as $deleteId) {
+				// Update forum_user_roles: reassign users from duplicate role to kept role
+				$qb = $this->db->getQueryBuilder();
+				$qb->update('forum_user_roles')
+					->set('role_id', $qb->createNamedParameter($keepId, IQueryBuilder::PARAM_INT))
+					->where($qb->expr()->eq('role_id', $qb->createNamedParameter($deleteId, IQueryBuilder::PARAM_INT)));
+
+				try {
+					$qb->executeStatement();
+				} catch (\Exception $e) {
+					// Might fail due to unique constraint if user already has the kept role - that's fine
+					$this->logger->debug("Forum migration: Could not reassign user roles from $deleteId to $keepId: " . $e->getMessage());
+				}
+
+				// Delete orphaned user_roles entries (users who already had the kept role)
+				$qb = $this->db->getQueryBuilder();
+				$qb->delete('forum_user_roles')
+					->where($qb->expr()->eq('role_id', $qb->createNamedParameter($deleteId, IQueryBuilder::PARAM_INT)));
+				$qb->executeStatement();
+
+				// Update forum_category_perms: reassign permissions from duplicate role to kept role
+				$qb = $this->db->getQueryBuilder();
+				$qb->update('forum_category_perms')
+					->set('role_id', $qb->createNamedParameter($keepId, IQueryBuilder::PARAM_INT))
+					->where($qb->expr()->eq('role_id', $qb->createNamedParameter($deleteId, IQueryBuilder::PARAM_INT)));
+
+				try {
+					$qb->executeStatement();
+				} catch (\Exception $e) {
+					// Might fail due to unique constraint - that's fine
+					$this->logger->debug("Forum migration: Could not reassign category perms from $deleteId to $keepId: " . $e->getMessage());
+				}
+
+				// Delete orphaned category_perms entries
+				$qb = $this->db->getQueryBuilder();
+				$qb->delete('forum_category_perms')
+					->where($qb->expr()->eq('role_id', $qb->createNamedParameter($deleteId, IQueryBuilder::PARAM_INT)));
+				$qb->executeStatement();
+			}
+
+			// Now delete the duplicate roles
+			$qb = $this->db->getQueryBuilder();
+			$qb->delete('forum_roles')
+				->where($qb->expr()->in('id', $qb->createNamedParameter($deleteIds, IQueryBuilder::PARAM_INT_ARRAY)));
+			$qb->executeStatement();
+
+			$duplicatesRemoved += count($deleteIds);
+		}
+
+		if ($duplicatesRemoved > 0) {
+			$output->info("  Removed $duplicatesRemoved duplicate roles");
+			$this->logger->info("Forum migration: Removed $duplicatesRemoved duplicate roles");
+		} else {
+			$output->info('  No duplicate roles found');
+		}
+	}
+}