Graphite: second function as another function argument parsing error fix (#85224)

* update language around query editor parsing issues

* add special handling for second function arguments in divideSeriesLists

* additional check for first argument as series(string) and not a function

docs/sources/datasources/graphite/query-editor/index.md

@@ -55,6 +55,10 @@ Some functions like aliasByNode support an optional second argument. To add an a
 
 To learn more, refer to [Graphite's documentation on functions](https://graphite.readthedocs.io/en/latest/functions.html).
 
+{{% admonition type="warning" %}}
+Some functions take a second argument that may be a function that returns a series. If you are adding a second argument that is a function, it is suggested to use a series reference from a second query instead of the function itself. The query editor does not currently support parsing of a second argument that is a function when switching between the query editor and the code editor.
+{{% /admonition %}}
+
 ### Sort labels
 
 If you have the same labels on multiple graphs, they are both sorted differently and use different colors.

public/app/plugins/datasource/graphite/graphite_query.ts

@@ -114,6 +114,8 @@ export default class GraphiteQuery {
         // bug fix for parsing multiple functions as params
         handleMultipleSeriesByTagsParams(astNode);
 
+        handleDivideSeriesListsNestedFunctions(astNode);
+
         each(astNode.params, (param) => {
           this.parseTargetRecursive(param, innerFunc);
         });
@@ -367,3 +369,67 @@ function handleMultipleSeriesByTagsParams(astNode: AstNode) {
     });
   }
 }
+
+/**
+ * Converts all nested functions as parametors (recursively) to strings
+ */
+function handleDivideSeriesListsNestedFunctions(astNode: AstNode) {
+  // if divideSeriesLists function, the second parameters should be strings
+  if (astNode.name === 'divideSeriesLists' && astNode.params && astNode.params.length >= 2) {
+    astNode.params = astNode.params.map((p: AstNode, idx: number) => {
+      if (idx === 1 && p.type === 'function') {
+        // convert nested 2nd functions as parametors to a strings
+        // all nested functions should be strings
+        // if the node is a function it will have params
+        // if these params are functions, they will have params
+        // at some point we will have to add the params as strings
+        // then wrap them in the function
+        let functionString = '';
+        let s = p.name + '(' + nestedFunctionsToString(p, functionString);
+
+        p = {
+          type: 'string',
+          value: s,
+        };
+      }
+
+      return p;
+    });
+  }
+
+  return astNode;
+}
+
+function nestedFunctionsToString(node: AstNode, functionString: string): string | undefined {
+  let count = 0;
+  if (node.params) {
+    count++;
+
+    const paramsLength = node.params?.length ?? 0;
+
+    node.params.forEach((innerNode: AstNode, idx: number) => {
+      if (idx < paramsLength - 1) {
+        functionString += switchCase(innerNode, functionString) + ',';
+      } else {
+        functionString += switchCase(innerNode, functionString);
+      }
+    });
+
+    return functionString + ')';
+  } else {
+    return (functionString += switchCase(node, functionString));
+  }
+}
+
+function switchCase(node: AstNode, functionString: string) {
+  switch (node.type) {
+    case 'function':
+      functionString += node.name + '(';
+      return nestedFunctionsToString(node, functionString);
+    case 'metric':
+      const segmentString = join(map(node.segments, 'value'), '.');
+      return segmentString;
+    default:
+      return node.value;
+  }
+}

public/app/plugins/datasource/graphite/specs/graphite_query.test.ts

@@ -111,11 +111,11 @@ describe('Graphite query model', () => {
     });
   });
 
-  describe('when query has multiple seriesByTags functions as parameters it updates the model target correctly', () => {
+  describe('When the second parameter of a function is a function, the graphite parser breaks', () => {
     /* 
       all functions that take parameters as functions can have a bug where writing a query
-      in code with two seriesByTags funcs as params and then 
+      in code where the second parameter of the function IS A FUNCTION, 
-      switching from code to builder parsers the second function in a way that 
+      then switching from code to builder parsers the second function in a way that 
       changes the order of the params and wraps the first param in the second param.
       
       asPercent(seriesByTag('namespace=asd'), (seriesByTag('namespace=fgh')) 
@@ -126,33 +126,123 @@ describe('Graphite query model', () => {
       where each function is wrapped in another function
       https://github.com/grafana/grafana/blob/main/public/app/plugins/datasource/graphite/graphite_query.ts#LL187C8-L187C8
 
-      Parsing the second seriesByTag function as param as a string fixes this issue 
+      Parsing the second "function as param" as a string fixes this issue 
 
       This is one of the edge cases that could be a reason for either refactoring or rebuilding the Graphite query builder
     */
-    beforeEach(() => {
+    describe('when query has multiple seriesByTags functions as parameters it updates the model target correctly', () => {
-      ctx.target = { refId: 'A', target: `asPercent(seriesByTag('namespace=asd'), seriesByTag('namespace=fgh'))` };
+      beforeEach(() => {
-      ctx.targets = [ctx.target];
+        ctx.target = { refId: 'A', target: `asPercent(seriesByTag('namespace=asd'), seriesByTag('namespace=fgh'))` };
-      ctx.queryModel = new GraphiteQuery(ctx.datasource, ctx.target, ctx.templateSrv);
+        ctx.targets = [ctx.target];
+        ctx.queryModel = new GraphiteQuery(ctx.datasource, ctx.target, ctx.templateSrv);
+      });
+
+      it('should parse the second function param as a string and not a second function', () => {
+        const targets = [
+          {
+            refId: 'A',
+            datasource: {
+              type: 'graphite',
+              uid: 'zzz',
+            },
+            target: "asPercent(seriesByTag('namespace=jkl'), seriesByTag('namespace=fgh'))",
+            textEditor: false,
+            key: '123',
+          },
+        ];
+        expect(ctx.queryModel.segments.length).toBe(0);
+        expect(ctx.queryModel.functions.length).toBe(2);
+        ctx.queryModel.updateModelTarget(targets);
+        expect(ctx.queryModel.target.target).not.toContain('seriesByTag(seriesByTag(');
+      });
     });
 
-    it('should parse the second function param as a string and not a second function', () => {
+    describe('when query has divideSeriesLists function where second parameter is a function is parses correctly', () => {
-      const targets = [
+      it('should parse the second function param as a string and not parse it as a second function', () => {
-        {
+        const functionAsParam = 'scaleToSeconds(carbon.agents.0df7e0ba2701-a.cache.queries,1)';
+
+        ctx.target = {
           refId: 'A',
-          datasource: {
+          target: `divideSeriesLists(scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries), 1), ${functionAsParam})`,
-            type: 'graphite',
+        };
-            uid: 'zzz',
+        ctx.targets = [ctx.target];
+        ctx.queryModel = new GraphiteQuery(ctx.datasource, ctx.target, ctx.templateSrv);
+
+        const targets = [
+          {
+            refId: 'A',
+            datasource: {
+              type: 'graphite',
+              uid: 'zzz',
+            },
+            target: `divideSeriesLists(scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries), 1), ${functionAsParam})`,
+            textEditor: false,
+            key: '123',
           },
-          target: "asPercent(seriesByTag('namespace=jkl'), seriesByTag('namespace=fgh'))",
+        ];
-          textEditor: false,
+        expect(ctx.queryModel.segments.length).toBe(5);
-          key: '123',
+        expect(ctx.queryModel.functions.length).toBe(3);
-        },
+        ctx.queryModel.updateModelTarget(targets);
-      ];
+        expect(ctx.queryModel.target.target).toContain(functionAsParam);
-      expect(ctx.queryModel.segments.length).toBe(0);
+      });
-      expect(ctx.queryModel.functions.length).toBe(2);
+
-      ctx.queryModel.updateModelTarget(targets);
+      it('should recursively parse a second function argument that contains another function as a string', () => {
-      expect(ctx.queryModel.target.target).not.toContain('seriesByTag(seriesByTag(');
+        const nestedFunctionAsParam =
+          'scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries,1))';
+
+        ctx.target = {
+          refId: 'A',
+          target: `divideSeriesLists(scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries), 1), ${nestedFunctionAsParam})`,
+        };
+        ctx.targets = [ctx.target];
+        ctx.queryModel = new GraphiteQuery(ctx.datasource, ctx.target, ctx.templateSrv);
+
+        const targets = [
+          {
+            refId: 'A',
+            datasource: {
+              type: 'graphite',
+              uid: 'zzz',
+            },
+            target: `divideSeriesLists(scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries), 1), ${nestedFunctionAsParam})`,
+            textEditor: false,
+            key: '123',
+          },
+        ];
+        expect(ctx.queryModel.segments.length).toBe(5);
+        expect(ctx.queryModel.functions.length).toBe(3);
+        ctx.queryModel.updateModelTarget(targets);
+        expect(ctx.queryModel.target.target).toContain(nestedFunctionAsParam);
+      });
+
+      it('should recursively parse a second function argument where the first argument is a series', () => {
+        const nestedFunctionAsParam =
+          'scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries,1))';
+
+        ctx.target = {
+          refId: 'A',
+          target: `divideSeriesLists(carbon.agents.0df7e0ba2701-a.cache.queries, ${nestedFunctionAsParam})`,
+        };
+        ctx.targets = [ctx.target];
+        ctx.queryModel = new GraphiteQuery(ctx.datasource, ctx.target, ctx.templateSrv);
+
+        const targets = [
+          {
+            refId: 'A',
+            datasource: {
+              type: 'graphite',
+              uid: 'zzz',
+            },
+            target: `divideSeriesLists(scaleToSeconds(nonNegativeDerivative(carbon.agents.0df7e0ba2701-a.cache.queries), 1), ${nestedFunctionAsParam})`,
+            textEditor: false,
+            key: '123',
+          },
+        ];
+        expect(ctx.queryModel.segments.length).toBe(5);
+        expect(ctx.queryModel.functions.length).toBe(1);
+        ctx.queryModel.updateModelTarget(targets);
+        expect(ctx.queryModel.target.target).toContain(nestedFunctionAsParam);
+      });
     });
   });
 });