From 8848ba03045f69cba40dd9094ade214f1c1b56cc Mon Sep 17 00:00:00 2001 From: Chen Asraf Date: Sat, 3 Jan 2026 02:09:03 +0200 Subject: [PATCH] fix: roles seed --- lib/Migration/SeedHelper.php | 22 ++- lib/Migration/Version14Date20260101000000.php | 10 +- lib/Migration/Version15Date20260103000000.php | 169 ++++++++++++++++++ 3 files changed, 189 insertions(+), 12 deletions(-) create mode 100644 lib/Migration/Version15Date20260103000000.php diff --git a/lib/Migration/SeedHelper.php b/lib/Migration/SeedHelper.php index b050086..ac7af82 100644 --- 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"); diff --git a/lib/Migration/Version14Date20260101000000.php b/lib/Migration/Version14Date20260101000000.php index 7e12cfe..cee2daa 100644 --- 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 } } diff --git a/lib/Migration/Version15Date20260103000000.php b/lib/Migration/Version15Date20260103000000.php new file mode 100644 index 0000000..e40bc51 --- /dev/null +++ b/lib/Migration/Version15Date20260103000000.php @@ -0,0 +1,169 @@ + +// 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'); + } + } +}