From c4cffcc2e7db73dafd5912b2af4d1e545c166278 Mon Sep 17 00:00:00 2001
From: Phan An <me@phanan.net>
Date: Mon, 1 Aug 2022 12:42:33 +0200
Subject: [PATCH] feat: use UUIDs for song IDs

---
 .../DeleteNonExistingRecordsPostSync.php      |  9 ++-
 app/Models/Album.php                          |  2 +-
 app/Models/Artist.php                         |  2 +-
 app/Models/Song.php                           | 10 ++-
 app/Models/SupportsDeleteWhereIDsNotIn.php    | 67 -------------------
 app/Models/SupportsDeleteWhereValueNotIn.php  | 57 ++++++++++++++++
 app/Observers/SongObserver.php                | 14 ----
 app/Providers/EventServiceProvider.php        |  7 +-
 app/Repositories/SongRepository.php           |  3 +-
 app/Services/FileSynchronizer.php             | 13 +---
 ...22_08_01_093952_use_uuids_for_song_ids.php | 43 ++++++++++++
 .../__tests__/factory/interactionFactory.ts   |  3 +-
 .../js/__tests__/factory/songFactory.ts       |  3 +-
 resources/assets/js/router.ts                 |  2 +-
 routes/api.v6.php                             |  3 +-
 .../Services/MediaSyncServiceTest.php         |  4 +-
 16 files changed, 128 insertions(+), 114 deletions(-)
 delete mode 100644 app/Models/SupportsDeleteWhereIDsNotIn.php
 create mode 100644 app/Models/SupportsDeleteWhereValueNotIn.php
 delete mode 100644 app/Observers/SongObserver.php
 create mode 100644 database/migrations/2022_08_01_093952_use_uuids_for_song_ids.php

diff --git a/app/Listeners/DeleteNonExistingRecordsPostSync.php b/app/Listeners/DeleteNonExistingRecordsPostSync.php
index a2ef3a4e..c33dc0cc 100644
--- a/app/Listeners/DeleteNonExistingRecordsPostSync.php
+++ b/app/Listeners/DeleteNonExistingRecordsPostSync.php
@@ -5,7 +5,6 @@ namespace App\Listeners;
 use App\Events\MediaSyncCompleted;
 use App\Models\Song;
 use App\Repositories\SongRepository;
-use App\Services\Helper;
 use App\Values\SyncResult;
 
 class DeleteNonExistingRecordsPostSync
@@ -16,12 +15,12 @@ class DeleteNonExistingRecordsPostSync
 
     public function handle(MediaSyncCompleted $event): void
     {
-        $hashes = $event->results
+        $paths = $event->results
             ->valid()
-            ->map(static fn (SyncResult $result) => Helper::getFileHash($result->path))
-            ->merge($this->songRepository->getAllHostedOnS3()->pluck('id'))
+            ->map(static fn (SyncResult $result) => $result->path)
+            ->merge($this->songRepository->getAllHostedOnS3()->pluck('path'))
             ->toArray();
 
-        Song::deleteWhereIDsNotIn($hashes);
+        Song::deleteWhereValueNotIn($paths, 'path');
     }
 }
diff --git a/app/Models/Album.php b/app/Models/Album.php
index db6df044..f527d186 100644
--- a/app/Models/Album.php
+++ b/app/Models/Album.php
@@ -47,7 +47,7 @@ class Album extends Model
 {
     use HasFactory;
     use Searchable;
-    use SupportsDeleteWhereIDsNotIn;
+    use SupportsDeleteWhereValueNotIn;
 
     public const UNKNOWN_ID = 1;
     public const UNKNOWN_NAME = 'Unknown Album';
diff --git a/app/Models/Artist.php b/app/Models/Artist.php
index 064e7f00..87421d24 100644
--- a/app/Models/Artist.php
+++ b/app/Models/Artist.php
@@ -43,7 +43,7 @@ class Artist extends Model
 {
     use HasFactory;
     use Searchable;
-    use SupportsDeleteWhereIDsNotIn;
+    use SupportsDeleteWhereValueNotIn;
 
     public const UNKNOWN_ID = 1;
     public const UNKNOWN_NAME = 'Unknown Artist';
diff --git a/app/Models/Song.php b/app/Models/Song.php
index 2072df1a..1325dbb9 100644
--- a/app/Models/Song.php
+++ b/app/Models/Song.php
@@ -12,6 +12,7 @@ use Illuminate\Database\Eloquent\Relations\BelongsToMany;
 use Illuminate\Database\Eloquent\Relations\HasMany;
 use Illuminate\Database\Query\JoinClause;
 use Illuminate\Support\Collection;
+use Illuminate\Support\Str;
 use Laravel\Scout\Searchable;
 
 /**
@@ -49,9 +50,11 @@ class Song extends Model
 {
     use HasFactory;
     use Searchable;
-    use SupportsDeleteWhereIDsNotIn;
+    use SupportsDeleteWhereValueNotIn;
     use SupportsS3;
 
+    public const ID_REGEX = '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}';
+
     public $incrementing = false;
     protected $guarded = [];
 
@@ -66,6 +69,11 @@ class Song extends Model
 
     protected $keyType = 'string';
 
+    protected static function booted(): void
+    {
+        static::creating(static fn (self $song) => $song->id = Str::uuid()->toString());
+    }
+
     public function artist(): BelongsTo
     {
         return $this->belongsTo(Artist::class);
diff --git a/app/Models/SupportsDeleteWhereIDsNotIn.php b/app/Models/SupportsDeleteWhereIDsNotIn.php
deleted file mode 100644
index 2ac088fd..00000000
--- a/app/Models/SupportsDeleteWhereIDsNotIn.php
+++ /dev/null
@@ -1,67 +0,0 @@
-<?php
-
-namespace App\Models;
-
-use Illuminate\Database\Eloquent\Builder;
-use Illuminate\Support\Facades\DB;
-
-/**
- * With reference to GitHub issue #463.
- * MySQL and PostgresSQL seem to have a limit of 2^16-1 (65535) elements in an IN statement.
- * This trait provides a method as a workaround to this limitation.
- *
- * @method static Builder whereIn($keys, array $values)
- * @method static Builder whereNotIn($keys, array $values)
- * @method static Builder select(string $string)
- */
-trait SupportsDeleteWhereIDsNotIn
-{
-    /**
-     * Deletes all records whose IDs are not in an array.
-     *
-     * @param array<string>|array<int> $ids the array of IDs
-     * @param string $key name of the primary key
-     */
-    public static function deleteWhereIDsNotIn(array $ids, string $key = 'id'): void
-    {
-        $maxChunkSize = config('database.default') === 'sqlite-persistent' ? 999 : 65535;
-
-        // If the number of entries is lower than, or equals to maxChunkSize, just go ahead.
-        if (count($ids) <= $maxChunkSize) {
-            static::whereNotIn($key, $ids)->delete();
-
-            return;
-        }
-
-        // Otherwise, we get the actual IDs that should be deleted…
-        $allIDs = static::select($key)->get()->pluck($key)->all();
-        $whereInIDs = array_diff($allIDs, $ids);
-
-        // …and see if we can delete them instead.
-        if (count($whereInIDs) < $maxChunkSize) {
-            static::whereIn($key, $whereInIDs)->delete();
-
-            return;
-        }
-
-        // If that's not possible (i.e. this array has more than maxChunkSize elements, too)
-        // then we'll delete chunk by chunk.
-        static::deleteByChunk($ids, $key, $maxChunkSize);
-    }
-
-    /**
-     * Delete records chunk by chunk.
-     *
-     * @param array<string>|array<int> $ids The array of record IDs to delete
-     * @param string $key Name of the primary key
-     * @param int $chunkSize Size of each chunk. Defaults to 2^16-1 (65535)
-     */
-    public static function deleteByChunk(array $ids, string $key = 'id', int $chunkSize = 65535): void
-    {
-        DB::transaction(static function () use ($ids, $key, $chunkSize): void {
-            foreach (array_chunk($ids, $chunkSize) as $chunk) {
-                static::whereIn($key, $chunk)->delete();
-            }
-        });
-    }
-}
diff --git a/app/Models/SupportsDeleteWhereValueNotIn.php b/app/Models/SupportsDeleteWhereValueNotIn.php
new file mode 100644
index 00000000..f33390e5
--- /dev/null
+++ b/app/Models/SupportsDeleteWhereValueNotIn.php
@@ -0,0 +1,57 @@
+<?php
+
+namespace App\Models;
+
+use Illuminate\Database\Eloquent\Builder;
+use Illuminate\Support\Facades\DB;
+
+/**
+ * With reference to GitHub issue #463.
+ * MySQL and PostgresSQL seem to have a limit of 2^16-1 (65535) elements in an IN statement.
+ * This trait provides a method as a workaround to this limitation.
+ *
+ * @method static Builder whereIn($keys, array $values)
+ * @method static Builder whereNotIn($keys, array $values)
+ * @method static Builder select(string $string)
+ */
+trait SupportsDeleteWhereValueNotIn
+{
+    /**
+     * Deletes all records whose certain value is not in an array.
+     */
+    public static function deleteWhereValueNotIn(array $values, string $field = 'id'): void
+    {
+        $maxChunkSize = config('database.default') === 'sqlite-persistent' ? 999 : 65535;
+
+        // If the number of entries is lower than, or equals to maxChunkSize, just go ahead.
+        if (count($values) <= $maxChunkSize) {
+            static::whereNotIn($field, $values)->delete();
+
+            return;
+        }
+
+        // Otherwise, we get the actual IDs that should be deleted…
+        $allIDs = static::select($field)->get()->pluck($field)->all();
+        $whereInIDs = array_diff($allIDs, $values);
+
+        // …and see if we can delete them instead.
+        if (count($whereInIDs) < $maxChunkSize) {
+            static::whereIn($field, $whereInIDs)->delete();
+
+            return;
+        }
+
+        // If that's not possible (i.e. this array has more than maxChunkSize elements, too)
+        // then we'll delete chunk by chunk.
+        static::deleteByChunk($values, $field, $maxChunkSize);
+    }
+
+    public static function deleteByChunk(array $values, string $field = 'id', int $chunkSize = 65535): void
+    {
+        DB::transaction(static function () use ($values, $field, $chunkSize): void {
+            foreach (array_chunk($values, $chunkSize) as $chunk) {
+                static::whereIn($field, $chunk)->delete();
+            }
+        });
+    }
+}
diff --git a/app/Observers/SongObserver.php b/app/Observers/SongObserver.php
deleted file mode 100644
index 8ca7f0af..00000000
--- a/app/Observers/SongObserver.php
+++ /dev/null
@@ -1,14 +0,0 @@
-<?php
-
-namespace App\Observers;
-
-use App\Models\Song;
-use App\Services\Helper;
-
-class SongObserver
-{
-    public function creating(Song $song): void
-    {
-        $song->id = Helper::getFileHash($song->path);
-    }
-}
diff --git a/app/Providers/EventServiceProvider.php b/app/Providers/EventServiceProvider.php
index 079e00c4..b5569871 100644
--- a/app/Providers/EventServiceProvider.php
+++ b/app/Providers/EventServiceProvider.php
@@ -16,12 +16,10 @@ use App\Listeners\PruneLibrary;
 use App\Listeners\UnloveMultipleTracksOnLastfm;
 use App\Listeners\UpdateLastfmNowPlaying;
 use App\Models\Album;
-use App\Models\Song;
 use App\Observers\AlbumObserver;
-use App\Observers\SongObserver;
-use Illuminate\Foundation\Support\Providers\EventServiceProvider as ServiceProvider;
+use Illuminate\Foundation\Support\Providers\EventServiceProvider as BaseServiceProvider;
 
-class EventServiceProvider extends ServiceProvider
+class EventServiceProvider extends BaseServiceProvider
 {
     protected $listen = [
         SongLikeToggled::class => [
@@ -54,7 +52,6 @@ class EventServiceProvider extends ServiceProvider
     {
         parent::boot();
 
-        Song::observe(SongObserver::class);
         Album::observe(AlbumObserver::class);
     }
 }
diff --git a/app/Repositories/SongRepository.php b/app/Repositories/SongRepository.php
index af02a24c..a2be86d4 100644
--- a/app/Repositories/SongRepository.php
+++ b/app/Repositories/SongRepository.php
@@ -8,7 +8,6 @@ use App\Models\Playlist;
 use App\Models\Song;
 use App\Models\User;
 use App\Repositories\Traits\Searchable;
-use App\Services\Helper;
 use Illuminate\Contracts\Database\Query\Builder;
 use Illuminate\Contracts\Pagination\Paginator;
 use Illuminate\Support\Collection;
@@ -32,7 +31,7 @@ class SongRepository extends Repository
 
     public function getOneByPath(string $path): ?Song
     {
-        return $this->getOneById(Helper::getFileHash($path));
+        return Song::where('path', $path)->first();
     }
 
     /** @return Collection|array<Song> */
diff --git a/app/Services/FileSynchronizer.php b/app/Services/FileSynchronizer.php
index 3e10ac55..7ed65bb7 100644
--- a/app/Services/FileSynchronizer.php
+++ b/app/Services/FileSynchronizer.php
@@ -20,12 +20,6 @@ class FileSynchronizer
     private ?int $fileModifiedTime = null;
     private ?string $filePath = null;
 
-    /**
-     * A (MD5) hash of the file's path.
-     * This value is unique, and can be used to query a Song record.
-     */
-    private ?string $fileHash = null;
-
     /**
      * The song model that's associated with the current file.
      */
@@ -46,9 +40,8 @@ class FileSynchronizer
     {
         $file = $path instanceof SplFileInfo ? $path : new SplFileInfo($path);
 
-        $this->filePath = $file->getPathname();
-        $this->fileHash = Helper::getFileHash($this->filePath);
-        $this->song = $this->songRepository->getOneById($this->fileHash); // @phpstan-ignore-line
+        $this->filePath = $file->getRealPath();
+        $this->song = $this->songRepository->getOneByPath($this->filePath);
         $this->fileModifiedTime = Helper::getModifiedTime($file);
 
         return $this;
@@ -96,7 +89,7 @@ class FileSynchronizer
         $data['album_id'] = $album->id;
         $data['artist_id'] = $artist->id;
 
-        $this->song = Song::updateOrCreate(['id' => $this->fileHash], $data);
+        $this->song = Song::updateOrCreate(['path' => $this->filePath], $data);
 
         return SyncResult::success($this->filePath);
     }
diff --git a/database/migrations/2022_08_01_093952_use_uuids_for_song_ids.php b/database/migrations/2022_08_01_093952_use_uuids_for_song_ids.php
new file mode 100644
index 00000000..bbf9c723
--- /dev/null
+++ b/database/migrations/2022_08_01_093952_use_uuids_for_song_ids.php
@@ -0,0 +1,43 @@
+<?php
+
+use App\Models\Song;
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\DB;
+use Illuminate\Support\Facades\Schema;
+use Illuminate\Support\Str;
+
+return new class extends Migration
+{
+    public function up(): void
+    {
+        Schema::table('songs', static function (Blueprint $table): void {
+            $table->string('id', 36)->change();
+        });
+
+        Schema::table('playlist_song', static function (Blueprint $table): void {
+            $table->string('song_id', 36)->change();
+
+            if (DB::getDriverName() !== 'sqlite') {
+                $table->dropForeign('playlist_song_song_id_foreign');
+            }
+
+            $table->foreign('song_id')->references('id')->on('songs')->cascadeOnDelete()->cascadeOnUpdate();
+        });
+
+        Schema::table('interactions', static function (Blueprint $table): void {
+            $table->string('song_id', 36)->change();
+
+            if (DB::getDriverName() !== 'sqlite') {
+                $table->dropForeign('playlist_song_song_id_foreign');
+            }
+
+            $table->foreign('song_id')->references('id')->on('songs')->cascadeOnDelete()->cascadeOnUpdate();
+        });
+
+        Song::all()->each(static function (Song $song): void {
+            $song->id = Str::uuid();
+            $song->save();
+        });
+    }
+};
diff --git a/resources/assets/js/__tests__/factory/interactionFactory.ts b/resources/assets/js/__tests__/factory/interactionFactory.ts
index 4e0a4f57..d36392a4 100644
--- a/resources/assets/js/__tests__/factory/interactionFactory.ts
+++ b/resources/assets/js/__tests__/factory/interactionFactory.ts
@@ -1,10 +1,9 @@
-import crypto from 'crypto-random-string'
 import { Faker } from '@faker-js/faker'
 
 export default (faker: Faker): Interaction => ({
   type: 'interactions',
   id: faker.datatype.number({ min: 1 }),
-  song_id: crypto(32),
+  song_id: faker.datatype.uuid(),
   liked: faker.datatype.boolean(),
   play_count: faker.datatype.number({ min: 1 })
 })
diff --git a/resources/assets/js/__tests__/factory/songFactory.ts b/resources/assets/js/__tests__/factory/songFactory.ts
index 6ce909a1..5c8c01f3 100644
--- a/resources/assets/js/__tests__/factory/songFactory.ts
+++ b/resources/assets/js/__tests__/factory/songFactory.ts
@@ -1,4 +1,3 @@
-import crypto from 'crypto-random-string'
 import { Faker, faker } from '@faker-js/faker'
 
 const generate = (partOfCompilation = false): Song => {
@@ -14,7 +13,7 @@ const generate = (partOfCompilation = false): Song => {
     album_artist_id: partOfCompilation ? artistId + 1 : artistId,
     album_artist_name: partOfCompilation ? artistName : faker.name.findName(),
     album_cover: faker.image.imageUrl(),
-    id: crypto(32),
+    id: faker.datatype.uuid(),
     title: faker.lorem.sentence(),
     length: faker.datatype.number(),
     track: faker.datatype.number(),
diff --git a/resources/assets/js/router.ts b/resources/assets/js/router.ts
index 730fb726..eb0ee08a 100644
--- a/resources/assets/js/router.ts
+++ b/resources/assets/js/router.ts
@@ -26,7 +26,7 @@ class Router {
       '/album/(\\d+)': async (id: string) => loadMainView('Album', parseInt(id)),
       '/artist/(\\d+)': async (id: string) => loadMainView('Artist', parseInt(id)),
       '/playlist/(\\d+)': (id: number) => use(playlistStore.byId(~~id), playlist => loadMainView('Playlist', playlist)),
-      '/song/([a-z0-9]{32})': async (id: string) => {
+      '/song/([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})': async (id: string) => {
         const song = await songStore.resolve(id)
         if (!song) {
           this.go('home')
diff --git a/routes/api.v6.php b/routes/api.v6.php
index f627d309..61179f14 100644
--- a/routes/api.v6.php
+++ b/routes/api.v6.php
@@ -18,6 +18,7 @@ use App\Http\Controllers\V6\API\QueueController;
 use App\Http\Controllers\V6\API\RecentlyPlayedSongController;
 use App\Http\Controllers\V6\API\SongController;
 use App\Http\Controllers\V6\API\SongSearchController;
+use App\Models\Song;
 use Illuminate\Support\Facades\Route;
 
 Route::prefix('api')->middleware('api')->group(static function (): void {
@@ -38,7 +39,7 @@ Route::prefix('api')->middleware('api')->group(static function (): void {
         Route::post('playlists/{playlist}/songs', [PlaylistSongController::class, 'add']);
         Route::delete('playlists/{playlist}/songs', [PlaylistSongController::class, 'remove']);
 
-        Route::apiResource('songs', SongController::class)->where(['song' => '[a-f0-9]{32}']);
+        Route::apiResource('songs', SongController::class)->where(['song' => Song::ID_REGEX]);
         Route::get('songs/recently-played', [RecentlyPlayedSongController::class, 'index']);
         Route::get('songs/favorite', [FavoriteSongController::class, 'index']);
 
diff --git a/tests/Integration/Services/MediaSyncServiceTest.php b/tests/Integration/Services/MediaSyncServiceTest.php
index caaaad1e..f5476664 100644
--- a/tests/Integration/Services/MediaSyncServiceTest.php
+++ b/tests/Integration/Services/MediaSyncServiceTest.php
@@ -168,8 +168,8 @@ class MediaSyncServiceTest extends TestCase
 
         // Song should be added back with all info
         self::assertEquals(
-            Arr::except(Song::findOrFail($song->id)->toArray(), 'created_at'),
-            Arr::except($song->toArray(), 'created_at')
+            Arr::except(Song::where('path', $song->path)->first()->toArray(), ['id', 'created_at']),
+            Arr::except($song->toArray(), ['id', 'created_at'])
         );
     }