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
- Create a character, put items into its inventory.
- Delete the character in the game (
DeleteCharacterAction, which just calls DeleteAsync(character)).
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
- 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.
- 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.
- 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.MagicEffectDefinition — MagicEffectDefinition 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.MasterDefinition — MasterSkillDefinition 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.SpawnArea → config.MonsterSpawnArea, which also appears in GameMapDefinition.MonsterSpawns.
QuestReward.ItemReward → data.Item.
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.
EfCoreModelGeneratoremitsON DELETE CASCADEfor every[MemberOfAggregate]member: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:
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.ItemRewardand the castle siege respawn areas.To Reproduce
DeleteCharacterAction, which just callsDeleteAsync(character)).select count(*) from data."ItemStorage"— the inventory row is still there, and so are its items.BotGenerator.DeleteAllBotsAsyncalready 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
ItemStorage."CharacterId"), and let the generator emitHasOne(...).WithOne().HasForeignKey<Child>(...). This is the modelling-correct fix, butItemStorageserves 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 inJsonQueryBuilder.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.plpgsqltrigger function plus oneafter deletetrigger per one-to-one member of an aggregate, emitted byEfCoreModelGeneratornext 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, sodelete from data."Account" where "Id" = ...alone cleans up the characters and each of their inventories, without loading anything.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.MagicEffectDefinition—MagicEffectDefinitionlives inGameConfiguration.MagicEffectsand is referenced (unmarked) bySkill.MagicEffectDefandItemDefinition.ConsumeEffect. Deleting one buff entry off an NPC would take the effect definition away from a skill and an item.Skill.MasterDefinition—MasterSkillDefinitionis in the generator's ownStandaloneTypeslist.Both are excluded automatically by the implementation in #934 (the target type is in a
GameConfigurationcollection, or inStandaloneTypes), 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.SpawnArea→config.MonsterSpawnArea, which also appears inGameMapDefinition.MonsterSpawns.QuestReward.ItemReward→data.Item.