diff --git a/server/scripts/tx-writes-baseline.json b/server/scripts/tx-writes-baseline.json index dcb76f5f1..91f80552f 100644 --- a/server/scripts/tx-writes-baseline.json +++ b/server/scripts/tx-writes-baseline.json @@ -28,7 +28,6 @@ "src/nest/integrations/airtrail-link.service.ts#AirtrailLinkService.pushReservationToAirtrail": 2, "src/nest/integrations/airtrail-sync.service.ts#AirtrailSyncService.runAirtrailSync": 2, "src/nest/integrations/airtrail-sync.service.ts#AirtrailSyncService.syncOwner": 8, - "src/nest/integrations/airtrail.service.ts#AirtrailService.saveSettings": 2, "src/nest/integrations/dawarich-sync.service.ts#DawarichSyncService.runSync": 2, "src/nest/integrations/dawarich-sync.service.ts#DawarichSyncService.syncTripWindow": 3, "src/nest/integrations/dawarich-sync.service.ts#DawarichSyncService.syncUserOnce": 3, diff --git a/server/src/nest/integrations/airtrail.service.ts b/server/src/nest/integrations/airtrail.service.ts index adb97c8b5..a8328be79 100644 --- a/server/src/nest/integrations/airtrail.service.ts +++ b/server/src/nest/integrations/airtrail.service.ts @@ -2,6 +2,7 @@ import { UserAirtrailRepository } from '../../db/repositories/UserAirtrail.repos import { checkSsrf } from '../../utils/ssrfGuard'; import { AuditService } from '../audit/audit.service'; import { maybe_encrypt_api_key, decrypt_api_key } from '../common/crypto/apiKeyCrypto'; +import { UnitOfWork } from '../database/unit-of-work'; import { AirtrailAuthError, AirtrailRequestError, type AirtrailCreds } from './airtrail.client'; import { AirtrailClient } from './airtrail.client'; import { normalizeFlight } from './airtrail.mapper'; @@ -34,10 +35,9 @@ interface UserConnRow { * `UserAirtrailRepository`'s (same `users` table) `getAirtrailConnRow`/`getAirtrailWriteEnabled`/ * `setAirtrailSettingsWithKey`/`setAirtrailSettings`/`clearAirtrailApiKey` * methods (R7 — the repository sees only opaque, already-encrypted TEXT; - * `maybe_encrypt_api_key`/`decrypt_api_key` stay here). `saveSettings` keeps - * its pre-existing asymmetry with `DawarichService#saveSettings` - * (Task 3): Dawarich wraps its writes in `uow.transactional`, this one does - * not — preserved exactly, not "fixed" to match. + * `maybe_encrypt_api_key`/`decrypt_api_key` stay here). `saveSettings` writes + * in one `uow.transactional`, as `DawarichService#saveSettings` does: the new + * address and the key it may no longer carry change together or not at all. */ @Injectable() export class AirtrailService { @@ -45,6 +45,7 @@ export class AirtrailService { private readonly airtrailRepo: UserAirtrailRepository, private readonly audit: AuditService, private readonly client: AirtrailClient, + private readonly uow: UnitOfWork, ) {} private async readRow(userId: number): Promise { @@ -122,34 +123,32 @@ export class AirtrailService { // A key belongs to the instance it was issued on, as in testConnection. A // new host with the key field left masked or blank would otherwise send the // old instance's key to the new one on the next sync, so the key goes with - // the host. A path edit on the same origin keeps it. The key is cleared - // before the new address is written, so no moment stores the new address - // beside the old key. - if (newKey === undefined) { - const previousUrl = (await this.readRow(userId))?.airtrail_url ?? null; - if (movedHost(previousUrl, trimmedUrl)) await this.airtrailRepo.clearAirtrailApiKey(userId); - } + // the host. A path edit on the same origin keeps it. An address cleared with + // no key left drops the key too: it would be a credential for no server. + const previousUrl = newKey === undefined ? ((await this.readRow(userId))?.airtrail_url ?? null) : null; + const dropKey = newKey === undefined && (!trimmedUrl || movedHost(previousUrl, trimmedUrl)); - if (newKey !== undefined) { - await this.airtrailRepo.setAirtrailSettingsWithKey( - userId, - trimmedUrl || null, - newKey, - allowInsecureTls ? 1 : 0, - writeEnabled ? 1 : 0, - ); - } else { + // The address and the key it may no longer carry change together, so no + // moment stores the new address beside the old key. + await this.uow.transactional(async () => { + if (newKey !== undefined) { + await this.airtrailRepo.setAirtrailSettingsWithKey( + userId, + trimmedUrl || null, + newKey, + allowInsecureTls ? 1 : 0, + writeEnabled ? 1 : 0, + ); + return; + } await this.airtrailRepo.setAirtrailSettings( userId, trimmedUrl || null, allowInsecureTls ? 1 : 0, writeEnabled ? 1 : 0, ); - // Clearing the URL with no key left makes the connection meaningless — drop the key too. - if (!trimmedUrl) { - await this.airtrailRepo.clearAirtrailApiKey(userId); - } - } + if (dropKey) await this.airtrailRepo.clearAirtrailApiKey(userId); + }); return { success: true, warning }; } diff --git a/server/tests/unit/nest/airtrail.service.test.ts b/server/tests/unit/nest/airtrail.service.test.ts index f6aee5a47..b56a43d99 100644 --- a/server/tests/unit/nest/airtrail.service.test.ts +++ b/server/tests/unit/nest/airtrail.service.test.ts @@ -17,6 +17,7 @@ import { Users } from '../../../src/db/entities/Users.entity'; import { UserAirtrailRepository } from '../../../src/db/repositories/UserAirtrail.repository'; import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; import { AuditService } from '../../../src/nest/audit/audit.service'; +import { UnitOfWork } from '../../../src/nest/database/unit-of-work'; import type { AirtrailClient } from '../../../src/nest/integrations/airtrail.client'; import { AirtrailService } from '../../../src/nest/integrations/airtrail.service'; import { createSnapshotTestDb } from '../../helpers/db-mock'; @@ -42,9 +43,14 @@ const listFlights = vi.fn(); beforeAll(async () => { t = await createTestOrm(testDb); usersRepo = t.repo(Users); - svc = new AirtrailService(new UserAirtrailRepository(t.orm.em), new AuditService(t.repo(AuditLog), usersRepo), { - listFlights, - } as unknown as AirtrailClient); + svc = new AirtrailService( + new UserAirtrailRepository(t.orm.em), + new AuditService(t.repo(AuditLog), usersRepo), + { + listFlights, + } as unknown as AirtrailClient, + new UnitOfWork(t.orm.em), + ); }); beforeEach(() => { resetTestDb(testDb); diff --git a/server/tests/unit/nest/integrations/airtrailTestConnection.test.ts b/server/tests/unit/nest/integrations/airtrailTestConnection.test.ts index f250ba71f..a6af90477 100644 --- a/server/tests/unit/nest/integrations/airtrailTestConnection.test.ts +++ b/server/tests/unit/nest/integrations/airtrailTestConnection.test.ts @@ -3,6 +3,7 @@ import { AuditLog } from '../../../../src/db/entities/AuditLog.entity'; import { Users } from '../../../../src/db/entities/Users.entity'; import { UserAirtrailRepository } from '../../../../src/db/repositories/UserAirtrail.repository'; import { AuditService } from '../../../../src/nest/audit/audit.service'; +import { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; import type { AirtrailClient } from '../../../../src/nest/integrations/airtrail.client'; import { AirtrailService } from '../../../../src/nest/integrations/airtrail.service'; import { createUser } from '../../../helpers/factories'; @@ -24,9 +25,14 @@ let t: TestOrm; beforeAll(async () => { t = await createTestOrm(db); - svc = new AirtrailService(new UserAirtrailRepository(t.orm.em), new AuditService(t.repo(AuditLog), t.repo(Users)), { - listFlights, - } as unknown as AirtrailClient); + svc = new AirtrailService( + new UserAirtrailRepository(t.orm.em), + new AuditService(t.repo(AuditLog), t.repo(Users)), + { + listFlights, + } as unknown as AirtrailClient, + new UnitOfWork(t.orm.em), + ); }); afterAll(async () => { diff --git a/server/tests/unit/nest/integrations/airtrailWriteback.test.ts b/server/tests/unit/nest/integrations/airtrailWriteback.test.ts index 18c69d21f..9c8f3b24b 100644 --- a/server/tests/unit/nest/integrations/airtrailWriteback.test.ts +++ b/server/tests/unit/nest/integrations/airtrailWriteback.test.ts @@ -10,6 +10,7 @@ import { AuditLog } from '../../../../src/db/entities/AuditLog.entity'; import { Users } from '../../../../src/db/entities/Users.entity'; import { UserAirtrailRepository } from '../../../../src/db/repositories/UserAirtrail.repository'; import { AuditService } from '../../../../src/nest/audit/audit.service'; +import { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; import { AirtrailClient } from '../../../../src/nest/integrations/airtrail.client'; import { AirtrailService } from '../../../../src/nest/integrations/airtrail.service'; import { createUser } from '../../../helpers/factories'; @@ -39,6 +40,7 @@ beforeAll(async () => { new UserAirtrailRepository(t.orm.em), new AuditService(t.repo(AuditLog), t.repo(Users)), new AirtrailClient(), + new UnitOfWork(t.orm.em), ); getConnectionSettings = (...args) => svc.getConnectionSettings(...args); isAirtrailWriteEnabled = (...args) => svc.isAirtrailWriteEnabled(...args); @@ -72,45 +74,32 @@ describe('airtrail writeback opt-in persistence (#1240)', () => { }); }); -describe('airtrail saveSettings preserves its pre-existing NO-transaction asymmetry with Dawarich (Plan 3h Task 4, R7/Risks)', () => { - it('ATC-TX-001: AirtrailService is constructed with no UnitOfWork at all — saveSettings has nothing to wrap its writes in', () => { - // `DawarichService#saveSettings`'s equivalent DOES take a `UnitOfWork` and - // wraps its writes in `uow.transactional`; AirtrailService's own - // constructor (`UserAirtrailRepository`, `AuditService`, `AirtrailClient`) never - // gained one during the Plan 3h Task 4 conversion — a structural - // guarantee that a future edit re-introducing a transactional wrap here - // would have to ALSO add a new constructor param to do it, not just - // touch `saveSettings`'s body. +describe('airtrail saveSettings writes in one transaction, as Dawarich does', () => { + it('ATC-TX-001: AirtrailService is constructed with a UnitOfWork to wrap its writes in', () => { const paramTypes = Reflect.getMetadata('design:paramtypes', AirtrailService) as unknown[] | undefined; - expect(paramTypes).toBeDefined(); - expect(paramTypes!.map((p) => (p as { name?: string })?.name)).toEqual([ + expect(paramTypes?.map((p) => (p as { name?: string })?.name)).toEqual([ 'UserAirtrailRepository', 'AuditService', 'AirtrailClient', + 'UnitOfWork', ]); }); - it('ATC-TX-002: the URL-cleared branch performs its two writes as genuinely separate repository calls, not one atomic statement', async () => { + it('ATC-TX-002: clearing the URL drops the key inside the same transaction as the address write', async () => { const { user } = createUser(db); await saveSettings(user.id, 'https://at.example.com', 'secret-key', false, false, null); - const airtrailRepo = (svc as unknown as { airtrailRepo: UserAirtrailRepository }).airtrailRepo; - const setSpy = vi.spyOn(airtrailRepo, 'setAirtrailSettings'); - const clearSpy = vi.spyOn(airtrailRepo, 'clearAirtrailApiKey'); + const inner = svc as unknown as { airtrailRepo: UserAirtrailRepository; uow: UnitOfWork }; + const txSpy = vi.spyOn(inner.uow, 'transactional'); + const setSpy = vi.spyOn(inner.airtrailRepo, 'setAirtrailSettings'); + const clearSpy = vi.spyOn(inner.airtrailRepo, 'clearAirtrailApiKey'); try { - // Clearing the URL with no key left drops the key too (ATC4 then ATC5) — - // TWO independent nativeUpdate calls, not a single transactional unit. await saveSettings(user.id, '', undefined, false, false, null); + expect(txSpy).toHaveBeenCalledTimes(1); expect(setSpy).toHaveBeenCalledTimes(1); expect(clearSpy).toHaveBeenCalledTimes(1); - // clearAirtrailApiKey only runs AFTER setAirtrailSettings's own write - // resolved (sequential awaits in the service body, never Promise.all - // or a single wrapped call) — the same "report, don't fix" asymmetry - // the plan's Risks section flags. - const setOrder = setSpy.mock.invocationCallOrder[0]; - const clearOrder = clearSpy.mock.invocationCallOrder[0]; - expect(clearOrder).toBeGreaterThan(setOrder); } finally { + txSpy.mockRestore(); setSpy.mockRestore(); clearSpy.mockRestore(); }