diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 862168563f..1a7212d758 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -69,6 +69,7 @@ Missing any of these = changes requested. - **Boolean flags: positive naming**: `safetyModeActive` not `safetyModuleInactive` - **Short, descriptive names — no redundant prefixes**: `uid` not `transactionRequestUid`, `balances` not `assetBalances`, `txId` not `transactionId` - **Variable names must precisely reflect the data**: `priceChf` not `amountChf` for a price +- **`wait` is reserved for long-polling routes**: see [Long-Polling Endpoints Must Be Named `wait`](#long-polling-endpoints-must-be-named-wait) — the segment is load-bearing for latency monitoring ### Methods @@ -965,6 +966,36 @@ Keep old endpoints for backward compatibility but annotate: @ApiOperation({ deprecated: true }) ``` +### Long-Polling Endpoints Must Be Named `wait` + +An endpoint that does nothing but wait for someone else to act — its response time is determined solely by when another request or process triggers the event, and it performs no work of its own meanwhile — **must** carry `wait` as its own path segment, unless it is listed as an explicit exemption below. Conversely, an endpoint expected to answer quickly **must not** use `wait` as a path segment. + +This does **not** cover an endpoint that starts an operation and then waits for it to finish — broadcasting a transaction and awaiting its confirmation, for example. That duration reflects work the API itself set in motion, which makes it a legitimate monitoring signal, so those endpoints stay visible and must **not** be named `wait`. Current examples: `PUT /v1/sell/paymentInfos/:id/confirm` and `PUT /v1/swap/paymentInfos/:id/confirm` (both in their `authorization` branch), `PUT /v1/realunit/sell/:id/confirm` (`eip7702` branch) and `PUT /v1/realunit/transfer/:id/confirm`. + +Endpoints that block by design: + +| Path | Blocks until | `wait` segment | +| ------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------- | +| `GET /v1/lnurlp/wait/:id` | the payment resolves: completed, canceled, expired — or, for `MULTIPLE`-mode links, when a quote reaches the configured completion threshold (the payment itself may stay `Pending`) | yes | +| `GET /v1/paymentLink/payment/wait` | the same, for the authenticated payment-link flow | yes | +| `GET /v1/lnurlp/:id` | a pending payment appears; bounded by `timeout` (default 10 s, caller-controllable) | no — exempt | +| `GET /v1/lnurlp/tx/:id` | the payer's own broadcast reaches one confirmation (`tx` branch); 15 polls at 1 s. The `hex` branch broadcasts without awaiting confirmation, except on ICP, where it first waits for the payer's allowance (up to 3 attempts, 2 s apart) | no — exempt | +| `GET /v1/node/:node/tx/:txId` | the transaction reaches one confirmation; bounded at 600 s | no — exempt | +| `GET /v1/node/:node/:mode/tx/:txId` | the same | no — exempt | + +**The `wait` segment is the default; exemptions must be explicit.** A *passively* waiting route without one is acceptable only if it is listed in the table above together with the reason it cannot carry the segment. A passively waiting route that is neither named `wait` nor listed here is a defect — fix it by renaming the route or by adding an entry, never by leaving it undocumented. Routes of the second kind above — those awaiting an operation they started themselves — need no entry; they are outside this rule by design. + +The four current exemptions keep their paths because those are fixed from outside: `/v1/lnurlp/:id` is the LNURL pay-request path encoded into LNURLs already in circulation, `/v1/lnurlp/tx/:id` is handed to the payer inside the payment request itself, and the two node routes are admin-only and `@ApiExcludeEndpoint()`. Because they carry no `wait` segment they stay visible in latency monitoring — read their duration as expected behavior, not as a regression. + +This is not cosmetic. Latency monitoring excludes routes matching `^.*/wait(/.*)?$` from its slowest-requests view. A long poll's duration measures how long a *customer* took to act, not how long the API computed — leaving it in that view pushes the genuine outliers out of a list with a fixed row cap. + +Getting the name wrong breaks monitoring in one of two directions: + +- **A passively waiting endpoint without a `wait` segment** appears as a permanent latency outlier and masks real regressions — that is exactly what the exemptions above cost us today, which is why the list must stay short and justified. +- **A fast endpoint with a `wait` segment** is silently dropped from the latency view — if it ever becomes slow, nobody notices. + +The pattern is segment-anchored, so `/waitlist`, `/waitTime`, `/awaiting` and `/waiting/:id` are unaffected; only a complete `wait` segment matches. Matching runs on the server-side route template (`http.route`), never on the raw request path, which is caller-controlled. + ### RealUnit: `/quote/*` vs `/brokerbot/*` The RealUnit purchase and sale flows historically lived under `/v1/realunit/brokerbot/*`. That naming is misleading: most of those endpoints never touch the on-chain Brokerbot smart contract. Treat them as two distinct subsystems: diff --git a/migration/1785510000000-AddTradingOrderRuleIdIndex.js b/migration/1785510000000-AddTradingOrderRuleIdIndex.js new file mode 100644 index 0000000000..ef5e0fd4d9 --- /dev/null +++ b/migration/1785510000000-AddTradingOrderRuleIdIndex.js @@ -0,0 +1,145 @@ +/** + * @typedef {import('typeorm').MigrationInterface} MigrationInterface + * @typedef {import('typeorm').QueryRunner} QueryRunner + */ + +/** + * Add a composite index on `trading_order ("tradingRuleId", "id")` so the per-minute + * "latest trading order per rule" lookup stops doing a full sequential scan of the table. + * Row counts and sizes are given further down, together with where each was measured. + * + * The query this targets is: + * `SELECT MAX("tradingOrder"."id") AS "tradingOrderId" FROM "trading_order" "tradingOrder" + * INNER JOIN "trading_rule" "tradingRule" ON "tradingRule"."id" = "tradingOrder"."tradingRuleId" + * GROUP BY "tradingOrder"."tradingRuleId"`. + * Source: `TradingRuleService.getCurrentTradingOrders`, called from `LogJobService` once per + * minute as part of the financial-log job. + * + * Production `EXPLAIN (ANALYZE, BUFFERS)` for this exact aggregate query (measured externally + * against production; not reproducible from this repository) showed Execution Time 489 ms, + * Buffers: shared hit=3734 read=85614 — only 4.2% of blocks came from the 1 GB `shared_buffers` + * cache, the rest was freshly read from disk on every one-minute run. Table size at the time of + * measurement in production: 698 MB. Result: 17 rows (there are exactly 17 `trading_rule` rows). + * + * Column order `("tradingRuleId", "id")` is intentional: equality on `tradingRuleId` first so + * Postgres can jump straight to one rule's leaf range, then `id` so a backward index scan finds + * the maximum id for that rule without a sort or a table-wide aggregate. The same physical order + * is what enables the Index Only Scan plan change for the unchanged aggregate described below; + * it would also be what a correlated per-rule lookup needs, if this repository's test + * infrastructure allowed one (see the comment on the service method for that trade-off). + * + * This migration ships ONLY the index; the query above is not rewritten. Measured externally + * against a Postgres 17.10 rebuild with production planner settings, loaded with 5,421,152 rows + * (730 MB) and the 17 `trading_rule` rows in real production distribution (not reproducible + * from this repository): + * - Without this index: Parallel Seq Scan, 93,486 buffer blocks, 8,181 ms (warm). + * - With this index: Parallel Index Only Scan, Heap Fetches: 0, 15,020 buffer blocks, 5,676 ms + * (warm). + * The planner picks this index for exactly the query above with no code change — this migration + * is therefore not a no-op. The index itself is 116 MB. That matches the fresh production + * measurement above (489 ms, shared hit=3734 read=85614, only 4.2% cache hit rate, 698 MB + * table): a 116 MB index has a realistic chance of staying resident in the 1 GB + * `shared_buffers`; the substantially larger table demonstrably does not. A correlated per-rule + * lookup (`SELECT r.id, (SELECT MAX(o.id) FROM trading_order o WHERE o."tradingRuleId" = r.id) + * FROM trading_rule r;`) would be faster still with this index — Index Only Scan Backward, 52 + * buffer blocks, 1.5 ms (warm), measured on the same rebuild (same external measurement + * disclaimer: not reproducible from this repository) — but is NOT shipped here: it would need a + * correlated subquery that this repository's pg-mem-based test suite (pg-mem 3.0.14) cannot + * execute. See the comment on `TradingRuleService.getCurrentTradingOrders` for that trade-off. + * + * The existing single-column index `IDX_f862025cb7ca5a2d66d14fb89a` on + * `trading_order ("tradingRuleId")` is NOT removed by this migration. It was created by + * `AddForeignKeyIndexes1779802432879` (`CREATE INDEX "IDX_f862025cb7ca5a2d66d14fb89a" ON + * "trading_order" ("tradingRuleId")`). The new composite index makes that single-column index + * functionally redundant for most purposes (any query that can use the single-column index on + * `tradingRuleId` can equally use the new composite, since `tradingRuleId` is its leading + * column), but dropping the old index is out of scope for this change and is left for a + * separate, later migration. + * + * CREATE INDEX CONCURRENTLY is not used: migrations in this codebase run transactionally and + * boot-blockingly (see `src/config/config.ts`, `migrationsRun` gated by the `SQL_MIGRATE` env + * var). CREATE INDEX CONCURRENTLY is not allowed inside a transaction and would crash the + * migration. + * + * Lock behaviour, stated precisely: all pending migrations run inside a single database + * transaction (TypeORM default `migrationsTransactionMode: "all"`), and PostgreSQL only + * releases locks at COMMIT, not at the end of each statement. Evidence: + * `node_modules/typeorm/data-source/DataSource.js` (`migrationExecutor.transaction = + * options?.transaction || this.options?.migrationsTransactionMode || "all"` — default `"all"`); + * `src/config/config.ts` only sets `migrationsRun` and never overrides + * `migrationsTransactionMode` (corroborated by the comment in + * `src/shared/models/asset/__tests__/add-binance-custody-assets-ondo-ada.migration.spec.ts` — + * "no migrationsTransactionMode override → default 'all'"); + * `node_modules/typeorm/migration/MigrationExecutor.js` starts one transaction for pending + * migrations and commits only at the end. A plain CREATE INDEX holds a SHARE lock for the + * entire build; reads continue throughout, but writes to the table are blocked while that lock + * is held. Because locks are held until COMMIT, if other migrations are pending in the same + * batch, this index's SHARE lock is held until all of them commit together, not just until this + * statement finishes. `SET LOCAL lock_timeout` bounds only how long we wait to ACQUIRE the lock, + * not how long we hold it once acquired, and it is scoped to the whole transaction — that is + * exactly why it is set only once at the top of `up()` and once at the top of `down()`, not per + * statement (there is only one `CREATE INDEX` / one `DROP INDEX` in this migration). + * + * Risk framing: this migration runs boot-blockingly at app startup (`migrationsRun`, gated by + * the `SQL_MIGRATE` env var), so the starting instance itself is not yet serving requests and is + * not itself a writer. Concurrent writers would be a still-running predecessor instance during a + * rolling deploy, or external consumers. If a lock conflict occurs, the migration aborts after + * `lock_timeout` and so does the app start — that is fail-closed and intentional, but it is a + * deploy abort and must be named as such. + * + * `down()` reverses this with `DROP INDEX` and is subject to a stricter lock: PostgreSQL takes + * an ACCESS EXCLUSIVE lock for `DROP INDEX` (vs. the SHARE lock `CREATE INDEX` takes above), and + * ACCESS EXCLUSIVE conflicts with every other lock mode, including the AccessShareLock a plain + * `SELECT` takes — so `down()` blocks reads as well as writes, not writes alone. Because + * `down()` also runs inside the single-batch transaction (same TypeORM default + * `migrationsTransactionMode: "all"`), the same hold-until-COMMIT reasoning applies. + * + * Honest disclaimer: whether the Postgres planner will actually pick this new index for the + * unchanged aggregate query above has NOT been verified in production itself, because the index + * does not exist there yet. The rebuild measurement above already shows that the plan changes + * from Parallel Seq Scan to Parallel Index Only Scan when this index exists — but that is still + * not confirmation on production itself. As a point of reference (not a guarantee), the same + * style of prediction was made for the `created` index in + * `AddTradingOrderCreatedIndex1785470000000` (selectivity + cost-model reasoning only, no prior + * plan-change observation) and was confirmed after that deploy (measured externally against + * production; not reproducible from this repository): the query's plan changed from a Seq Scan + * to an Index Scan, execution time dropped from 141.283 ms to 30.3 ms, and buffer reads dropped + * from 89,282 to 2,716. + * + * Index name: `IDX_710fd49e19d248643cb2afa70f` on `trading_order ("tradingRuleId", "id")`. + * This is not an arbitrary name but the deterministic name TypeORM's DefaultNamingStrategy would + * generate itself, since custom index naming is disallowed by CONTRIBUTING.md. The name is + * `IDX_` followed by the first 26 hex characters of `sha1('trading_order_id_tradingRuleId')` + * (table name + `_` + the two column names `id` and `tradingRuleId` sorted alphabetically and + * joined with `_`, per TypeORM's DefaultNamingStrategy — `id` sorts before `tradingRuleId`). + * The physical index column order remains `("tradingRuleId", "id")` as required for the equality + * + max-id access path above; only the name derivation sorts the column names. + * + * @class + * @implements {MigrationInterface} + */ +module.exports = class AddTradingOrderRuleIdIndex1785510000000 { + name = 'AddTradingOrderRuleIdIndex1785510000000'; + + /** + * @param {QueryRunner} queryRunner + */ + async up(queryRunner) { + // SET LOCAL is scoped to the whole transaction. Bounds WAIT time to acquire the lock, not how + // long the lock is held. Set once: this migration has a single CREATE INDEX statement. + await queryRunner.query(`SET LOCAL lock_timeout = '5s'`); + await queryRunner.query( + `CREATE INDEX "IDX_710fd49e19d248643cb2afa70f" ON "trading_order" ("tradingRuleId", "id")`, + ); + } + + /** + * @param {QueryRunner} queryRunner + */ + async down(queryRunner) { + // SET LOCAL is scoped to the whole transaction. Bounds WAIT time to acquire the lock, not how + // long the lock is held. Set once: this migration has a single DROP INDEX statement. + await queryRunner.query(`SET LOCAL lock_timeout = '5s'`); + await queryRunner.query(`DROP INDEX "public"."IDX_710fd49e19d248643cb2afa70f"`); + } +}; diff --git a/src/shared/models/setting/__tests__/setting.repository.spec.ts b/src/shared/models/setting/__tests__/setting.repository.spec.ts index 3a93abc107..e343023697 100644 --- a/src/shared/models/setting/__tests__/setting.repository.spec.ts +++ b/src/shared/models/setting/__tests__/setting.repository.spec.ts @@ -1,6 +1,35 @@ +import { Like } from 'typeorm'; import { Setting } from '../setting.entity'; import { SettingRepository } from '../setting.repository'; +describe('SettingRepository.getStatusSettings', () => { + function repositoryWithFind(result: Setting[]) { + const repository = Object.create(SettingRepository.prototype) as SettingRepository; + const find = jest.fn().mockResolvedValue(result); + Object.defineProperty(repository, 'find', { value: find }); + return { repository, find }; + } + + it('finds status settings in ascending id order', async () => { + const settings = [Object.assign(new Setting(), { id: 1, key: 'paymentStatus', value: 'active' })]; + const { repository, find } = repositoryWithFind(settings); + + await expect(repository.getStatusSettings()).resolves.toBe(settings); + + expect(find).toHaveBeenCalledWith({ + where: { key: Like('%Status') }, + order: { id: 'ASC' }, + }); + }); + + it('passes through an empty result', async () => { + const settings: Setting[] = []; + const { repository } = repositoryWithFind(settings); + + await expect(repository.getStatusSettings()).resolves.toBe(settings); + }); +}); + describe('SettingRepository.setDateMax', () => { function repositoryWithTransaction(transactionManager: Record) { const repository = Object.create(SettingRepository.prototype) as SettingRepository; diff --git a/src/shared/models/setting/setting.repository.ts b/src/shared/models/setting/setting.repository.ts index 25666f97eb..107d5eb7fb 100644 --- a/src/shared/models/setting/setting.repository.ts +++ b/src/shared/models/setting/setting.repository.ts @@ -1,6 +1,6 @@ import { Injectable } from '@nestjs/common'; import { CachedRepository } from 'src/shared/repositories/cached.repository'; -import { EntityManager } from 'typeorm'; +import { EntityManager, Like } from 'typeorm'; import { Setting } from './setting.entity'; @Injectable() @@ -31,4 +31,8 @@ export class SettingRepository extends CachedRepository { this.invalidateCache(); } + + async getStatusSettings(): Promise { + return this.find({ where: { key: Like('%Status') }, order: { id: 'ASC' } }); + } } diff --git a/src/shared/models/setting/setting.service.ts b/src/shared/models/setting/setting.service.ts index a0b17f0fef..680ae01222 100644 --- a/src/shared/models/setting/setting.service.ts +++ b/src/shared/models/setting/setting.service.ts @@ -16,6 +16,12 @@ export class SettingService { return this.settingRepo.find(); } + // Loads only settings whose key ends with "Status" instead of transferring the entire table: in production, + // getAll() transfers about 1.5 MB, including one entry of about 1.5 MB, to return only a few bytes of status data. + async getStatusSettings(): Promise { + return this.settingRepo.getStatusSettings(); + } + async get(key: string, defaultValue?: string): Promise { return this.settingRepo.findOneBy({ key }).then((d) => d?.value ?? defaultValue); } diff --git a/src/subdomains/core/statistic/__tests__/statistic.service.spec.ts b/src/subdomains/core/statistic/__tests__/statistic.service.spec.ts new file mode 100644 index 0000000000..02efff7a79 --- /dev/null +++ b/src/subdomains/core/statistic/__tests__/statistic.service.spec.ts @@ -0,0 +1,55 @@ +import { createMock } from '@golevelup/ts-jest'; +import { Test, TestingModule } from '@nestjs/testing'; +import { Setting } from 'src/shared/models/setting/setting.entity'; +import { SettingService } from 'src/shared/models/setting/setting.service'; +import { BuyService } from 'src/subdomains/core/buy-crypto/routes/buy/buy.service'; +import { SellService } from 'src/subdomains/core/sell-crypto/route/sell.service'; +import { StatisticService } from 'src/subdomains/core/statistic/statistic.service'; +import { UserService } from 'src/subdomains/generic/user/models/user/user.service'; + +describe('StatisticService', () => { + let service: StatisticService; + let settingService: jest.Mocked; + + beforeEach(async () => { + settingService = createMock(); + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + StatisticService, + { provide: BuyService, useValue: createMock() }, + { provide: SellService, useValue: createMock() }, + { provide: SettingService, useValue: settingService }, + { provide: UserService, useValue: createMock() }, + ], + }).compile(); + + service = module.get(StatisticService); + }); + + describe('getStatus', () => { + it('loads only status settings', async () => { + settingService.getStatusSettings.mockResolvedValue([]); + + await service.getStatus(); + + expect(settingService.getStatusSettings).toHaveBeenCalled(); + expect(settingService.getAll).not.toHaveBeenCalled(); + }); + + it('maps status settings to status keys and values', async () => { + settingService.getStatusSettings.mockResolvedValue([ + Object.assign(new Setting(), { key: 'buyStatus', value: 'Available' }), + Object.assign(new Setting(), { key: 'sellStatus', value: 'Limited' }), + ]); + + await expect(service.getStatus()).resolves.toEqual({ buy: 'Available', sell: 'Limited' }); + }); + + it('returns an empty object when there are no status settings', async () => { + settingService.getStatusSettings.mockResolvedValue([]); + + await expect(service.getStatus()).resolves.toEqual({}); + }); + }); +}); diff --git a/src/subdomains/core/statistic/statistic.service.ts b/src/subdomains/core/statistic/statistic.service.ts index d8ba363623..889efd607e 100644 --- a/src/subdomains/core/statistic/statistic.service.ts +++ b/src/subdomains/core/statistic/statistic.service.ts @@ -41,10 +41,8 @@ export class StatisticService implements OnModuleInit { } async getStatus(): Promise { - const settings = await this.settingService.getAll(); - return settings - .filter((s) => s.key.endsWith('Status')) - .reduce((prev, curr) => ({ ...prev, [curr.key.replace('Status', '')]: curr.value }), {}); + const settings = await this.settingService.getStatusSettings(); + return settings.reduce((prev, curr) => ({ ...prev, [curr.key.replace('Status', '')]: curr.value }), {}); } getAll(): StatisticDto { diff --git a/src/subdomains/core/trading/services/__tests__/trading-rule.service.pg.spec.ts b/src/subdomains/core/trading/services/__tests__/trading-rule.service.pg.spec.ts new file mode 100644 index 0000000000..f11d939706 --- /dev/null +++ b/src/subdomains/core/trading/services/__tests__/trading-rule.service.pg.spec.ts @@ -0,0 +1,147 @@ +import { createMock } from '@golevelup/ts-jest'; +import { DataType, newDb } from 'pg-mem'; +import { Column, DataSource, Entity, FindOperator, JoinColumn, ManyToOne, PrimaryColumn } from 'typeorm'; +import { TradingRuleService } from '../trading-rule.service'; +import { TradingService } from '../trading.service'; + +// the real TradingOrder / TradingRule entities cannot be registered standalone (relations pull +// in the whole entity graph), so these tables mirror only the columns getCurrentTradingOrders +// actually touches — under the real table names +@Entity({ name: 'trading_rule' }) +class TradingRuleTable { + @PrimaryColumn() + id: number; +} + +@Entity({ name: 'trading_order' }) +class TradingOrderTable { + @PrimaryColumn() + id: number; + + @Column({ type: 'int' }) + tradingRuleId: number; + + // Relation path for `.innerJoin('tradingOrder.tradingRule', ...)`; createForeignKeyConstraints + // is false so the intentionally orphaned fixture row (tradingRuleId with no matching rule) stays insertable. + @ManyToOne(() => TradingRuleTable, { nullable: false, createForeignKeyConstraints: false }) + @JoinColumn({ name: 'tradingRuleId' }) + tradingRule: TradingRuleTable; +} + +// runs getCurrentTradingOrders against a Postgres-semantics engine (pg-mem) to verify the +// aggregation semantics, because a mocked query builder never executes SQL and a wrong shape +// (e.g. MAX swapped for MIN, or the INNER JOIN removed) would otherwise go unnoticed +describe('TradingRuleService.getCurrentTradingOrders (postgres semantics)', () => { + let dataSource: DataSource; + let service: TradingRuleService; + + beforeAll(async () => { + const db = newDb(); + // TypeORM runs SELECT version() / current_database() on connect; pg-mem does not ship them + db.public.registerFunction({ name: 'version', returns: DataType.text, implementation: () => 'PostgreSQL 15.0' }); + db.public.registerFunction({ name: 'current_database', returns: DataType.text, implementation: () => 'test' }); + + dataSource = (await db.adapters.createTypeormDataSource({ + type: 'postgres', + entities: [TradingRuleTable, TradingOrderTable], + synchronize: true, + })) as DataSource; + await dataSource.initialize(); + }); + + afterAll(async () => { + if (dataSource?.isInitialized) await dataSource.destroy(); + }); + + beforeEach(async () => { + await dataSource.getRepository(TradingOrderTable).clear(); + await dataSource.getRepository(TradingRuleTable).clear(); + + const tradingService = createMock(); + service = new TradingRuleService(tradingService); + (service as any).orderRepo = dataSource.getRepository(TradingOrderTable); + (service as any).ruleRepo = dataSource.getRepository(TradingRuleTable); + }); + + async function seedFixture(): Promise { + const ruleRepo = dataSource.getRepository(TradingRuleTable); + const orderRepo = dataSource.getRepository(TradingOrderTable); + + // rule 1: several orders → expect max id 30 + // rule 2: exactly one order → expect id 40 + // rule 3: no orders → must not appear + // orphan order 99: tradingRuleId matches no rule → must not appear (INNER JOIN exclusion) + await ruleRepo.save([{ id: 1 }, { id: 2 }, { id: 3 }]); + await orderRepo.save([ + { id: 10, tradingRuleId: 1 }, + { id: 20, tradingRuleId: 1 }, + { id: 30, tradingRuleId: 1 }, + { id: 40, tradingRuleId: 2 }, + { id: 99, tradingRuleId: 999 }, + ]); + } + + it('returns the highest-id order per rule, skips empty rules and orphans', async () => { + await seedFixture(); + + const result = await service.getCurrentTradingOrders(); + const resultIds = result.map((order) => order.id).sort((a, b) => a - b); + + // concrete ids per rule (fails if MAX is swapped for MIN or any wrong pick) + expect(resultIds).toEqual([30, 40]); + expect(result).toHaveLength(2); + expect(result.some((order) => order.id === 10 || order.id === 20)).toBe(false); + expect(result.some((order) => order.id === 99)).toBe(false); + }); + + it('returns an empty array when trading_rule is empty', async () => { + const result = await service.getCurrentTradingOrders(); + + expect(result).toEqual([]); + }); + + it('never passes null or undefined order ids into findBy In(...)', async () => { + await dataSource.getRepository(TradingRuleTable).save([{ id: 1 }, { id: 2 }]); + await dataSource.getRepository(TradingOrderTable).save([ + { id: 10, tradingRuleId: 1 }, + { id: 20, tradingRuleId: 1 }, + ]); + + const findBySpy = jest.spyOn(service['orderRepo'], 'findBy'); + try { + await service.getCurrentTradingOrders(); + + expect(findBySpy).toHaveBeenCalled(); + const findByArg = findBySpy.mock.calls[0][0]; + + // findBy accepts a single condition or an array of them; getCurrentTradingOrders only ever + // passes a single { id: In(...) } condition, so an array here would itself be a regression. + if (Array.isArray(findByArg)) { + throw new Error('expected findBy to receive a single FindOptionsWhere condition, not an array of them'); + } + + const idCondition = findByArg.id; + + // In(...) produces a real TypeORM FindOperator instance. A bare value would slip past the + // array check below, so reject it here. This does not pin the operator to In specifically: + // Any(), Not() and friends are FindOperator instances too, and swapping In for Any would + // carry the same list of ids -- which is what this test is actually about. + if (!(idCondition instanceof FindOperator)) { + throw new Error(`expected findBy id condition to be a FindOperator, got: ${String(idCondition)}`); + } + + // FindOperator.value is typed against the entity's own field type (number here), but + // In(...) stores the full array as the operator's underlying value. Widen through the real + // FindOperator class -- not an invented shape -- to read it without lying about its declared + // element type; unknown is the one legitimate single-step escape hatch for this widening. + const idValues = (idCondition as FindOperator).value; + if (!Array.isArray(idValues)) { + throw new Error('expected the FindOperator to carry an array of ids'); + } + + expect(idValues.every((id) => id !== null && id !== undefined)).toBe(true); + } finally { + findBySpy.mockRestore(); + } + }); +}); diff --git a/src/subdomains/core/trading/services/trading-rule.service.ts b/src/subdomains/core/trading/services/trading-rule.service.ts index 837f1aa657..1368a767cd 100644 --- a/src/subdomains/core/trading/services/trading-rule.service.ts +++ b/src/subdomains/core/trading/services/trading-rule.service.ts @@ -21,6 +21,15 @@ export class TradingRuleService { // --- PUBLIC API --- // + // One statement, not a per-rule loop: all rules' maxima must come from the same + // READ-COMMITTED snapshot, because LogJobService writes the FinanceLog from this result. + // Separate statements per rule could observe an insert into trading_order mid-loop and mix + // maxima from different points in time — a single GROUP BY aggregate cannot do that. + // The composite index on trading_order ("tradingRuleId", "id") (see the + // AddTradingOrderRuleIdIndex migration) lets Postgres answer this with an Index Only Scan. + // A correlated per-rule lookup would be faster still, but pg-mem (this repo's test engine for + // this query, see trading-rule.service.pg.spec.ts) cannot execute a correlated subquery — + // don't "optimize" this into one without first solving that. async getCurrentTradingOrders(): Promise { const lastTradingOrderIds = await this.orderRepo .createQueryBuilder('tradingOrder') diff --git a/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.controller.spec.ts b/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.controller.spec.ts new file mode 100644 index 0000000000..2e01f396c1 --- /dev/null +++ b/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.controller.spec.ts @@ -0,0 +1,95 @@ +import { createMock } from '@golevelup/ts-jest'; +import { BadRequestException } from '@nestjs/common'; +import { Test, TestingModule } from '@nestjs/testing'; +import { DashboardFinancialController } from '../dashboard-financial.controller'; +import { DashboardFinancialService } from '../dashboard-financial.service'; +import { FinancialLogResponseDto } from '../dto/financial-log.dto'; + +describe('DashboardFinancialController', () => { + let controller: DashboardFinancialController; + let dashboardFinancialService: DashboardFinancialService; + + beforeEach(async () => { + dashboardFinancialService = createMock(); + + const module: TestingModule = await Test.createTestingModule({ + controllers: [DashboardFinancialController], + providers: [{ provide: DashboardFinancialService, useValue: dashboardFinancialService }], + }).compile(); + + controller = module.get(DashboardFinancialController); + }); + + describe('getFinancialLog', () => { + const from = '2026-07-01T00:00:00.000Z'; + const emptyResponse: FinancialLogResponseDto = { entries: [] }; + + it.each([ + { byType: undefined as string | undefined, expected: true, label: 'omitted' }, + { byType: '', expected: true, label: "empty string ''" }, + { byType: 'true', expected: true, label: "'true'" }, + { byType: '0', expected: true, label: "'0'" }, + { byType: 'False', expected: true, label: "'False'" }, + { byType: 'false', expected: false, label: "'false'" }, + ])( + 'forwards includeByType=$expected when byType is $label (and from/dailySample unchanged)', + async ({ byType, expected }) => { + const spy = jest.spyOn(dashboardFinancialService, 'getFinancialLog').mockResolvedValue(emptyResponse); + + if (byType === undefined) { + await controller.getFinancialLog(from, 'true'); + } else { + await controller.getFinancialLog(from, 'true', byType); + } + + expect(spy).toHaveBeenCalledWith(new Date(from), true, expected); + }, + ); + + it.each([ + { dailySample: undefined as string | undefined, expected: true, label: 'omitted' }, + { dailySample: '', expected: true, label: "empty string ''" }, + { dailySample: 'true', expected: true, label: "'true'" }, + { dailySample: '0', expected: true, label: "'0'" }, + { dailySample: 'False', expected: true, label: "'False'" }, + { dailySample: 'false', expected: false, label: "'false'" }, + ])( + 'forwards dailySample=$expected when dailySample is $label (byType held at the opposite value)', + async ({ dailySample, expected }) => { + const spy = jest.spyOn(dashboardFinancialService, 'getFinancialLog').mockResolvedValue(emptyResponse); + const byType = expected ? 'false' : 'true'; + + await controller.getFinancialLog(from, dailySample, byType); + + expect(spy).toHaveBeenCalledWith(new Date(from), expected, !expected); + }, + ); + + it('passes from, dailySample and includeByType through in order without transposition', async () => { + const spy = jest.spyOn(dashboardFinancialService, 'getFinancialLog').mockResolvedValue(emptyResponse); + + await controller.getFinancialLog('2026-06-15T00:00:00.000Z', 'false', 'true'); + + expect(spy).toHaveBeenCalledWith(new Date('2026-06-15T00:00:00.000Z'), false, true); + }); + + it.each([ + { from: undefined as string | undefined, label: 'omitted' }, + { from: '', label: "empty string ''" }, + ])('passes undefined to the service when from is $label', async ({ from }) => { + const spy = jest.spyOn(dashboardFinancialService, 'getFinancialLog').mockResolvedValue(emptyResponse); + + await controller.getFinancialLog(from, 'true', 'true'); + + expect(spy).toHaveBeenCalledWith(undefined, true, true); + }); + + it('throws BadRequestException and does not call the service when from is not a valid date', async () => { + const spy = jest.spyOn(dashboardFinancialService, 'getFinancialLog').mockResolvedValue(emptyResponse); + + await expect(controller.getFinancialLog('not-a-date', 'true', 'true')).rejects.toThrow(BadRequestException); + + expect(spy).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.service.spec.ts b/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.service.spec.ts index b9f983e1cd..13e7cb95bb 100644 --- a/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.service.spec.ts +++ b/src/subdomains/supporting/dashboard/__tests__/dashboard-financial.service.spec.ts @@ -231,7 +231,7 @@ describe('DashboardFinancialService', () => { const result = await service.getFinancialLog(from, true); expect(getBtcCoinSpy).toHaveBeenCalled(); - expect(getSummariesSpy).toHaveBeenCalledWith(7, from, true); + expect(getSummariesSpy).toHaveBeenCalledWith(7, from, true, undefined, undefined, undefined, undefined); // Ordering matters: btcAssetId is a SQL projection parameter, so getBtcCoin must finish first. expect(getBtcCoinSpy.mock.invocationCallOrder[0]).toBeLessThan(getSummariesSpy.mock.invocationCallOrder[0]); @@ -257,7 +257,40 @@ describe('DashboardFinancialService', () => { await service.getFinancialLog(); - expect(getSummariesSpy).toHaveBeenCalledWith(undefined, undefined, undefined); + expect(getSummariesSpy).toHaveBeenCalledWith( + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + ); + }); + + it('forwards includeByType=false through to getFinancialLogSummaries and the response omits balancesByType entirely (not an empty object, not null)', async () => { + const btcAsset = { id: 7 } as Awaited>; + const summaries: FinancialLogSummary[] = [ + { + created: new Date('2026-07-14T00:00:00Z'), + id: 1, + totalBalanceChf: 100, + plusBalanceChf: 120, + minusBalanceChf: 20, + fxPnlChf: 1.5, + btcPriceChf: 64000, + // no balancesByType key: the repository omits it entirely when includeByType is false + }, + ]; + jest.spyOn(assetService, 'getBtcCoin').mockResolvedValue(btcAsset); + const getSummariesSpy = jest.spyOn(logService, 'getFinancialLogSummaries').mockResolvedValue(summaries); + + const from = new Date('2026-07-01T00:00:00Z'); + const result = await service.getFinancialLog(from, true, false); + + expect(getSummariesSpy).toHaveBeenCalledWith(7, from, true, undefined, undefined, undefined, false); + expect('balancesByType' in result.entries[0]).toBe(false); + expect(JSON.parse(JSON.stringify(result.entries[0]))).not.toHaveProperty('balancesByType'); }); }); }); diff --git a/src/subdomains/supporting/dashboard/dashboard-financial.controller.ts b/src/subdomains/supporting/dashboard/dashboard-financial.controller.ts index 550afd1341..8034236e05 100644 --- a/src/subdomains/supporting/dashboard/dashboard-financial.controller.ts +++ b/src/subdomains/supporting/dashboard/dashboard-financial.controller.ts @@ -25,11 +25,13 @@ export class DashboardFinancialController { async getFinancialLog( @Query('from') from?: string, @Query('dailySample') dailySample?: string, + @Query('byType') byType?: string, ): Promise { const fromDate = this.parseDate(from); const sample = dailySample !== 'false'; + const includeByType = byType !== 'false'; - return this.dashboardFinancialService.getFinancialLog(fromDate, sample); + return this.dashboardFinancialService.getFinancialLog(fromDate, sample, includeByType); } @Get('latest') diff --git a/src/subdomains/supporting/dashboard/dashboard-financial.service.ts b/src/subdomains/supporting/dashboard/dashboard-financial.service.ts index e7f46a22fa..37432322ce 100644 --- a/src/subdomains/supporting/dashboard/dashboard-financial.service.ts +++ b/src/subdomains/supporting/dashboard/dashboard-financial.service.ts @@ -23,12 +23,20 @@ export class DashboardFinancialService { private readonly refRewardService: RefRewardService, ) {} - async getFinancialLog(from?: Date, dailySample?: boolean): Promise { + async getFinancialLog(from?: Date, dailySample?: boolean, includeByType?: boolean): Promise { // BTC price is projected in SQL and needs btcAssetId as a parameter, so resolve getBtcCoin first. // One extra sequential roundtrip vs the previous Promise.all, judged negligible against the // eliminated transfer volume of the full message JSON. const btcAsset = await this.assetService.getBtcCoin(); - const summaries = await this.logService.getFinancialLogSummaries(btcAsset?.id, from, dailySample); + const summaries = await this.logService.getFinancialLogSummaries( + btcAsset?.id, + from, + dailySample, + undefined, + undefined, + undefined, + includeByType, + ); const entries = summaries.map((summary) => this.mapSummaryToEntry(summary)); return { entries }; @@ -255,7 +263,7 @@ export class DashboardFinancialService { minusBalanceChf: summary.minusBalanceChf ?? 0, fxPnlChf: summary.fxPnlChf ?? 0, btcPriceChf: summary.btcPriceChf, - balancesByType: summary.balancesByType, + ...(summary.balancesByType !== undefined ? { balancesByType: summary.balancesByType } : {}), }; } } diff --git a/src/subdomains/supporting/dashboard/dto/financial-log.dto.ts b/src/subdomains/supporting/dashboard/dto/financial-log.dto.ts index b53f4fb8c3..8ea95f8fa7 100644 --- a/src/subdomains/supporting/dashboard/dto/financial-log.dto.ts +++ b/src/subdomains/supporting/dashboard/dto/financial-log.dto.ts @@ -11,8 +11,13 @@ export class FinancialLogEntryDto { * plusBalanceChf/minusBalanceChf can be missing per type when the source FinancialDataLog * snapshot omitted one of the two keys (see FinancialLogSummary.balancesByType); a missing * value is left out of the JSON response the same way it always was, never defaulted to 0. + * + * The whole field is only present when the caller did not opt out via the `byType` query + * parameter on GET /v1/dashboard/financial/log (`byType=false`); when opted out it is omitted + * entirely from the response — never an empty object, never null — because it makes up 82% of + * the payload and the Overview screen that calls this endpoint on every refresh never reads it. */ - balancesByType: Record; + balancesByType?: Record; } export class FinancialLogResponseDto { diff --git a/src/subdomains/supporting/log/__tests__/log.repository.spec.ts b/src/subdomains/supporting/log/__tests__/log.repository.spec.ts index 61c6ea1496..f962ddb2b0 100644 --- a/src/subdomains/supporting/log/__tests__/log.repository.spec.ts +++ b/src/subdomains/supporting/log/__tests__/log.repository.spec.ts @@ -691,5 +691,96 @@ describe('LogRepository', () => { // Wrong-typed values become undefined and are dropped on serialisation — never null/0/string/boolean. expect(JSON.parse(JSON.stringify(rows[0].balancesByType.Crypto))).toEqual({}); }); + + it('omits balancesByFinancialType from the SELECT list when includeByType is explicitly false (the actual DB-time and payload-size saving)', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + + await repo.getFinancialLogSummaries(7, undefined, undefined, undefined, undefined, undefined, false); + + const [sql] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).not.toContain('balancesByFinancialType'); + }); + + it('still selects and returns balancesByType when includeByType is not passed at all (backward compatibility)', async () => { + const repo = new LogRepository({} as EntityManager); + const created = new Date('2026-07-14T00:00:00Z'); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([ + { + created, + id: 1, + totalBalanceChf: 100, + plusBalanceChf: 100, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: 0, + balancesByFinancialType: { Crypto: { plusBalanceChf: 10, minusBalanceChf: 5 } }, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(7); + + const [sql] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain(`message::jsonb -> 'balancesByFinancialType' AS "balancesByFinancialType"`); + expect(rows[0].balancesByType).toEqual({ Crypto: { plusBalanceChf: 10, minusBalanceChf: 5 } }); + }); + + it('omits the balancesByType key entirely (not undefined-valued, not an empty object) from the mapped summary when includeByType is false', async () => { + const repo = new LogRepository({} as EntityManager); + jest.spyOn(repo, 'query').mockResolvedValue([ + { + created: new Date('2026-07-14T00:00:00Z'), + id: 1, + totalBalanceChf: 100, + plusBalanceChf: 100, + minusBalanceChf: 0, + fxPnlChf: null, + btcPriceChf: 0, + }, + ]); + + const rows = await repo.getFinancialLogSummaries( + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + false, + ); + + expect(rows).toHaveLength(1); + expect('balancesByType' in rows[0]).toBe(false); + }); + + it('keeps the same $N placeholder positions when includeByType=false is combined with every other optional parameter (btcAssetId, from, to, after, limit)', async () => { + const repo = new LogRepository({} as EntityManager); + const querySpy = jest.spyOn(repo, 'query').mockResolvedValue([]); + jest.spyOn(repo, 'createQueryBuilder').mockReturnValue(financialLogQueryBuilderStub(true) as never); + const from = new Date('2026-01-01T00:00:00Z'); + const to = new Date('2026-02-01T00:00:00Z'); + + await repo.getFinancialLogSummaries(7, from, false, to, 50, 10, false); + + const [sql, params] = querySpy.mock.calls[0] as [string, unknown[]]; + expect(sql).toContain('$5::text'); + expect(sql).toContain('created >= $6'); + expect(sql).toContain('created <= $7'); + expect(sql).toContain('(created, id) > ((SELECT c.created FROM log c WHERE c.id = $8), $9)'); + expect(sql).toContain('LIMIT $10'); + expect(sql).not.toContain('balancesByFinancialType'); + expect(params).toEqual([ + 'LogService', + FINANCIAL_DATA_LOG_SUBSYSTEM, + LogSeverity.INFO, + true, + '7', + from, + to, + 10, + 10, + 50, + ]); + }); }); }); diff --git a/src/subdomains/supporting/log/__tests__/log.service.spec.ts b/src/subdomains/supporting/log/__tests__/log.service.spec.ts index 8c06347f83..366850d949 100644 --- a/src/subdomains/supporting/log/__tests__/log.service.spec.ts +++ b/src/subdomains/supporting/log/__tests__/log.service.spec.ts @@ -288,9 +288,17 @@ describe('LogService', () => { ]; const spy = jest.spyOn(logRepo, 'getFinancialLogSummaries').mockResolvedValue(summaries); - await expect(service.getFinancialLogSummaries(7, from, true, to, 25, 10)).resolves.toEqual(summaries); + await expect(service.getFinancialLogSummaries(7, from, true, to, 25, 10, false)).resolves.toEqual(summaries); - expect(spy).toHaveBeenCalledWith(7, from, true, to, 25, 10); + expect(spy).toHaveBeenCalledWith(7, from, true, to, 25, 10, false); + }); + + it('forwards includeByType as undefined when the caller omits it, so the repository default (true) applies', async () => { + const spy = jest.spyOn(logRepo, 'getFinancialLogSummaries').mockResolvedValue([]); + + await service.getFinancialLogSummaries(7); + + expect(spy).toHaveBeenCalledWith(7, undefined, undefined, undefined, undefined, undefined, undefined); }); }); diff --git a/src/subdomains/supporting/log/log.repository.ts b/src/subdomains/supporting/log/log.repository.ts index 79f1becb03..b927ade16b 100644 --- a/src/subdomains/supporting/log/log.repository.ts +++ b/src/subdomains/supporting/log/log.repository.ts @@ -71,8 +71,14 @@ export interface FinancialLogSummary { /** * plusBalanceChf/minusBalanceChf can be `undefined` per type: a real production row had a * `balancesByFinancialType` entry missing one of the two keys (see getFinancialLogSummaries below). + * + * The whole property is only present when getFinancialLogSummaries' `includeByType` parameter is + * true (the default, for backward compatibility); when explicitly false the key is absent (not an + * empty object) because the underlying `balancesByFinancialType` jsonb sub-tree was never selected + * from the database in the first place — that omission from the SELECT list, not a post-hoc + * discard, is the actual DB-time/payload saving. */ - balancesByType: Record; + balancesByType?: Record; } @Injectable() @@ -396,6 +402,10 @@ ORDER BY l.created ASC, l.id ASC`; to?: Date, limit?: number, after?: number, // id of the last row of the previous page; NEVER a Date/created value + includeByType = true, // selects/omits the balancesByFinancialType sub-tree from the SELECT list; + // true (the default) reproduces the exact pre-existing response for every caller that does not + // pass this parameter; only an explicit false skips the sub-tree. This default is intentional and + // required by this spec's backward-compatibility guarantee — it is not masking an error case. ): Promise { const params: unknown[] = []; let i = 1; @@ -463,31 +473,39 @@ ORDER BY l.created ASC, l.id ASC`; params.push(limit); } - const sql = ` -SELECT created AS "created", - id AS "id", - CASE + const selectColumns = [ + `created AS "created"`, + `id AS "id"`, + `CASE WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'totalBalanceChf') = 'number' THEN (message::jsonb -> 'balancesTotal' ->> 'totalBalanceChf')::float8 ELSE NULL - END AS "totalBalanceChf", - CASE + END AS "totalBalanceChf"`, + `CASE WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'plusBalanceChf') = 'number' THEN (message::jsonb -> 'balancesTotal' ->> 'plusBalanceChf')::float8 ELSE NULL - END AS "plusBalanceChf", - CASE + END AS "plusBalanceChf"`, + `CASE WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'minusBalanceChf') = 'number' THEN (message::jsonb -> 'balancesTotal' ->> 'minusBalanceChf')::float8 ELSE NULL - END AS "minusBalanceChf", - CASE + END AS "minusBalanceChf"`, + `CASE WHEN jsonb_typeof(message::jsonb -> 'balancesTotal' -> 'fxPnlChf') = 'number' THEN (message::jsonb -> 'balancesTotal' ->> 'fxPnlChf')::float8 ELSE NULL - END AS "fxPnlChf", - ${btcPriceSelect} AS "btcPriceChf", - message::jsonb -> 'balancesByFinancialType' AS "balancesByFinancialType" + END AS "fxPnlChf"`, + `${btcPriceSelect} AS "btcPriceChf"`, + ]; + // The actual DB-time/payload saving: when not requested, this sub-tree is never in the SELECT + // list at all (not selected and then discarded after the fact). + if (includeByType) { + selectColumns.push(`message::jsonb -> 'balancesByFinancialType' AS "balancesByFinancialType"`); + } + + const sql = ` +SELECT ${selectColumns.join(',\n ')} FROM log WHERE ${conditions.join(' AND ')} ORDER BY created ASC, id ASC @@ -501,7 +519,7 @@ ${limitClause}`; minusBalanceChf: number | string | null; fxPnlChf: number | string | null; btcPriceChf: number | string | null; - balancesByFinancialType: unknown; + balancesByFinancialType?: unknown; }[]; const rows: FinancialLogSummary[] = raw.map((r) => { @@ -512,35 +530,41 @@ ${limitClause}`; // btcPriceChf: absent/unusable path → 0, matching extractBtcPrice's `?.priceChf ?? 0`. const btcPriceChf = r.btcPriceChf == null ? 0 : Number(r.btcPriceChf); - const balancesByType: Record = {}; - if (r.balancesByFinancialType != null) { - // Always an already-parsed object/array here, never a JSON string: pg-types registers JSON.parse - // as the type parser for jsonb (OID 3802) and this repo configures no custom type parser, so the - // driver never hands back a raw string for this column. - const byType = r.balancesByFinancialType as Record< - string, - { plusBalanceChf?: number; minusBalanceChf?: number } - >; - // Only real numbers are kept for plusBalanceChf / minusBalanceChf; any non-number value - // (string, boolean, null, nested object, or missing key) becomes undefined so the result - // matches the number | undefined contract. On current production data this is a no-op - // (287,989 entries both numbers, one missing plusBalanceChf key — no string/boolean/null), - // and exists only to protect the contract for future/other data. The previous mapLogToEntry - // passed contract-breaking values through unchanged; this closes that hole. Same hardening - // idea as the five scalar fields above (jsonb_typeof = 'number' in SQL), applied in - // TypeScript because balancesByFinancialType is passed through as a raw JSON object. Note: - // this is a type check, not a finiteness check — it also lets `Infinity` through (e.g. from - // a JSON number like `1e999`, which `JSON.parse` turns into `Infinity`); `NaN` cannot occur - // in valid jsonb. - const asNumber = (v: unknown): number | undefined => (typeof v === 'number' ? v : undefined); - for (const [type, data] of Object.entries(byType)) { - // Optional chaining keeps non-object entries (null / number / string / boolean) from throwing: - // property access yields undefined and the row is retained with empty fields, rather than - // failing the whole request. - balancesByType[type] = { - plusBalanceChf: asNumber(data?.plusBalanceChf), - minusBalanceChf: asNumber(data?.minusBalanceChf), - }; + // Only computed/present at all when includeByType is true (see the SELECT-list construction + // above): the key is entirely absent on the returned summary otherwise (conditional spread + // below), not an empty object and not null. + let balancesByType: Record | undefined; + if (includeByType) { + balancesByType = {}; + if (r.balancesByFinancialType != null) { + // Always an already-parsed object/array here, never a JSON string: pg-types registers JSON.parse + // as the type parser for jsonb (OID 3802) and this repo configures no custom type parser, so the + // driver never hands back a raw string for this column. + const byType = r.balancesByFinancialType as Record< + string, + { plusBalanceChf?: number; minusBalanceChf?: number } + >; + // Only real numbers are kept for plusBalanceChf / minusBalanceChf; any non-number value + // (string, boolean, null, nested object, or missing key) becomes undefined so the result + // matches the number | undefined contract. On current production data this is a no-op + // (287,989 entries both numbers, one missing plusBalanceChf key — no string/boolean/null), + // and exists only to protect the contract for future/other data. The previous mapLogToEntry + // passed contract-breaking values through unchanged; this closes that hole. Same hardening + // idea as the five scalar fields above (jsonb_typeof = 'number' in SQL), applied in + // TypeScript because balancesByFinancialType is passed through as a raw JSON object. Note: + // this is a type check, not a finiteness check — it also lets `Infinity` through (e.g. from + // a JSON number like `1e999`, which `JSON.parse` turns into `Infinity`); `NaN` cannot occur + // in valid jsonb. + const asNumber = (v: unknown): number | undefined => (typeof v === 'number' ? v : undefined); + for (const [type, data] of Object.entries(byType)) { + // Optional chaining keeps non-object entries (null / number / string / boolean) from throwing: + // property access yields undefined and the row is retained with empty fields, rather than + // failing the whole request. + balancesByType[type] = { + plusBalanceChf: asNumber(data?.plusBalanceChf), + minusBalanceChf: asNumber(data?.minusBalanceChf), + }; + } } } @@ -552,7 +576,7 @@ ${limitClause}`; minusBalanceChf: r.minusBalanceChf == null ? null : Number(r.minusBalanceChf), fxPnlChf: r.fxPnlChf == null ? null : Number(r.fxPnlChf), btcPriceChf, - balancesByType, + ...(includeByType ? { balancesByType } : {}), }; }); diff --git a/src/subdomains/supporting/log/log.service.ts b/src/subdomains/supporting/log/log.service.ts index da29c6fa5f..de207e4126 100644 --- a/src/subdomains/supporting/log/log.service.ts +++ b/src/subdomains/supporting/log/log.service.ts @@ -161,8 +161,9 @@ export class LogService { to?: Date, limit?: number, after?: number, // id of the last row of the previous page; NEVER a Date/created value + includeByType?: boolean, ): Promise { - return this.logRepo.getFinancialLogSummaries(btcAssetId, from, dailySample, to, limit, after); + return this.logRepo.getFinancialLogSummaries(btcAssetId, from, dailySample, to, limit, after, includeByType); } async getLatestFinancialLog(): Promise {