refactor(common): one shared comparator instead of three inline copies

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.
This commit is contained in:
Maurice
2026-08-25 19:14:29 +02:00
committed by Maurice
parent 071f516c0f
commit 62b13a278e
4 changed files with 67 additions and 3 deletions
+19
View File
@@ -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);
+2 -1
View File
@@ -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<T extends Record<string, unknown>>(
if (LOCKED.has(key)) blocked.push(key);
else (allowed as Record<string, unknown>)[key] = value;
}
return { allowed, blocked: blocked.sort((a, b) => (a < b ? -1 : a > b ? 1 : 0)) };
return { allowed, blocked: blocked.sort(byCodeUnit) };
}
@@ -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>([
RequestMethod.POST,
@@ -86,14 +87,14 @@ export function validateBodyContracts(
problems.push(
`${violations.length} mutation handler(s) read @Body() without a createZodDto class ` +
`(add a <domain>.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) {
+43
View File
@@ -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']);
});
});