Merge pull request #681 from Marvell-Consulting/SW-1229-Prevent-bad-sort-orders

Prevent non-numerical sort orders

Author: j-maynard

src/interfaces/filter-row.ts

@@ -5,6 +5,6 @@ export interface FilterRow {
   dimension_name: string;
   description: string;
   hierarchy: string;
-  sort_order?: string | null;
+  sort_order?: string | null; // Backed by a BIGINT column in the DB, but represented here as a string.
   reference_count?: string | null;
 }

src/services/cube-builder.ts

@@ -575,7 +575,7 @@ function createCubeBaseTables(revisionId: string, buildId: string, factTableQuer
          fact_table_column VARCHAR,
          dimension_name VARCHAR,
          description VARCHAR,
-         sort_order TEXT,
+         sort_order BIGINT,
          hierarchy VARCHAR,
          reference_count BIGINT DEFAULT 0,
          PRIMARY KEY (reference, language, fact_table_column)
@@ -1266,7 +1266,7 @@ function setupMeasureAndDataValuesNoLookup(
     );
     statements.push(
       pgformat(
-        `INSERT INTO %I.%I SELECT DISTINCT CAST(%I AS VARCHAR), %L, %L, %L, CAST(%I AS VARCHAR), NULL, NULL, 0 FROM %I.%I ORDER BY %I;`,
+        `INSERT INTO %I.%I SELECT DISTINCT CAST(%I AS VARCHAR), %L, %L, %L, CAST(%I AS VARCHAR), NULL::BIGINT, NULL, 0 FROM %I.%I ORDER BY %I;`,
         buildId,
         FILTER_TABLE_NAME,
         measureColumn.columnName,
@@ -1303,7 +1303,7 @@ export const measureTableCreateStatement = (
                         language TEXT,
                         description TEXT,
                         notes TEXT,
-                        sort_order TEXT,
+                        sort_order BIGINT,
                         format TEXT,
                         decimals INTEGER,
                         measure_type TEXT,
@@ -1522,7 +1522,7 @@ function setupMeasureAndDataValuesWithLookup(
     const columnName = t('column_headers.measure', { lng: locale });
     statements.push(
       pgformat(
-        `INSERT INTO %I.%I SELECT CAST(reference AS VARCHAR), language, %L, %L, description, sort_order, CAST(hierarchy AS VARCHAR), 0 FROM %I.measure WHERE language = %L ORDER BY sort_order, reference;`,
+        `INSERT INTO %I.%I SELECT CAST(reference AS VARCHAR), language, %L, %L, description, CAST (sort_order AS BIGINT), CAST(hierarchy AS VARCHAR), 0 FROM %I.measure WHERE language = %L ORDER BY sort_order, reference;`,
         buildId,
         FILTER_TABLE_NAME,
         measureColumn.columnName,
@@ -1582,7 +1582,7 @@ function rawDimensionProcessor(
     statements.push(
       pgformat(
         `INSERT INTO %I.%I
-       SELECT DISTINCT CAST(%I AS VARCHAR), %L, %L, %L, CAST (%I AS VARCHAR), NULL, NULL, 0
+       SELECT DISTINCT CAST(%I AS VARCHAR), %L, %L, %L, CAST (%I AS VARCHAR), NULL::BIGINT, NULL, 0
        FROM %I.%I ORDER BY %I;`,
         buildId,
         FILTER_TABLE_NAME,
@@ -1708,7 +1708,7 @@ export function setupLookupTableDimension(
         `INSERT INTO %I.%I
               SELECT reference, language, fact_table_column, dimension_name, description, sort_order, hierarchy, 0
               FROM (SELECT DISTINCT
-              CAST(%I AS VARCHAR) AS reference, language, %L AS fact_table_column, %L AS dimension_name, description, sort_order AS sort_order, hierarchy
+              CAST(%I AS VARCHAR) AS reference, language, %L AS fact_table_column, %L AS dimension_name, description, CAST (sort_order AS BIGINT) AS sort_order, hierarchy
             FROM %I.%I
             WHERE language = %L
             ORDER BY sort_order %s, description);`,
@@ -1889,7 +1889,7 @@ function setupTextDimension(
     statements.push(
       pgformat(
         `INSERT INTO %I.%I
-         SELECT DISTINCT CAST(%I AS VARCHAR), %L, %L, %L, CAST (%I AS VARCHAR), NULL, NULL, 0
+         SELECT DISTINCT CAST(%I AS VARCHAR), %L, %L, %L, CAST (%I AS VARCHAR), NULL::BIGINT, NULL, 0
          FROM %I.%I;`,
         buildId,
         FILTER_TABLE_NAME,
@@ -2086,9 +2086,17 @@ export function updateFilterTableCounts(buildId: string, factTable: FactTableCol
   };
 }
 
-function alterFilterTableBlock(cubeId: string): TransactionBlock {
+export function alterFilterTableBlock(cubeId: string): TransactionBlock {
   const statements: string[] = ['BEGIN TRANSACTION;'];
-  statements.push(pgformat('ALTER TABLE %I.%I ADD IF NOT EXISTS sort_order TEXT;', cubeId, FILTER_TABLE_NAME));
+  statements.push(pgformat('ALTER TABLE %I.%I ADD IF NOT EXISTS sort_order BIGINT;', cubeId, FILTER_TABLE_NAME));
+  // Migrate existing TEXT columns to BIGINT; non-numeric values are nulled out rather than failing.
+  statements.push(
+    pgformat(
+      `ALTER TABLE %I.%I ALTER COLUMN sort_order TYPE BIGINT USING (CASE WHEN sort_order::TEXT ~ '^-?[0-9]+$' THEN (sort_order::TEXT)::BIGINT ELSE NULL END);`,
+      cubeId,
+      FILTER_TABLE_NAME
+    )
+  );
   statements.push(
     pgformat('ALTER TABLE %I.%I ADD IF NOT EXISTS reference_count BIGINT DEFAULT 0;', cubeId, FILTER_TABLE_NAME)
   );
@@ -2105,7 +2113,7 @@ function createFilterTableSortOrders(cubeId: string, dataset: Dataset): Transact
     if (!dim.lookupTable) continue;
     statements.push(
       pgformat(
-        'UPDATE %I.%I SET sort_order = lookup.sort_order FROM (SELECT CAST(%I AS VARCHAR) AS reference, language, sort_order FROM %I.%I) AS lookup WHERE %I.reference = lookup.reference AND %I.language = lookup.language AND %I.fact_table_column = %L;',
+        'UPDATE %I.%I SET sort_order = lookup.sort_order FROM (SELECT CAST(%I AS VARCHAR) AS reference, language, CAST (sort_order AS BIGINT) as sort_order FROM %I.%I) AS lookup WHERE %I.reference = lookup.reference AND %I.language = lookup.language AND %I.fact_table_column = %L;',
         cubeId,
         FILTER_TABLE_NAME,
         dim.factTableColumn,
@@ -2122,7 +2130,7 @@ function createFilterTableSortOrders(cubeId: string, dataset: Dataset): Transact
   if (dataset.measure?.lookupTable) {
     statements.push(
       pgformat(
-        'UPDATE %I.%I SET sort_order = lookup.sort_order FROM (SELECT CAST(reference AS VARCHAR) AS reference, language, sort_order FROM %I.measure) AS lookup WHERE %I.reference = lookup.reference AND %I.language = lookup.language AND %I.fact_table_column = %L;',
+        'UPDATE %I.%I SET sort_order = lookup.sort_order FROM (SELECT CAST(reference AS VARCHAR) AS reference, language, CAST (sort_order AS BIGINT) AS sort_order FROM %I.measure) AS lookup WHERE %I.reference = lookup.reference AND %I.language = lookup.language AND %I.fact_table_column = %L;',
         cubeId,
         FILTER_TABLE_NAME,
         cubeId,

src/services/lookup-table-handler.ts

@@ -151,10 +151,19 @@ export const validateLookupTable = async (
       lookupReferenceColumn
     );
   } catch (err) {
-    logger.error(err, `Something went wrong trying to convert the lookup table to SW3 format`);
-    return viewErrorGenerators(500, dataset.id, 'patch', 'errors.dimension_validation.lookup_table_loading_failed', {
-      mismatch: false
+    void cleanUpPostgresValidationSchema(mockCubeId, lookupTable.id).catch((cleanupErr) => {
+      logger.error(cleanupErr, 'Something went wrong trying to clean up the mock cube');
     });
+    if (err instanceof Error && err.message.includes('invalid input syntax for type bigint')) {
+      return viewErrorGenerators(400, dataset.id, 'patch', 'errors.dimension_validation.sort_contains_text', {
+        mismatch: false
+      });
+    } else {
+      logger.error(err, `Something went wrong trying to convert the lookup table to SW3 format`);
+      return viewErrorGenerators(500, dataset.id, 'patch', 'errors.dimension_validation.lookup_table_loading_failed', {
+        mismatch: false
+      });
+    }
   }
 
   const hierarchyErrors = await validateLookupTableHierarchyValues(
@@ -190,6 +199,9 @@ export const validateLookupTable = async (
     await saveValidatedLookupTableToDatabase(mockCubeId, lookupTable.id);
   } catch (err) {
     logger.error(err, 'Something went wrong trying to save the lookup table or clean up the mock cube.');
+    void cleanUpPostgresValidationSchema(mockCubeId, lookupTable.id).catch((cleanupErr) => {
+      logger.error(cleanupErr, 'Something went wrong trying to clean up the mock cube');
+    });
     return viewErrorGenerators(500, dataset.id, 'patch', `errors.lookup_table_validation.unknown_error`, {});
   }
 

src/utils/file-utils.ts

@@ -81,7 +81,7 @@ export const createLookupTableQuery = (
     });
   }
   return pgformat(
-    'CREATE TABLE %I.%I (%I %s NOT NULL, language VARCHAR(5) NOT NULL, description TEXT NOT NULL, notes TEXT, sort_order INTEGER, hierarchy VARCHAR %s);',
+    'CREATE TABLE %I.%I (%I %s NOT NULL, language VARCHAR(5) NOT NULL, description TEXT NOT NULL, notes TEXT, sort_order BIGINT, hierarchy VARCHAR %s);',
     schemaName,
     lookupTableName,
     referenceColumnName,
@@ -122,7 +122,7 @@ export async function loadFileIntoLookupTablesSchema(
         );
         const notesCol = extractor.notesColumns?.find((col) => col.lang.toLowerCase() === locale.toLowerCase());
         const notesColStr = notesCol ? pgformat('%I', notesCol.name) : 'NULL';
-        const sortStr = extractor.sortColumn ? pgformat('%I', extractor.sortColumn) : 'NULL';
+        const sortStr = extractor.sortColumn ? pgformat('CAST (%I AS BIGINT)', extractor.sortColumn) : 'NULL::BIGINT';
         const hierarchyCol = hierarchySelectExpression(extractor.hierarchyColumn);
         dataExtractorParts.push(
           pgformat(
@@ -149,7 +149,7 @@ export async function loadFileIntoLookupTablesSchema(
     } else {
       const languageMatcher = languageMatcherCaseStatement(extractor.languageColumn);
       const notesStr = extractor.notesColumns ? pgformat('%I', extractor.notesColumns[0].name) : 'NULL';
-      const sortStr = extractor.sortColumn ? pgformat('%I', extractor.sortColumn) : 'NULL';
+      const sortStr = extractor.sortColumn ? pgformat('CAST (%I AS BIGINT)', extractor.sortColumn) : 'NULL::BIGINT';
       const hierarchyStr = hierarchySelectExpression(extractor.hierarchyColumn);
       const dataExtractorParts = pgformat(
         `SELECT %I AS %I, %s as language, %I as description, %s as notes, %s as sort_order, %s as hierarchy FROM %I.%I;`,
@@ -247,7 +247,7 @@ export async function convertLookupTableToSW3Format(
       );
       const notesCol = extractor.notesColumns?.find((col) => col.lang.toLowerCase() === locale.toLowerCase());
       const notesColStr = notesCol ? pgformat('%I', notesCol.name) : 'NULL';
-      const sortStr = extractor.sortColumn ? pgformat('%I', extractor.sortColumn) : 'NULL';
+      const sortStr = extractor.sortColumn ? pgformat('CAST (%I AS BIGINT)', extractor.sortColumn) : 'NULL::BIGINT';
       const hierarchyCol = hierarchySelectExpression(extractor.hierarchyColumn);
       dataExtractorParts.push(
         pgformat(
@@ -269,7 +269,7 @@ export async function convertLookupTableToSW3Format(
   } else {
     const languageMatcher = languageMatcherCaseStatement(extractor.languageColumn);
     const notesStr = extractor.notesColumns ? pgformat('%I', extractor.notesColumns[0].name) : 'NULL';
-    const sortStr = extractor.sortColumn ? pgformat('%I', extractor.sortColumn) : 'NULL';
+    const sortStr = extractor.sortColumn ? pgformat('CAST (%I AS BIGINT)', extractor.sortColumn) : 'NULL::BIGINT';
     const hierarchyStr = hierarchySelectExpression(extractor.hierarchyColumn);
     const dataExtractorParts = pgformat(
       `SELECT %I AS %I, %s as language, %I as description, %s as notes, %s as sort_order, %s as hierarchy %s FROM %I.%I;`,

test/unit/services/cube-builder.test.ts

@@ -55,6 +55,7 @@ import {
   setupCubeBuilder,
   setupLookupTableDimension,
   updateFilterTableCounts,
+  alterFilterTableBlock,
   FACT_TABLE_NAME,
   FILTER_TABLE_NAME,
   METADATA_TABLE_NAME,
@@ -370,7 +371,7 @@ describe('measureTableCreateStatement', () => {
     expect(sql).toContain('hierarchy TEXT');
     expect(sql).toContain('language TEXT');
     expect(sql).toContain('description TEXT');
-    expect(sql).toContain('sort_order TEXT');
+    expect(sql).toContain('sort_order BIGINT');
     expect(sql).toContain('format TEXT');
     expect(sql).toContain('decimals INTEGER');
     expect(sql).toContain('measure_type TEXT');
@@ -830,6 +831,26 @@ describe('setupLookupTableDimension — filter table sort direction', () => {
       });
     });
   }
+
+  describe('sort_order BIGINT casting', () => {
+    it('casts sort_order as BIGINT in the filter_table INSERT for lookup table dimensions', () => {
+      const { stmts } = callSetup(DimensionType.LookupTable);
+      const inserts = filterInserts(stmts);
+      expect(inserts.length).toBeGreaterThan(0);
+      for (const stmt of inserts) {
+        expect(stmt).toContain('CAST (sort_order AS BIGINT)');
+      }
+    });
+
+    it('casts sort_order as BIGINT for date-period lookup table dimensions too', () => {
+      const { stmts } = callSetup(DimensionType.DatePeriod);
+      const inserts = filterInserts(stmts);
+      expect(inserts.length).toBeGreaterThan(0);
+      for (const stmt of inserts) {
+        expect(stmt).toContain('CAST (sort_order AS BIGINT)');
+      }
+    });
+  });
 });
 
 // ===========================================================================
@@ -935,3 +956,48 @@ describe('updateFilterTableCounts', () => {
     expect(countUpdates).toHaveLength(0);
   });
 });
+
+// ===========================================================================
+describe('alterFilterTableBlock', () => {
+  const CUBE_ID = 'test-cube-id';
+
+  it('wraps statements in a transaction', () => {
+    const { statements } = alterFilterTableBlock(CUBE_ID);
+    expect(statements[0]).toBe('BEGIN TRANSACTION;');
+    expect(statements[statements.length - 1]).toBe('COMMIT;');
+  });
+
+  it('returns buildStage UpdateFilterTable', () => {
+    const block = alterFilterTableBlock(CUBE_ID);
+    expect(block.buildStage).toBe(BuildStage.UpdateFilterTable);
+  });
+
+  it('adds sort_order column as BIGINT if not exists', () => {
+    const { statements } = alterFilterTableBlock(CUBE_ID);
+    const addStmt = statements.find((s) => s.includes('ADD IF NOT EXISTS') && s.includes('sort_order'));
+    expect(addStmt).toBeDefined();
+    expect(addStmt).toContain('sort_order BIGINT');
+  });
+
+  it('includes ALTER COLUMN to migrate existing TEXT sort_order to BIGINT', () => {
+    const { statements } = alterFilterTableBlock(CUBE_ID);
+    const alterStmt = statements.find((s) => s.includes('ALTER COLUMN') && s.includes('sort_order'));
+    expect(alterStmt).toBeDefined();
+    expect(alterStmt).toContain('TYPE BIGINT');
+    expect(alterStmt).toContain('USING');
+  });
+
+  it('ALTER COLUMN statement comes after ADD IF NOT EXISTS so the column is guaranteed to exist', () => {
+    const { statements } = alterFilterTableBlock(CUBE_ID);
+    const addIdx = statements.findIndex((s) => s.includes('ADD IF NOT EXISTS') && s.includes('sort_order'));
+    const alterIdx = statements.findIndex((s) => s.includes('ALTER COLUMN') && s.includes('sort_order'));
+    expect(addIdx).toBeGreaterThanOrEqual(0);
+    expect(alterIdx).toBeGreaterThan(addIdx);
+  });
+
+  it('nulls non-numeric values in the USING clause rather than failing', () => {
+    const { statements } = alterFilterTableBlock(CUBE_ID);
+    const alterStmt = statements.find((s) => s.includes('ALTER COLUMN') && s.includes('sort_order'));
+    expect(alterStmt).toContain('ELSE NULL');
+  });
+});

test/unit/services/lookup-table-handler.test.ts

@@ -478,6 +478,42 @@ describe('validateLookupTable', () => {
       expect(result.errors[0].message.key).toBe('errors.dimension_validation.lookup_table_loading_failed');
     });
 
+    it('returns 400 with sort_contains_text when convertLookupTableToSW3Format throws a bigint syntax error', async () => {
+      const dataset = makeDataset();
+      setupJoinColumnFoundMocks();
+      (convertLookupTableToSW3Format as jest.Mock).mockRejectedValueOnce(
+        new Error('invalid input syntax for type bigint: "not a number"')
+      );
+
+      const result = (await validateLookupTable(
+        validProtoTable,
+        dataset,
+        draftRevision,
+        dimension,
+        'en-GB'
+      )) as ViewErrDTO;
+
+      expect(result.status).toBe(400);
+      expect(result.errors[0].message.key).toBe('errors.dimension_validation.sort_contains_text');
+    });
+
+    it('returns 500 (not 400) when convertLookupTableToSW3Format throws an unrelated error', async () => {
+      const dataset = makeDataset();
+      setupJoinColumnFoundMocks();
+      (convertLookupTableToSW3Format as jest.Mock).mockRejectedValueOnce(new Error('some other db error'));
+
+      const result = (await validateLookupTable(
+        validProtoTable,
+        dataset,
+        draftRevision,
+        dimension,
+        'en-GB'
+      )) as ViewErrDTO;
+
+      expect(result.status).toBe(500);
+      expect(result.errors[0].message.key).toBe('errors.dimension_validation.lookup_table_loading_failed');
+    });
+
     it('returns the language error when validateLookupTableLanguages returns an error DTO', async () => {
       const dataset = makeDataset();
       setupJoinColumnFoundMocks();