From dac46eb41f5c6eb78bae233e893e8c10a4a7f328 Mon Sep 17 00:00:00 2001 From: Konrad Lalik Date: Wed, 17 Dec 2025 16:58:21 +0100 Subject: [PATCH] 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. --- .../__snapshots__/reducer.test.tsx.snap | 4 +- .../reducer.test.tsx | 71 +++++++++++++++++ .../query-and-alert-condition/reducer.ts | 17 ++++- .../components/rule-editor/util.test.ts | 76 +++++++++++++++++++ .../unified/components/rule-editor/util.ts | 64 ++++++++++++++++ 5 files changed, 226 insertions(+), 6 deletions(-) diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/reducer.test.tsx.snap b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/reducer.test.tsx.snap index 33f3b0ea4e8..32ed403141f 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/reducer.test.tsx.snap +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/reducer.test.tsx.snap @@ -316,9 +316,7 @@ exports[`Query and expressions reducer should set data queries 1`] = ` "type": "and", }, "query": { - "params": [ - "A", - ], + "params": [], }, "reducer": { "params": [], diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.test.tsx b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.test.tsx index a4a214b8348..f6ab83d2737 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.test.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.test.tsx @@ -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 = { + 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(); + }); + }); }); diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.ts b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.ts index 1514533cce0..1f70e3792cb 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.ts +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.ts @@ -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)); diff --git a/public/app/features/alerting/unified/components/rule-editor/util.test.ts b/public/app/features/alerting/unified/components/rule-editor/util.test.ts index 534156ac1f4..c502caec246 100644 --- a/public/app/features/alerting/unified/components/rule-editor/util.test.ts +++ b/public/app/features/alerting/unified/components/rule-editor/util.test.ts @@ -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', () => { diff --git a/public/app/features/alerting/unified/components/rule-editor/util.ts b/public/app/features/alerting/unified/components/rule-editor/util.ts index b8e20cba6b4..0820275ebb7 100644 --- a/public/app/features/alerting/unified/components/rule-editor/util.ts +++ b/public/app/features/alerting/unified/components/rule-editor/util.ts @@ -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 + '}';