fix: added lock for playlist to prevent race conditions (#771)

AnimeThemes · Jan 6, 2025 · d609fbd · d609fbd
1 parent e10bcfc
commit d609fbd
Show file tree

Hide file tree

Showing 63 changed files with 1,028 additions and 408 deletions.
diff --git a/app/Actions/Http/Api/List/Playlist/Track/DestroyTrackAction.php b/app/Actions/Http/Api/List/Playlist/Track/DestroyTrackAction.php
@@ -32,6 +32,10 @@ public function destroy(Playlist $playlist, PlaylistTrack $track): Model
         try {
             DB::beginTransaction();
 
+            // Lock tracks to prevent race conditions.
+            $playlist->tracks()->getQuery()->lockForUpdate()->get();
+            $playlist->query()->lockForUpdate()->first();
+
             $removeAction = new RemoveTrackAction();
 
             $removeAction->remove($playlist, $track);

diff --git a/app/Actions/Http/Api/List/Playlist/Track/ForceDeleteTrackAction.php b/app/Actions/Http/Api/List/Playlist/Track/ForceDeleteTrackAction.php
@@ -31,6 +31,10 @@ public function forceDelete(Playlist $playlist, PlaylistTrack $track): string
         try {
             DB::beginTransaction();
 
+            // Lock tracks to prevent race conditions.
+            $playlist->tracks()->getQuery()->lockForUpdate()->get();
+            $playlist->query()->lockForUpdate()->first();
+
             $removeAction = new RemoveTrackAction();
 
             $removeAction->remove($playlist, $track);

diff --git a/app/Actions/Http/Api/List/Playlist/Track/RestoreTrackAction.php b/app/Actions/Http/Api/List/Playlist/Track/RestoreTrackAction.php
@@ -32,6 +32,10 @@ public function restore(Playlist $playlist, PlaylistTrack $track): Model
         try {
             DB::beginTransaction();
 
+            // Lock tracks to prevent race conditions.
+            $playlist->tracks()->getQuery()->lockForUpdate()->get();
+            $playlist->query()->lockForUpdate()->first();
+
             $restoreAction = new RestoreAction();
 
             $restoreAction->restore($track);

diff --git a/app/Actions/Http/Api/List/Playlist/Track/StoreTrackAction.php b/app/Actions/Http/Api/List/Playlist/Track/StoreTrackAction.php
@@ -45,6 +45,10 @@ public function store(Playlist $playlist, Builder $builder, array $parameters):
         try {
             DB::beginTransaction();
 
+            // Lock tracks to prevent race conditions.
+            $playlist->tracks()->getQuery()->lockForUpdate()->get();
+            $playlist->query()->lockForUpdate()->first();
+
             $storeAction = new StoreAction();
 
             /** @var PlaylistTrack $track */

diff --git a/app/Actions/Http/Api/List/Playlist/Track/UpdateTrackAction.php b/app/Actions/Http/Api/List/Playlist/Track/UpdateTrackAction.php
@@ -42,6 +42,10 @@ public function update(Playlist $playlist, PlaylistTrack $track, array $paramete
         try {
             DB::beginTransaction();
 
+            // Lock tracks to prevent race conditions.
+            $playlist->tracks()->getQuery()->lockForUpdate()->get();
+            $playlist->query()->lockForUpdate()->first();
+
             $updateAction = new UpdateAction();
 
             $updateAction->update($track, $trackParameters);

diff --git a/app/Actions/Models/List/ExternalProfile/ExternalEntry/BaseExternalEntryAction.php b/app/Actions/Models/List/ExternalProfile/ExternalEntry/BaseExternalEntryAction.php
@@ -6,6 +6,7 @@
 
 use App\Enums\Models\List\ExternalProfileSite;
 use App\Enums\Models\Wiki\ResourceSite;
+use App\Models\List\ExternalProfile;
 use Illuminate\Support\Arr;
 
 /**
@@ -20,9 +21,9 @@ abstract class BaseExternalEntryAction
     /**
      * Create a new action instance.
      *
-     * @param  array  $profileParameters
+     * @param  ExternalProfile|array  $profile
      */
-    public function __construct(protected array $profileParameters)
+    public function __construct(protected ExternalProfile|array $profile)
     {
     }
 
@@ -33,7 +34,12 @@ public function __construct(protected array $profileParameters)
      */
     public function getProfileSite(): ExternalProfileSite
     {
-        return ExternalProfileSite::fromLocalizedName(Arr::get($this->profileParameters, 'site'));
+        if ($this->profile instanceof ExternalProfile) {
+            return $this->profile->site;
+        }
+
+        // TODO: change 'site' to a constant variable in API.
+        return ExternalProfileSite::fromLocalizedName(Arr::get($this->profile, 'site'));
     }
 
     /**
@@ -53,7 +59,12 @@ protected function getResourceSite(): ResourceSite
      */
     public function getUsername(): string
     {
-        return Arr::get($this->profileParameters, 'name');
+        if ($this->profile instanceof ExternalProfile) {
+            return $this->profile->name;
+        }
+
+        // TODO: change 'name' to a constant variable in API.
+        return Arr::get($this->profile, 'name');
     }
 
     /**

diff --git a/app/Actions/Models/List/ExternalProfile/ExternalEntry/BaseExternalEntryTokenAction.php b/app/Actions/Models/List/ExternalProfile/ExternalEntry/BaseExternalEntryTokenAction.php
@@ -14,11 +14,24 @@
  */
 abstract class BaseExternalEntryTokenAction
 {
+    /**
+     * The response of the external API.
+     *
+     * @var array|null $response
+     */
     protected ?array $response = null;
+
+    /**
+     * The id of the external user.
+     *
+     * @var int|null $userId
+     */
     protected ?int $userId = null;
 
     /**
      * Create a new action instance.
+     *
+     * @param  ExternalToken  $token
      */
     public function __construct(protected ExternalToken $token)
     {
@@ -49,10 +62,7 @@ public function getToken(): string
      *
      * @return string|null
      */
-    public function getUsername(): ?string
-    {
-        return null;
-    }
+    abstract public function getUsername(): ?string;
 
     /**
      * Get the entries of the response.

diff --git a/app/Actions/Models/List/ExternalProfile/ExternalEntry/Token/MalExternalEntryTokenAction.php b/app/Actions/Models/List/ExternalProfile/ExternalEntry/Token/MalExternalEntryTokenAction.php
@@ -18,6 +18,11 @@
  */
 class MalExternalEntryTokenAction extends BaseExternalEntryTokenAction
 {
+    /**
+     * The response of the user endpoint in external API.
+     *
+     * @var array|null $userResponse
+     */
     protected ?array $userResponse = null;
 
     /**

diff --git a/app/Actions/Models/List/ExternalProfile/ExternalToken/BaseExternalTokenAction.php b/app/Actions/Models/List/ExternalProfile/ExternalToken/BaseExternalTokenAction.php
@@ -5,24 +5,20 @@
 namespace App\Actions\Models\List\ExternalProfile\ExternalToken;
 
 use App\Models\List\External\ExternalToken;
+use Exception;
 
 /**
  * Class BaseExternalTokenAction.
  */
 abstract class BaseExternalTokenAction
 {
-    /**
-     * Create a new action instance.
-     */
-    public function __construct()
-    {
-    }
-
     /**
      * Use the authorization code to get the tokens and store them.
      *
      * @param  array  $parameters
-     * @return ExternalToken|null
+     * @return ExternalToken
+     *
+     * @throws Exception
      */
-    abstract public function store(array $parameters): ?ExternalToken;
+    abstract public function store(array $parameters): ExternalToken;
 }
diff --git a/app/Actions/Models/List/ExternalProfile/ExternalToken/Site/MalExternalTokenAction.php b/app/Actions/Models/List/ExternalProfile/ExternalToken/Site/MalExternalTokenAction.php
@@ -35,8 +35,6 @@ public function store(array $parameters): ExternalToken
 
         $codeVerifier = Cache::get("mal-external-token-request-{$state}");
 
-        Cache::forget("mal-external-token-request-{$state}");
-
         try {
             $response = Http::asForm()
                 ->post('https://myanimelist.net/v1/oauth2/token', [
@@ -65,6 +63,8 @@ public function store(array $parameters): ExternalToken
             Log::error($e->getMessage());
 
             throw $e;
+        } finally {
+            Cache::forget("mal-external-token-request-{$state}");
         }
     }
 }
diff --git a/app/Actions/Models/List/ExternalProfile/StoreExternalProfileTokenAction.php b/app/Actions/Models/List/ExternalProfile/StoreExternalProfileTokenAction.php
@@ -16,6 +16,7 @@
 use Illuminate\Support\Arr;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\Log;
+use RuntimeException;
 
 /**
  * Class StoreExternalProfileTokenAction.
@@ -25,28 +26,24 @@ class StoreExternalProfileTokenAction
     protected Collection $resources;
 
     /**
-     * Find or store external profile given determined external token.
+     * Get the first record or store external profile given determined external token.
      *
      * @param  ExternalToken  $token
      * @param  array  $parameters
      * @return ExternalProfile|null
      *
      * @throws Exception
      */
-    public function findOrCreate(ExternalToken $token, array $parameters): ?ExternalProfile
+    public function firstOrCreate(ExternalToken $token, array $parameters): ?ExternalProfile
     {
         try {
             $site = ExternalProfileSite::fromLocalizedName(Arr::get($parameters, 'site'));
 
-            $action = $this->getActionClass($site, $token);
-
-            if ($action === null) {
-                return null;
-            }
+            $action = static::getActionClass($site, $token);
 
             $userId = $action->getUserId();
 
-            $profile = $this->findForUserIdOrCreate($userId, $site, $action, $parameters);
+            $profile = $this->firstForUserIdOrCreate($userId, $site, $action, $parameters);
 
             return $profile;
 
@@ -58,15 +55,15 @@ public function findOrCreate(ExternalToken $token, array $parameters): ?External
     }
 
     /**
-     * Find or create the profile for a userId and site.
+     * Get the first record or create the profile for a userId and site.
      *
      * @param  int  $userId
      * @param  ExternalProfileSite  $site
      * @param  BaseExternalEntryTokenAction  $action
      * @param  array  $parameters
      * @return ExternalProfile
      */
-    protected function findForUserIdOrCreate(int $userId, ExternalProfileSite $site, BaseExternalEntryTokenAction $action, array $parameters): ExternalProfile
+    protected function firstForUserIdOrCreate(int $userId, ExternalProfileSite $site, BaseExternalEntryTokenAction $action, array $parameters): ExternalProfile
     {
         $claimedProfile = ExternalProfile::query()
             ->where(ExternalProfile::ATTRIBUTE_EXTERNAL_USER_ID, $userId)
@@ -113,14 +110,16 @@ protected function findForUserIdOrCreate(int $userId, ExternalProfileSite $site,
      *
      * @param  ExternalProfileSite  $site
      * @param  ExternalToken  $token
-     * @return BaseExternalEntryTokenAction|null
+     * @return BaseExternalEntryTokenAction
+     *
+     * @throws RuntimeException
      */
-    protected function getActionClass(ExternalProfileSite $site, ExternalToken $token): ?BaseExternalEntryTokenAction
+    public static function getActionClass(ExternalProfileSite $site, ExternalToken $token): BaseExternalEntryTokenAction
     {
         return match ($site) {
             ExternalProfileSite::ANILIST => new AnilistExternalEntryTokenAction($token),
             ExternalProfileSite::MAL => new MalExternalEntryTokenAction($token),
-            default => null,
+            default => throw new RuntimeException("External entry token action not configured for site {$site->localize()}"),
         };
     }
 }
diff --git a/app/Actions/Models/List/ExternalProfile/StoreExternalProfileUsernameAction.php b/app/Actions/Models/List/ExternalProfile/StoreExternalProfileUsernameAction.php
@@ -10,30 +10,32 @@
 use App\Enums\Models\List\ExternalProfileSite;
 use App\Enums\Models\List\ExternalProfileVisibility;
 use App\Models\List\ExternalProfile;
-use Error;
 use Exception;
 use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Support\Arr;
 use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Log;
+use RuntimeException;
 
 /**
  * Class StoreExternalProfileUsernameAction.
  */
 class StoreExternalProfileUsernameAction
 {
     /**
-     * Find or store an external profile and its entries given determined username.
+     * Get the first record or store an external profile and its entries given determined username.
      *
      * @param  Builder  $builder
      * @param  array  $profileParameters
      * @return ExternalProfile
      *
      * @throws Exception
      */
-    public function findOrCreate(Builder $builder, array $profileParameters): ExternalProfile
+    public function firstOrCreate(Builder $builder, array $profileParameters): ExternalProfile
     {
         try {
+            DB::beginTransaction();
+
             $profileSite = ExternalProfileSite::fromLocalizedName(Arr::get($profileParameters, 'site'));
 
             $findProfile = ExternalProfile::query()
@@ -42,16 +44,11 @@ public function findOrCreate(Builder $builder, array $profileParameters): Extern
                 ->first();
 
             if ($findProfile instanceof ExternalProfile) {
+                DB::rollBack();
                 return $findProfile;
             }
 
-            DB::beginTransaction();
-
-            $action = $this->getActionClass($profileSite, $profileParameters);
-
-            if ($action === null) {
-                throw new Error("Action not found for site {$profileSite->localize()}", 404);
-            }
+            $action = static::getActionClass($profileSite, $profileParameters);
 
             $storeAction = new StoreAction();
 
@@ -79,14 +76,16 @@ public function findOrCreate(Builder $builder, array $profileParameters): Extern
      * Get the mapping for the entries class.
      *
      * @param  ExternalProfileSite  $site
-     * @param  array  $profileParameters
-     * @return BaseExternalEntryAction|null
+     * @param  ExternalProfile|array  $profile
+     * @return BaseExternalEntryAction
+     *
+     * @throws RuntimeException
      */
-    protected function getActionClass(ExternalProfileSite $site, array $profileParameters): ?BaseExternalEntryAction
+    public static function getActionClass(ExternalProfileSite $site, ExternalProfile|array $profile): BaseExternalEntryAction
     {
         return match ($site) {
-            ExternalProfileSite::ANILIST => new AnilistExternalEntryAction($profileParameters),
-            default => null,
+            ExternalProfileSite::ANILIST => new AnilistExternalEntryAction($profile),
+            default => throw new RuntimeException("External entry action not configured for site {$site->localize()}"),
         };
     }
 }