Skip to content

Commit f234c67

Browse files
committed
fix(tables): refuse scale suffixes and resolve the lone-separator ambiguity
A verification pass over the previous commit found that my own fix opened a new hole of the class it closed. Widening the currency marker to 1-3 letters made scale suffixes parse: `1.2 M` read as 1.2, so a column of `1.2 M` / `3.4 M` — an ordinary spreadsheet paste — converted cleanly and rewrote every cell a millionfold too small. Before the widening those were rejected and the data was safe. Now refused explicitly, while `kr` and `zł` still parse despite starting with the same letters. Stripping the marker also newly routed formatted zero-decimal amounts into the lone-separator branch, where a single dot was always decimal: `1.235 ¥` read as 1.235 rather than 1235. That is a wrong number where there used to be a refusal, which is the worse failure. A lone separator followed by three digits is now resolved by two signals — a marker means a formatter produced it, and formatters group; a currency carrying three decimals (KWD, TND) reads them as decimals. `coerce` passes the column's code, so the parser can ask. Bare typed input keeps the decimal reading. Also: the unchanged-type throw fell through to a 500 for every type except currency, whose message happened to contain the word; it now maps to 400. And the last three comments describing the removed two-transaction architecture are gone — the previous commit claimed they were and two survived. Documented, and failing closed rather than guessing: markers written flush against the digits (`Rp12,00`) stay rejected, because a letter touching a digit is the only thing distinguishing a currency marker from a part number, and reading `SKU400` as 400 invents a value where refusing merely inconveniences.
1 parent ee0157d commit f234c67

6 files changed

Lines changed: 107 additions & 25 deletions

File tree

apps/sim/app/api/table/[tableId]/columns/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,8 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
323323
msg.includes('incompatible') ||
324324
msg.includes('duplicate') ||
325325
msg.includes('option') ||
326-
msg.includes('currency')
326+
msg.includes('currency') ||
327+
msg.includes('is already type')
327328
) {
328329
return NextResponse.json({ error: msg }, { status: 400 })
329330
}

apps/sim/app/api/v1/tables/[tableId]/columns/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,8 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
368368
msg.includes('incompatible') ||
369369
msg.includes('duplicate') ||
370370
msg.includes('option') ||
371-
msg.includes('currency')
371+
msg.includes('currency') ||
372+
msg.includes('is already type')
372373
) {
373374
return NextResponse.json({ error: msg }, { status: 400 })
374375
}

apps/sim/lib/table/__tests__/currency.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,29 @@ describe('parseCurrencyInput', () => {
160160
expect(parseCurrencyInput('CHF 1’234.56')).toBe(1234.56)
161161
})
162162

163+
it('resolves a lone separator using the marker and the currency', () => {
164+
// `1.235 ¥` is 1235 yen; a typed `1.235` is one-and-a-bit. A marker means a
165+
// formatter produced it, and formatters group — except for the currencies
166+
// that genuinely carry three decimals.
167+
expect(parseCurrencyInput('1.235 ¥', 'JPY')).toBe(1235)
168+
expect(parseCurrencyInput('0,500 KWD', 'KWD')).toBe(0.5)
169+
expect(parseCurrencyInput('12,000 TND', 'TND')).toBe(12)
170+
// Bare input keeps the typed reading.
171+
expect(parseCurrencyInput('1.234')).toBe(1.234)
172+
expect(parseCurrencyInput('1,500')).toBe(1500)
173+
})
174+
175+
it('refuses a scale suffix rather than shrinking the value', () => {
176+
// `1.2 M` read as 1.2 would rewrite a column of millions a millionfold too
177+
// small — the same invented-value failure as an identifier, inverted.
178+
expect(parseCurrencyInput('1.2 M')).toBeNull()
179+
expect(parseCurrencyInput('5 K')).toBeNull()
180+
expect(parseCurrencyInput('3.4 bn')).toBeNull()
181+
expect(parseCurrencyInput('10 B')).toBeNull()
182+
// `kr` is a currency marker, not a scale suffix, despite starting with k.
183+
expect(parseCurrencyInput('1 234,56 kr')).toBe(1234.56)
184+
})
185+
163186
it('rejects an identifier whose letters touch its digits', () => {
164187
// The distinguishing rule: a currency marker is always separated from the
165188
// number by a space or a symbol, so letters touching digits mean this is a

apps/sim/lib/table/column-types/currency.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,11 @@ export const currencyColumnType: ColumnTypeDefinition = {
2727
typeaheadPattern: /[\d.,\-\p{Sc}]/u,
2828
parseErrorMessage: 'Invalid amount',
2929

30-
coerce(value) {
30+
coerce(value, column) {
3131
// Stored as a bare number, but accepts the formatted shapes an amount
3232
// arrives in — `$1,234.56`, `1 234,56 €`, `(12.00)` — so a paste, CSV
3333
// import, or tool write lands as a number rather than being nulled.
34-
const parsed = parseCurrencyInput(value)
34+
const parsed = parseCurrencyInput(value, column.currencyCode)
3535
return parsed === null ? { ok: false } : { ok: true, value: parsed }
3636
},
3737

apps/sim/lib/table/columns/service.ts

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -749,15 +749,14 @@ export async function updateColumnType(
749749
// once the column is text/number/etc. Check compatibility against the option
750750
// NAME — that's what the cell will actually become (migrated below).
751751
const convertingAwayFromSelect = column.type === 'select' && !isSelectType
752-
// The constraint the column ends up with, which may be arriving in this same
753-
// request. `updateColumnConstraints` runs as its own transaction afterwards,
754-
// so validating against the column's CURRENT flag would let the conversion
755-
// commit and only then fail the constraint.
752+
// The constraint the column ends up with, which may be arriving in this
753+
// same request — this write applies it, so the scan below has to judge
754+
// against the target value rather than the current one.
756755
const targetRequired = !!(data.required ?? column.required)
757756

758757
// Rows missing the key (or holding null/`[]`) are filtered out of `rows`
759758
// entirely, so the loop below can never see them — they have to be counted
760-
// separately, through the same predicate the constraint write will use.
759+
// separately, through the same predicate `applyConstraints` uses.
761760
if (targetRequired) {
762761
const emptyCount = await countEmptyCells(trx, data.tableId, columnKey)
763762
if (emptyCount > 0) {

apps/sim/lib/table/currency.ts

Lines changed: 74 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,12 @@ const BIDI_MARKS = /[\u200e\u200f\u061c\u202a-\u202e\u2066-\u2069]/g
2424
/** Minus-sign characters `Intl` emits in place of the ASCII hyphen. */
2525
const UNICODE_MINUS = /[\u2212\u2012\u2013\uFE63\uFF0D]/g
2626

27+
/**
28+
* A magnitude suffix (`1.2 M`, `5 K`, `3.4 bn`). Not a currency marker, and
29+
* stripping it would silently shrink the value by orders of magnitude.
30+
*/
31+
const SCALE_SUFFIX = /\d\s*(?:k|m|b|t|bn|mn|tn|mm|mil|bil)\.?\s*$/i
32+
2733
/** A letter directly adjacent to a digit: an identifier, not an amount. */
2834
const LETTER_TOUCHING_DIGIT = /\p{L}\d|\d\p{L}/u
2935

@@ -171,15 +177,23 @@ export function getCurrencyOptions(): readonly CurrencyOption[] {
171177
* therefore read as fifteen hundred — a known ambiguity that resolves in favor
172178
* of the far more common reading.
173179
*
174-
* Reads ASCII digits only. Locales that format with their own numeral systems
175-
* (Arabic-Indic `١٢٣`, for instance) are rejected rather than misread —
176-
* supporting them is a wider decision than this type, since it would also
177-
* touch `number`, display, and sorting.
180+
* Two input families are deliberately refused rather than guessed at, because
181+
* both would otherwise be misread as an amount rather than rejected:
182+
*
183+
* - Markers written flush against the digits (`Rp12,00`). A letter touching a
184+
* digit is the only thing separating a currency marker from a part number,
185+
* and reading `SKU400` as 400 invents a value where refusing merely
186+
* inconveniences. Markers separated by a space or a symbol all work.
187+
* - Locales formatting with their own numeral systems (Arabic-Indic `١٢٣`).
188+
* Supporting them is a wider decision than this type, since it would also
189+
* touch `number`, display, and sorting.
190+
*
191+
* Both fail closed — `null`, never a wrong number.
178192
*
179193
* Returns `null` when no amount can be read, so callers can distinguish
180194
* "unparseable" from a legitimate `0`.
181195
*/
182-
export function parseCurrencyInput(raw: unknown): number | null {
196+
export function parseCurrencyInput(raw: unknown, currencyCode?: string): number | null {
183197
if (typeof raw === 'number') return Number.isFinite(raw) ? raw : null
184198
if (typeof raw !== 'string') return null
185199

@@ -219,14 +233,23 @@ export function parseCurrencyInput(raw: unknown): number | null {
219233
// by a space or a symbol, so this distinguishes them without a symbol list.
220234
if (LETTER_TOUCHING_DIGIT.test(body)) return null
221235

236+
// A scale suffix is not a currency marker, and dropping it loses orders of
237+
// magnitude: `1.2 M` would read as 1.2, so a column of `1.2 M` / `3.4 M`
238+
// would convert cleanly and rewrite every cell a millionfold too small.
239+
// Refuse rather than guess what the writer meant.
240+
if (SCALE_SUFFIX.test(body)) return null
241+
222242
// Strip the currency marker: up to three letters (an ISO code, `kr`, `zł`)
223243
// optionally joined to a symbol (`R$`, `CHF`), at either end. Bounded at
224244
// three so prose does not qualify — `Revenue 5` keeps its letters and is
225245
// rejected below.
226-
const cleaned = body
227-
.replace(CURRENCY_MARKER_PREFIX, '$1')
228-
.replace(CURRENCY_MARKER_SUFFIX, '')
229-
.replace(/[\s\u00a0\u202f\u2019']/gu, '')
246+
const withoutPrefix = body.replace(CURRENCY_MARKER_PREFIX, '$1')
247+
const withoutMarkers = withoutPrefix.replace(CURRENCY_MARKER_SUFFIX, '')
248+
// Whether a marker was present at all. A marked string came from a
249+
// formatter; a bare one was typed by a person. That distinguishes the two
250+
// readings of a lone separator followed by three digits, below.
251+
const hadMarker = withoutMarkers !== body
252+
const cleaned = withoutMarkers.replace(/[\s\u00a0\u202f\u2019']/gu, '')
230253
// What remains must be ONLY digits and separators. Anything else — a
231254
// US-format date, leftover prose — is not an amount.
232255
if (!AMOUNT_SHAPE.test(cleaned)) return null
@@ -246,19 +269,25 @@ export function parseCurrencyInput(raw: unknown): number | null {
246269
if (decimalParts.length > 1 || !hasValidGrouping(integerPart, groupSeparator)) return null
247270
normalized = `${integerPart.split(groupSeparator).join('')}.${decimalParts[0]}`
248271
} else if (lastComma !== -1) {
249-
// A single comma followed by exactly three digits is grouping (`1,500`);
250-
// anything else is a decimal comma (`1,50`).
251-
const grouping = digitsAndSeps.indexOf(',') !== lastComma || /,\d{3}$/.test(digitsAndSeps)
272+
const repeated = digitsAndSeps.indexOf(',') !== lastComma
273+
const grouping =
274+
repeated || loneSeparatorIsGrouping(digitsAndSeps, ',', hadMarker, currencyCode)
252275
if (grouping) {
253276
if (!hasValidGrouping(digitsAndSeps, ',')) return null
254277
normalized = digitsAndSeps.split(',').join('')
255278
} else {
256279
normalized = digitsAndSeps.replace(',', '.')
257280
}
258-
} else if (lastDot !== -1 && digitsAndSeps.indexOf('.') !== lastDot) {
259-
// More than one dot can only be grouping: `1.234.567`.
260-
if (!hasValidGrouping(digitsAndSeps, '.')) return null
261-
normalized = digitsAndSeps.split('.').join('')
281+
} else if (lastDot !== -1) {
282+
const repeated = digitsAndSeps.indexOf('.') !== lastDot
283+
const grouping =
284+
repeated || loneSeparatorIsGrouping(digitsAndSeps, '.', hadMarker, currencyCode)
285+
if (grouping) {
286+
if (!hasValidGrouping(digitsAndSeps, '.')) return null
287+
normalized = digitsAndSeps.split('.').join('')
288+
} else {
289+
normalized = digitsAndSeps
290+
}
262291
} else {
263292
normalized = digitsAndSeps
264293
}
@@ -268,6 +297,35 @@ export function parseCurrencyInput(raw: unknown): number | null {
268297
return negative ? -parsed : parsed
269298
}
270299

300+
/**
301+
* Whether a lone separator followed by exactly three digits is grouping rather
302+
* than a decimal point — `1.235 ¥` is one thousand two hundred thirty-five yen,
303+
* while a typed `1.235` is one and a bit.
304+
*
305+
* Two signals resolve it. A marker means the string came from a formatter, and
306+
* formatters group; a bare string was typed by a person, who meant decimals.
307+
* And a currency with three decimal places (KWD, TND) legitimately ends in
308+
* three digits after its separator, so for those the reading is always decimal.
309+
*/
310+
function loneSeparatorIsGrouping(
311+
digitsAndSeps: string,
312+
separator: string,
313+
hadMarker: boolean,
314+
currencyCode: string | undefined
315+
): boolean {
316+
if (!new RegExp(`\\${separator}\\d{3}$`).test(digitsAndSeps)) return false
317+
if (separator === ',' && !hadMarker) return true
318+
if (!hadMarker) return false
319+
return currencyFractionDigits(currencyCode) !== 3
320+
}
321+
322+
/** A currency's conventional decimal places, defaulting to 2 when unknown. */
323+
function currencyFractionDigits(currencyCode: string | undefined): number {
324+
if (!currencyCode) return 2
325+
const formatter = currencyFormatter(resolveCurrencyCode(currencyCode), undefined)
326+
return formatter?.resolvedOptions().maximumFractionDigits ?? 2
327+
}
328+
271329
/**
272330
* Formatters are cached by locale + code: a grid paints thousands of currency
273331
* cells per scroll, and constructing an `Intl.NumberFormat` per cell is orders

0 commit comments

Comments
 (0)