Delete the members of a member of an aggregate, too

src/GameLogic/Bots/BotGenerator.cs

@@ -492,29 +492,16 @@ private async ValueTask<BotAccountDeleteOutcome> TryDeleteBotAccountAsync(string
         try
         {
             // Load the account again, this time with its whole graph: the paging query returns the
-            // accounts untracked and without their characters, and deleting such a shallow account
-            // leaves its item storages behind. A character's inventory is referenced by the character,
-            // so no delete cascade ever reaches it - those storages, and every item lying in them, would
-            // stay in the database forever as unreachable rows.
+            // accounts untracked and without their characters, and only a loaded member of the
+            // aggregate can be deleted with it. DeleteAsync goes through the whole graph now, so the
+            // item storages (the vault and the inventories of the characters) are deleted with the
+            // account - they are referenced BY their owner, so no delete cascade reaches them.
             account = await context.GetAccountByLoginNameAsync(loginName, cancellationToken).ConfigureAwait(false);
             if (account is null)
             {
                 return BotAccountDeleteOutcome.NotFound;
             }
 
-            foreach (var character in account.Characters)
-            {
-                if (character.Inventory is { } inventory)
-                {
-                    await context.DeleteAsync(inventory).ConfigureAwait(false);
-                }
-            }
-
-            if (account.Vault is { } vault)
-            {
-                await context.DeleteAsync(vault).ConfigureAwait(false);
-            }
-
             var deleteQueued = await context.DeleteAsync(account).ConfigureAwait(false);
 
             // Save per account, so a single failure does not roll back the accounts already deleted.

src/Persistence/EntityFramework/EntityFrameworkContextBase.cs

@@ -21,6 +21,20 @@ namespace MUnique.OpenMU.Persistence.EntityFramework;
 /// </summary>
 internal class EntityFrameworkContextBase : IContext
 {
+    /// <summary>
+    /// The types whose objects are held by a collection of the <see cref="GameConfiguration"/>.
+    /// Such an object is a root of its own and shared between its owners - a magic effect definition
+    /// is used by a buff, a skill and an item definition alike - so deleting one of its owners must
+    /// not walk into it and take its members along.
+    /// </summary>
+    private static readonly IReadOnlySet<Type> SharedConfigurationTypes = typeof(GameConfiguration)
+        .GetProperties()
+        .Select(property => property.PropertyType)
+        .Select(GetCollectionInterface)
+        .Where(collectionInterface => collectionInterface is not null)
+        .Select(collectionInterface => collectionInterface!.GetGenericArguments()[0])
+        .ToHashSet();
+
     private readonly bool _isOwner;
     private readonly IConfigurationChangeListener? _changeListener;
     private readonly AsyncLock _lock = new();
@@ -155,7 +169,10 @@ public async ValueTask<bool> DeleteAsync<T>(T obj)
                 break;
             default:
                 this.Context.Remove(obj);
-                this.ForEachAggregate(obj, a => this.Context.Remove(a));
+
+                // Stops at the shared configuration objects: deleting a monster definition must not
+                // take the magic effect definition of its buffs with it.
+                this.ForEachAggregate(obj, a => this.Context.Remove(a), stopAtSharedConfiguration: true);
                 break;
         }
 
@@ -353,11 +370,21 @@ private bool DetachInternal(object item)
 
         var previousState = entry.State;
         entry.State = EntityState.Detached;
-        this.ForEachAggregate(item, obj => this.DetachInternal(obj));
+
+        // ForEachAggregate goes through the whole aggregate, so the action must not recurse itself.
+        this.ForEachAggregate(item, this.DetachSingle, stopAtSharedConfiguration: false);
 
         return previousState != EntityState.Added;
     }
 
+    private void DetachSingle(object item)
+    {
+        if (this.Context.Entry(item) is { } entry)
+        {
+            entry.State = EntityState.Detached;
+        }
+    }
+
     private IRepository<T> GetRepository<T>()
         where T : class
     {
@@ -379,7 +406,28 @@ private IRepository GetRepository(Type type)
         throw new RepositoryNotFoundException(type);
     }
 
-    private void ForEachAggregate(object obj, Action<object> action)
+    /// <summary>
+    /// Executes the given action for every member of the aggregate of the given object, including
+    /// the members of these members.
+    /// </summary>
+    /// <param name="obj">The aggregate root.</param>
+    /// <param name="action">The action to execute for each member.</param>
+    /// <param name="stopAtSharedConfiguration">
+    /// If set to <c>true</c>, the members of a <see cref="SharedConfigurationTypes">shared configuration
+    /// object</see> are left alone. Required when deleting, not when detaching.
+    /// </param>
+    /// <remarks>
+    /// The recursion matters for the deletion: a member which is referenced BY its owner (e.g. the
+    /// inventory of a character) holds its foreign key at the owner, so no delete cascade of the
+    /// database ever reaches it. Deleting an account removed its characters, but their inventories -
+    /// and every item lying in them - stayed in the database as unreachable rows.
+    /// </remarks>
+    private void ForEachAggregate(object obj, Action<object> action, bool stopAtSharedConfiguration)
+    {
+        this.ForEachAggregate(obj, action, stopAtSharedConfiguration, true, new HashSet<object>(ReferenceEqualityComparer.Instance));
+    }
+
+    private void ForEachAggregate(object obj, Action<object> action, bool stopAtSharedConfiguration, bool areDirectMembers, HashSet<object> handledMembers)
     {
         var aggregateProperties = obj.GetType()
             .GetProperties(BindingFlags.FlattenHierarchy | BindingFlags.Public | BindingFlags.Instance)
@@ -392,14 +440,60 @@ private void ForEachAggregate(object obj, Action<object> action)
             {
                 foreach (var value in enumerable)
                 {
-                    action(value);
+                    this.HandleAggregateMember(value, action, stopAtSharedConfiguration, areDirectMembers, handledMembers);
                 }
             }
             else if (propertyValue is { })
             {
-                action(propertyValue);
+                this.HandleAggregateMember(propertyValue, action, stopAtSharedConfiguration, areDirectMembers, handledMembers);
+            }
+        }
+    }
+
+    private void HandleAggregateMember(object member, Action<object> action, bool stopAtSharedConfiguration, bool isDirectMember, HashSet<object> handledMembers)
+    {
+        // By reference: the entities compare equal by their id, and freshly created ones share the
+        // default id. Handling a member twice would be wrong anyway, and a graph which references
+        // itself would recurse forever.
+        if (!handledMembers.Add(member))
+        {
+            return;
+        }
+
+        if (stopAtSharedConfiguration && !isDirectMember && IsSharedConfiguration(member))
+        {
+            // Reached through the aggregate of a member, e.g. the magic effect definition of a buff
+            // of a monster definition - which a skill and an item definition may use as well. The
+            // direct members are left as they were: only the recursion is new here.
+            return;
+        }
+
+        action(member);
+        this.ForEachAggregate(member, action, stopAtSharedConfiguration, false, handledMembers);
+    }
+
+    private static Type? GetCollectionInterface(Type type)
+    {
+        if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(ICollection<>))
+        {
+            return type;
+        }
+
+        return type.GetInterfaces()
+            .FirstOrDefault(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(ICollection<>));
+    }
+
+    private static bool IsSharedConfiguration(object member)
+    {
+        for (Type? type = member.GetType(); type is not null; type = type.BaseType)
+        {
+            if (SharedConfigurationTypes.Contains(type))
+            {
+                return true;
             }
         }
+
+        return false;
     }
 
     [System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "Catching all Exceptions.")]

src/Persistence/EntityFramework/Migrations/20260922183000_CleanUpOrphanedAggregateMembers.cs

@@ -0,0 +1,101 @@
+// <copyright file="20260922183000_CleanUpOrphanedAggregateMembers.cs" company="MUnique">
+// Licensed under the MIT License. See LICENSE file in the project root for full license information.
+// </copyright>
+
+#nullable disable
+
+namespace MUnique.OpenMU.Persistence.EntityFramework.Migrations
+{
+    using Microsoft.EntityFrameworkCore.Infrastructure;
+    using Microsoft.EntityFrameworkCore.Migrations;
+
+    /// <inheritdoc />
+    [DbContext(typeof(EntityDataContext))]
+    [Migration("20260922183000_CleanUpOrphanedAggregateMembers")]
+    public partial class CleanUpOrphanedAggregateMembers : Migration
+    {
+        /// <inheritdoc />
+        protected override void Up(MigrationBuilder migrationBuilder)
+        {
+            // Removes the rows which were orphaned while EntityFrameworkContextBase.ForEachAggregate
+            // did not recurse into the members of a member: deleting an account removed its characters,
+            // but not their inventories - and an inventory is referenced BY its character, so no delete
+            // cascade of the database ever reached it. The same happened to every member which is
+            // referenced by its owner and sits two levels below the deleted object.
+            // The children of a removed row follow through its own cascade.
+            migrationBuilder.Sql(
+                @"do $$
+declare
+    target record;
+    reference record;
+    conditions text;
+begin
+    -- A row of these tables is only reachable through a reference held by its owner, so a row which
+    -- nothing points at is unreachable. Tables whose rows can also be owned through a foreign key of
+    -- their own are deliberately not listed (Item, PowerUpDefinition, MonsterSpawnArea,
+    -- CastleSiegeZoneDefinition, MagicEffectDefinition): an item in a storage holds that reference
+    -- itself, so nothing points at it is true for almost every item of every player.
+    for target in
+        select * from (values
+                ('config', 'AreaSkillSettings', '{}'::text[]),
+                ('config', 'BattleZoneDefinition', '{}'::text[]),
+                ('config', 'CastleSiegeConfiguration', array['CastleSiegeNpcDefinition', 'CastleSiegeStateScheduleEntry', 'CastleSiegeUpgradeDefinition', 'CastleSiegeZoneDefinition']::text[]),
+                ('config', 'DuelConfiguration', array['DuelArea']::text[]),
+                ('config', 'MasterSkillDefinition', '{}'::text[]),
+                ('config', 'PowerUpDefinitionValue', array['AttributeRelationship']::text[]),
+                ('config', 'Rectangle', '{}'::text[]),
+                ('config', 'SimpleCraftingSettings', array['ItemCraftingRequiredItem', 'ItemCraftingResultItem']::text[]),
+                ('config', 'SkillComboDefinition', array['SkillComboStep']::text[]),
+                ('data', 'AppearanceData', array['ItemAppearance']::text[]),
+                ('data', 'ItemStorage', array['Item']::text[])
+        ) as v(schema_name, table_name, owned_tables)
+    loop
+        conditions := '';
+
+        -- Every foreign key which points at the target, read from the catalog instead of a written
+        -- list: one which is missed would delete rows which are in use. The tables of the target's
+        -- own aggregate are skipped - they point at their owner, and an owner which only its own
+        -- members still reference is exactly what is unreachable.
+        for reference in
+            select source_schema.nspname as source_schema,
+                   source_table.relname as source_table,
+                   source_column.attname as source_column,
+                   target_column.attname as target_column
+            from pg_constraint c
+                join pg_class source_table on source_table.oid = c.conrelid
+                join pg_namespace source_schema on source_schema.oid = source_table.relnamespace
+                join pg_class referenced_table on referenced_table.oid = c.confrelid
+                join pg_namespace referenced_schema on referenced_schema.oid = referenced_table.relnamespace
+                join lateral unnest(c.conkey, c.confkey) as key_columns(source_attnum, target_attnum) on true
+                join pg_attribute source_column
+                    on source_column.attrelid = source_table.oid and source_column.attnum = key_columns.source_attnum
+                join pg_attribute target_column
+                    on target_column.attrelid = referenced_table.oid and target_column.attnum = key_columns.target_attnum
+            where c.contype = 'f'
+              and referenced_schema.nspname::text = target.schema_name
+              and referenced_table.relname::text = target.table_name
+              and not (source_table.relname::text = any (target.owned_tables))
+        loop
+            conditions := conditions || format(
+                ' and not exists (select 1 from %I.%I r where r.%I = x.%I)',
+                reference.source_schema, reference.source_table, reference.source_column, reference.target_column);
+        end loop;
+
+        if conditions = '' then
+            -- Nothing points at this table at all, so an orphan cannot be told from a row in use.
+            continue;
+        end if;
+
+        execute format('delete from %I.%I x where true%s', target.schema_name, target.table_name, conditions);
+    end loop;
+end $$;
+");
+        }
+
+        /// <inheritdoc />
+        protected override void Down(MigrationBuilder migrationBuilder)
+        {
+            // Nothing to do - the removed rows were unreachable and can't be restored.
+        }
+    }
+}

tests/MUnique.OpenMU.Persistence.Initialization.Tests/AggregateDeletionEfCoreTests.cs

@@ -0,0 +1,97 @@
+// <copyright file="AggregateDeletionEfCoreTests.cs" company="MUnique">
+// Licensed under the MIT License. See LICENSE file in the project root for full license information.
+// </copyright>
+
+namespace MUnique.OpenMU.Persistence.Initialization.Tests;
+
+using Microsoft.EntityFrameworkCore;
+using Microsoft.Extensions.Logging.Abstractions;
+using MUnique.OpenMU.DataModel.Configuration;
+using MUnique.OpenMU.DataModel.Entities;
+using MUnique.OpenMU.Persistence.EntityFramework;
+using ItemStorageEntity = MUnique.OpenMU.Persistence.EntityFramework.Model.ItemStorage;
+
+/// <summary>
+/// Tests the deletion of an aggregate with the entity framework core, which requires a running postgres database.
+/// </summary>
+/// <remarks>
+/// The interesting part is what the database is left with, so this can't be tested without one.
+/// </remarks>
+[TestFixture]
+internal class AggregateDeletionEfCoreTests
+{
+    /// <summary>
+    /// Deletes an account which owns a vault and a character with an inventory, and checks that no
+    /// item storage is left behind. The inventory is the case which used to leak: it's referenced BY
+    /// the character, so no delete cascade of the database reaches it, and the traversal of the
+    /// aggregate didn't go deeper than the account's own members.
+    /// </summary>
+    [Test]
+    [Ignore("This is not a real test which should run automatically. It requires a database.")]
+    public async Task DeletingAnAccountDeletesTheItemStoragesOfItsCharactersAsync()
+    {
+        var contextProvider = await CreateInitializedDatabaseAsync().ConfigureAwait(false);
+        GameConfiguration configuration;
+        using (var context = contextProvider.CreateNewContext())
+        {
+            configuration = (await context.GetAsync<GameConfiguration>().ConfigureAwait(false)).Single();
+        }
+
+        using (var context = contextProvider.CreateNewPlayerContext(configuration))
+        {
+            var account = context.CreateNew<Account>();
+            account.LoginName = "deleteme";
+            account.PasswordHash = "hash";
+            account.Vault = context.CreateNew<ItemStorage>();
+
+            var characterClass = configuration.CharacterClasses.First(c => c is { CanGetCreated: true, HomeMap: not null });
+            var character = context.CreateNew<Character>();
+            character.Name = "DeleteMe";
+            character.CharacterClass = characterClass;
+            character.CurrentMap = characterClass.HomeMap;
+            character.CreateDate = DateTime.UtcNow;
+            character.KeyConfiguration = new byte[30];
+            character.Inventory = context.CreateNew<ItemStorage>();
+            account.Characters.Add(character);
+
+            await context.SaveChangesAsync().ConfigureAwait(false);
+        }
+
+        Assert.That(await CountItemStoragesAsync().ConfigureAwait(false), Is.EqualTo(2), "The vault and the inventory should have been created.");
+
+        using (var context = contextProvider.CreateNewPlayerContext(configuration))
+        {
+            var account = await context.GetAccountByLoginNameAsync("deleteme").ConfigureAwait(false);
+            Assert.That(account, Is.Not.Null);
+            await context.DeleteAsync(account!).ConfigureAwait(false);
+
+            // Must not throw: nothing may delete a row which the context has marked as deleted itself.
+            await context.SaveChangesAsync().ConfigureAwait(false);
+        }
+
+        Assert.That(await CountItemStoragesAsync().ConfigureAwait(false), Is.EqualTo(0));
+    }
+
+    private static async ValueTask<PersistenceContextProvider> CreateInitializedDatabaseAsync()
+    {
+        await ReCreateDatabaseAsync().ConfigureAwait(false);
+
+        var contextProvider = new PersistenceContextProvider(new NullLoggerFactory(), null);
+        await new VersionSeasonSix.DataInitialization(contextProvider, new NullLoggerFactory())
+            .CreateInitialDataAsync(1, true).ConfigureAwait(false);
+
+        return contextProvider;
+    }
+
+    private static async ValueTask ReCreateDatabaseAsync()
+    {
+        var contextProvider = new PersistenceContextProvider(new NullLoggerFactory(), null);
+        using var update = await contextProvider.ReCreateDatabaseAsync().ConfigureAwait(false);
+    }
+
+    private static async ValueTask<int> CountItemStoragesAsync()
+    {
+        await using var context = new EntityDataContext();
+        return await context.Set<ItemStorageEntity>().CountAsync().ConfigureAwait(false);
+    }
+}