From 26fe9f64972d1607eb911f0cf4cefb272682c4f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Thu, 30 Jul 2026 18:38:45 +0200 Subject: [PATCH 1/2] Apply expected consumption for all limits in one write An evaluation that touches two usage limits recorded one of them. _applyExpectedConsumption reads the whole contract, increments a single usage level in its own copy, and writes the whole contract back. evaluateFeature called it once per limit through Promise.all, so every call read the same starting state and only the last write survived - a lost update, silent, on the path that decides whether somebody may use a feature. _applyExpectedConsumptions applies every limit to one contract copy and writes once: correct, and one round trip instead of one per limit. Limits are all validated before anything is written, so naming a limit that does not exist cannot leave the others half-applied. The single-limit method stays, delegating. Seven tests against stubbed repository and cache, including one that pins why the batch method exists: calling the single-limit method concurrently is still lossy, because read-modify-write on a whole document cannot be made safe by calling it more carefully. Making that safe means an atomic $inc in the repository, which is a larger change than this one. Their feature-evaluation suite (26 tests) passes unchanged. --- api/src/main/services/ContractService.ts | 57 ++++-- .../feature-evaluation/featureEvaluation.ts | 23 +-- .../contract.expected-consumption.test.ts | 171 ++++++++++++++++++ 3 files changed, 227 insertions(+), 24 deletions(-) create mode 100644 api/src/test/contract.expected-consumption.test.ts diff --git a/api/src/main/services/ContractService.ts b/api/src/main/services/ContractService.ts index bb0b6cb..09cb5e0 100644 --- a/api/src/main/services/ContractService.ts +++ b/api/src/main/services/ContractService.ts @@ -459,6 +459,27 @@ class ContractService { usageLimitId: string, expectedConsumption: number ): Promise { + await this._applyExpectedConsumptions(userId, { [usageLimitId]: expectedConsumption }); + } + + /** + * Apply several expected consumptions to a contract in one read and one write. + * + * Applying them one at a time loses all but one. Each application reads the + * whole contract, increments a single usage level in its own copy, and writes + * the whole contract back - so N concurrent applications all start from the + * same state and the last write wins. An evaluation touching two limits + * recorded one of them. + */ + async _applyExpectedConsumptions( + userId: string, + expectedConsumptions: Record + ): Promise { + const usageLimitIds = Object.keys(expectedConsumptions); + if (usageLimitIds.length === 0) { + return; + } + let contract = await this.cacheService.get(`contracts.${userId}`); if (!contract) { @@ -469,28 +490,38 @@ class ContractService { throw new Error(`Contract with userId ${userId} not found`); } - const serviceName: string = usageLimitId.split('-')[0]; - const usageLimit: string = usageLimitId.split('-')[1]; + // Every limit is validated before anything is written, so a request naming + // one limit that does not exist cannot leave the others half-applied. + const targets = usageLimitIds.map(usageLimitId => { + const serviceName: string = usageLimitId.split('-')[0]; + const usageLimit: string = usageLimitId.split('-')[1]; + + if (!contract.usageLevels[serviceName]?.[usageLimit]) { + throw new Error(`Usage level ${usageLimit} not found in contract for userId ${userId}`); + } + + return { serviceName, usageLimit, amount: expectedConsumptions[usageLimitId] }; + }); - if (contract.usageLevels[serviceName][usageLimit]) { + const appliedAt = new Date().getTime(); + + for (const { serviceName, usageLimit, amount } of targets) { await this.cacheService.set( - `${new Date().getTime()}.usageLevels.${userId}.${serviceName}.${usageLimit}`, + `${appliedAt}.usageLevels.${userId}.${serviceName}.${usageLimit}`, contract.usageLevels[serviceName][usageLimit].consumed, 120 ); // 120 secs = 2 mins - contract.usageLevels[serviceName][usageLimit].consumed += expectedConsumption; - - const updatedContract = await this.contractRepository.update(userId, contract); + contract.usageLevels[serviceName][usageLimit].consumed += amount; + } - if (!updatedContract) { - throw new Error(`Failed to update contract for userId ${userId}`); - } + const updatedContract = await this.contractRepository.update(userId, contract); - await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour - } else { - throw new Error(`Usage level ${usageLimit} not found in contract for userId ${userId}`); + if (!updatedContract) { + throw new Error(`Failed to update contract for userId ${userId}`); } + + await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour } async _revertExpectedConsumption( diff --git a/api/src/main/utils/feature-evaluation/featureEvaluation.ts b/api/src/main/utils/feature-evaluation/featureEvaluation.ts index fd0fd9e..4dd6b36 100644 --- a/api/src/main/utils/feature-evaluation/featureEvaluation.ts +++ b/api/src/main/utils/feature-evaluation/featureEvaluation.ts @@ -61,20 +61,21 @@ async function evaluateFeature( } } - // Then apply all consumptions after validation has passed + // Then apply all consumptions after validation has passed. + // + // In one call rather than one per limit: each application reads the whole + // contract, increments one usage level and writes the whole contract + // back, so running them concurrently made every one of them start from + // the same state and only the last write survive. if (options.userId) { const contractService: ContractService = container.resolve('contractService'); - const limits = Object.keys(featureEvaluation.used); - await Promise.all( - limits.map(limit => - contractService._applyExpectedConsumption( - options.userId!, - limit, - expectedConsumption[limit] - ) - ) - ); + const consumptions: Record = {}; + for (const limit of Object.keys(featureEvaluation.used)) { + consumptions[limit] = expectedConsumption[limit]; + } + + await contractService._applyExpectedConsumptions(options.userId, consumptions); } } } diff --git a/api/src/test/contract.expected-consumption.test.ts b/api/src/test/contract.expected-consumption.test.ts new file mode 100644 index 0000000..7349d8d --- /dev/null +++ b/api/src/test/contract.expected-consumption.test.ts @@ -0,0 +1,171 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import container from '../main/config/container'; + +/** + * Applying expected consumption to more than one usage limit. + * + * Each application reads the whole contract, increments one usage level in its + * own copy, and writes the whole contract back. Doing that once per limit - + * concurrently, as `evaluateFeature` did - means every application starts from + * the same state and only the last write survives, so an evaluation touching + * two limits recorded one of them. + * + * These tests work against stubbed repository and cache so they can assert on + * the reads and writes themselves, which is where the defect lives. + */ + +function aContract() { + return { + userContact: { userId: 'user1', username: 'user1' }, + contractedServices: { petclinic: '2025' }, + subscriptionPlans: { petclinic: 'BASIC' }, + usageLevels: { + petclinic: { + maxPets: { consumed: 0 }, + maxVisits: { consumed: 0 }, + }, + }, + }; +} + +function withStubs(contract: any) { + // The repository hands back a *copy* on read and keeps whatever it is given, + // exactly as a database does - which is what makes a lost update visible. + const state = { current: contract }; + let writes = 0; + let reads = 0; + + const contractRepository = { + findByUserId: vi.fn(async () => { + reads += 1; + return JSON.parse(JSON.stringify(state.current)); + }), + update: vi.fn(async (_userId: string, updated: any) => { + writes += 1; + state.current = JSON.parse(JSON.stringify(updated)); + return state.current; + }), + }; + + const cacheService = { + get: vi.fn(async () => null), + set: vi.fn(async () => undefined), + del: vi.fn(async () => undefined), + }; + + const original = container.resolve.bind(container); + vi.spyOn(container, 'resolve').mockImplementation((name: any) => { + if (name === 'contractRepository') return contractRepository as any; + if (name === 'cacheService') return cacheService as any; + return original(name); + }); + + return { state, contractRepository, cacheService, counts: () => ({ reads, writes }) }; +} + +async function aService() { + const { default: ContractService } = await import('../main/services/ContractService'); + return new (ContractService as any)(); +} + +describe('Applying expected consumption to several limits', () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + it('records every limit, not just the last one written', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 3, + }); + + expect(stubs.state.current.usageLevels.petclinic.maxPets.consumed).toBe(1); + expect(stubs.state.current.usageLevels.petclinic.maxVisits.consumed).toBe(3); + }); + + it('reads and writes the contract once however many limits there are', async () => { + // Not only correctness: one round trip instead of one per limit. + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 1, + }); + + expect(stubs.counts()).toEqual({ reads: 1, writes: 1 }); + }); + + it('pins why the batch method exists: one call per limit still races', async () => { + // This is what `evaluateFeature` used to do - one call per limit, in + // parallel - and it is still lossy, because read-modify-write on a whole + // document cannot be made safe by calling it more carefully. + // + // Asserted rather than fixed here so nobody simplifies the call site back + // to Promise.all: the read-modify-write itself would have to become an + // atomic $inc for that to be safe, which is a larger change than this one. + const stubs = withStubs(aContract()); + const service = await aService(); + + await Promise.all([ + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1), + service._applyExpectedConsumption('user1', 'petclinic-maxVisits', 1), + ]); + + const levels = stubs.state.current.usageLevels.petclinic; + const recorded = levels.maxPets.consumed + levels.maxVisits.consumed; + expect(recorded, 'one increment is lost, which is the point').toBe(1); + }); + + it('still applies a single limit', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumption('user1', 'petclinic-maxPets', 2); + + expect(stubs.state.current.usageLevels.petclinic.maxPets.consumed).toBe(2); + }); + + it('does nothing at all when given nothing', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', {}); + + expect(stubs.counts()).toEqual({ reads: 0, writes: 0 }); + }); + + it('refuses the whole request when one limit does not exist', async () => { + // Rather than applying the valid ones and then throwing, which would leave + // the contract half-updated. + const stubs = withStubs(aContract()); + const service = await aService(); + + await expect( + service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-nosuchlimit': 1, + }) + ).rejects.toThrow(/not found in contract/); + + expect(stubs.state.current.usageLevels.petclinic.maxPets.consumed).toBe(0); + expect(stubs.counts().writes).toBe(0); + }); + + it('keeps the previous value of every limit for reverting', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 1, + }); + + const cachedKeys = stubs.cacheService.set.mock.calls.map((call: any[]) => call[0]); + expect(cachedKeys.some((key: string) => key.includes('maxPets'))).toBe(true); + expect(cachedKeys.some((key: string) => key.includes('maxVisits'))).toBe(true); + }); +}); From 70931b42e5299ac6d4dc8b2a71b2f08c7a575aff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Fri, 31 Jul 2026 11:23:26 +0200 Subject: [PATCH 2/2] Let the database settle the increment, with $inc Batching a whole evaluation into one read and one write stopped an evaluation from losing its own limits, but not two requests from losing each other's: both read the same consumed value, both write the same total, and one consumption disappears. The increment is now handed to Mongo as $inc and evaluated against the stored document, so concurrent calls compose. The filter requires every usage level to exist, which keeps validation and the write in one operation instead of leaving a window between them. --- .../mongoose/ContractRepository.ts | 40 ++++ api/src/main/services/ContractService.ts | 82 ++++++--- .../contract.expected-consumption.test.ts | 173 +++++++++++++----- 3 files changed, 226 insertions(+), 69 deletions(-) diff --git a/api/src/main/repositories/mongoose/ContractRepository.ts b/api/src/main/repositories/mongoose/ContractRepository.ts index ac5ad13..02e89e5 100644 --- a/api/src/main/repositories/mongoose/ContractRepository.ts +++ b/api/src/main/repositories/mongoose/ContractRepository.ts @@ -185,6 +185,46 @@ class ContractRepository extends RepositoryBase { return contract ? toPlainObject(contract.toJSON()) : null; } + /** + * Add to several usage levels of one contract in a single atomic update. + * + * `$inc` is evaluated by the database against the stored document rather than + * against a copy the process read earlier, so concurrent increments compose + * instead of overwriting one another. Read-modify-write cannot do this from + * the application: two callers who read the same value both write the same + * total, and one consumption disappears. + * + * Every path is required to exist by the filter, so a limit that is not part + * of the contract matches no document and is reported to the caller instead + * of being created by the update - `$inc` would otherwise happily add the + * field. That check and the increment are one operation, so a limit cannot be + * validated and then vanish before the write. + * + * @param increments usage level path (`service.limit`) to amount to add. + * @returns the contract as it is after the increment, or null when the filter + * matched nothing. + */ + async incrementUsageLevels( + userId: string, + increments: Record + ): Promise { + const filter: Record = { 'userContact.userId': userId }; + const inc: Record = {}; + + for (const [path, amount] of Object.entries(increments)) { + filter[`usageLevels.${path}.consumed`] = { $exists: true }; + inc[`usageLevels.${path}.consumed`] = amount; + } + + const contract = await ContractMongoose.findOneAndUpdate( + filter, + { $inc: inc }, + { new: true } + ); + + return contract ? toPlainObject(contract.toJSON()) : null; + } + async changeServiceName(oldServiceName: string, newServiceName: string, organizationId: string): Promise { const oldServiceKey = oldServiceName.toLowerCase(); const newServiceKey = newServiceName.toLowerCase(); diff --git a/api/src/main/services/ContractService.ts b/api/src/main/services/ContractService.ts index 09cb5e0..0df8ee4 100644 --- a/api/src/main/services/ContractService.ts +++ b/api/src/main/services/ContractService.ts @@ -463,13 +463,19 @@ class ContractService { } /** - * Apply several expected consumptions to a contract in one read and one write. + * Apply several expected consumptions to a contract in one atomic update. * - * Applying them one at a time loses all but one. Each application reads the - * whole contract, increments a single usage level in its own copy, and writes - * the whole contract back - so N concurrent applications all start from the - * same state and the last write wins. An evaluation touching two limits - * recorded one of them. + * Read-modify-write loses consumptions, and it does so in two ways. Applying + * the limits of one evaluation in turn made each application read the whole + * contract, change a single usage level in its own copy and write the whole + * contract back, so an evaluation touching two limits recorded one of them. + * Batching the limits into a single read and write fixes that, but not the + * case of two requests arriving together: both read the same consumed value, + * both write the same total, and one consumption is gone. + * + * The database is the only place that can settle this, so the increment is + * handed to it as `$inc` and evaluated against the stored document. Whatever + * order concurrent calls arrive in, every one of them is added. */ async _applyExpectedConsumptions( userId: string, @@ -480,48 +486,68 @@ class ContractService { return; } - let contract = await this.cacheService.get(`contracts.${userId}`); - - if (!contract) { - contract = await this.contractRepository.findByUserId(userId); - } - - if (!contract) { - throw new Error(`Contract with userId ${userId} not found`); - } - - // Every limit is validated before anything is written, so a request naming - // one limit that does not exist cannot leave the others half-applied. const targets = usageLimitIds.map(usageLimitId => { const serviceName: string = usageLimitId.split('-')[0]; const usageLimit: string = usageLimitId.split('-')[1]; - if (!contract.usageLevels[serviceName]?.[usageLimit]) { - throw new Error(`Usage level ${usageLimit} not found in contract for userId ${userId}`); - } - return { serviceName, usageLimit, amount: expectedConsumptions[usageLimitId] }; }); + const increments: Record = {}; + for (const { serviceName, usageLimit, amount } of targets) { + increments[`${serviceName}.${usageLimit}`] = amount; + } + + const updatedContract = await this.contractRepository.incrementUsageLevels(userId, increments); + + if (!updatedContract) { + // The update requires the contract and every named usage level to exist, + // so it matched nothing. Which of the two is missing only matters for the + // message, and is worth a read to get right. + await this._explainMissingUsageLevels(userId, targets); + } + const appliedAt = new Date().getTime(); for (const { serviceName, usageLimit, amount } of targets) { + // What the level held before this call, for `_revertExpectedConsumption`. + // Derived from the result rather than from a prior read, so it is this + // caller's own contribution that gets taken back even if others landed in + // between. await this.cacheService.set( `${appliedAt}.usageLevels.${userId}.${serviceName}.${usageLimit}`, - contract.usageLevels[serviceName][usageLimit].consumed, + updatedContract!.usageLevels[serviceName][usageLimit].consumed - amount, 120 ); // 120 secs = 2 mins + } + + await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour + } + + /** + * Say which part of the contract was missing, having established that one was. + */ + private async _explainMissingUsageLevels( + userId: string, + targets: { serviceName: string; usageLimit: string }[] + ): Promise { + const contract = await this.contractRepository.findByUserId(userId); - contract.usageLevels[serviceName][usageLimit].consumed += amount; + if (!contract) { + throw new Error(`Contract with userId ${userId} not found`); } - const updatedContract = await this.contractRepository.update(userId, contract); + const missing = targets.find( + ({ serviceName, usageLimit }) => !contract.usageLevels[serviceName]?.[usageLimit] + ); - if (!updatedContract) { - throw new Error(`Failed to update contract for userId ${userId}`); + if (missing) { + throw new Error( + `Usage level ${missing.usageLimit} not found in contract for userId ${userId}` + ); } - await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour + throw new Error(`Failed to update contract for userId ${userId}`); } async _revertExpectedConsumption( diff --git a/api/src/test/contract.expected-consumption.test.ts b/api/src/test/contract.expected-consumption.test.ts index 7349d8d..d903f54 100644 --- a/api/src/test/contract.expected-consumption.test.ts +++ b/api/src/test/contract.expected-consumption.test.ts @@ -2,16 +2,24 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import container from '../main/config/container'; /** - * Applying expected consumption to more than one usage limit. + * Applying expected consumption to usage limits. * - * Each application reads the whole contract, increments one usage level in its - * own copy, and writes the whole contract back. Doing that once per limit - - * concurrently, as `evaluateFeature` did - means every application starts from - * the same state and only the last write survives, so an evaluation touching - * two limits recorded one of them. + * Read-modify-write loses consumptions in two ways. Applying the limits of one + * evaluation in turn made each application read the whole contract, change one + * usage level in its own copy and write the whole contract back, so an + * evaluation touching two limits recorded one of them. Batching them into a + * single read and write fixed that, and left the other: two requests arriving + * together both read the same consumed value, both write the same total, and + * one consumption disappears. * - * These tests work against stubbed repository and cache so they can assert on - * the reads and writes themselves, which is where the defect lives. + * The increment is now handed to the database as `$inc`, which is the only + * place it can be settled. + * + * These tests work against a stubbed repository and cache so they can assert on + * the reads and writes themselves, which is where the defect lived. The stub + * models the database honestly: `incrementUsageLevels` adds to whatever is + * *stored* at the moment it runs, which is exactly the guarantee `$inc` gives + * and exactly the one an application-side read-modify-write cannot. */ function aContract() { @@ -28,22 +36,48 @@ function aContract() { }; } +const copy = (value: any) => JSON.parse(JSON.stringify(value)); + function withStubs(contract: any) { - // The repository hands back a *copy* on read and keeps whatever it is given, - // exactly as a database does - which is what makes a lost update visible. const state = { current: contract }; let writes = 0; let reads = 0; + let increments = 0; const contractRepository = { findByUserId: vi.fn(async () => { reads += 1; - return JSON.parse(JSON.stringify(state.current)); + return copy(state.current); }), + + // Kept so a regression to read-modify-write is visible rather than a crash. + // The await between reading and writing is what any real round trip has, + // and what lets a second caller slip in between the two. update: vi.fn(async (_userId: string, updated: any) => { writes += 1; - state.current = JSON.parse(JSON.stringify(updated)); - return state.current; + await Promise.resolve(); + state.current = copy(updated); + return copy(state.current); + }), + + incrementUsageLevels: vi.fn(async (_userId: string, byPath: Record) => { + increments += 1; + await Promise.resolve(); + + // The filter requires every path to exist; a miss matches no document. + for (const path of Object.keys(byPath)) { + const [serviceName, usageLimit] = path.split('.'); + if (!state.current.usageLevels[serviceName]?.[usageLimit]) { + return null; + } + } + + for (const [path, amount] of Object.entries(byPath)) { + const [serviceName, usageLimit] = path.split('.'); + state.current.usageLevels[serviceName][usageLimit].consumed += amount; + } + + return copy(state.current); }), }; @@ -60,7 +94,13 @@ function withStubs(contract: any) { return original(name); }); - return { state, contractRepository, cacheService, counts: () => ({ reads, writes }) }; + return { + state, + contractRepository, + cacheService, + counts: () => ({ reads, writes, increments }), + consumed: () => state.current.usageLevels.petclinic, + }; } async function aService() { @@ -68,7 +108,7 @@ async function aService() { return new (ContractService as any)(); } -describe('Applying expected consumption to several limits', () => { +describe('Applying expected consumption', () => { beforeEach(() => { vi.restoreAllMocks(); }); @@ -82,31 +122,39 @@ describe('Applying expected consumption to several limits', () => { 'petclinic-maxVisits': 3, }); - expect(stubs.state.current.usageLevels.petclinic.maxPets.consumed).toBe(1); - expect(stubs.state.current.usageLevels.petclinic.maxVisits.consumed).toBe(3); + expect(stubs.consumed().maxPets.consumed).toBe(1); + expect(stubs.consumed().maxVisits.consumed).toBe(3); }); - it('reads and writes the contract once however many limits there are', async () => { - // Not only correctness: one round trip instead of one per limit. + it('loses nothing when two requests arrive together', async () => { + // The case the batch alone could not fix, and the reason for `$inc`: two + // callers spending the same limit at the same time. Under read-modify-write + // both start from 0, both write 1, and one consumption is gone. const stubs = withStubs(aContract()); const service = await aService(); - await service._applyExpectedConsumptions('user1', { - 'petclinic-maxPets': 1, - 'petclinic-maxVisits': 1, - }); + await Promise.all([ + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1), + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1), + ]); + + expect(stubs.consumed().maxPets.consumed).toBe(2); + }); + + it('loses nothing across many concurrent requests', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); - expect(stubs.counts()).toEqual({ reads: 1, writes: 1 }); + await Promise.all( + Array.from({ length: 20 }, () => + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1) + ) + ); + + expect(stubs.consumed().maxPets.consumed).toBe(20); }); - it('pins why the batch method exists: one call per limit still races', async () => { - // This is what `evaluateFeature` used to do - one call per limit, in - // parallel - and it is still lossy, because read-modify-write on a whole - // document cannot be made safe by calling it more carefully. - // - // Asserted rather than fixed here so nobody simplifies the call site back - // to Promise.all: the read-modify-write itself would have to become an - // atomic $inc for that to be safe, which is a larger change than this one. + it('composes concurrent requests that touch different limits', async () => { const stubs = withStubs(aContract()); const service = await aService(); @@ -115,9 +163,22 @@ describe('Applying expected consumption to several limits', () => { service._applyExpectedConsumption('user1', 'petclinic-maxVisits', 1), ]); - const levels = stubs.state.current.usageLevels.petclinic; - const recorded = levels.maxPets.consumed + levels.maxVisits.consumed; - expect(recorded, 'one increment is lost, which is the point').toBe(1); + expect(stubs.consumed().maxPets.consumed).toBe(1); + expect(stubs.consumed().maxVisits.consumed).toBe(1); + }); + + it('touches the contract once however many limits there are', async () => { + // Not only correctness: one round trip instead of one per limit, and no + // read at all, since the database does the arithmetic. + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 1, + }); + + expect(stubs.counts()).toEqual({ reads: 0, writes: 0, increments: 1 }); }); it('still applies a single limit', async () => { @@ -126,7 +187,7 @@ describe('Applying expected consumption to several limits', () => { await service._applyExpectedConsumption('user1', 'petclinic-maxPets', 2); - expect(stubs.state.current.usageLevels.petclinic.maxPets.consumed).toBe(2); + expect(stubs.consumed().maxPets.consumed).toBe(2); }); it('does nothing at all when given nothing', async () => { @@ -135,12 +196,13 @@ describe('Applying expected consumption to several limits', () => { await service._applyExpectedConsumptions('user1', {}); - expect(stubs.counts()).toEqual({ reads: 0, writes: 0 }); + expect(stubs.counts()).toEqual({ reads: 0, writes: 0, increments: 0 }); }); it('refuses the whole request when one limit does not exist', async () => { - // Rather than applying the valid ones and then throwing, which would leave - // the contract half-updated. + // Rather than applying the valid ones and then failing, which would leave + // the contract half-updated. The check is part of the same operation, so a + // limit cannot be validated and then vanish before the write. const stubs = withStubs(aContract()); const service = await aService(); @@ -151,8 +213,18 @@ describe('Applying expected consumption to several limits', () => { }) ).rejects.toThrow(/not found in contract/); - expect(stubs.state.current.usageLevels.petclinic.maxPets.consumed).toBe(0); - expect(stubs.counts().writes).toBe(0); + expect(stubs.consumed().maxPets.consumed).toBe(0); + }); + + it('says so when there is no contract at all', async () => { + const stubs = withStubs(aContract()); + stubs.contractRepository.incrementUsageLevels.mockResolvedValue(null); + stubs.contractRepository.findByUserId.mockResolvedValue(null); + const service = await aService(); + + await expect( + service._applyExpectedConsumptions('user1', { 'petclinic-maxPets': 1 }) + ).rejects.toThrow(/Contract with userId user1 not found/); }); it('keeps the previous value of every limit for reverting', async () => { @@ -168,4 +240,23 @@ describe('Applying expected consumption to several limits', () => { expect(cachedKeys.some((key: string) => key.includes('maxPets'))).toBe(true); expect(cachedKeys.some((key: string) => key.includes('maxVisits'))).toBe(true); }); + + it('records this caller’s own starting point, not whatever it read', async () => { + // The snapshot kept for reverting is derived from the result of the + // increment, so it is this caller's contribution that gets taken back even + // when other calls landed in between. + const contract = aContract(); + contract.usageLevels.petclinic.maxPets.consumed = 7; + + const stubs = withStubs(contract); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { 'petclinic-maxPets': 3 }); + + const snapshot = (stubs.cacheService.set.mock.calls as any[][]).find(call => + String(call[0]).includes('maxPets') + ); + expect(snapshot?.[1]).toBe(7); + expect(stubs.consumed().maxPets.consumed).toBe(10); + }); });