From d9585b166a5f7d4f38f849cd6a93a995690ef17c Mon Sep 17 00:00:00 2001 From: Bastian Wagner Date: Thu, 20 Aug 2026 00:36:54 +0200 Subject: [PATCH] fix(rewards): resolve item definitions up front and stabilize reward item order Resolve every rolled ItemDefinition before mutating the character or creating the CombatReward row, so a missing definition can no longer half-grant (XP/silver saved, reward row created, then throw). Also make the immediate grant response and a later loadRewards replay agree on item order by sorting both on itemDefinitionId instead of roll/insertion order. Co-Authored-By: Claude Opus 5 --- .../src/rewards/combat-reward.service.spec.ts | 94 ++++++++++++++++++- apps/api/src/rewards/combat-reward.service.ts | 48 ++++++++-- 2 files changed, 130 insertions(+), 12 deletions(-) diff --git a/apps/api/src/rewards/combat-reward.service.spec.ts b/apps/api/src/rewards/combat-reward.service.spec.ts index 850f02c..a2c57df 100644 --- a/apps/api/src/rewards/combat-reward.service.spec.ts +++ b/apps/api/src/rewards/combat-reward.service.spec.ts @@ -21,6 +21,7 @@ const ROAD_BANDIT_ID = '30000000-0000-4000-8000-000000000002'; const ASH_RAT_TABLE = '60000000-0000-4000-8000-000000000001'; const ROAD_BANDIT_TABLE = '60000000-0000-4000-8000-000000000002'; const BANDIT_BLADE = '50000000-0000-4000-8000-000000000002'; +const BANDIT_HOOD = '50000000-0000-4000-8000-000000000001'; interface State { characters: Character[]; @@ -45,8 +46,25 @@ class FakeRepository { return Promise.resolve(this.rows.find((row) => this.matches(row, where)) ?? null); } - find(options: { where: Partial }): Promise { - return Promise.resolve(this.rows.filter((row) => this.matches(row, options.where))); + find(options: { + where: Partial; + order?: Partial>; + }): Promise { + const matched = this.rows.filter((row) => this.matches(row, options.where)); + if (!options.order) { + return Promise.resolve(matched); + } + + // Mirrors TypeORM's `order` clause so tests can prove ordering comes from + // the query, not from insertion order happening to line up. + const [key, direction] = Object.entries(options.order)[0] as [keyof T, 'ASC' | 'DESC']; + const sorted = [...matched].sort((a, b) => { + const left = a[key]; + const right = b[key]; + const comparison = left < right ? -1 : left > right ? 1 : 0; + return direction === 'DESC' ? -comparison : comparison; + }); + return Promise.resolve(sorted); } create(values: Partial): T { @@ -138,6 +156,14 @@ function createState(overrides: Partial = {}): State { }; } +function fakeDataSource(state: State): EntityManager { + // `loadRewards`'s no-manager branch only calls `getRepository`, which + // `fakeManager` already implements identically for `DataSource`; reusing + // it (rather than duplicating the repository-resolution switch) keeps this + // fake backed by the exact same `state` rows as the transactional path. + return fakeManager(state); +} + function fakeLoot(...items: Array<{ itemDefinitionId: string; quantity: number }>): LootService { return { rollLoot: jest.fn().mockResolvedValue({ items }) } as unknown as LootService; } @@ -337,6 +363,22 @@ describe('CombatRewardService', () => { expect(replayed).toEqual(granted); expect(loot.rollLoot).toHaveBeenCalledTimes(1); }); + + it('reads a persisted reward through the injected DataSource when no manager is passed', async () => { + const state = createState(); + const loot = fakeLoot({ itemDefinitionId: BANDIT_BLADE, quantity: 1 }); + const dataSource = fakeDataSource(state); + const subject = new CombatRewardService(dataSource as never, loot, fixedRandom(0)); + const manager = fakeManager(state); + + const granted = await subject.grantVictoryRewards(manager, combat()); + + // No manager argument: this is the non-transactional read the next + // task uses to render a reward screen. + const replayed = await subject.loadRewards(COMBAT_ID); + + expect(replayed).toEqual(granted); + }); }); describe('failure handling', () => { @@ -350,6 +392,54 @@ describe('CombatRewardService', () => { ), ).rejects.toMatchObject({ code: 'REWARD_STATE_INVALID' }); expect(state.combatRewardItems).toHaveLength(0); + // Every rolled item definition is resolved before any mutation, so a + // missing one must leave no reward row and no character grant behind. + expect(state.combatRewards).toHaveLength(0); + expect(state.characters[0].experience).toBe(12); + expect(state.characters[0].silver).toBe(3); + }); + }); + + describe('item order', () => { + it('orders items by itemDefinitionId in the immediate grant and the replayed reward, regardless of roll order', async () => { + const state = createState({ + itemDefinitions: [ + { + id: BANDIT_BLADE, + key: 'bandit-blade', + name: 'Räuberklinge', + type: ItemType.WEAPON, + rarity: ItemRarity.COMMON, + iconPath: '/images/items/bandit-blade.png', + } as ItemDefinition, + { + id: BANDIT_HOOD, + key: 'bandit-hood', + name: 'Räuberkapuze', + type: ItemType.ARMOR, + rarity: ItemRarity.COMMON, + iconPath: '/images/items/bandit-hood.png', + } as ItemDefinition, + ], + }); + const banditCombat = combat({ monsterDefinitionId: ROAD_BANDIT_ID }); + // Roll order is blade-then-hood, but BANDIT_HOOD's id sorts before + // BANDIT_BLADE's, so this only passes if both response paths sort by + // itemDefinitionId rather than returning rows in roll/insertion order. + const loot = fakeLoot( + { itemDefinitionId: BANDIT_BLADE, quantity: 1 }, + { itemDefinitionId: BANDIT_HOOD, quantity: 1 }, + ); + const subject = service(state, loot, fixedRandom(0)); + const manager = fakeManager(state); + + const granted = await subject.grantVictoryRewards(manager, banditCombat); + const replayed = await subject.loadRewards(banditCombat.id, manager); + + const expectedKeys = ['bandit-hood', 'bandit-blade']; + expect(granted.items.map((item) => item.item.key)).toEqual(expectedKeys); + expect(replayed?.items.map((item) => item.item.key)).toEqual(expectedKeys); + expect(replayed).toEqual(granted); }); }); }); diff --git a/apps/api/src/rewards/combat-reward.service.ts b/apps/api/src/rewards/combat-reward.service.ts index cc9ef25..5cd8147 100644 --- a/apps/api/src/rewards/combat-reward.service.ts +++ b/apps/api/src/rewards/combat-reward.service.ts @@ -84,6 +84,25 @@ export class CombatRewardService { ); const roll = await this.lootService.rollLoot(monster.lootTableId, manager); + // Resolve every rolled item definition up front, before any mutation, so + // a missing definition throws `rewardStateInvalid()` before the + // character's XP/silver are touched or a `CombatReward` row is created. + // This keeps a failed grant from leaving partial writes behind. + const definitions = manager.getRepository(ItemDefinition); + const resolvedDefinitions = new Map(); + for (const rolled of roll.items) { + if (resolvedDefinitions.has(rolled.itemDefinitionId)) { + continue; + } + const definition = await definitions.findOneBy({ + id: rolled.itemDefinitionId, + }); + if (!definition) { + throw rewardStateInvalid(); + } + resolvedDefinitions.set(rolled.itemDefinitionId, definition); + } + const characters = manager.getRepository(Character); const character = await characters.findOne({ where: { id: combat.characterId }, @@ -105,18 +124,12 @@ export class CombatRewardService { }), ); - const definitions = manager.getRepository(ItemDefinition); const characterItems = manager.getRepository(CharacterItem); const rewardItems = manager.getRepository(CombatRewardItem); - const items: CombatRewardItemDto[] = []; + const granted: Array<{ itemDefinitionId: string; dto: CombatRewardItemDto }> = []; for (const rolled of roll.items) { - const definition = await definitions.findOneBy({ - id: rolled.itemDefinitionId, - }); - if (!definition) { - throw rewardStateInvalid(); - } + const definition = resolvedDefinitions.get(rolled.itemDefinitionId)!; const existingStack = await characterItems.findOne({ where: { @@ -146,9 +159,18 @@ export class CombatRewardService { }), ); - items.push(this.toItemDto(characterItem.id, definition, rolled.quantity)); + granted.push({ + itemDefinitionId: rolled.itemDefinitionId, + dto: this.toItemDto(characterItem.id, definition, rolled.quantity), + }); } + // The immediate response and a later `loadRewards` replay must agree on + // item order; both sort on the same stable key (itemDefinitionId, which + // `toDto`'s query also orders by) rather than roll order. + granted.sort((a, b) => a.itemDefinitionId.localeCompare(b.itemDefinitionId)); + const items = granted.map((entry) => entry.dto); + return { experience, silver, items }; } @@ -169,9 +191,15 @@ export class CombatRewardService { scope: RepositoryScope, reward: CombatReward, ): Promise { + // Ordered by itemDefinitionId to agree with the sort `grantVictoryRewards` + // applies to its own response — the immediate grant and a later replay + // must list items identically. const rewardItems = await scope .getRepository(CombatRewardItem) - .find({ where: { combatRewardId: reward.id } }); + .find({ + where: { combatRewardId: reward.id }, + order: { itemDefinitionId: 'ASC' }, + }); const definitions = scope.getRepository(ItemDefinition); const items: CombatRewardItemDto[] = [];