fix(rest)!: /import reads a comma in a number cell only as a thousands group, refusing the rest (#20497)

.changeset/20497-import-number-thousands-group.md

@@ -0,0 +1,47 @@
+---
+'@objectstack/rest': minor
+---
+
+fix(rest): `POST /api/v1/data/:object/import` reads a comma in a number cell only as a thousands group, and refuses every other comma instead of storing a different number (#20497)
+
+Clause-②: no (narrowing)
+
+**BREAKING for callers of the import door.** A cell for a numeric field
+(`number`, `currency`, `percent` and the other numeric value types) that
+carries a comma is now admitted only when the comma groups thousands: 1 to 3
+leading digits, then groups of exactly three digits, and only before any `.`
+(`1,000`, `12,345.67`, `-1,234,567.89`). Any other
+comma makes the cell that row's `invalid_number` error, the same code the
+plain write doors (create, update, batch) already answer for the cell. The
+reader used to strip every comma before parsing, so each of these was stored
+as a DIFFERENT number while the import reported `ok 1, errors 0`. For each
+shape, change the cell FROM the refused spelling TO the one that says what you
+mean:
+
+- **A decimal comma.** FROM `3,14`, `1,5`, `0,5`, `1.000,5` (stored as `314`,
+  `15`, `5`, `1.0005`) TO a `.` decimal point with no grouping, or with comma
+  grouping: `3.14`, `1.5`, `0.5`, `1000.5` or `1,000.5`. A file exported with a
+  decimal-comma locale has to be converted before import. No locale is
+  guessed: `1,500` is always one thousand five hundred.
+- **A comma that does not group thousands.** FROM `1,2,3`, `1,23`, `1,0000`,
+  `1234,567`, `,123`, `1,000,` or a comma after the `.` (`12,345.6,7`), each
+  stored with its commas removed, TO the number with no separators, or with
+  well-formed thousands grouping.
+- **Grouping by twos (`12,34,567`, `1,00,000`).** FROM that grouping TO
+  `1234567` / `100000`, or `1,234,567` / `100,000`. These used to import as
+  the number they denote. They are refused now because the reader cannot tell
+  a two-digit group from a decimal comma, and it no longer guesses.
+
+**What is not affected.** Every cell without a comma reads exactly as before.
+That is measured over the platform numeric grammar's 41 case rows: the only
+row whose import reading changed is `1.000,5`. A well-formed thousands grouping
+reads as before, with or without a leading currency symbol, a trailing `%`, a
+sign or accounting parentheses (`$1,000`, `1,234%`, `(1,234)`). A JSON number,
+and an xlsx cell Excel stores as a number, never pass through this reading.
+The dry run answers the same verdicts as the real write. A refused cell fails
+only its own row, and the rest of the batch imports as before.
+
+**If you are refused.** The row's result carries `code: 'invalid_number'` and
+quotes the cell, so the file can be corrected and re-imported.
+
+<!-- adr-0087: not-required (no-migration-prescription) Nothing authorable is removed, renamed or reshaped: no spec key, no export, no stored row. The import door's cell reader accepts fewer spellings of a number inside an imported file, which is data, not metadata, so `objectstack migrate meta` has nothing to reach. The row's invalid_number error quotes the refused cell, and the repair is to write the number with a `.` decimal point. -->

packages/rest/src/import-coerce.test.ts

@@ -49,6 +49,36 @@ describe('parseNumberCell', () => {
     expect(parseNumberCell('12x3')).toBeUndefined();
     expect(parseNumberCell('')).toBeUndefined();
   });
+
+  // [#20497] A comma is read only where it groups thousands: 1 to 3 leading
+  // digits, then groups of exactly three, and only before any `.`.
+  it.each([
+    ['1,000', 1000],
+    ['12,345.67', 12345.67],
+    ['1,234,567.89', 1234567.89],
+    ['-1,234', -1234],
+    ['(1,234)', -1234],
+    ['$1,000', 1000],
+    ['1,234%', 1234],
+    ['1,000e3', 1_000_000],
+  ])('admits the well-formed thousands grouping %j as %s', (cell, n) => {
+    expect(parseNumberCell(cell)).toBe(n);
+  });
+
+  it.each([
+    // The card's four cells: stored as 314, 15, 1.0005 and 123 before.
+    '3,14', '1,5', '1.000,5', '1,2,3',
+    // A decimal comma, however it is dressed.
+    '0,5', '(3,14)', '$1,5', '1,5%',
+    // A group that is not exactly three digits, or a comma out of place.
+    '1,23', '1,0000', '1234,567', ',123', '-,123', '1,000,', '.5,000',
+    // A comma after the `.`.
+    '12,345.6,7',
+    // A grouping other than thousands — no locale is guessed.
+    '12,34,567', '1,00,000',
+  ])('refuses %j rather than reading it as some other number', (cell) => {
+    expect(parseNumberCell(cell)).toBeUndefined();
+  });
 });
 
 describe('parseDateCell', () => {

packages/rest/src/import-coerce.ts

@@ -218,12 +218,33 @@ export function parseBooleanCell(raw: unknown): boolean | undefined {
 
 // ── numbers ────────────────────────────────────────────────────────
 
+/**
+ * The one comma placement a numeric cell may carry: a thousands grouping of the
+ * integer part — an optional sign, 1 to 3 leading digits, then one or more
+ * groups of exactly three digits, ending the integer part (end of cell, `.` or
+ * the exponent marker follows) — and no comma anywhere after it. `1,000`,
+ * `12,345.67`, `-1,234,567` match; `3,14`, `1,5`, `1.000,5`, `1,2,3`,
+ * `1234,567`, `1,0000` and `12,345.6,7` do not.
+ */
+const THOUSANDS_GROUPED_INTEGER = /^[+-]?\d{1,3}(?:,\d{3})+(?![\d,])[^,]*$/;
+
 /**
  * Parse a numeric cell, tolerating the punctuation spreadsheets add: thousands
  * separators (`1,234`), a leading currency symbol (`$` `¥` `€` `£` `￥`), a
  * trailing percent sign (`25%` → `25`), and accounting-style parenthesised
  * negatives (`(1,234)` → `-1234`). Returns `undefined` when the residue is not
  * a finite number.
+ *
+ * A comma is a thousands separator and nothing else, and only where it groups
+ * thousands (#20497): 1 to 3 leading digits, then groups of exactly three,
+ * and only before any `.` (`1,000`, `12,345.67`) — see
+ * {@link THOUSANDS_GROUPED_INTEGER}. Any other comma makes the cell
+ * unparseable (`undefined`, so the row's `invalid_number` error), never a
+ * different number: a decimal-comma spelling (`3,14`, `1,5`, `1.000,5`) or a
+ * stray comma (`1,2,3`) used to have every comma stripped and was stored as
+ * `314`, `15`, `1.0005`, `123`. No locale is guessed — `1,500` is always one
+ * thousand five hundred, never 1.5 — so a file written with a decimal comma is
+ * refused cell by cell rather than read as some other value.
  */
 export function parseNumberCell(raw: unknown): number | undefined {
   if (typeof raw === 'number') return Number.isFinite(raw) ? raw : undefined;
@@ -233,7 +254,10 @@ export function parseNumberCell(raw: unknown): number | undefined {
   if (/^\(.*\)$/.test(s)) { negative = true; s = s.slice(1, -1).trim(); }
   s = s.replace(/^[$¥€£￥]\s*/, '');   // leading currency symbol
   s = s.replace(/%$/, '').trim();       // trailing percent
-  s = s.replace(/,/g, '');              // thousands separators
+  if (s.includes(',')) {
+    if (!THOUSANDS_GROUPED_INTEGER.test(s)) return undefined;
+    s = s.replace(/,/g, '');            // a well-formed thousands grouping
+  }
   if (s === '' || !/^[+-]?\d*\.?\d+(e[+-]?\d+)?$/i.test(s)) return undefined;
   const n = Number(s);
   if (!Number.isFinite(n)) return undefined;

packages/rest/src/import-number-thousands-group.test.ts

@@ -0,0 +1,187 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#20497] `POST /api/v1/data/:object/import` reads a comma in a number cell
+ * as a thousands separator ONLY where it groups thousands — 1 to 3 leading
+ * digits, then groups of exactly three, and only before any `.` — and refuses
+ * every other comma as that row's `invalid_number` error, through the real
+ * route over a real `SqlDriver` (better-sqlite3 `:memory:`).
+ *
+ * Measured through this route, JSON rows, `writeMode: 'insert'`, on
+ * `InMemoryDriver` and on `SqlDriver` (better-sqlite3) alike, at the base
+ * (`9449512a31`) and at the head of the PR that landed this file:
+ *
+ * | cell | base: stored · answer | head |
+ * |:--|:--|:--|
+ * | `'3,14'` | `314` · ok 1, errors 0 | row refused, `invalid_number`, nothing stored |
+ * | `'1,5'` | `15` · ok 1, errors 0 | row refused, `invalid_number`, nothing stored |
+ * | `'1.000,5'` | `1.0005` · ok 1, errors 0 | row refused, `invalid_number`, nothing stored |
+ * | `'1,2,3'` | `123` · ok 1, errors 0 | row refused, `invalid_number`, nothing stored |
+ * | `'1,000'` / `'12,345.67'` / `'(1,234)'` | `1000` / `12345.67` / `-1234` | unchanged |
+ *
+ * The `InMemoryDriver` row is this file's by construction, not by a second
+ * arm: the cell is judged by the import's own reader (`parseNumberCell`)
+ * before any driver is reached, so one verdict holds on every driver. A test
+ * import of `@objectstack/driver-memory` is also not this file's to add — its
+ * test consumers are a ruled, ledgered set
+ * (`scripts/driver-memory-census.ledger.json`, `pnpm check:driver-memory-census`).
+ *
+ * The plain create door already answered `400 VALIDATION_FAILED` with field
+ * code `invalid_number` for each of the four; the import row now answers the
+ * same code. The reader's own case table (every documented form, the comma
+ * probes) is `import-coerce.test.ts`'s `parseNumberCell` block; this file pins
+ * the door.
+ */
+
+import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import { ObjectQL } from '@objectstack/objectql';
+import { SqlDriver } from '@objectstack/driver-sql';
+import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
+import { RestServer } from './rest-server';
+
+const OBJECT = 'import_thousands_20497';
+
+const LEDGER = {
+  name: OBJECT, label: 'Ledger 20497', systemFields: false,
+  fields: {
+    id: { name: 'id', type: 'text' as const, primaryKey: true },
+    amount: { name: 'amount', type: 'number' as const, label: 'Amount' },
+  },
+};
+
+/** The card's four cells: each used to be stored as a different number. */
+const REFUSED = ['3,14', '1,5', '1.000,5', '1,2,3'] as const;
+
+/** The admitted controls: a well-formed thousands grouping keeps its reading. */
+const ADMITTED: ReadonlyArray<readonly [cell: string, stored: number]> = [
+  ['1,000', 1000],
+  ['12,345.67', 12345.67],
+  ['(1,234)', -1234],
+];
+
+function makeSqliteDriver() {
+  return new SqlDriver({
+    client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true,
+  });
+}
+
+function createMockServer() {
+  const noop = () => {};
+  return { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} };
+}
+
+function makeRes() {
+  const res: any = {
+    write: () => true, end: () => {},
+    header: () => res,
+    status: (code: number) => { res._status = code; return res; },
+    json: (body: any) => { res._json = body; return res; },
+  };
+  return res;
+}
+
+const liveEngines: ObjectQL[] = [];
+afterEach(async () => {
+  while (liveEngines.length) {
+    try { await liveEngines.pop()?.destroy(); } catch { /* noop */ }
+  }
+});
+
+async function boot() {
+  const engine = new ObjectQL();
+  liveEngines.push(engine);
+  engine.registerDriver(makeSqliteDriver(), true);
+  await engine.init();
+  engine.registry.registerObject(LEDGER as any);
+  await engine.syncSchemas();
+  const protocol = new ObjectStackProtocolImplementation(engine as any);
+  const rest = new RestServer(createMockServer() as any, protocol as any, { api: { requireAuth: false } } as any);
+  (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' });
+  rest.registerRoutes();
+  const find = (method: string, path: string) =>
+    rest.getRoutes().find((r: any) => r.method === method && r.path === path) as any;
+  const importRoute = find('POST', '/api/v1/data/:object/import');
+  const createRoute = find('POST', '/api/v1/data/:object');
+  expect(importRoute).toBeDefined();
+  expect(createRoute).toBeDefined();
+  const send = async (route: any, body: unknown) => {
+    const res = makeRes();
+    await route.handler({ params: { object: OBJECT }, body } as any, res);
+    return res;
+  };
+  return {
+    engine,
+    importRows: (body: Record<string, unknown>) => send(importRoute, body),
+    create: (body: Record<string, unknown>) => send(createRoute, body),
+  };
+}
+
+describe('[#20497] /import — a comma is read only as a thousands group', () => {
+  let ctx: Awaited<ReturnType<typeof boot>>;
+  beforeEach(async () => { ctx = await boot(); });
+
+  it.each(REFUSED)('refuses %j as that row\'s invalid_number error and writes its sibling row', async (cell) => {
+    const res = await ctx.importRows({
+      format: 'json', writeMode: 'insert',
+      rows: [{ id: 'bad', amount: cell }, { id: 'good', amount: 7 }],
+    });
+
+    expect(res._status ?? 200).toBe(200);
+    expect(res._json).toMatchObject({ total: 2, ok: 1, errors: 1, created: 1 });
+    expect(res._json.results[0]).toMatchObject({
+      row: 1, ok: false, action: 'failed', field: 'amount', code: 'invalid_number',
+    });
+    // Refused, not stored as some other number.
+    expect(await ctx.engine.findOne(OBJECT, { where: { id: 'bad' } })).toBeNull();
+    expect((await ctx.engine.findOne(OBJECT, { where: { id: 'good' } }))?.amount).toBe(7);
+  });
+
+  it.each(ADMITTED)('admits the thousands grouping %j and stores %s', async (cell, stored) => {
+    const res = await ctx.importRows({
+      format: 'json', writeMode: 'insert',
+      rows: [{ id: 'r', amount: cell }],
+    });
+
+    expect(res._json).toMatchObject({ total: 1, ok: 1, errors: 0, created: 1 });
+    expect(res._json.results[0]).toMatchObject({ row: 1, ok: true, action: 'created' });
+    expect((await ctx.engine.findOne(OBJECT, { where: { id: 'r' } }))?.amount).toBe(stored);
+  });
+
+  it('gives the same verdicts to quoted CSV cells', async () => {
+    const csv = [
+      'ID,Amount',
+      ...REFUSED.map((cell, i) => `r${i},"${cell}"`),
+      ...ADMITTED.map(([cell], j) => `c${j},"${cell}"`),
+    ].join('\n');
+    const res = await ctx.importRows({ format: 'csv', csv, writeMode: 'insert', mapping: { ID: 'id', Amount: 'amount' } });
+
+    expect(res._json).toMatchObject({ total: REFUSED.length + ADMITTED.length, ok: ADMITTED.length, errors: REFUSED.length });
+    const failedFields = res._json.results.filter((r: any) => !r.ok).map((r: any) => [r.field, r.code]);
+    expect(failedFields).toEqual(REFUSED.map(() => ['amount', 'invalid_number']));
+    for (const [j, [, stored]] of ADMITTED.entries()) {
+      expect((await ctx.engine.findOne(OBJECT, { where: { id: `c${j}` } }))?.amount).toBe(stored);
+    }
+  });
+
+  it('dry run predicts the same refusals and persists nothing', async () => {
+    const res = await ctx.importRows({
+      format: 'json', writeMode: 'insert', dryRun: true,
+      rows: REFUSED.map((amount, i) => ({ id: `d${i}`, amount })),
+    });
+
+    expect(res._json).toMatchObject({ dryRun: true, total: REFUSED.length, ok: 0, errors: REFUSED.length });
+    for (const r of res._json.results) expect(r).toMatchObject({ ok: false, field: 'amount', code: 'invalid_number' });
+    for (const [i] of REFUSED.entries()) {
+      expect(await ctx.engine.findOne(OBJECT, { where: { id: `d${i}` } })).toBeNull();
+    }
+  });
+
+  it('answers the code the plain create door answers for the same cells', async () => {
+    for (const [i, amount] of REFUSED.entries()) {
+      const res = await ctx.create({ id: `p${i}`, amount });
+      expect(res._status).toBe(400);
+      expect(res._json).toMatchObject({ code: 'VALIDATION_FAILED' });
+      expect(res._json.fields[0]).toMatchObject({ field: 'amount', code: 'invalid_number' });
+    }
+  });
+});