Merge pull request #457 from buster-so/cursor/validate-chart-axis-configurations-6704

Validate chart axis configurations

packages/ai/src/tools/visualization-tools/bar-line-axis-validator.test.ts

@@ -0,0 +1,291 @@
+import { describe, expect, it } from 'vitest';
+import { validateAndAdjustBarLineAxes } from './bar-line-axis-validator';
+import type { MetricYml } from './version-history-types';
+
+describe('validateAndAdjustBarLineAxes', () => {
+  it('should return valid for non-bar/line charts', () => {
+    const metricYml: MetricYml = {
+      name: 'Test Metric',
+      description: 'Test description',
+      timeFrame: '2024',
+      sql: 'SELECT * FROM test',
+      chartConfig: {
+        selectedChartType: 'pie',
+        columnLabelFormats: {},
+      },
+    };
+
+    const result = validateAndAdjustBarLineAxes(metricYml);
+    expect(result.isValid).toBe(true);
+    expect(result.shouldSwapAxes).toBe(false);
+  });
+
+  it('should return valid when Y axis has numeric columns', () => {
+    const metricYml: MetricYml = {
+      name: 'Sales by Month',
+      description: 'Monthly sales data',
+      timeFrame: '2024',
+      sql: 'SELECT month, sales FROM data',
+      chartConfig: {
+        selectedChartType: 'line',
+        columnLabelFormats: {
+          month: {
+            columnType: 'date',
+            style: 'date',
+            replaceMissingDataWith: null,
+            numberSeparatorStyle: null,
+          },
+          sales: {
+            columnType: 'number',
+            style: 'currency',
+            replaceMissingDataWith: 0,
+            numberSeparatorStyle: ',',
+          },
+        },
+        barAndLineAxis: {
+          x: ['month'],
+          y: ['sales'],
+        },
+      },
+    };
+
+    const result = validateAndAdjustBarLineAxes(metricYml);
+    expect(result.isValid).toBe(true);
+    expect(result.shouldSwapAxes).toBe(false);
+  });
+
+  it('should swap axes when Y has non-numeric and X has numeric columns', () => {
+    const metricYml: MetricYml = {
+      name: 'Sales Rep by Revenue',
+      description: 'Revenue by sales rep',
+      timeFrame: '2024',
+      sql: 'SELECT revenue, sales_rep FROM data',
+      chartConfig: {
+        selectedChartType: 'bar',
+        columnLabelFormats: {
+          revenue: {
+            columnType: 'number',
+            style: 'currency',
+            replaceMissingDataWith: 0,
+            numberSeparatorStyle: ',',
+          },
+          sales_rep: {
+            columnType: 'string',
+            style: 'string',
+            replaceMissingDataWith: null,
+            numberSeparatorStyle: null,
+          },
+        },
+        barAndLineAxis: {
+          x: ['revenue'],
+          y: ['sales_rep'],
+        },
+      },
+    };
+
+    const result = validateAndAdjustBarLineAxes(metricYml);
+    expect(result.isValid).toBe(true);
+    expect(result.shouldSwapAxes).toBe(true);
+    expect(result.adjustedYml).toBeDefined();
+    expect(result.adjustedYml?.chartConfig?.barAndLineAxis?.x).toEqual(['sales_rep']);
+    expect(result.adjustedYml?.chartConfig?.barAndLineAxis?.y).toEqual(['revenue']);
+  });
+
+  it('should return error when Y has non-numeric and X also has non-numeric columns', () => {
+    const metricYml: MetricYml = {
+      name: 'Categories by Region',
+      description: 'Product categories by region',
+      timeFrame: '2024',
+      sql: 'SELECT region, category FROM data',
+      chartConfig: {
+        selectedChartType: 'line',
+        columnLabelFormats: {
+          region: {
+            columnType: 'string',
+            style: 'string',
+            replaceMissingDataWith: null,
+            numberSeparatorStyle: null,
+          },
+          category: {
+            columnType: 'string',
+            style: 'string',
+            replaceMissingDataWith: null,
+            numberSeparatorStyle: null,
+          },
+        },
+        barAndLineAxis: {
+          x: ['region'],
+          y: ['category'],
+        },
+      },
+    };
+
+    const result = validateAndAdjustBarLineAxes(metricYml);
+    expect(result.isValid).toBe(false);
+    expect(result.shouldSwapAxes).toBe(false);
+    expect(result.error).toBeDefined();
+    expect(result.error).toContain('Bar and line charts require numeric values on the Y axis');
+    expect(result.error).toContain('category (string)');
+  });
+
+  it('should handle multiple columns on axes', () => {
+    const metricYml: MetricYml = {
+      name: 'Multi Column Chart',
+      description: 'Chart with multiple columns',
+      timeFrame: '2024',
+      sql: 'SELECT date, region, sales, profit FROM data',
+      chartConfig: {
+        selectedChartType: 'line',
+        columnLabelFormats: {
+          date: {
+            columnType: 'date',
+            style: 'date',
+            replaceMissingDataWith: null,
+            numberSeparatorStyle: null,
+          },
+          region: {
+            columnType: 'string',
+            style: 'string',
+            replaceMissingDataWith: null,
+            numberSeparatorStyle: null,
+          },
+          sales: {
+            columnType: 'number',
+            style: 'currency',
+            replaceMissingDataWith: 0,
+            numberSeparatorStyle: ',',
+          },
+          profit: {
+            columnType: 'number',
+            style: 'currency',
+            replaceMissingDataWith: 0,
+            numberSeparatorStyle: ',',
+          },
+        },
+        barAndLineAxis: {
+          x: ['date'],
+          y: ['sales', 'profit'],
+        },
+      },
+    };
+
+    const result = validateAndAdjustBarLineAxes(metricYml);
+    expect(result.isValid).toBe(true);
+    expect(result.shouldSwapAxes).toBe(false);
+  });
+
+  it('should return error when Y has mixed numeric and non-numeric columns', () => {
+    const metricYml: MetricYml = {
+      name: 'Mixed Y Axis',
+      description: 'Chart with mixed column types on Y',
+      timeFrame: '2024',
+      sql: 'SELECT month, sales, category FROM data',
+      chartConfig: {
+        selectedChartType: 'bar',
+        columnLabelFormats: {
+          month: {
+            columnType: 'date',
+            style: 'date',
+            replaceMissingDataWith: null,
+            numberSeparatorStyle: null,
+          },
+          sales: {
+            columnType: 'number',
+            style: 'currency',
+            replaceMissingDataWith: 0,
+            numberSeparatorStyle: ',',
+          },
+          category: {
+            columnType: 'string',
+            style: 'string',
+            replaceMissingDataWith: null,
+            numberSeparatorStyle: null,
+          },
+        },
+        barAndLineAxis: {
+          x: ['month'],
+          y: ['sales', 'category'],
+        },
+      },
+    };
+
+    const result = validateAndAdjustBarLineAxes(metricYml);
+    expect(result.isValid).toBe(false);
+    expect(result.shouldSwapAxes).toBe(false);
+    expect(result.error).toContain('category (string)');
+  });
+
+  it('should handle date columns as non-numeric', () => {
+    const metricYml: MetricYml = {
+      name: 'Date on Y Axis',
+      description: 'Chart with date on Y axis',
+      timeFrame: '2024',
+      sql: 'SELECT sales, order_date FROM data',
+      chartConfig: {
+        selectedChartType: 'line',
+        columnLabelFormats: {
+          sales: {
+            columnType: 'number',
+            style: 'currency',
+            replaceMissingDataWith: 0,
+            numberSeparatorStyle: ',',
+          },
+          order_date: {
+            columnType: 'date',
+            style: 'date',
+            replaceMissingDataWith: null,
+            numberSeparatorStyle: null,
+          },
+        },
+        barAndLineAxis: {
+          x: ['sales'],
+          y: ['order_date'],
+        },
+      },
+    };
+
+    const result = validateAndAdjustBarLineAxes(metricYml);
+    expect(result.isValid).toBe(true);
+    expect(result.shouldSwapAxes).toBe(true);
+    expect(result.adjustedYml?.chartConfig?.barAndLineAxis?.x).toEqual(['order_date']);
+    expect(result.adjustedYml?.chartConfig?.barAndLineAxis?.y).toEqual(['sales']);
+  });
+
+  it('should handle missing barAndLineAxis gracefully', () => {
+    const metricYml: MetricYml = {
+      name: 'Missing Axis Config',
+      description: 'Chart without axis config',
+      timeFrame: '2024',
+      sql: 'SELECT * FROM data',
+      chartConfig: {
+        selectedChartType: 'bar',
+        columnLabelFormats: {},
+      },
+    };
+
+    const result = validateAndAdjustBarLineAxes(metricYml);
+    expect(result.isValid).toBe(true);
+    expect(result.shouldSwapAxes).toBe(false);
+  });
+
+  it('should handle empty axis arrays', () => {
+    const metricYml: MetricYml = {
+      name: 'Empty Axes',
+      description: 'Chart with empty axes',
+      timeFrame: '2024',
+      sql: 'SELECT * FROM data',
+      chartConfig: {
+        selectedChartType: 'line',
+        columnLabelFormats: {},
+        barAndLineAxis: {
+          x: [],
+          y: [],
+        },
+      },
+    };
+
+    const result = validateAndAdjustBarLineAxes(metricYml);
+    expect(result.isValid).toBe(false);
+    expect(result.shouldSwapAxes).toBe(false);
+  });
+});

packages/ai/src/tools/visualization-tools/bar-line-axis-validator.ts

@@ -0,0 +1,99 @@
+import type { MetricYml } from './version-history-types';
+
+export interface AxisValidationResult {
+  isValid: boolean;
+  shouldSwapAxes: boolean;
+  error?: string;
+  adjustedYml?: MetricYml;
+}
+
+/**
+ * Validates and potentially adjusts bar/line chart axes to ensure numeric fields are on Y axis
+ * @param metricYml The parsed metric YAML configuration
+ * @returns Validation result indicating if axes should be swapped or if there's an error
+ */
+export function validateAndAdjustBarLineAxes(metricYml: MetricYml): AxisValidationResult {
+  // Only validate bar and line charts
+  const chartType = metricYml.chartConfig?.selectedChartType;
+  if (chartType !== 'bar' && chartType !== 'line') {
+    return { isValid: true, shouldSwapAxes: false };
+  }
+
+  const barAndLineAxis = metricYml.chartConfig?.barAndLineAxis;
+  const columnLabelFormats = metricYml.chartConfig?.columnLabelFormats;
+
+  if (!barAndLineAxis || !columnLabelFormats) {
+    return { isValid: true, shouldSwapAxes: false };
+  }
+
+  if (!barAndLineAxis.x?.length || !barAndLineAxis.y?.length) {
+    return {
+      isValid: false,
+      shouldSwapAxes: false,
+      error: 'Bar and line charts require at least one column for each axis. Please specify both X and Y axis columns.',
+    };
+  }
+  const xColumns = barAndLineAxis.x;
+  const yColumns = barAndLineAxis.y;
+
+  // Check if all Y-axis columns are numeric
+  const yAxisNumericStatus = yColumns.map((col: string) => {
+    const format = columnLabelFormats[col];
+    return format?.columnType === 'number';
+  });
+
+  const allYColumnsNumeric = yAxisNumericStatus.every((isNumeric: boolean) => isNumeric);
+
+  // If all Y columns are numeric, no adjustment needed
+  if (allYColumnsNumeric) {
+    return { isValid: true, shouldSwapAxes: false };
+  }
+
+  // At least one Y column is non-numeric, check if we can swap with X
+  const xAxisNumericStatus = xColumns.map((col: string) => {
+    const format = columnLabelFormats[col];
+    return format?.columnType === 'number';
+  });
+
+  const allXColumnsNumeric = xAxisNumericStatus.every((isNumeric: boolean) => isNumeric);
+
+  // If X columns are numeric and Y columns are not, swap them
+  if (allXColumnsNumeric && !allYColumnsNumeric) {
+    // Create adjusted YAML with swapped axes
+    const adjustedYml: MetricYml = {
+      ...metricYml,
+      chartConfig: {
+        ...metricYml.chartConfig,
+        barAndLineAxis: {
+          ...barAndLineAxis,
+          x: yColumns, // Swap: Y becomes X
+          y: xColumns, // Swap: X becomes Y
+        },
+      },
+    };
+
+    return {
+      isValid: true,
+      shouldSwapAxes: true,
+      adjustedYml,
+    };
+  }
+
+  // Y has non-numeric columns and X is also non-numeric (or empty)
+  // This is an error condition
+  const nonNumericYColumns = yColumns.filter(
+    (_col: string, index: number) => !yAxisNumericStatus[index]
+  );
+  const columnTypes = nonNumericYColumns
+    .map((col: string) => {
+      const format = columnLabelFormats[col];
+      return `${col} (${format?.columnType || 'unknown'})`;
+    })
+    .join(', ');
+
+  return {
+    isValid: false,
+    shouldSwapAxes: false,
+    error: `Bar and line charts require numeric values on the Y axis. The following columns are non-numeric: ${columnTypes}. Please adjust your SQL query to ensure numeric columns are used for the Y axis, or use a different chart type.`,
+  };
+}

packages/ai/src/tools/visualization-tools/create-metrics-file-tool.ts

@@ -9,6 +9,7 @@ import { z } from 'zod';
 import { getWorkflowDataSourceManager } from '../../utils/data-source-manager';
 import { createPermissionErrorMessage, validateSqlPermissions } from '../../utils/sql-permissions';
 import type { AnalystRuntimeContext } from '../../workflows/analyst-workflow';
+import { validateAndAdjustBarLineAxes } from './bar-line-axis-validator';
 import { trackFileAssociations } from './file-tracking-helper';
 import { createInitialMetricVersionHistory, validateMetricYml } from './version-history-helpers';
 import type { MetricYml } from './version-history-types';
@@ -1183,12 +1184,27 @@ async function processMetricFile(
     const parsedYml = yaml.parse(fixedYmlContent);
     const metricYml = validateMetricYml(parsedYml);
 
+    // Validate and adjust bar/line chart axes
+    const axisValidation = validateAndAdjustBarLineAxes(metricYml);
+    if (!axisValidation.isValid) {
+      return {
+        success: false,
+        error: axisValidation.error || 'Invalid bar/line chart axis configuration',
+      };
+    }
+
+    // Use adjusted YAML if axes were swapped
+    const finalMetricYml =
+      axisValidation.shouldSwapAxes && axisValidation.adjustedYml
+        ? axisValidation.adjustedYml
+        : metricYml;
+
     // Generate deterministic UUID (simplified version)
     const metricId = randomUUID();
 
     // Validate SQL by running it
     const sqlValidationResult = await validateSql(
-      metricYml.sql,
+      finalMetricYml.sql,
       dataSourceId,
       workflowId,
       userId,
@@ -1206,7 +1222,7 @@ async function processMetricFile(
     const now = new Date().toISOString();
     const metricFile: FileWithId = {
       id: metricId,
-      name: metricYml.name,
+      name: finalMetricYml.name,
       file_type: 'metric',
       result_message: sqlValidationResult.message || '',
       results: sqlValidationResult.results || [],
@@ -1218,7 +1234,7 @@ async function processMetricFile(
     return {
       success: true,
       metricFile,
-      metricYml,
+      metricYml: finalMetricYml,
       message: sqlValidationResult.message || '',
       results: sqlValidationResult.results || [],
     };

packages/ai/src/tools/visualization-tools/modify-metrics-file-tool.ts

@@ -10,6 +10,7 @@ import { z } from 'zod';
 import { getWorkflowDataSourceManager } from '../../utils/data-source-manager';
 import { createPermissionErrorMessage, validateSqlPermissions } from '../../utils/sql-permissions';
 import type { AnalystRuntimeContext } from '../../workflows/analyst-workflow';
+import { validateAndAdjustBarLineAxes } from './bar-line-axis-validator';
 import { trackFileAssociations } from './file-tracking-helper';
 import {
   addMetricVersionToHistory,
@@ -455,7 +456,34 @@ async function processMetricFileUpdate(
     const parsedYml = yaml.parse(fixedYmlContent);
     const metricYml = validateMetricYml(parsedYml);
 
-    const newMetricYml = metricYml;
+    // Validate and adjust bar/line chart axes
+    const axisValidation = validateAndAdjustBarLineAxes(metricYml);
+    if (!axisValidation.isValid) {
+      const error = axisValidation.error || 'Invalid bar/line chart axis configuration';
+      modificationResults.push({
+        file_id: existingFile.id,
+        file_name: existingFile.name,
+        success: false,
+        error,
+        modification_type: 'axis_validation',
+        timestamp,
+        duration,
+      });
+      return {
+        success: false,
+        modificationResults,
+        validationMessage: '',
+        validationResults: [],
+        validatedDatasetIds: [],
+        error,
+      };
+    }
+
+    // Use adjusted YAML if axes were swapped
+    const newMetricYml =
+      axisValidation.shouldSwapAxes && axisValidation.adjustedYml
+        ? axisValidation.adjustedYml
+        : metricYml;
 
     // Check if SQL has changed to avoid unnecessary validation
     const existingContent = existingFile.content as MetricYml | null;