Alerting: Clear dangling references when removing queries or expressions

Fixes an issue where removing a query or expression that was referenced by other expressions would leave dangling references, causing DAG creation errors and UI inconsistencies. When a query/expression is removed, all expressions that reference it now have their references automatically cleared: - Reduce/threshold/resample expressions: Set expression field to null - Math expressions: Remove $refId and ${refId} patterns from expression string - Classic conditions: Remove refId from params array This prevents console errors and ensures UI components (like Select dropdowns) properly display "no selection" instead of showing invalid references.
2025-12-17 16:58:21 +01:00
parent fe49ae05c0
commit dac46eb41f
5 changed files with 226 additions and 6 deletions
@@ -316,9 +316,7 @@ exports[`Query and expressions reducer should set data queries 1`] = `
              "type": "and",
            },
            "query": {
-              "params": [
-                "A",
-              ],
+              "params": [],
            },
            "reducer": {
              "params": [],
@@ -9,6 +9,8 @@ import {
 import { defaultCondition } from 'app/features/expressions/utils/expressionTypes';
 import { AlertQuery } from 'app/types/unified-alerting-dto';

+import { mockDataQuery, mockReduceExpression, mockThresholdExpression } from '../../../mocks';
+
 import {
  QueriesAndExpressionsState,
  addNewDataQuery,
@@ -453,4 +455,73 @@ describe('Query and expressions reducer', () => {
    );
    expect(newState).toMatchSnapshot();
  });
+
+  describe('dangling reference handling', () => {
+    it('should clear expression reference when removing a data query that is referenced by a reduce expression', () => {
+      const dataQuery = mockDataQuery({ refId: 'A' });
+      const reduceExpr = mockReduceExpression({ refId: 'B', expression: 'A' });
+
+      const initialState: QueriesAndExpressionsState = {
+        queries: [dataQuery, reduceExpr],
+      };
+
+      // Remove the data query A
+      const newState = queriesAndExpressionsReducer(initialState, removeExpression('A'));
+
+      // The reduce expression should still exist but its reference should be cleared
+      expect(newState.queries).toHaveLength(1);
+      expect(newState.queries[0].refId).toBe('B');
+      expect(newState.queries[0].model.expression).toBeNull();
+    });
+
+    it('should clear expression reference when removing a data query via setDataQueries', () => {
+      const dataQuery = mockDataQuery({ refId: 'A' });
+      const mathExpr: AlertQuery<ExpressionQuery> = {
+        refId: 'C',
+        queryType: 'expression',
+        datasourceUid: ExpressionDatasourceUID,
+        model: {
+          refId: 'C',
+          type: ExpressionQueryType.math,
+          expression: '$A + 10', // references data query A
+          datasource: {
+            type: '__expr__',
+            uid: '__expr__',
+          },
+        },
+      };
+
+      const initialState: QueriesAndExpressionsState = {
+        queries: [dataQuery, mathExpr],
+      };
+
+      // Remove all data queries (simulating user deleting query A)
+      const newState = queriesAndExpressionsReducer(initialState, setDataQueries([]));
+
+      // The math expression should still exist but reference to A should be cleared
+      expect(newState.queries).toHaveLength(1);
+      expect(newState.queries[0].refId).toBe('C');
+      // Math expressions with dangling refs should have them removed from the expression string
+      expect(newState.queries[0].model.expression).not.toContain('$A');
+    });
+
+    it('should clear expression reference when removing an expression that is referenced by another expression', () => {
+      const dataQuery = mockDataQuery({ refId: 'A' });
+      const reduceExpr = mockReduceExpression({ refId: 'B', expression: 'A' });
+      const thresholdExpr = mockThresholdExpression({ refId: 'C', expression: 'B' });
+
+      const initialState: QueriesAndExpressionsState = {
+        queries: [dataQuery, reduceExpr, thresholdExpr],
+      };
+
+      // Remove expression B which is referenced by C
+      const newState = queriesAndExpressionsReducer(initialState, removeExpression('B'));
+
+      // Both A and C should remain, but C's reference to B should be cleared
+      expect(newState.queries).toHaveLength(2);
+      expect(newState.queries.map((q) => q.refId)).toEqual(['A', 'C']);
+      const thresholdQuery = newState.queries.find((q) => q.refId === 'C');
+      expect(thresholdQuery?.model.expression).toBeNull();
+    });
+  });
 });
@@ -23,7 +23,7 @@ import { logError } from '../../../Analytics';
 import { getDefaultOrFirstCompatibleDataSource } from '../../../utils/datasource';
 import { getDefaultQueries, getInstantFromDataQuery } from '../../../utils/rule-form';
 import { createDagFromQueries, getOriginOfRefId } from '../dag';
-import { queriesWithUpdatedReferences, refIdExists } from '../util';
+import { queriesWithRemovedReferences, queriesWithUpdatedReferences, refIdExists } from '../util';

 // this one will be used as the refID when we create a new reducer for the threshold expression
 export const NEW_REDUCER_REF = 'reducer';
@@ -101,7 +101,17 @@ export const queriesAndExpressionsReducer = createReducer(initialState, (builder
      });
    })
    .addCase(setDataQueries, (state, { payload }) => {
-      const expressionQueries = state.queries.filter((query) => isExpressionQuery(query.model));
+      const previousDataQueries = state.queries.filter((query) => !isExpressionQuery(query.model));
+      const removedRefIds = previousDataQueries
+        .filter((q) => !payload.some((p) => p.refId === q.refId))
+        .map((q) => q.refId);
+
+      let expressionQueries = state.queries.filter((query) => isExpressionQuery(query.model));
+
+      for (const removedRefId of removedRefIds) {
+        expressionQueries = queriesWithRemovedReferences(expressionQueries, removedRefId);
+      }
+
      state.queries = [...payload, ...expressionQueries];
    })
    .addCase(setRecordingRulesQueries, (state, { payload }) => {
@@ -153,7 +163,8 @@ export const queriesAndExpressionsReducer = createReducer(initialState, (builder
      });
    })
    .addCase(removeExpression, (state, { payload }) => {
-      state.queries = state.queries.filter((query) => query.refId !== payload);
+      const filteredQueries = state.queries.filter((query) => query.refId !== payload);
+      state.queries = queriesWithRemovedReferences(filteredQueries, payload);
    })
    .addCase(removeExpressions, (state) => {
      state.queries = state.queries.filter((query) => !isExpressionQuery(query.model));
@@ -7,7 +7,9 @@ import {
  containsPathSeparator,
  findRenamedDataQueryReferences,
  getThresholdsForQueries,
+  queriesWithRemovedReferences,
  queriesWithUpdatedReferences,
+  removeMathExpressionRef,
  updateMathExpressionRefs,
 } from './util';

@@ -228,6 +230,80 @@ describe('rule-editor', () => {
      expect(updateMathExpressionRefs('$A3 + $B', 'A', 'C')).toBe('$A3 + $B');
    });
  });
+
+  describe('queriesWithRemovedReferences', () => {
+    it('should clear reference in reduce expression when data query is removed', () => {
+      const queries: AlertQuery[] = [dataSource, reduceExpression];
+      const updatedQueries = queriesWithRemovedReferences(queries, 'A');
+
+      expect(updatedQueries[0]).toEqual(dataSource);
+      expect(updatedQueries[1].model.expression).toBeNull();
+    });
+
+    it('should clear reference in threshold expression when expression is removed', () => {
+      const queries: AlertQuery[] = [dataSource, reduceExpression, thresholdExpression];
+      const updatedQueries = queriesWithRemovedReferences(queries, 'B');
+
+      expect(updatedQueries[0]).toEqual(dataSource);
+      expect(updatedQueries[1]).toEqual(reduceExpression);
+      expect(updatedQueries[2].model.expression).toBeNull();
+    });
+
+    it('should remove reference from math expression', () => {
+      const queries: AlertQuery[] = [dataSource, mathExpression];
+      const updatedQueries = queriesWithRemovedReferences(queries, 'A');
+
+      const mathModel = updatedQueries[1].model as ExpressionQuery;
+      expect(mathModel.expression).not.toContain('$A');
+      expect(mathModel.expression).not.toContain('${A}');
+    });
+
+    it('should remove refId from classic condition params', () => {
+      const queries: AlertQuery[] = [dataSource, classicCondition];
+      const updatedQueries = queriesWithRemovedReferences(queries, 'A');
+
+      const classicModel = updatedQueries[1].model as ExpressionQuery;
+      expect(classicModel.conditions?.[0].query.params).toEqual([]);
+    });
+
+    it('should not modify queries that do not reference the removed refId', () => {
+      const dataSource2 = { ...dataSource, refId: 'B' };
+      const reduceB = { ...reduceExpression, refId: 'C', model: { ...reduceExpression.model, expression: 'B' } };
+      const queries: AlertQuery[] = [dataSource, dataSource2, reduceB];
+
+      const updatedQueries = queriesWithRemovedReferences(queries, 'A');
+
+      expect(updatedQueries[0]).toEqual(dataSource);
+      expect(updatedQueries[1]).toEqual(dataSource2);
+      expect(updatedQueries[2]).toEqual(reduceB);
+    });
+
+    it('should handle resample expressions', () => {
+      const queries: AlertQuery[] = [dataSource, resampleExpression];
+      const updatedQueries = queriesWithRemovedReferences(queries, 'A');
+
+      expect(updatedQueries[1].model.expression).toBeNull();
+    });
+  });
+
+  describe('removeMathExpressionRef', () => {
+    it('should remove $A pattern', () => {
+      expect(removeMathExpressionRef('$A + 10', 'A')).toBe('+ 10');
+    });
+
+    it('should remove ${A} pattern', () => {
+      expect(removeMathExpressionRef('${A} + 10', 'A')).toBe('+ 10');
+    });
+
+    it('should remove multiple references', () => {
+      const result = removeMathExpressionRef('$A + $A * 2', 'A');
+      expect(result.replace(/\s+/g, ' ').trim()).toBe('+ * 2');
+    });
+
+    it('should not remove partial matches', () => {
+      expect(removeMathExpressionRef('$ABC + 10', 'A')).toBe('$ABC + 10');
+    });
+  });
 });

 describe('containsPathSeparator', () => {
@@ -75,6 +75,70 @@ export function queriesWithUpdatedReferences(
  });
 }

+export function queriesWithRemovedReferences(queries: AlertQuery[], removedRefId: string): AlertQuery[] {
+  return queries.map((query) => {
+    if (!isExpressionQuery(query.model)) {
+      return query;
+    }
+
+    const isMathExpression = query.model.type === 'math';
+    const isReduceExpression = query.model.type === 'reduce';
+    const isResampleExpression = query.model.type === 'resample';
+    const isClassicExpression = query.model.type === 'classic_conditions';
+    const isThresholdExpression = query.model.type === 'threshold';
+    const isSqlExpression = query.model.type === 'sql';
+
+    if (isMathExpression) {
+      const updatedExpression = removeMathExpressionRef(query.model.expression ?? '', removedRefId);
+      return {
+        ...query,
+        model: {
+          ...query.model,
+          expression: updatedExpression || undefined,
+        },
+      };
+    }
+
+    if (isResampleExpression || isReduceExpression || isThresholdExpression) {
+      const isReferencing = query.model.expression === removedRefId;
+      return {
+        ...query,
+        model: {
+          ...query.model,
+          // Set to null so Select component shows no selection
+          expression: isReferencing ? null : query.model.expression,
+        },
+      };
+    }
+
+    if (isSqlExpression) {
+      // SQL expressions reference table names, not query refIds in the same way
+      // For now, we'll leave SQL expressions unchanged as they work differently
+      return query;
+    }
+
+    if (isClassicExpression) {
+      const conditions = query.model.conditions?.map((condition) => ({
+        ...condition,
+        query: {
+          ...condition.query,
+          params: condition.query.params.filter((param: string) => param !== removedRefId),
+        },
+      }));
+
+      return { ...query, model: { ...query.model, conditions } };
+    }
+
+    return query;
+  });
+}
+
+export function removeMathExpressionRef(expression: string, refIdToRemove: string): string {
+  // Remove both $refId and ${refId} patterns
+  const refPattern = new RegExp('(\\$' + refIdToRemove + '\\b)|(\\${' + refIdToRemove + '})', 'gm');
+  return expression.replace(refPattern, '').trim();
+}
+
 export function updateMathExpressionRefs(expression: string, previousRefId: string, newRefId: string): string {
  const oldExpression = new RegExp('(\\$' + previousRefId + '\\b)|(\\${' + previousRefId + '})', 'gm');
  const newExpression = '${' + newRefId + '}';