From c7c549c409738e9955e7c9164d742608a46fb685 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 19 Aug 2026 11:07:50 +0000 Subject: [PATCH] fix(sql): treat DECIMAL(p) as scale 0 in narrowing and revert risk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SQL defines DECIMAL(p) as DECIMAL(p,0). Migration pre-flight and History revert both required an explicit scale on both sides before flagging a reduction, so numeric(10,2) → numeric(10) truncated fractional digits with no NARROWING_TYPE_CHANGE warning and classified the revert as safe. Co-authored-by: huy.phan9 --- .../src/modules/lokee-weave/reversal.test.ts | 17 ++++++++ .../sql/src/modules/lokee-weave/reversal.ts | 13 +++++- .../src/modules/migration-validation.test.ts | 43 +++++++++++++++++++ .../sql/src/modules/migration-validation.ts | 7 ++- 4 files changed, 78 insertions(+), 2 deletions(-) diff --git a/packages/sql/src/modules/lokee-weave/reversal.test.ts b/packages/sql/src/modules/lokee-weave/reversal.test.ts index afaacd70..52a500e6 100644 --- a/packages/sql/src/modules/lokee-weave/reversal.test.ts +++ b/packages/sql/src/modules/lokee-weave/reversal.test.ts @@ -29,6 +29,12 @@ describe('parseTypeText', () => { expect(parseTypeText('numeric(10,2)')).toEqual({ base: 'numeric', precision: 10, scale: 2 }); }); + it('reads DECIMAL(p) as precision with scale 0', () => { + expect(parseTypeText('numeric(10)')).toEqual({ base: 'numeric', precision: 10, scale: 0 }); + expect(parseTypeText('decimal(8)')).toEqual({ base: 'decimal', precision: 8, scale: 0 }); + expect(parseTypeText('NUMBER(19)')).toEqual({ base: 'number', precision: 19, scale: 0 }); + }); + it('reads an unparameterised type', () => { expect(parseTypeText('integer')).toEqual({ base: 'integer' }); }); @@ -149,6 +155,17 @@ describe('classifyReversal — the cases users actually ask about', () => { expect(verdict.dataLoss).toContain('2 decimal places'); }); + it('warns when reverting to DECIMAL(p) from a scaled decimal', () => { + const verdict = classifyReversal( + 'column:ORDER.TOTAL', + column({ dataType: 'numeric(10,2)' }), + column({ dataType: 'numeric(10)' }) + ); + expect(verdict.risk).toBe('lossy'); + expect(verdict.summary).toMatch(/scale 2 → 0/); + expect(verdict.dataLoss).toContain('0 decimal places'); + }); + it('warns when numeric precision is reduced', () => { const verdict = classifyReversal( 'column:ORDER.TOTAL', diff --git a/packages/sql/src/modules/lokee-weave/reversal.ts b/packages/sql/src/modules/lokee-weave/reversal.ts index 94ec6e81..86d7d5f4 100644 --- a/packages/sql/src/modules/lokee-weave/reversal.ts +++ b/packages/sql/src/modules/lokee-weave/reversal.ts @@ -74,7 +74,18 @@ export function parseTypeText(text: string | null | undefined): ParsedType | nul .map((a) => a.trim()); const nums = args.map((a) => (/^\d+$/.test(a) ? Number(a) : undefined)); // `varchar(max)` carries no usable bound — treat as unbounded. - if (args.length === 1) return { base, length: nums[0] }; + if (args.length === 1) { + // SQL / Oracle: DECIMAL(p) / NUMERIC(p) / NUMBER(p) ≡ (p,0). Storing the + // sole arg as `length` made scale comparisons skip DECIMAL(10,2) → + // DECIMAL(10) and classify a truncating revert as safe. + if ( + (base === 'numeric' || base === 'decimal' || base === 'number' || base === 'dec') && + nums[0] !== undefined + ) { + return { base, precision: nums[0], scale: 0 }; + } + return { base, length: nums[0] }; + } return { base, precision: nums[0], scale: nums[1] }; } diff --git a/packages/sql/src/modules/migration-validation.test.ts b/packages/sql/src/modules/migration-validation.test.ts index 3dbda9f3..d88011cd 100644 --- a/packages/sql/src/modules/migration-validation.test.ts +++ b/packages/sql/src/modules/migration-validation.test.ts @@ -91,6 +91,49 @@ describe('findNarrowingTypeChanges', () => { expect(findNarrowingTypeChanges(tables, { ORDERS: true }, postgresSqlDialect)).toHaveLength(1); }); + it('flags decimal scale decrease', () => { + const tables = [ + diff({ + tableName: 'ORDERS', + objectType: 'TABLE', + status: 'MODIFIED', + columnDiffs: [ + { + name: 'AMOUNT', + status: 'MODIFIED', + source: { type: 'numeric(10,0)', nullable: true }, + target: { type: 'numeric(10,2)', nullable: true }, + }, + ], + }), + ]; + expect(findNarrowingTypeChanges(tables, { ORDERS: true }, postgresSqlDialect)).toHaveLength(1); + }); + + it('flags DECIMAL(p) as scale-0 narrowing (SQL: DECIMAL(p) ≡ DECIMAL(p,0))', () => { + // Omitted scale used to skip the check entirely — migrate truncated cents + // with no NARROWING_TYPE_CHANGE warning. + const tables = [ + diff({ + tableName: 'ORDERS', + objectType: 'TABLE', + status: 'MODIFIED', + columnDiffs: [ + { + name: 'AMOUNT', + status: 'MODIFIED', + source: { type: 'numeric(10)', nullable: true }, + target: { type: 'numeric(10,2)', nullable: true }, + }, + ], + }), + ]; + const issues = findNarrowingTypeChanges(tables, { ORDERS: true }, postgresSqlDialect); + expect(issues).toHaveLength(1); + expect(issues[0]).toMatchObject({ code: 'NARROWING_TYPE_CHANGE', tableName: 'ORDERS' }); + expect(issues[0]!.message).toMatch(/numeric\(10,2\).*numeric\(10\)/); + }); + it('does not flag a widening change', () => { const tables = [ diff({ diff --git a/packages/sql/src/modules/migration-validation.ts b/packages/sql/src/modules/migration-validation.ts index 01be6ec1..9fb9f018 100644 --- a/packages/sql/src/modules/migration-validation.ts +++ b/packages/sql/src/modules/migration-validation.ts @@ -74,7 +74,12 @@ function isNarrowing(source: CanonicalType, target: CanonicalType): boolean { if (source.length !== undefined && target.length !== undefined && target.length < source.length) return true; if (source.base === 'decimal') { if (source.precision !== undefined && target.precision !== undefined && target.precision < source.precision) return true; - if (source.scale !== undefined && target.scale !== undefined && target.scale < source.scale) return true; + // SQL: DECIMAL(p) ≡ DECIMAL(p,0). A missing scale on a sized decimal is + // zero, not "unknown" — treating it as unknown skipped DECIMAL(10,2) → + // DECIMAL(10) and let migrates truncate fractional digits unwarned. + const sourceScale = source.precision !== undefined ? (source.scale ?? 0) : undefined; + const targetScale = target.precision !== undefined ? (target.scale ?? 0) : undefined; + if (sourceScale !== undefined && targetScale !== undefined && targetScale < sourceScale) return true; } return false; }