From 62b13a278e91174be689712568df84f8d5c3dcfc Mon Sep 17 00:00:00 2001 From: Maurice Date: Tue, 25 Aug 2026 19:14:29 +0200 Subject: [PATCH] refactor(common): one shared comparator instead of three inline copies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The coverage ratchet caught something worth keeping: an inline `(a, b) => (a < b ? -1 : a > b ? 1 : 0)` is a new function with four branches at every call site, and two of the three in src/nest/common sit on error paths that a passing test never walks. Three restatements of the default sort order were enough to pull the domain under its pinned thresholds (functions 97.8% against 99, branches 87.92% against 89). Writing tests to walk those error paths would have been solving the wrong problem. byCodeUnit says the same thing once, is covered once, and the callers read better for it. Its test asserts equality with a bare `.sort()` rather than hand-written expectations, since "identical to the default order" is the whole contract. It also pins what byCodeUnit is NOT: `['Zelt', 'Übernachtung']` keeps that order, where localeCompare swaps it — so the next person reaching for a comparator on a list a human reads picks the other one. --- server/src/nest/common/compare.ts | 19 ++++++++ server/src/nest/common/managed.ts | 3 +- .../nest/common/validate-body-contracts.ts | 5 ++- server/tests/unit/nest/compare.test.ts | 43 +++++++++++++++++++ 4 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 server/src/nest/common/compare.ts create mode 100644 server/tests/unit/nest/compare.test.ts diff --git a/server/src/nest/common/compare.ts b/server/src/nest/common/compare.ts new file mode 100644 index 000000000..89dd0e8b7 --- /dev/null +++ b/server/src/nest/common/compare.ts @@ -0,0 +1,19 @@ +/** + * The comparator a bare `.sort()` already applies, written out. + * + * `Array.prototype.sort()` with no argument compares the UTF-16 code units of + * each element's string form. For ISO dates, ASCII keys, currency codes and the + * like that is the order the code wants, but Sonar's S2871 flags the bare call + * because the same syntax silently mis-sorts numbers ([10, 9, 1] becomes + * [1, 10, 9]). + * + * Shared rather than inlined at each call site: an inline arrow is a new + * function on every sort, and the ones that live on error paths are never + * executed by a passing test — which drags a whole domain's coverage down for a + * comparator that does nothing but restate the default. One exported function, + * covered once. + * + * NOT for user-visible names. Code-unit order files every accented word behind + * the entire ASCII range, so a list a person reads wants `localeCompare`. + */ +export const byCodeUnit = (a: string, b: string): number => (a < b ? -1 : a > b ? 1 : 0); diff --git a/server/src/nest/common/managed.ts b/server/src/nest/common/managed.ts index fd99c0198..0f216e9fd 100644 --- a/server/src/nest/common/managed.ts +++ b/server/src/nest/common/managed.ts @@ -1,5 +1,6 @@ import { SetMetadata } from '@nestjs/common'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; +import { byCodeUnit } from './compare'; /** Metadata key `@ManagedForbidden()` writes. */ export const MANAGED_FORBIDDEN = 'trek:managed-forbidden'; @@ -166,5 +167,5 @@ export function splitManagedKeys>( if (LOCKED.has(key)) blocked.push(key); else (allowed as Record)[key] = value; } - return { allowed, blocked: blocked.sort((a, b) => (a < b ? -1 : a > b ? 1 : 0)) }; + return { allowed, blocked: blocked.sort(byCodeUnit) }; } diff --git a/server/src/nest/common/validate-body-contracts.ts b/server/src/nest/common/validate-body-contracts.ts index b03255d9c..81b1f4f44 100644 --- a/server/src/nest/common/validate-body-contracts.ts +++ b/server/src/nest/common/validate-body-contracts.ts @@ -5,6 +5,7 @@ import { RouteParamtypes } from '@nestjs/common/enums/route-paramtypes.enum'; import { ModulesContainer } from '@nestjs/core'; import { isZodDto } from 'nestjs-zod/dto'; import { BODY_CONTRACT_ALLOW_LIST } from './body-contract-allow-list'; +import { byCodeUnit } from './compare'; const MUTATION_METHODS = new Set([ RequestMethod.POST, @@ -86,14 +87,14 @@ export function validateBodyContracts( problems.push( `${violations.length} mutation handler(s) read @Body() without a createZodDto class ` + `(add a .dto.ts wrapper over the @trek/shared schema — do NOT extend the allow-list): ` + - violations.sort((a, b) => (a < b ? -1 : a > b ? 1 : 0)).join(', '), + violations.sort(byCodeUnit).join(', '), ); } if (stale.length) { problems.push( `stale body-contract allow-list entr${stale.length === 1 ? 'y' : 'ies'} ` + `(route removed or now validated — delete from body-contract-allow-list.ts): ` + - stale.sort((a, b) => (a < b ? -1 : a > b ? 1 : 0)).join(', '), + stale.sort(byCodeUnit).join(', '), ); } if (problems.length) { diff --git a/server/tests/unit/nest/compare.test.ts b/server/tests/unit/nest/compare.test.ts new file mode 100644 index 000000000..ffcdb94b2 --- /dev/null +++ b/server/tests/unit/nest/compare.test.ts @@ -0,0 +1,43 @@ +/** + * byCodeUnit restates what a bare `.sort()` already did, so the contract is + * "identical to the default order" — and that is what these check, against the + * default rather than against hand-written expectations. + */ +import { describe, it, expect } from 'vitest'; +import { byCodeUnit } from '../../../src/nest/common/compare'; + +describe('byCodeUnit', () => { + it('CMP-001: reports less, greater and equal', () => { + expect(byCodeUnit('a', 'b')).toBe(-1); + expect(byCodeUnit('b', 'a')).toBe(1); + expect(byCodeUnit('a', 'a')).toBe(0); + }); + + it('CMP-002: orders ISO dates chronologically, which is why the callers can use it', () => { + expect(['2026-03-01', '2026-01-15', '2026-02-28'].sort(byCodeUnit)) + .toEqual(['2026-01-15', '2026-02-28', '2026-03-01']); + }); + + it('CMP-003: reproduces the default sort exactly for the ASCII the callers pass', () => { + const samples = [ + ['EUR', 'usd', 'CHF', 'JPY'], + ['b', 'A', 'a', 'B', ''], + ['plugin:one', 'plugin:One', 'plugin:1', 'plugin:'], + ['2026-01-01', '2026-1-1', '2026-01-1'], + ['x'], + [], + ]; + for (const sample of samples) { + expect([...sample].sort(byCodeUnit)).toEqual([...sample].sort()); + } + }); + + it('CMP-004: is NOT locale-aware — the reason the doc comment warns callers off names', () => { + // Code-unit order files every accented word behind the whole ASCII range. + // A list a person reads wants localeCompare instead; this asserts the + // difference so nobody reaches for byCodeUnit there by accident. + expect(['Zelt', 'Übernachtung'].sort(byCodeUnit)).toEqual(['Zelt', 'Übernachtung']); + expect(['Zelt', 'Übernachtung'].sort((a, b) => a.localeCompare(b, 'de'))) + .toEqual(['Übernachtung', 'Zelt']); + }); +});