Alerting: Patch missing expression model refIds (#114477)

public/app/features/alerting/unified/components/expressions/Expression.tsx

@@ -48,7 +48,6 @@ interface ExpressionProps {
   onSetCondition: (refId: string) => void;
   onUpdateRefId: (oldRefId: string, newRefId: string) => void;
   onRemoveExpression: (refId: string) => void;
-  onUpdateExpressionType: (refId: string, type: ExpressionQueryType) => void;
   onChangeQuery: (query: ExpressionQuery) => void;
 }
 
@@ -62,7 +61,6 @@ export const Expression: FC<ExpressionProps> = ({
   onSetCondition,
   onUpdateRefId,
   onRemoveExpression,
-  onUpdateExpressionType, // this method is not used? maybe we should remove it
   onChangeQuery,
 }) => {
   const styles = useStyles2(getStyles);

public/app/features/alerting/unified/components/rule-editor/ExpressionsEditor.tsx

@@ -4,7 +4,7 @@ import { useMemo } from 'react';
 import { GrafanaTheme2, PanelData } from '@grafana/data';
 import { useStyles2 } from '@grafana/ui';
 import { isExpressionQuery } from 'app/features/expressions/guards';
-import { ExpressionQuery, ExpressionQueryType } from 'app/features/expressions/types';
+import { ExpressionQuery } from 'app/features/expressions/types';
 import { AlertQuery } from 'app/types/unified-alerting-dto';
 
 import { Expression } from '../expressions/Expression';
@@ -18,7 +18,6 @@ interface Props {
   queries: AlertQuery[];
   onRemoveExpression: (refId: string) => void;
   onUpdateRefId: (oldRefId: string, newRefId: string) => void;
-  onUpdateExpressionType: (refId: string, type: ExpressionQueryType) => void;
   onUpdateQueryExpression: (query: ExpressionQuery) => void;
 }
 
@@ -29,12 +28,15 @@ export const ExpressionsEditor = ({
   panelData,
   onUpdateRefId,
   onRemoveExpression,
-  onUpdateExpressionType,
   onUpdateQueryExpression,
 }: Props) => {
   const expressionQueries = useMemo(() => {
     return queries.reduce((acc: ExpressionQuery[], query) => {
-      return isExpressionQuery(query.model) ? acc.concat(query.model) : acc;
+      if (isExpressionQuery(query.model)) {
+        acc.push(query.model);
+      }
+
+      return acc;
     }, []);
   }, [queries]);
   const styles = useStyles2(getStyles);
@@ -64,7 +66,6 @@ export const ExpressionsEditor = ({
             onSetCondition={onSetCondition}
             onRemoveExpression={onRemoveExpression}
             onUpdateRefId={onUpdateRefId}
-            onUpdateExpressionType={onUpdateExpressionType}
             onChangeQuery={onUpdateQueryExpression}
           />
         );

public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx

@@ -73,7 +73,6 @@ import {
   updateExpression,
   updateExpressionRefId,
   updateExpressionTimeRange,
-  updateExpressionType,
 } from './reducer';
 import { useAdvancedMode } from './useAdvancedMode';
 import { useAlertQueryRunner } from './useAlertQueryRunner';
@@ -591,9 +590,6 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange, mod
                     dispatch(removeExpression(refId));
                   }}
                   onUpdateRefId={onUpdateRefId}
-                  onUpdateExpressionType={(refId, type) => {
-                    dispatch(updateExpressionType({ refId, type }));
-                  }}
                   onUpdateQueryExpression={(model) => {
                     dispatch(updateExpression(model));
                   }}

public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/reducer.test.tsx.snap

@@ -442,55 +442,3 @@ exports[`Query and expressions reducer should update an expression refId and rew
   ],
 }
 `;
-
-exports[`Query and expressions reducer should update expression type 1`] = `
-{
-  "queries": [
-    {
-      "datasourceUid": "abc123",
-      "model": {
-        "refId": "A",
-      },
-      "queryType": "query",
-      "refId": "A",
-    },
-    {
-      "datasourceUid": "__expr__",
-      "model": {
-        "conditions": [
-          {
-            "evaluator": {
-              "params": [
-                0,
-                0,
-              ],
-              "type": "gt",
-            },
-            "operator": {
-              "type": "and",
-            },
-            "query": {
-              "params": [],
-            },
-            "reducer": {
-              "params": [],
-              "type": "avg",
-            },
-            "type": "query",
-          },
-        ],
-        "datasource": {
-          "name": "Expression",
-          "type": "__expr__",
-          "uid": "__expr__",
-        },
-        "expression": "",
-        "refId": "B",
-        "type": "reduce",
-      },
-      "queryType": "",
-      "refId": "B",
-    },
-  ],
-}
-`;

public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.test.tsx

@@ -22,7 +22,6 @@ import {
   updateExpression,
   updateExpressionRefId,
   updateExpressionTimeRange,
-  updateExpressionType,
 } from './reducer';
 
 const reduceExpression: AlertQuery<ExpressionQuery> = {
@@ -388,22 +387,6 @@ describe('Query and expressions reducer', () => {
 
     expect(newState).toMatchSnapshot();
   });
-
-  it('should update expression type', () => {
-    const initialState: QueriesAndExpressionsState = {
-      queries: [alertQuery, expressionQuery],
-    };
-
-    const newState = queriesAndExpressionsReducer(
-      initialState,
-      updateExpressionType({
-        refId: 'B',
-        type: ExpressionQueryType.reduce,
-      })
-    );
-
-    expect(newState).toMatchSnapshot();
-  });
   it('should remove first reducer', () => {
     const initialState: QueriesAndExpressionsState = {
       queries: [alertQuery, reduceExpression, thresholdExpression],

public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.ts

@@ -283,23 +283,6 @@ export const queriesAndExpressionsReducer = createReducer(initialState, (builder
           queryType: 'expression',
         });
       }
-    })
-    .addCase(updateExpressionType, (state, action) => {
-      state.queries = state.queries.map((query) => {
-        return query.refId === action.payload.refId
-          ? {
-              ...query,
-              model: {
-                ...expressionDatasource.newQuery({
-                  type: action.payload.type,
-                  conditions: [{ ...defaultCondition, query: { params: [] } }],
-                  expression: '',
-                }),
-                refId: action.payload.refId,
-              },
-            }
-          : query;
-      });
     });
 });
 

public/app/features/alerting/unified/utils/rule-form.test.ts

@@ -7,6 +7,7 @@ import {
   GrafanaAlertStateDecision,
   GrafanaRuleDefinition,
   RulerAlertingRuleDTO,
+  RulerGrafanaRuleDTO,
 } from 'app/types/unified-alerting-dto';
 
 import { EvalFunction } from '../../state/alertDef';
@@ -20,6 +21,7 @@ import {
   alertingRulerRuleToRuleForm,
   cleanAnnotations,
   cleanLabels,
+  fixMissingRefIdsInExpressionModel,
   formValuesToRulerGrafanaRuleDTO,
   formValuesToRulerRuleDTO,
   getContactPointsFromDTO,
@@ -520,3 +522,70 @@ describe('getDefaultExpressions', () => {
     expect(thresholdModel.expression).toBe('X');
   });
 });
+
+describe('fixMissingRefIdsInExpressionModel', () => {
+  it('should return non-Grafana managed rules unchanged', () => {
+    const cloudAlertingRule: RulerAlertingRuleDTO = {
+      alert: 'CloudAlert',
+      expr: 'up == 0',
+      for: '5m',
+      labels: { severity: 'critical' },
+      annotations: { summary: 'Instance down' },
+    };
+
+    const result = fixMissingRefIdsInExpressionModel(cloudAlertingRule);
+
+    expect(result).toEqual(cloudAlertingRule);
+    expect(result).toBe(cloudAlertingRule); // should be the exact same reference
+  });
+
+  it('should copy refId from query to model when model.refId is missing in Grafana managed rules', () => {
+    const ruleWithMissingRefId: RulerGrafanaRuleDTO = {
+      grafana_alert: {
+        uid: 'test-uid',
+        title: 'Test Alert',
+        namespace_uid: 'namespace-uid',
+        rule_group: 'test-group',
+        condition: 'B',
+        no_data_state: GrafanaAlertStateDecision.NoData,
+        exec_err_state: GrafanaAlertStateDecision.Alerting,
+        is_paused: false,
+        data: [
+          {
+            refId: 'A',
+            datasourceUid: 'datasource-uid',
+            queryType: '',
+            relativeTimeRange: { from: 600, to: 0 },
+            // @ts-ignore
+            model: {
+              // refId is missing here
+              datasource: {
+                type: 'grafana-testdata-datasource',
+                uid: 'PD8C576611E62080A',
+              },
+            },
+          },
+          {
+            refId: 'B',
+            datasourceUid: ExpressionDatasourceUID,
+            queryType: '',
+            // @ts-ignore
+            model: {
+              // refId is missing here
+              type: ExpressionQueryType.reduce,
+              expression: 'A',
+            },
+          },
+        ],
+      },
+      for: '5m',
+      labels: {},
+      annotations: {},
+    };
+
+    const result = fixMissingRefIdsInExpressionModel(ruleWithMissingRefId);
+
+    expect(result.grafana_alert.data[0].model.refId).toBe('A');
+    expect(result.grafana_alert.data[1].model.refId).toBe('B');
+  });
+});

public/app/features/alerting/unified/utils/rule-form.ts

@@ -1,3 +1,5 @@
+import { produce } from 'immer';
+
 import {
   DataSourceInstanceSettings,
   IntervalValues,
@@ -278,14 +280,16 @@ function getEditorSettingsFromDTO(ga: GrafanaRuleDefinition) {
 
 export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleFormValues {
   const { ruleSourceName, namespace, group, rule } = ruleWithLocation;
-  const isGrafanaRecordingRule = rulerRuleType.grafana.recordingRule(rule);
+  const normalizedRule = fixMissingRefIdsInExpressionModel(rule);
+
+  const isGrafanaRecordingRule = rulerRuleType.grafana.recordingRule(normalizedRule);
 
   const defaultFormValues = getDefaultFormValues(isGrafanaRecordingRule ? RuleFormType.grafanaRecording : undefined);
   if (isGrafanaRulesSource(ruleSourceName)) {
     // GRAFANA-MANAGED RULES
     if (isGrafanaRecordingRule) {
       // grafana recording rule
-      const ga = rule.grafana_alert;
+      const ga = normalizedRule.grafana_alert;
       return {
         ...defaultFormValues,
         name: ga.title,
@@ -294,16 +298,16 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF
         evaluateEvery: group.interval || defaultFormValues.evaluateEvery,
         queries: ga.data,
         condition: ga.condition,
-        annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(rule.annotations, false)),
+        annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(normalizedRule.annotations, false)),
-        labels: listifyLabelsOrAnnotations(rule.labels, true),
+        labels: listifyLabelsOrAnnotations(normalizedRule.labels, true),
         folder: { title: namespace, uid: ga.namespace_uid },
         isPaused: ga.is_paused,
         metric: ga.record?.metric,
         targetDatasourceUid: ga.record?.target_datasource_uid || defaultFormValues.targetDatasourceUid,
       };
-    } else if (rulerRuleType.grafana.rule(rule)) {
+    } else if (rulerRuleType.grafana.rule(normalizedRule)) {
       // grafana alerting rule
-      const ga = rule.grafana_alert;
+      const ga = normalizedRule.grafana_alert;
       const routingSettings: AlertManagerManualRouting | undefined = getContactPointsFromDTO(ga);
       if (ga.no_data_state !== undefined && ga.exec_err_state !== undefined) {
         return {
@@ -312,14 +316,14 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF
           type: RuleFormType.grafana,
           group: group.name,
           evaluateEvery: group.interval || defaultFormValues.evaluateEvery,
-          evaluateFor: rule.for || '0',
+          evaluateFor: normalizedRule.for || '0',
-          keepFiringFor: rule.keep_firing_for || '0',
+          keepFiringFor: normalizedRule.keep_firing_for || '0',
           noDataState: ga.no_data_state,
           execErrState: ga.exec_err_state,
           queries: ga.data,
           condition: ga.condition,
-          annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(rule.annotations, false)),
+          annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(normalizedRule.annotations, false)),
-          labels: listifyLabelsOrAnnotations(rule.labels, true),
+          labels: listifyLabelsOrAnnotations(normalizedRule.labels, true),
           folder: { title: namespace, uid: ga.namespace_uid },
           isPaused: ga.is_paused,
 
@@ -338,7 +342,7 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF
     }
   } else {
     // DATASOURCE-MANAGED RULES
-    if (rulerRuleType.dataSource.alertingRule(rule)) {
+    if (rulerRuleType.dataSource.alertingRule(normalizedRule)) {
       const datasourceUid = getDataSourceSrv().getInstanceSettings(ruleSourceName)?.uid ?? '';
 
       const defaultQuery = {
@@ -346,27 +350,27 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF
         datasourceUid,
         queryType: '',
         relativeTimeRange: getDefaultRelativeTimeRange(),
-        expr: rule.expr,
+        expr: normalizedRule.expr,
         model: {
           refId: 'A',
           hide: false,
-          expr: rule.expr,
+          expr: normalizedRule.expr,
         },
       };
 
-      const alertingRuleValues = alertingRulerRuleToRuleForm(rule);
+      const alertingRuleValues = alertingRulerRuleToRuleForm(normalizedRule);
 
       return {
         ...defaultFormValues,
         ...alertingRuleValues,
         queries: [defaultQuery],
-        annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(rule.annotations, false)),
+        annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(normalizedRule.annotations, false)),
         type: RuleFormType.cloudAlerting,
         dataSourceName: ruleSourceName,
         namespace,
         group: group.name,
       };
-    } else if (rulerRuleType.dataSource.recordingRule(rule)) {
+    } else if (rulerRuleType.dataSource.recordingRule(normalizedRule)) {
       const datasourceUid = getDataSourceSrv().getInstanceSettings(ruleSourceName)?.uid ?? '';
 
       const defaultQuery = {
@@ -374,15 +378,15 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF
         datasourceUid,
         queryType: '',
         relativeTimeRange: getDefaultRelativeTimeRange(),
-        expr: rule.expr,
+        expr: normalizedRule.expr,
         model: {
           refId: 'A',
           hide: false,
-          expr: rule.expr,
+          expr: normalizedRule.expr,
         },
       };
 
-      const recordingRuleValues = recordingRulerRuleToRuleForm(rule);
+      const recordingRuleValues = recordingRulerRuleToRuleForm(normalizedRule);
 
       return {
         ...defaultFormValues,
@@ -399,6 +403,23 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF
   }
 }
 
+/**
+ * This function isn't supposed to be needed, but we've noticed some customers are creating rules via Provisioning or
+ * other interfaces where they aren't including the RefId in the "model" of the expression so copy the refId from the query definition.
+ */
+export function fixMissingRefIdsInExpressionModel<T extends RulerRuleDTO>(rule: T): T {
+  // non-Grafana managed rules don't use expression nodes so we return the rule as-is
+  if (!rulerRuleType.grafana.rule(rule)) {
+    return rule;
+  }
+
+  return produce(rule, (draft) => {
+    draft.grafana_alert.data.forEach((query) => {
+      query.model.refId = query.model.refId ?? query.refId;
+    });
+  });
+}
+
 export function grafanaRuleDtoToFormValues(rule: RulerGrafanaRuleDTO, namespace: string): RuleFormValues {
   const isGrafanaRecordingRule = rulerRuleType.grafana.recordingRule(rule);
   const defaultFormValues = getDefaultFormValues(isGrafanaRecordingRule ? RuleFormType.grafanaRecording : undefined);