Alerting: Always omit IDs from all routes when updating notification policy tree (#84304)

public/app/features/alerting/unified/NotificationPolicies.tsx

@@ -39,6 +39,7 @@ import { initialAsyncRequestState } from './utils/redux';
 import {
   InsertPosition,
   addRouteToReferenceRoute,
+  cleanRouteIDs,
   mergePartialAmRouteWithRouteTree,
   omitRouteFromRouteTree,
 } from './utils/routeTree';
@@ -152,11 +153,14 @@ const AmRoutes = () => {
     updateRouteTree(newRouteTree);
   }
 
-  function updateRouteTree(routeTree: Route) {
+  function updateRouteTree(routeTree: Route | RouteWithID) {
     if (!result) {
       return;
     }
 
+    // make sure we omit all IDs from our routes
+    const newRouteTree = cleanRouteIDs(routeTree);
+
     setUpdatingTree(true);
 
     dispatch(
@@ -165,7 +169,7 @@ const AmRoutes = () => {
           ...result,
           alertmanager_config: {
             ...result.alertmanager_config,
-            route: routeTree,
+            route: newRouteTree,
           },
         },
         oldConfig: result,

public/app/features/alerting/unified/utils/routeTree.test.ts

@@ -3,7 +3,7 @@ import { RouteWithID } from 'app/plugins/datasource/alertmanager/types';
 import { FormAmRoute } from '../types/amroutes';
 
 import { GRAFANA_DATASOURCE_NAME } from './datasource';
-import { addRouteToReferenceRoute, findRouteInTree, omitRouteFromRouteTree } from './routeTree';
+import { addRouteToReferenceRoute, cleanRouteIDs, findRouteInTree, omitRouteFromRouteTree } from './routeTree';
 
 describe('findRouteInTree', () => {
   it('should find the correct route', () => {
@@ -69,9 +69,10 @@ describe('omitRouteFromRouteTree', () => {
       ],
     };
 
-    expect(omitRouteFromRouteTree({ id: 'route-2' }, tree)).toStrictEqual({
+    expect(omitRouteFromRouteTree({ id: 'route-2' }, tree)).toEqual({
+      id: 'route-1',
       receiver: 'root',
-      routes: [{ receiver: 'receiver-3', routes: undefined }],
+      routes: [{ id: 'route-3', receiver: 'receiver-3', routes: undefined }],
     });
   });
 
@@ -85,3 +86,22 @@ describe('omitRouteFromRouteTree', () => {
     }).toThrow();
   });
 });
+
+describe('cleanRouteIDs', () => {
+  it('should remove IDs from routesr recursively', () => {
+    expect(
+      cleanRouteIDs({
+        id: '1',
+        receiver: '1',
+        routes: [
+          { id: '2', receiver: '2' },
+          { id: '3', receiver: '3' },
+        ],
+      })
+    ).toEqual({ receiver: '1', routes: [{ receiver: '2' }, { receiver: '3' }] });
+  });
+
+  it('should also accept regular routes', () => {
+    expect(cleanRouteIDs({ receiver: 'test' })).toEqual({ receiver: 'test' });
+  });
+});

public/app/features/alerting/unified/utils/routeTree.ts

@@ -28,22 +28,16 @@ export const mergePartialAmRouteWithRouteTree = (
 
     if (currentRoute.id === partialFormRoute.id) {
       const newRoute = formAmRouteToAmRoute(alertManagerSourceName, partialFormRoute, routeTree);
-      updatedRoute = omit(
-        {
-          ...currentRoute,
-          ...newRoute,
-        },
-        'id'
-      );
+      updatedRoute = {
+        ...currentRoute,
+        ...newRoute,
+      };
     }
 
-    return omit(
-      {
-        ...updatedRoute,
-        routes: currentRoute.routes?.map(findAndReplace),
-      },
-      'id'
-    );
+    return {
+      ...updatedRoute,
+      routes: currentRoute.routes?.map(findAndReplace),
+    };
   }
 
   return findAndReplace(routeTree);
@@ -51,26 +45,23 @@ export const mergePartialAmRouteWithRouteTree = (
 
 // remove a route from the policy tree, returns a new tree
 // make sure to omit the "id" because Prometheus / Loki / Mimir will reject the payload
-export const omitRouteFromRouteTree = (findRoute: RouteWithID, routeTree: RouteWithID): Route => {
+export const omitRouteFromRouteTree = (findRoute: RouteWithID, routeTree: RouteWithID): RouteWithID => {
   if (findRoute.id === routeTree.id) {
     throw new Error('You cant remove the root policy');
   }
 
-  function findAndOmit(currentRoute: RouteWithID): Route {
-    return omit(
-      {
-        ...currentRoute,
-        routes: currentRoute.routes?.reduce((acc: Route[] = [], route) => {
-          if (route.id === findRoute.id) {
-            return acc;
-          }
-
-          acc.push(findAndOmit(route));
+  function findAndOmit(currentRoute: RouteWithID): RouteWithID {
+    return {
+      ...currentRoute,
+      routes: currentRoute.routes?.reduce((acc: RouteWithID[] = [], route) => {
+        if (route.id === findRoute.id) {
           return acc;
-        }, []),
-      },
-      'id'
-    );
+        }
+
+        acc.push(route);
+        return acc;
+      }, []),
+    };
   }
 
   return findAndOmit(routeTree);
@@ -85,7 +76,7 @@ export const addRouteToReferenceRoute = (
   referenceRoute: RouteWithID,
   routeTree: RouteWithID,
   position: InsertPosition
-): Route => {
+): RouteWithID => {
   const newRoute = formAmRouteToAmRoute(alertManagerSourceName, partialFormRoute, routeTree);
 
   return produce(routeTree, (draftTree) => {
@@ -148,6 +139,16 @@ export function findRouteInTree(
   return [matchingRoute, matchingRouteParent, matchingRoutePositionInParent];
 }
 
+export function cleanRouteIDs(route: Route | RouteWithID): Route {
+  return omit(
+    {
+      ...route,
+      routes: route.routes?.map((route) => cleanRouteIDs(route)),
+    },
+    'id'
+  );
+}
+
 export function findExistingRoute(id: string, routeTree: RouteWithID): RouteWithID | undefined {
   return routeTree.id === id ? routeTree : routeTree.routes?.find((route) => findExistingRoute(id, route));
 }