fix(reputation): validate the character exists before granting reputation

This commit is contained in:
Bastian Wagner
2026-08-21 09:30:04 +02:00
parent 79d4e04172
commit 321738496d
2 changed files with 62 additions and 2 deletions

View File

@@ -1,4 +1,6 @@
import { HttpException } from '@nestjs/common';
import { DataSource, EntityManager, EntityTarget } from 'typeorm'; import { DataSource, EntityManager, EntityTarget } from 'typeorm';
import { Character } from '../characters/entities/character.entity';
import { CharacterReputation } from './entities/character-reputation.entity'; import { CharacterReputation } from './entities/character-reputation.entity';
import { ReputationFaction } from './entities/reputation-faction.entity'; import { ReputationFaction } from './entities/reputation-faction.entity';
import { ReputationDomainError } from './reputation.errors'; import { ReputationDomainError } from './reputation.errors';
@@ -8,6 +10,7 @@ const FACTION_ID = '80000000-0000-4000-8000-000000000001';
const CHARACTER_ID = '10000000-0000-4000-8000-000000000001'; const CHARACTER_ID = '10000000-0000-4000-8000-000000000001';
interface State { interface State {
characters: Character[];
factions: ReputationFaction[]; factions: ReputationFaction[];
characterReputation: CharacterReputation[]; characterReputation: CharacterReputation[];
} }
@@ -72,6 +75,8 @@ class FakeDataSource {
} }
private repoFor<T extends { id: string }>(target: EntityTarget<T>, inTransaction: boolean) { private repoFor<T extends { id: string }>(target: EntityTarget<T>, inTransaction: boolean) {
if (target === Character)
return new FakeRepository(this.state.characters, 'character', inTransaction) as never;
if (target === ReputationFaction) if (target === ReputationFaction)
return new FakeRepository(this.state.factions, 'faction', inTransaction) as never; return new FakeRepository(this.state.factions, 'faction', inTransaction) as never;
if (target === CharacterReputation) if (target === CharacterReputation)
@@ -80,6 +85,10 @@ class FakeDataSource {
} }
} }
function character(overrides: Partial<Character> = {}): Character {
return { id: CHARACTER_ID, ...overrides } as Character;
}
function faction(overrides: Partial<ReputationFaction> = {}): ReputationFaction { function faction(overrides: Partial<ReputationFaction> = {}): ReputationFaction {
return { return {
id: FACTION_ID, id: FACTION_ID,
@@ -93,7 +102,7 @@ function faction(overrides: Partial<ReputationFaction> = {}): ReputationFaction
} }
function createState(overrides: Partial<State> = {}): State { function createState(overrides: Partial<State> = {}): State {
return { factions: [faction()], characterReputation: [], ...overrides }; return { characters: [character()], factions: [faction()], characterReputation: [], ...overrides };
} }
function createService(state: State) { function createService(state: State) {
@@ -115,6 +124,31 @@ async function expectReputationDomainError(promise: Promise<unknown>, code: stri
expect(error.code).toBe(code); expect(error.code).toBe(code);
} }
/**
* `characterNotFound` is re-exported from `travel.errors` and returns a
* `TravelDomainError`, not a `ReputationDomainError` -- so this asserts on
* the thrown HttpException's response shape (its `code`/status) rather than
* its class, matching this codebase's convention for shared error helpers.
*/
async function expectHttpErrorWithCode(
promise: Promise<unknown>,
code: string,
status: number,
): Promise<void> {
let error: unknown;
try {
await promise;
} catch (cause) {
error = cause;
}
expect(error).toBeInstanceOf(HttpException);
if (!(error instanceof HttpException)) {
throw new Error('Expected HttpException');
}
expect(error.getStatus()).toBe(status);
expect(error.getResponse()).toMatchObject({ code });
}
describe('ReputationService', () => { describe('ReputationService', () => {
describe('grantReputation', () => { describe('grantReputation', () => {
it('creates a reputation row starting from 0 on the first grant', async () => { it('creates a reputation row starting from 0 on the first grant', async () => {
@@ -184,6 +218,16 @@ describe('ReputationService', () => {
'REPUTATION_FACTION_NOT_FOUND', 'REPUTATION_FACTION_NOT_FOUND',
); );
}); });
it('rejects an unknown character', async () => {
const { service } = createService(createState());
await expectHttpErrorWithCode(
service.grantReputation('unknown-character', 'border-guard', 10),
'CHARACTER_NOT_FOUND',
404,
);
});
}); });
describe('getCharacterReputation', () => { describe('getCharacterReputation', () => {

View File

@@ -1,8 +1,9 @@
import { Injectable } from '@nestjs/common'; import { Injectable } from '@nestjs/common';
import { DataSource, EntityManager } from 'typeorm'; import { DataSource, EntityManager } from 'typeorm';
import { Character } from '../characters/entities/character.entity';
import { CharacterReputation } from './entities/character-reputation.entity'; import { CharacterReputation } from './entities/character-reputation.entity';
import { ReputationFaction } from './entities/reputation-faction.entity'; import { ReputationFaction } from './entities/reputation-faction.entity';
import { reputationFactionNotFound } from './reputation.errors'; import { characterNotFound, reputationFactionNotFound } from './reputation.errors';
import { resolveReputationRank } from './reputation-rank'; import { resolveReputationRank } from './reputation-rank';
export interface ReputationGrantResult { export interface ReputationGrantResult {
@@ -37,9 +38,24 @@ export class ReputationService {
manager?: EntityManager, manager?: EntityManager,
): Promise<ReputationGrantResult> { ): Promise<ReputationGrantResult> {
const run = async (txManager: EntityManager): Promise<ReputationGrantResult> => { const run = async (txManager: EntityManager): Promise<ReputationGrantResult> => {
const characters = txManager.getRepository(Character);
const factions = txManager.getRepository(ReputationFaction); const factions = txManager.getRepository(ReputationFaction);
const reputations = txManager.getRepository(CharacterReputation); const reputations = txManager.getRepository(CharacterReputation);
// When TurnInService (Task 6) passes its own `manager`, it will already
// hold a pessimistic write lock on this same character row from earlier
// in that transaction. Re-locking it here is a no-op re-lock in
// Postgres, not a deadlock risk -- CombatService.performAction relies on
// the same behavior for grantVictoryRewards. Do not remove this check
// to "avoid" the re-lock.
const character = await characters.findOne({
where: { id: characterId },
lock: { mode: 'pessimistic_write' },
});
if (!character) {
throw characterNotFound();
}
const faction = await factions.findOneBy({ key: factionKey }); const faction = await factions.findOneBy({ key: factionKey });
if (!faction) { if (!faction) {
throw reputationFactionNotFound(); throw reputationFactionNotFound();