From 0d161021abfb27a31f8091c3eb8045f207c75113 Mon Sep 17 00:00:00 2001
From: FarbodZamani <53179227+ferishili@users.noreply.github.com>
Date: Wed, 4 Jun 2025 11:58:32 +0200
Subject: [PATCH] Fix playlist sync db issue (#1366)

* Enhance playlist migration by enforcing foreign key constraints and ensuring non-nullable columns

* make sure the playlist table has no null config_id

* Draft: check for invalid records!

* add more error handling

* try different fkey name

* try separate db query for the same fkey
---
 lib/Helpers/PlaylistMigration.php | 182 +++++++++++++++++++-----------
 1 file changed, 117 insertions(+), 65 deletions(-)

diff --git a/lib/Helpers/PlaylistMigration.php b/lib/Helpers/PlaylistMigration.php
index c28c6d71..62d11734 100644
--- a/lib/Helpers/PlaylistMigration.php
+++ b/lib/Helpers/PlaylistMigration.php
@@ -19,79 +19,131 @@ class PlaylistMigration
     {
         $db = DBManager::get();
 
-        // Migrate existing playlists to Opencast
-        $playlists = Playlists::findBySQL('service_playlist_id IS NULL');
-
-        // Try three times
-        $tries = 0;
-        while (!empty($playlists) && $tries < 3) {
-            foreach ($playlists as $playlist) {
-                $config_id = $playlist->config_id;
-                if (empty($config_id)) {
-                    // Default opencast server if playlist belongs to no opencast server
-                    $config_id = \Config::get()->OPENCAST_DEFAULT_SERVER;
-                }
+        // Default opencast server if playlist belongs to no opencast server
+        $default_config_id = \Config::get()->OPENCAST_DEFAULT_SERVER;
+
+        try {
+            // Migrate existing playlists to Opencast
+            $playlists = Playlists::findBySQL('service_playlist_id IS NULL');
 
-                // Get playlist videos
-                $playlist_videos = self::getPlaylistVideos($playlist);
-
-                $api_playlists_client = ApiPlaylistsClient::getInstance($config_id);
-                $oc_playlist = $api_playlists_client->createPlaylist(
-                    self::getOcPlaylistData($playlist, $playlist_videos)
-                );
-
-                if ($oc_playlist) {
-                    // Store oc playlist reference in Stud.IP if successfully created
-                    $playlist->config_id = $config_id;
-                    $playlist->service_playlist_id = $oc_playlist->id;
-                    $playlist->store();
-
-                    Playlists::checkPlaylistACL($oc_playlist, $playlist);
-
-                    // Store entry ids
-                    for ($i = 0; $i < count($playlist_videos); $i++) {
-                        $stmt = $db->prepare("UPDATE oc_playlist_video
-                            SET `service_entry_id` = :service_entry_id
-                            WHERE playlist_id = :playlist_id AND video_id = :video_id
-                        ");
-                        $stmt->execute($data = [
-                            'service_entry_id' => $oc_playlist->entries[$i]->id,
-                            'playlist_id' => $playlist->id,
-                            'video_id' => $playlist_videos[$i]['id']
-                        ]);
+            // Try three times
+            $tries = 0;
+            while (!empty($playlists) && $tries < 10) {
+                foreach ($playlists as $playlist) {
+                    $config_id = $playlist->config_id ?? null;
+                    if (empty($config_id)) {
+                        // Default opencast server if playlist belongs to no opencast server
+                        $config_id = $default_config_id;
                     }
-                }
-            }
 
-            $playlists = Playlists::findBySQL('service_playlist_id IS NULL');
-            $tries++;
-        }
+                    // Get playlist videos
+                    $playlist_videos = self::getPlaylistVideos($playlist);
+
+                    $api_playlists_client = ApiPlaylistsClient::getInstance($config_id);
+                    $oc_playlist = $api_playlists_client->createPlaylist(
+                        self::getOcPlaylistData($playlist, $playlist_videos)
+                    );
+
+                    if ($oc_playlist) {
+                        // Store oc playlist reference in Stud.IP if successfully created
+                        $playlist->config_id = $config_id;
+                        $playlist->service_playlist_id = $oc_playlist->id;
+                        $playlist->store();
+
+                        Playlists::checkPlaylistACL($oc_playlist, $playlist);
+
+                        // Store entry ids
+                        for ($i = 0; $i < count($playlist_videos); $i++) {
+                            $stmt = $db->prepare("UPDATE oc_playlist_video
+                                SET `service_entry_id` = :service_entry_id
+                                WHERE playlist_id = :playlist_id AND video_id = :video_id
+                            ");
+                            $stmt->execute($data = [
+                                'service_entry_id' => $oc_playlist->entries[$i]->id,
+                                'playlist_id' => $playlist->id,
+                                'video_id' => $playlist_videos[$i]['id']
+                            ]);
+                        }
+                    }
+                }
 
-        // Forbid playlist without related oc playlist
-        $db->exec('ALTER TABLE `oc_playlist`
-            CHANGE COLUMN `config_id` `config_id` int NOT NULL,
-            CHANGE COLUMN `service_playlist_id` `service_playlist_id` varchar(64) UNIQUE NOT NULL'
-        );
+                $playlists = Playlists::findBySQL('service_playlist_id IS NULL');
+                $tries++;
+            }
 
-        // Forbid playlist video without related oc playlist entry
-        $db->exec('ALTER TABLE `oc_playlist_video`
-            CHANGE COLUMN `service_entry_id` `service_entry_id` int NOT NULL'
-        );
+            // What is the point of letting the process continue if there are still playlists with null service_playlist_id of duplicated service_playlist_ids?
+            $duplicate_service_playlist_ids = $db->query(
+                "SELECT service_playlist_id, COUNT(*) as count
+                FROM oc_playlist
+                WHERE service_playlist_id IS NOT NULL
+                GROUP BY service_playlist_id
+                HAVING count > 1"
+            )->fetchAll(\PDO::FETCH_ASSOC);
+
+            if (!empty($playlists) || !empty($duplicate_service_playlist_ids)) {
+                $message = "Migration failed due to invalid data records:\n";
+                if (!empty($playlists)) {
+                    $message .= "Playlists with null service_playlist_id:\n";
+                    foreach ($playlists as $playlist) {
+                        $message .= "Playlist ID: {$playlist->id}\n";
+                    }
+                }
+                if (!empty($duplicate_service_playlist_ids)) {
+                    $message .= "Duplicate service_playlist_ids:\n";
+                    foreach ($duplicate_service_playlist_ids as $record) {
+                        $message .= "Service Playlist ID: {$record['service_playlist_id']}, Count: {$record['count']}\n";
+                    }
+                }
+                throw new \Exception($message);
+            }
 
-        \SimpleOrMap::expireTableScheme();
+            // We need another step to make sure config id is set and it is not null before altering the table with not-null config_id.
+            $null_config_playlists = Playlists::findBySQL('config_id IS NULL');
 
-        // Add playlists sync cronjob
-        $scheduler = \CronjobScheduler::getInstance();
+            while (!empty($null_config_playlists)) {
+                foreach ($null_config_playlists as $null_config_playlist) {
+                    // Store config id with default config id.
+                    $null_config_playlist->config_id = $default_config_id;
+                    $null_config_playlist->store();
+                }
+                $null_config_playlists = Playlists::findBySQL('config_id IS NULL');
+            }
 
-        if (!$task_id = \CronjobTask::findByFilename(self::CRONJOBS_DIR . 'opencast_sync_playlists.php')[0]->task_id) {
-            $task_id =  $scheduler->registerTask(self::CRONJOBS_DIR . 'opencast_sync_playlists.php', true);
-        }
+            // Forbid playlist without related oc playlist
+            // First drop foreign key constraint
+            $db->exec('ALTER TABLE `oc_playlist` DROP FOREIGN KEY `oc_playlist_ibfk_1`');
+            // Then change column to not null
+            $db->exec('ALTER TABLE `oc_playlist`
+                CHANGE COLUMN `config_id` `config_id` int NOT NULL,
+                CHANGE COLUMN `service_playlist_id` `service_playlist_id` varchar(64) UNIQUE NOT NULL'
+            );
+            // Then add foreign key constraint again
+            $db->exec('ALTER TABLE `oc_playlist`
+                ADD FOREIGN KEY `oc_playlist_ibfk_1` (`config_id`) REFERENCES `oc_config` (`id`)
+                ON DELETE RESTRICT ON UPDATE RESTRICT'
+            );
+            // Forbid playlist video without related oc playlist entry
+            $db->exec('ALTER TABLE `oc_playlist_video`
+                CHANGE COLUMN `service_entry_id` `service_entry_id` int NOT NULL'
+            );
+
+            \SimpleOrMap::expireTableScheme();
+
+            // Add playlists sync cronjob
+            $scheduler = \CronjobScheduler::getInstance();
+
+            if (!$task_id = \CronjobTask::findByFilename(self::CRONJOBS_DIR . 'opencast_sync_playlists.php')[0]->task_id) {
+                $task_id =  $scheduler->registerTask(self::CRONJOBS_DIR . 'opencast_sync_playlists.php', true);
+            }
 
-        // add the new cronjobs
-        if ($task_id) {
-            $scheduler->cancelByTask($task_id);
-            $scheduler->schedulePeriodic($task_id, -10);  // negative value means "every x minutes"
-            \CronjobSchedule::findByTask_id($task_id)[0]->activate();
+            // add the new cronjobs
+            if ($task_id) {
+                $scheduler->cancelByTask($task_id);
+                $scheduler->schedulePeriodic($task_id, -10);  // negative value means "every x minutes"
+                \CronjobSchedule::findByTask_id($task_id)[0]->activate();
+            }
+        } catch (\Throwable $th) {
+            throw new \Exception('Migration fehlgeschlagen: ' . $th->getMessage());
         }
     }
 
@@ -179,4 +231,4 @@ class PlaylistMigration
 
         return $result['Null'] != 'YES';
     }
-}
\ No newline at end of file
+}
-- 
GitLab