Merge pull request #18266 from mm-workspace/custom-display-preferences-transaction

Replace custom item display preferences in one transaction

Jellyfin.Server.Implementations/Users/DisplayPreferencesManager.cs

@@ -13,6 +13,8 @@ namespace Jellyfin.Server.Implementations.Users;
 /// </summary>
 public sealed class DisplayPreferencesManager : IDisplayPreferencesManager
 {
+    private const int MaxSaveAttempts = 3;
+
     private readonly IDbContextFactory<JellyfinDbContext> _dbContextFactory;
 
     /// <summary>
@@ -28,17 +30,31 @@ public sealed class DisplayPreferencesManager : IDisplayPreferencesManager
     public DisplayPreferences GetDisplayPreferences(Guid userId, Guid itemId, string client)
     {
         using var dbContext = _dbContextFactory.CreateDbContext();
-        var prefs = dbContext.DisplayPreferences
-            .Include(pref => pref.HomeSections)
-            .FirstOrDefault(pref =>
-                pref.UserId.Equals(userId) && pref.Client == client && pref.ItemId.Equals(itemId));
+        var prefs = FindDisplayPreferences(dbContext, userId, itemId, client);
+        if (prefs is not null)
+        {
+            return prefs;
+        }
 
-        if (prefs is null)
+        prefs = new DisplayPreferences(userId, itemId, client);
+        dbContext.DisplayPreferences.Add(prefs);
+        try
         {
-            prefs = new DisplayPreferences(userId, itemId, client);
-            dbContext.DisplayPreferences.Add(prefs);
             dbContext.SaveChanges();
         }
+        catch (DbUpdateException)
+        {
+            // Another request may have stored the preferences between the lookup and the insert, and the unique index
+            // rejected this one. Return the stored preferences; if there are none, the insert failed for another reason.
+            using var retryContext = _dbContextFactory.CreateDbContext();
+            var stored = FindDisplayPreferences(retryContext, userId, itemId, client);
+            if (stored is null)
+            {
+                throw;
+            }
+
+            return stored;
+        }
 
         return prefs;
     }
@@ -83,19 +99,35 @@ public sealed class DisplayPreferencesManager : IDisplayPreferencesManager
     /// <inheritdoc />
     public void SetCustomItemDisplayPreferences(Guid userId, Guid itemId, string client, Dictionary<string, string?> customPreferences)
     {
-        using var dbContext = _dbContextFactory.CreateDbContext();
-        dbContext.CustomItemDisplayPreferences.Where(prefs => prefs.UserId.Equals(userId)
-                            && prefs.ItemId.Equals(itemId)
-                            && prefs.Client == client)
-                            .ExecuteDelete();
-
-        foreach (var (key, value) in customPreferences)
+        // Another request can store one of these keys after this one's delete, and the unique index then rejects the
+        // insert. Replacing the set again gives the same result, so the replace is repeated.
+        for (var attempt = 1; ; attempt++)
         {
-            dbContext.CustomItemDisplayPreferences
-                .Add(new CustomItemDisplayPreferences(userId, itemId, client, key, value));
-        }
+            using var dbContext = _dbContextFactory.CreateDbContext();
+            using var transaction = dbContext.Database.BeginTransaction();
+            dbContext.CustomItemDisplayPreferences.Where(prefs => prefs.UserId.Equals(userId)
+                                && prefs.ItemId.Equals(itemId)
+                                && prefs.Client == client)
+                                .ExecuteDelete();
 
-        dbContext.SaveChanges();
+            foreach (var (key, value) in customPreferences)
+            {
+                dbContext.CustomItemDisplayPreferences
+                    .Add(new CustomItemDisplayPreferences(userId, itemId, client, key, value));
+            }
+
+            try
+            {
+                dbContext.SaveChanges();
+            }
+            catch (DbUpdateException) when (attempt < MaxSaveAttempts)
+            {
+                continue;
+            }
+
+            transaction.Commit();
+            return;
+        }
     }
 
     /// <inheritdoc/>
@@ -113,4 +145,9 @@ public sealed class DisplayPreferencesManager : IDisplayPreferencesManager
         dbContext.ItemDisplayPreferences.Attach(itemDisplayPreferences).State = EntityState.Modified;
         dbContext.SaveChanges();
     }
+
+    private static DisplayPreferences? FindDisplayPreferences(JellyfinDbContext dbContext, Guid userId, Guid itemId, string client)
+        => dbContext.DisplayPreferences
+            .Include(pref => pref.HomeSections)
+            .FirstOrDefault(pref => pref.UserId.Equals(userId) && pref.Client == client && pref.ItemId.Equals(itemId));
 }

tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerConcurrentCreateTests.cs

@@ -0,0 +1,77 @@
+using System;
+using Jellyfin.Database.Implementations.Entities;
+using Jellyfin.Server.Implementations.Tests.Item;
+using Jellyfin.Server.Implementations.Users;
+using Microsoft.EntityFrameworkCore.Diagnostics;
+using Xunit;
+
+namespace Jellyfin.Server.Implementations.Tests.Users;
+
+/// <summary>
+/// Two first-ever requests for the same preferences can both find nothing and both insert; the unique index lets one
+/// through. The other has to return the stored preferences instead of failing.
+/// </summary>
+public sealed class DisplayPreferencesManagerConcurrentCreateTests : SqliteDbTestFixture
+{
+    private const string Client = "client";
+
+    private static readonly Guid _userId = Guid.Parse("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa");
+    private static readonly Guid _itemId = Guid.Parse("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb");
+
+    private readonly CompetingInsert _competingInsert;
+    private readonly DisplayPreferencesManager _manager;
+
+    public DisplayPreferencesManagerConcurrentCreateTests()
+        : this(new CompetingInsert())
+    {
+    }
+
+    private DisplayPreferencesManagerConcurrentCreateTests(CompetingInsert competingInsert)
+        : base(competingInsert)
+    {
+        _competingInsert = competingInsert;
+        _manager = new DisplayPreferencesManager(CreateDbContextFactory());
+
+        using var context = CreateDbContext();
+        context.Users.Add(new User("user", "auth-provider", "reset-provider") { Id = _userId });
+        context.SaveChanges();
+    }
+
+    [Fact]
+    public void GetDisplayPreferences_AnotherRequestStoresThemFirst_ReturnsTheStoredPreferences()
+    {
+        var storedId = 0;
+        _competingInsert.Before = () =>
+        {
+            using var context = CreateDbContext();
+            var stored = new DisplayPreferences(_userId, _itemId, Client);
+            context.DisplayPreferences.Add(stored);
+            context.SaveChanges();
+            storedId = stored.Id;
+        };
+
+        var preferences = _manager.GetDisplayPreferences(_userId, _itemId, Client);
+
+        Assert.NotEqual(0, storedId);
+        Assert.Equal(storedId, preferences.Id);
+        using var check = CreateDbContext();
+        Assert.Single(check.DisplayPreferences);
+    }
+
+    /// <summary>
+    /// Stores the other request's row just before the next save, after the manager has looked for the preferences.
+    /// </summary>
+    private sealed class CompetingInsert : SaveChangesInterceptor
+    {
+        public Action? Before { get; set; }
+
+        public override InterceptionResult<int> SavingChanges(DbContextEventData eventData, InterceptionResult<int> result)
+        {
+            // Disarmed first, because the competing save goes through this interceptor as well.
+            var before = Before;
+            Before = null;
+            before?.Invoke();
+            return result;
+        }
+    }
+}

tests/Jellyfin.Server.Implementations.Tests/Users/DisplayPreferencesManagerFailedWriteTests.cs

@@ -0,0 +1,99 @@
+using System;
+using System.Collections.Generic;
+using System.Data.Common;
+using Jellyfin.Server.Implementations.Tests.Item;
+using Jellyfin.Server.Implementations.Users;
+using Microsoft.Data.Sqlite;
+using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.Diagnostics;
+using Xunit;
+
+namespace Jellyfin.Server.Implementations.Tests.Users;
+
+public sealed class DisplayPreferencesManagerFailedWriteTests : SqliteDbTestFixture
+{
+    private const string Client = "client";
+
+    private static readonly Guid _userId = Guid.Parse("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa");
+    private static readonly Guid _itemId = Guid.Parse("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb");
+
+    private readonly StatementInterceptor _statements;
+    private readonly DisplayPreferencesManager _manager;
+
+    public DisplayPreferencesManagerFailedWriteTests()
+        : this(new StatementInterceptor())
+    {
+    }
+
+    private DisplayPreferencesManagerFailedWriteTests(StatementInterceptor statements)
+        : base(statements)
+    {
+        _statements = statements;
+        _manager = new DisplayPreferencesManager(CreateDbContextFactory());
+    }
+
+    [Fact]
+    public void SetCustomItemDisplayPreferences_InsertFails_KeepsThePreviousPreferences()
+    {
+        _manager.SetCustomItemDisplayPreferences(_userId, _itemId, Client, new Dictionary<string, string?> { ["first"] = "1" });
+
+        _statements.FailInserts = true;
+        Assert.Throws<DbUpdateException>(() => _manager.SetCustomItemDisplayPreferences(_userId, _itemId, Client, new Dictionary<string, string?> { ["second"] = "2" }));
+
+        Assert.Equal(
+            new Dictionary<string, string?> { ["first"] = "1" },
+            _manager.ListCustomItemDisplayPreferences(_userId, _itemId, Client));
+    }
+
+    [Fact]
+    public void SetCustomItemDisplayPreferences_AnotherRequestStoredAKeyAfterTheDelete_StoresThePreferencesItWasGiven()
+    {
+        _manager.SetCustomItemDisplayPreferences(_userId, _itemId, Client, new Dictionary<string, string?> { ["first"] = "1" });
+
+        // The delete misses the row, as it does when another request stores it after this one's delete, so the unique
+        // index rejects this request's insert.
+        _statements.SkipNextDelete = true;
+        _manager.SetCustomItemDisplayPreferences(_userId, _itemId, Client, new Dictionary<string, string?> { ["first"] = "2" });
+
+        Assert.Equal(
+            new Dictionary<string, string?> { ["first"] = "2" },
+            _manager.ListCustomItemDisplayPreferences(_userId, _itemId, Client));
+    }
+
+    /// <summary>
+    /// Fails inserts into CustomItemDisplayPreferences with the error SQLite reports when the unique index rejects a row, or
+    /// leaves out the next delete from it.
+    /// </summary>
+    private sealed class StatementInterceptor : DbCommandInterceptor
+    {
+        public bool FailInserts { get; set; }
+
+        public bool SkipNextDelete { get; set; }
+
+        public override InterceptionResult<DbDataReader> ReaderExecuting(DbCommand command, CommandEventData eventData, InterceptionResult<DbDataReader> result)
+        {
+            FailInsert(command);
+            return result;
+        }
+
+        public override InterceptionResult<int> NonQueryExecuting(DbCommand command, CommandEventData eventData, InterceptionResult<int> result)
+        {
+            if (SkipNextDelete && command.CommandText.StartsWith("DELETE FROM \"CustomItemDisplayPreferences\"", StringComparison.Ordinal))
+            {
+                SkipNextDelete = false;
+                return InterceptionResult<int>.SuppressWithResult(0);
+            }
+
+            FailInsert(command);
+            return result;
+        }
+
+        private void FailInsert(DbCommand command)
+        {
+            if (FailInserts && command.CommandText.StartsWith("INSERT INTO \"CustomItemDisplayPreferences\"", StringComparison.Ordinal))
+            {
+                throw new SqliteException("SQLite Error 19: 'UNIQUE constraint failed: CustomItemDisplayPreferences.UserId, CustomItemDisplayPreferences.ItemId, CustomItemDisplayPreferences.Client, CustomItemDisplayPreferences.Key'.", 19);
+            }
+        }
+    }
+}