Skip to content

Deleting an entity orphans the members of its aggregate which it references itself (item storages, appearance data, ...) #933

Description

@sven-n

Describe the bug

Deleting a character leaves its inventory — and every item lying in it — in the database as unreachable rows. The same happens for an account's vault, and for 26 further one-to-one members of an aggregate.

The cause is the direction of the foreign key. EfCoreModelGenerator emits ON DELETE CASCADE for every [MemberOfAggregate] member:

// EfCoreModelGenerator.cs, ~line 317
if (isCollection)
{
    modelBuilder.Entity<T>().HasMany(entity => entity.RawX).WithOne().OnDelete(DeleteBehavior.Cascade);
}
else
{
    modelBuilder.Entity<T>().HasOne(entity => entity.RawX).WithOne().OnDelete(DeleteBehavior.Cascade);
}

For a collection that is correct: the foreign key is held by the child (Character."AccountId", Item."ItemStorageId"), and a cascade runs from the referenced row to the referencing one — deleting the account deletes its characters.

For a reference (one-to-one) the foreign key is held by the owner:

Character."InventoryId" → ItemStorage."Id"   (ON DELETE CASCADE)
Account."VaultId"       → ItemStorage."Id"   (ON DELETE CASCADE)

So the cascade we generate today means "deleting the item storage deletes the character" — the opposite of what's meant. Deleting the character does nothing to the storage, and no database can express the other direction with a plain foreign key. This affects 28 relationships, among them LetterBody.SenderAppearance, BattleZoneDefinition.Ground/LeftGoal/RightGoal, CharacterClass.ComboDefinition, MonsterDefinition.MerchantStore, Skill.AreaSkillSettings, QuestReward.ItemReward and the castle siege respawn areas.

To Reproduce

  1. Create a character, put items into its inventory.
  2. Delete the character in the game (DeleteCharacterAction, which just calls DeleteAsync(character)).
  3. select count(*) from data."ItemStorage" — the inventory row is still there, and so are its items.

BotGenerator.DeleteAllBotsAsync already works around this by loading the whole account graph and deleting the storages itself, but that only covers the bot purge; the normal game path leaks.

Expected behavior

Deleting the owner of an aggregate removes the whole aggregate, whichever side holds the foreign key.

Possible solutions

  1. Invert the foreign keys so the child carries the owner id (ItemStorage."CharacterId"), and let the generator emit HasOne(...).WithOne().HasForeignKey<Child>(...). This is the modelling-correct fix, but ItemStorage serves three owners (Character.Inventory, Account.Vault, MonsterDefinition.MerchantStore), so it needs three nullable owner columns or a split of the table, plus a data migration. It also needs a change in JsonQueryBuilder.AddNavigation, which currently bails out on inverse navigations ("inverse property, no data required") and would silently drop inventories and vaults from the loaded account JSON.
  2. Delete triggers, generated from the same annotation. A generic plpgsql trigger function plus one after delete trigger per one-to-one member of an aggregate, emitted by EfCoreModelGenerator next to the cascades it already generates. Much smaller than (1), and it also covers the shallow delete: PostgreSQL fires row triggers for rows deleted by a foreign key cascade, so delete from data."Account" where "Id" = ... alone cleans up the characters and each of their inventories, without loading anything.
  3. Handle it in the application layer (in the persistence layer's delete, not in BotGenerator). Works for every provider, but can never cover a shallow or bulk delete without loading the whole graph first.

(2) is implemented in #934. Whatever we pick, a one-time cleanup of the rows which are already orphaned is needed as well — no future cascade or trigger reaches them.

Additional context — the annotations need a review

[MemberOfAggregate] is not a clean "owned exclusively" list today, and that only stays harmless as long as the delete behaviour is broken in the direction described above. Two members reference a type which is shared:

  • Buff.MagicEffectDefinitionMagicEffectDefinition lives in GameConfiguration.MagicEffects and is referenced (unmarked) by Skill.MagicEffectDef and ItemDefinition.ConsumeEffect. Deleting one buff entry off an NPC would take the effect definition away from a skill and an item.
  • Skill.MasterDefinitionMasterSkillDefinition is in the generator's own StandaloneTypes list.

Both are excluded automatically by the implementation in #934 (the target type is in a GameConfiguration collection, or in StandaloneTypes), so they keep today's behaviour until it's decided whether the annotation or the sharing is wrong.

Two more where I could not tell from the code whether the referenced row is exclusively owned, and which would be good to confirm before the triggers go in:

  • MiniGameChangeEvent.SpawnAreaconfig.MonsterSpawnArea, which also appears in GameMapDefinition.MonsterSpawns.
  • QuestReward.ItemRewarddata.Item.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions