From cff13913f1f20fbd31df7d29ac23a17075ca8368 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Thu, 4 Jun 2026 17:47:44 +0200 Subject: [PATCH] fix(billing): replace \$sortArray with portable \$unwind+\$sort+\$group in listLedgerPage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit \$sortArray requires MongoDB ≥5.2 and fails on 5.0.x CI environments. Replaces it with \$unwind + \$sort + \$group which works on MongoDB ≥4.4. Adds a \$filter guard to prevent null sentinel in ledgerPage when ledger is empty. Closes #3774 --- .../billing.extraBalance.repository.js | 51 +++++-- ...lance.listLedger.perf.integration.tests.js | 17 +++ .../tests/billing.extraBalance.unit.tests.js | 133 ++++++++++++++++++ 3 files changed, 189 insertions(+), 12 deletions(-) diff --git a/modules/billing/repositories/billing.extraBalance.repository.js b/modules/billing/repositories/billing.extraBalance.repository.js index 77731c771..3035a1ef3 100644 --- a/modules/billing/repositories/billing.extraBalance.repository.js +++ b/modules/billing/repositories/billing.extraBalance.repository.js @@ -388,13 +388,21 @@ const getBalance = async (orgId) => { /** * @function listLedgerPage * @description Return a paginated slice of the ledger array for an organization using - * MongoDB aggregation `$slice` — only the requested page is transferred over the + * MongoDB aggregation — only the requested page is transferred over the * wire, avoiding full-document fetches for large ledgers (1000+ entries). * * Entries are sorted descending by `at` (newest first) at the aggregation layer. * The aggregation pipeline is: - * 1. $match — find the org's document - * 2. $project — sort + slice the ledger, plus cachedBalance and _id=0 + * 1. $match — find the org's document + * 2. $project — capture total + cachedBalance, normalise ledger with $ifNull + * 3. $unwind — explode ledger entries into individual documents + * 4. $sort — sort by ledger.at descending (compatible with MongoDB ≥4.4) + * 5. $group — reassemble into a single document, collecting sorted entries + * 6. $project — apply skip+limit slice and reshape to final shape + * + * Note: $sortArray (MongoDB ≥5.2) is intentionally avoided so the repository + * works on MongoDB 5.0.x (mongodb-memory-server default in CI). + * The $unwind + $sort + $group pattern is equivalent and fully portable. * * Returns null when no document exists for the org yet (balance = 0). * @@ -412,21 +420,40 @@ const listLedgerPage = async (orgId, skip, limit) => { const results = await BillingExtraBalance().aggregate([ { $match: { organization: new mongoose.Types.ObjectId(orgId) } }, + // Capture total count and cachedBalance before unwinding; normalise missing ledger to []. { $project: { - _id: 0, + _id: 1, cachedBalance: 1, total: { $size: { $ifNull: ['$ledger', []] } }, - // Sort descending by `at` then slice the requested page. - // $ifNull guards against missing/null ledger field on legacy docs. + ledger: { $ifNull: ['$ledger', []] }, + }, + }, + // Preserve docs with an empty ledger (preserveNullAndEmptyArrays keeps the root doc). + { $unwind: { path: '$ledger', preserveNullAndEmptyArrays: true } }, + // Sort entries descending by `at` (newest first). Compatible with MongoDB ≥4.4. + { $sort: { 'ledger.at': -1 } }, + // Reassemble the sorted entries back into a single document per org. + { + $group: { + _id: '$_id', + cachedBalance: { $first: '$cachedBalance' }, + total: { $first: '$total' }, + // $push preserves the $sort order guaranteed by the preceding $sort stage. + sortedLedger: { $push: '$ledger' }, + }, + }, + // Slice the sorted array to the requested page and drop internal fields. + // $filter removes the null sentinel emitted by $unwind when ledger was empty + // (preserveNullAndEmptyArrays keeps the parent doc alive but pushes null into $group). + { + $project: { + _id: 0, + cachedBalance: 1, + total: 1, ledgerPage: { $slice: [ - { - $sortArray: { - input: { $ifNull: ['$ledger', []] }, - sortBy: { at: -1 }, - }, - }, + { $filter: { input: '$sortedLedger', cond: { $ne: ['$$this', null] } } }, skip, limit, ], diff --git a/modules/billing/tests/billing.extraBalance.listLedger.perf.integration.tests.js b/modules/billing/tests/billing.extraBalance.listLedger.perf.integration.tests.js index 4d3a5786c..b4aee63d5 100644 --- a/modules/billing/tests/billing.extraBalance.listLedger.perf.integration.tests.js +++ b/modules/billing/tests/billing.extraBalance.listLedger.perf.integration.tests.js @@ -79,6 +79,23 @@ describe('BillingExtraBalanceRepository.listLedgerPage performance integration t expect(result).toBeNull(); }); + test('listLedgerPage: returns empty ledgerPage (not [null]) for org with empty ledger', async () => { + // Regression guard: $unwind preserveNullAndEmptyArrays emits a null sentinel when the + // ledger array is empty. Without the $filter guard, ledgerPage would be [null] instead of []. + await BillingExtraBalance.create({ + organization: orgId, + ledger: [], + cachedBalance: 0, + cachedBalanceAt: new Date(), + }); + + const result = await BillingExtraBalanceRepository.listLedgerPage(String(orgId), 0, 20); + + expect(result).not.toBeNull(); + expect(result.total).toBe(0); + expect(result.ledgerPage).toEqual([]); + }); + test('listLedgerPage: page 2 returns the correct slice (skip=20, limit=20)', async () => { const now = Date.now(); const ledger = Array.from({ length: 50 }, (_, i) => ({ diff --git a/modules/billing/tests/billing.extraBalance.unit.tests.js b/modules/billing/tests/billing.extraBalance.unit.tests.js index 0bbc5be8e..490107968 100644 --- a/modules/billing/tests/billing.extraBalance.unit.tests.js +++ b/modules/billing/tests/billing.extraBalance.unit.tests.js @@ -778,6 +778,139 @@ describe('BillingExtraBalance unit tests:', () => { }); }); + // ───────────────────────────────────────────────────────────────────────────── + // listLedgerPage — portable sort (no $sortArray, MongoDB ≥4.4 compatible) + // ───────────────────────────────────────────────────────────────────────────── + describe('listLedgerPage:', () => { + /** + * Build a fake aggregate stub that returns `rows` from `.exec()`. + * The pipeline itself is not inspected here — the integration tests + * (billing.extraBalance.listLedger.perf.integration.tests.js) exercise the + * real MongoDB pipeline and assert sort order + correct slice. + */ + const makeAggregateMock = (rows) => ({ + exec: jest.fn().mockResolvedValue(rows), + }); + + beforeEach(async () => { + jest.resetModules(); + // Re-declare mockModel with aggregate support for this suite. + mockModel = { + findOne: jest.fn(), + findOneAndUpdate: jest.fn(), + updateOne: jest.fn(), + updateMany: jest.fn(), + exists: jest.fn(), + aggregate: jest.fn(), + }; + // ObjectId must be a callable constructor (new mongoose.Types.ObjectId(id)) + // AND expose a static isValid method. + function MockObjectId(id) { + this.id = id; + this.toString = () => id; + } + MockObjectId.isValid = jest.fn(() => true); + jest.unstable_mockModule('mongoose', () => ({ + default: { + model: jest.fn(() => mockModel), + Types: { + ObjectId: MockObjectId, + }, + }, + })); + const mod = await import('../repositories/billing.extraBalance.repository.js'); + BillingExtraBalanceRepository = mod.default; + }); + + test('returns null when no document found', async () => { + mockModel.aggregate.mockReturnValue(makeAggregateMock([])); + const result = await BillingExtraBalanceRepository.listLedgerPage(orgId, 0, 20); + expect(result).toBeNull(); + }); + + test('returns null for invalid orgId', async () => { + const { default: mongoose } = await import('mongoose'); + // Override isValid on the already-set MockObjectId constructor + mongoose.Types.ObjectId.isValid.mockReturnValueOnce(false); + + const result = await BillingExtraBalanceRepository.listLedgerPage('bad-id', 0, 20); + expect(result).toBeNull(); + expect(mockModel.aggregate).not.toHaveBeenCalled(); + }); + + test('throws TypeError when skip is negative', async () => { + await expect( + BillingExtraBalanceRepository.listLedgerPage(orgId, -1, 20), + ).rejects.toThrow('skip must be a non-negative number'); + }); + + test('throws TypeError when limit is zero', async () => { + await expect( + BillingExtraBalanceRepository.listLedgerPage(orgId, 0, 0), + ).rejects.toThrow('limit must be a positive number'); + }); + + test('returns the aggregation result when document found', async () => { + const fakeResult = { + cachedBalance: 5000, + total: 3, + ledgerPage: [ + { kind: 'topup', amount: 300, at: new Date('2024-03-01') }, + { kind: 'topup', amount: 200, at: new Date('2024-02-01') }, + { kind: 'debit', amount: -100, at: new Date('2024-01-01') }, + ], + }; + mockModel.aggregate.mockReturnValue(makeAggregateMock([fakeResult])); + + const result = await BillingExtraBalanceRepository.listLedgerPage(orgId, 0, 20); + + expect(result).not.toBeNull(); + expect(result.total).toBe(3); + expect(result.cachedBalance).toBe(5000); + expect(result.ledgerPage).toHaveLength(3); + }); + + test('ledgerPage entries are descending by at (newest first)', async () => { + // Simulate what the real MongoDB pipeline returns: entries already sorted desc. + const now = Date.now(); + const fakeResult = { + cachedBalance: 1000, + total: 3, + ledgerPage: [ + { kind: 'topup', amount: 300, at: new Date(now) }, + { kind: 'topup', amount: 200, at: new Date(now - 1000) }, + { kind: 'debit', amount: -100, at: new Date(now - 2000) }, + ], + }; + mockModel.aggregate.mockReturnValue(makeAggregateMock([fakeResult])); + + const result = await BillingExtraBalanceRepository.listLedgerPage(orgId, 0, 20); + + // Assert descending order: each entry must be newer than or equal to the next. + for (let i = 0; i < result.ledgerPage.length - 1; i++) { + const curr = new Date(result.ledgerPage[i].at).getTime(); + const next = new Date(result.ledgerPage[i + 1].at).getTime(); + expect(curr).toBeGreaterThanOrEqual(next); + } + }); + + test('pipeline does not use $sortArray operator (portability guard)', async () => { + // Capture the pipeline passed to aggregate() and assert no stage uses $sortArray. + // This is a regression guard: if $sortArray is re-introduced, this test breaks + // and reminds the author that MongoDB ≥5.2 is required. + const capturedPipelines = []; + mockModel.aggregate.mockImplementation((pipeline) => { + capturedPipelines.push(pipeline); + return makeAggregateMock([{ cachedBalance: 0, total: 0, ledgerPage: [] }]); + }); + + await BillingExtraBalanceRepository.listLedgerPage(orgId, 0, 20); + + const pipelineStr = JSON.stringify(capturedPipelines); + expect(pipelineStr).not.toContain('$sortArray'); + }); + }); + // ───────────────────────────────────────────────────────────────────────────── // creditGrant — signup grant (no stripeSessionId) // ─────────────────────────────────────────────────────────────────────────────