From 1d710408dfd776a34c9e7f62eb4ed593cbbfe084 Mon Sep 17 00:00:00 2001 From: Brendan O'Handley Date: Wed, 10 May 2023 14:52:12 -0400 Subject: [PATCH] add bug fix for multiple nested functions as params (#66882) * add bug fix for multiple nested functions as params * add types for parser (#67067) * add types for parser * add comment for future work * handle any function with 2 seriesByTags func params, add test --- .betterer.results | 8 +- .../datasource/graphite/graphite_query.ts | 43 +++- .../app/plugins/datasource/graphite/parser.ts | 91 +++++--- .../graphite/specs/graphite_query.test.ts | 45 ++++ .../datasource/graphite/specs/parser.test.ts | 206 +++++++++++------- 5 files changed, 277 insertions(+), 116 deletions(-) diff --git a/.betterer.results b/.betterer.results index 5cd31161bc9..8f55ebac72e 100644 --- a/.betterer.results +++ b/.betterer.results @@ -4294,13 +4294,7 @@ exports[`better eslint`] = { "public/app/plugins/datasource/graphite/parser.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], [0, 0, 0, "Unexpected any. Specify a different type.", "1"], - [0, 0, 0, "Unexpected any. Specify a different type.", "2"], - [0, 0, 0, "Unexpected any. Specify a different type.", "3"], - [0, 0, 0, "Unexpected any. Specify a different type.", "4"], - [0, 0, 0, "Unexpected any. Specify a different type.", "5"], - [0, 0, 0, "Unexpected any. Specify a different type.", "6"], - [0, 0, 0, "Unexpected any. Specify a different type.", "7"], - [0, 0, 0, "Unexpected any. Specify a different type.", "8"] + [0, 0, 0, "Unexpected any. Specify a different type.", "2"] ], "public/app/plugins/datasource/graphite/specs/graphite_query.test.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"] diff --git a/public/app/plugins/datasource/graphite/graphite_query.ts b/public/app/plugins/datasource/graphite/graphite_query.ts index 17fcc31366f..c8e7254b78c 100644 --- a/public/app/plugins/datasource/graphite/graphite_query.ts +++ b/public/app/plugins/datasource/graphite/graphite_query.ts @@ -6,7 +6,7 @@ import { arrayMove } from 'app/core/utils/arrayMove'; import { GraphiteDatasource } from './datasource'; import { FuncInstance } from './gfunc'; -import { Parser } from './parser'; +import { AstNode, Parser } from './parser'; import { GraphiteSegment } from './types'; export type GraphiteTagOperator = '=' | '=~' | '!=' | '!=~'; @@ -110,6 +110,10 @@ export default class GraphiteQuery { const innerFunc = this.datasource.createFuncInstance(astNode.name, { withDefaultParams: false, }); + + // bug fix for parsing multiple functions as params + handleMultipleSeriesByTagsParams(astNode); + each(astNode.params, (param) => { this.parseTargetRecursive(param, innerFunc); }); @@ -326,3 +330,40 @@ export default class GraphiteQuery { function renderTagString(tag: { key: any; operator?: any; value?: any }) { return tag.key + tag.operator + tag.value; } + +/** + * mutates the second seriesByTag function into a string to fix a parsing bug + * @param astNode + * @param innerFunc + */ +function handleMultipleSeriesByTagsParams(astNode: AstNode) { + // if function has two params that are function seriesByTags keep the second as a string otherwise we have a parsing error + if (astNode.params && astNode.params.length >= 2) { + let count = 0; + astNode.params = astNode.params.map((p: AstNode) => { + if (p.type === 'function') { + count += 1; + } + + if (count === 2 && p.type === 'function' && p.name === 'seriesByTag') { + // convert second function to a string + const stringParams = + p.params && + p.params.reduce((acc: string, p: AstNode, idx: number, paramsArr: AstNode[]) => { + if (idx === 0 || idx !== paramsArr.length - 1) { + return `${acc}'${p.value}',`; + } + + return `${acc}'${p.value}'`; + }, ''); + + return { + type: 'string', + value: `${p.name}(${stringParams})`, + }; + } + + return p; + }); + } +} diff --git a/public/app/plugins/datasource/graphite/parser.ts b/public/app/plugins/datasource/graphite/parser.ts index b0ff68ef140..5ee0e44332b 100644 --- a/public/app/plugins/datasource/graphite/parser.ts +++ b/public/app/plugins/datasource/graphite/parser.ts @@ -3,12 +3,12 @@ import { GraphiteParserError } from './types'; import { isGraphiteParserError } from './utils'; export class Parser { - expression: any; + expression: string; lexer: Lexer; - tokens: any; + tokens: AstNode[]; index: number; - constructor(expression: any) { + constructor(expression: string) { this.expression = expression; this.lexer = new Lexer(expression); this.tokens = this.lexer.tokenize(); @@ -19,7 +19,7 @@ export class Parser { return this.start(); } - start() { + start(): AstNode | null { try { return this.functionCall() || this.metricExpression(); } catch (e) { @@ -31,9 +31,10 @@ export class Parser { }; } } + return null; } - curlyBraceSegment() { + curlyBraceSegment(): AstNode | null { if (this.match('identifier', '{') || this.match('{')) { let curlySegment = ''; @@ -62,7 +63,7 @@ export class Parser { } } - metricSegment() { + metricSegment(): AstNode | null { const curly = this.curlyBraceSegment(); if (curly) { return curly; @@ -70,7 +71,8 @@ export class Parser { if (this.match('identifier') || this.match('number') || this.match('bool')) { // hack to handle float numbers in metric segments - const parts = this.consumeToken().value.split('.'); + const tokenValue = this.consumeToken().value; + const parts = tokenValue && typeof tokenValue === 'string' ? tokenValue.split('.') : ''; if (parts.length === 2) { this.tokens.splice(this.index, 0, { type: '.' }); this.tokens.splice(this.index + 1, 0, { @@ -108,17 +110,21 @@ export class Parser { return node; } - metricExpression() { + metricExpression(): AstNode | null { if (!this.match('templateStart') && !this.match('identifier') && !this.match('number') && !this.match('{')) { return null; } - const node: any = { + const node: AstNode = { type: 'metric', segments: [], }; - node.segments.push(this.metricSegment()); + const segments = this.metricSegment(); + + if (node.segments && segments) { + node.segments.push(segments); + } while (this.match('.')) { this.consumeToken(); @@ -127,21 +133,28 @@ export class Parser { if (!segment) { this.errorMark('Expected metric identifier'); } - - node.segments.push(segment); + if (node.segments && segment) { + node.segments.push(segment); + } } return node; } - functionCall() { + functionCall(): AstNode | null { if (!this.match('identifier', '(')) { return null; } - const node: any = { + let name = ''; + const token = this.consumeToken(); + if (typeof token.value === 'string') { + name = token.value; + } + + const node: AstNode = { type: 'function', - name: this.consumeToken().value, + name: name, }; // consume left parenthesis @@ -158,7 +171,7 @@ export class Parser { return node; } - boolExpression() { + boolExpression(): AstNode | null { if (!this.match('bool')) { return null; } @@ -169,7 +182,7 @@ export class Parser { }; } - functionParameters(): any { + functionParameters(): AstNode[] | [] { if (this.match(')') || this.match('')) { return []; } @@ -182,21 +195,25 @@ export class Parser { this.metricExpression() || this.stringLiteral(); - if (!this.match(',')) { + if (!this.match(',') && param) { return [param]; } this.consumeToken(); - return [param].concat(this.functionParameters()); + + if (param) { + return [param].concat(this.functionParameters()); + } + return []; } - seriesRefExpression() { + seriesRefExpression(): AstNode | null { if (!this.match('identifier')) { return null; } const value = this.tokens[this.index].value; - if (!value.match(/\#[A-Z]/)) { + if (value && typeof value === 'string' && !value.match(/\#[A-Z]/)) { return null; } @@ -208,24 +225,28 @@ export class Parser { }; } - numericLiteral() { + numericLiteral(): AstNode | null { if (!this.match('number')) { return null; } - return { - type: 'number', - value: parseFloat(this.consumeToken().value), - }; + const token = this.consumeToken(); + if (token && token.value && typeof token.value === 'string') { + return { + type: 'number', + value: parseFloat(token.value), + }; + } + return null; } - stringLiteral() { + stringLiteral(): AstNode | null { if (!this.match('string')) { return null; } const token = this.consumeToken(); - if (token.isUnclosed) { + if (token.isUnclosed && token.pos) { const error: GraphiteParserError = { message: 'Unclosed string parameter', pos: token.pos, @@ -244,7 +265,7 @@ export class Parser { const type = currentToken ? currentToken.type : 'end of string'; const error: GraphiteParserError = { message: text + ' instead found ' + type, - pos: currentToken ? currentToken.pos : this.lexer.char, + pos: currentToken && currentToken.pos ? currentToken.pos : this.lexer.char, }; throw error; } @@ -264,3 +285,15 @@ export class Parser { return this.matchToken(token1, 0) && (!token2 || this.matchToken(token2, 1)); } } + +// Next steps, need to make this applicable to types in graphite_query.ts +export type AstNode = { + type: string; + name?: string; + params?: AstNode[]; + value?: string | number | boolean; + segments?: AstNode[]; + message?: string; + pos?: number; + isUnclosed?: boolean; +}; diff --git a/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts b/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts index 278d3ff8c25..014a2189bea 100644 --- a/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts +++ b/public/app/plugins/datasource/graphite/specs/graphite_query.test.ts @@ -110,4 +110,49 @@ describe('Graphite query model', () => { expect(ctx.queryModel.target.target).toBe('foo.bar'); }); }); + + describe('when query has multiple seriesByTags functions as parameters it updates the model target correctly', () => { + /* + 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 + 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')) + becomes + asPercent(seriesByTag(seriesByTag('namespace=asd'), 'namespace=fgh')) + + This is due to the core functionality of parsing changed targets by reducing them, + 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 + + This is one of the edge cases that could be a reason for either refactoring or rebuilding the Graphite query builder + */ + beforeEach(() => { + ctx.target = { refId: 'A', target: `asPercent(seriesByTag('namespace=asd'), seriesByTag('namespace=fgh'))` }; + 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('); + }); + }); }); diff --git a/public/app/plugins/datasource/graphite/specs/parser.test.ts b/public/app/plugins/datasource/graphite/specs/parser.test.ts index 05f4fc108d6..af53e45ce7d 100644 --- a/public/app/plugins/datasource/graphite/specs/parser.test.ts +++ b/public/app/plugins/datasource/graphite/specs/parser.test.ts @@ -5,199 +5,247 @@ describe('when parsing', () => { const parser = new Parser('metric.test.*.asd.count'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('metric'); - expect(rootNode.segments.length).toBe(5); - expect(rootNode.segments[0].value).toBe('metric'); + if (rootNode && rootNode.segments) { + expect(rootNode.type).toBe('metric'); + expect(rootNode.segments.length).toBe(5); + expect(rootNode.segments[0].value).toBe('metric'); + } }); it('simple metric expression with numbers in segments', () => { const parser = new Parser('metric.10.15_20.5'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('metric'); - expect(rootNode.segments.length).toBe(4); - expect(rootNode.segments[1].value).toBe('10'); - expect(rootNode.segments[2].value).toBe('15_20'); - expect(rootNode.segments[3].value).toBe('5'); + if (rootNode && rootNode.segments) { + expect(rootNode.type).toBe('metric'); + expect(rootNode.segments.length).toBe(4); + expect(rootNode.segments[1].value).toBe('10'); + expect(rootNode.segments[2].value).toBe('15_20'); + expect(rootNode.segments[3].value).toBe('5'); + } }); it('simple metric expression with "true" boolean in segments', () => { const parser = new Parser('metric.15_20.5.true'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('metric'); - expect(rootNode.segments.length).toBe(4); - expect(rootNode.segments[1].value).toBe('15_20'); - expect(rootNode.segments[2].value).toBe('5'); - expect(rootNode.segments[3].value).toBe('true'); + if (rootNode && rootNode.segments) { + expect(rootNode.type).toBe('metric'); + expect(rootNode.segments.length).toBe(4); + expect(rootNode.segments[1].value).toBe('15_20'); + expect(rootNode.segments[2].value).toBe('5'); + expect(rootNode.segments[3].value).toBe('true'); + } }); it('simple metric expression with "false" boolean in segments', () => { const parser = new Parser('metric.false.15_20.5'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('metric'); - expect(rootNode.segments.length).toBe(4); - expect(rootNode.segments[1].value).toBe('false'); - expect(rootNode.segments[2].value).toBe('15_20'); - expect(rootNode.segments[3].value).toBe('5'); + if (rootNode && rootNode.segments) { + expect(rootNode.type).toBe('metric'); + expect(rootNode.segments.length).toBe(4); + expect(rootNode.segments[1].value).toBe('false'); + expect(rootNode.segments[2].value).toBe('15_20'); + expect(rootNode.segments[3].value).toBe('5'); + } }); it('simple metric expression with curly braces', () => { const parser = new Parser('metric.se1-{count, max}'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('metric'); - expect(rootNode.segments.length).toBe(2); - expect(rootNode.segments[1].value).toBe('se1-{count,max}'); + if (rootNode && rootNode.segments) { + expect(rootNode.type).toBe('metric'); + expect(rootNode.segments.length).toBe(2); + expect(rootNode.segments[1].value).toBe('se1-{count,max}'); + } }); it('simple metric expression with curly braces at start of segment and with post chars', () => { const parser = new Parser('metric.{count, max}-something.count'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('metric'); - expect(rootNode.segments.length).toBe(3); - expect(rootNode.segments[1].value).toBe('{count,max}-something'); + if (rootNode && rootNode.segments) { + expect(rootNode.type).toBe('metric'); + expect(rootNode.segments.length).toBe(3); + expect(rootNode.segments[1].value).toBe('{count,max}-something'); + } }); it('simple function', () => { const parser = new Parser('sum(test)'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('function'); - expect(rootNode.params.length).toBe(1); + + if (rootNode && rootNode.params) { + expect(rootNode.type).toBe('function'); + expect(rootNode.params.length).toBe(1); + } }); it('simple function2', () => { const parser = new Parser('offset(test.metric, -100)'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('function'); - expect(rootNode.params[0].type).toBe('metric'); - expect(rootNode.params[1].type).toBe('number'); + + if (rootNode && rootNode.params) { + expect(rootNode.type).toBe('function'); + expect(rootNode.params[0].type).toBe('metric'); + expect(rootNode.params[1].type).toBe('number'); + } }); it('simple function with string arg', () => { const parser = new Parser("randomWalk('test')"); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('function'); - expect(rootNode.params.length).toBe(1); - expect(rootNode.params[0].type).toBe('string'); + + if (rootNode && rootNode.params) { + expect(rootNode.type).toBe('function'); + expect(rootNode.params.length).toBe(1); + expect(rootNode.params[0].type).toBe('string'); + } }); it('function with multiple args', () => { const parser = new Parser("sum(test, 1, 'test')"); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('function'); - expect(rootNode.params.length).toBe(3); - expect(rootNode.params[0].type).toBe('metric'); - expect(rootNode.params[1].type).toBe('number'); - expect(rootNode.params[2].type).toBe('string'); + if (rootNode && rootNode.params) { + expect(rootNode.type).toBe('function'); + expect(rootNode.params.length).toBe(3); + expect(rootNode.params[0].type).toBe('metric'); + expect(rootNode.params[1].type).toBe('number'); + expect(rootNode.params[2].type).toBe('string'); + } }); it('function with nested function', () => { const parser = new Parser('sum(scaleToSeconds(test, 1))'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('function'); - expect(rootNode.params.length).toBe(1); - expect(rootNode.params[0].type).toBe('function'); - expect(rootNode.params[0].name).toBe('scaleToSeconds'); - expect(rootNode.params[0].params.length).toBe(2); - expect(rootNode.params[0].params[0].type).toBe('metric'); - expect(rootNode.params[0].params[1].type).toBe('number'); + if (rootNode && rootNode.params && rootNode.params[0].params) { + expect(rootNode.type).toBe('function'); + expect(rootNode.params.length).toBe(1); + expect(rootNode.params[0].type).toBe('function'); + expect(rootNode.params[0].name).toBe('scaleToSeconds'); + expect(rootNode.params[0].params.length).toBe(2); + expect(rootNode.params[0].params[0].type).toBe('metric'); + expect(rootNode.params[0].params[1].type).toBe('number'); + } }); it('function with multiple series', () => { const parser = new Parser('sum(test.test.*.count, test.timers.*.count)'); const rootNode = parser.getAst(); - - expect(rootNode.type).toBe('function'); - expect(rootNode.params.length).toBe(2); - expect(rootNode.params[0].type).toBe('metric'); - expect(rootNode.params[1].type).toBe('metric'); + if (rootNode && rootNode.params) { + expect(rootNode.type).toBe('function'); + expect(rootNode.params.length).toBe(2); + expect(rootNode.params[0].type).toBe('metric'); + expect(rootNode.params[1].type).toBe('metric'); + } }); it('function with templated series', () => { const parser = new Parser('sum(test.[[server]].count)'); const rootNode = parser.getAst(); - expect(rootNode.message).toBe(undefined); - expect(rootNode.params[0].type).toBe('metric'); - expect(rootNode.params[0].segments[1].type).toBe('segment'); - expect(rootNode.params[0].segments[1].value).toBe('[[server]]'); + if (rootNode && rootNode.params && rootNode.params[0].segments) { + expect(rootNode.message).toBe(undefined); + expect(rootNode.params[0].type).toBe('metric'); + expect(rootNode.params[0].segments[1].type).toBe('segment'); + expect(rootNode.params[0].segments[1].value).toBe('[[server]]'); + } }); it('invalid metric expression', () => { const parser = new Parser('metric.test.*.asd.'); const rootNode = parser.getAst(); - expect(rootNode.message).toBe('Expected metric identifier instead found end of string'); - expect(rootNode.pos).toBe(19); + if (rootNode && rootNode.message && rootNode.pos) { + expect(rootNode.message).toBe('Expected metric identifier instead found end of string'); + expect(rootNode.pos).toBe(19); + } }); it('invalid function expression missing closing parenthesis', () => { const parser = new Parser('sum(test'); const rootNode = parser.getAst(); - expect(rootNode.message).toBe('Expected closing parenthesis instead found end of string'); - expect(rootNode.pos).toBe(9); + if (rootNode && rootNode.message && rootNode.pos) { + expect(rootNode.message).toBe('Expected closing parenthesis instead found end of string'); + expect(rootNode.pos).toBe(9); + } }); it('unclosed string in function', () => { const parser = new Parser("sum('test)"); const rootNode = parser.getAst(); - - expect(rootNode.message).toBe('Unclosed string parameter'); - expect(rootNode.pos).toBe(11); + if (rootNode && rootNode.message && rootNode.pos) { + expect(rootNode.message).toBe('Unclosed string parameter'); + expect(rootNode.pos).toBe(11); + } }); it('handle issue #69', () => { const parser = new Parser('cactiStyle(offset(scale(net.192-168-1-1.192-168-1-9.ping_value.*,0.001),-100))'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('function'); + if (rootNode) { + expect(rootNode.type).toBe('function'); + } }); it('handle float function arguments', () => { const parser = new Parser('scale(test, 0.002)'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('function'); - expect(rootNode.params[1].type).toBe('number'); - expect(rootNode.params[1].value).toBe(0.002); + + if (rootNode && rootNode.params) { + expect(rootNode.type).toBe('function'); + expect(rootNode.params[1].type).toBe('number'); + expect(rootNode.params[1].value).toBe(0.002); + } }); it('handle curly brace pattern at start', () => { const parser = new Parser('{apps}.test'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('metric'); - expect(rootNode.segments[0].value).toBe('{apps}'); - expect(rootNode.segments[1].value).toBe('test'); + if (rootNode && rootNode.segments) { + expect(rootNode.type).toBe('metric'); + expect(rootNode.segments[0].value).toBe('{apps}'); + expect(rootNode.segments[1].value).toBe('test'); + } }); it('series parameters', () => { const parser = new Parser('asPercent(#A, #B)'); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('function'); - expect(rootNode.params[0].type).toBe('series-ref'); - expect(rootNode.params[0].value).toBe('#A'); - expect(rootNode.params[1].value).toBe('#B'); + if (rootNode && rootNode.params) { + expect(rootNode.type).toBe('function'); + expect(rootNode.params[0].type).toBe('series-ref'); + expect(rootNode.params[0].value).toBe('#A'); + expect(rootNode.params[1].value).toBe('#B'); + } }); it('series parameters, issue 2788', () => { const parser = new Parser("summarize(diffSeries(#A, #B), '10m', 'sum', false)"); const rootNode = parser.getAst(); - expect(rootNode.type).toBe('function'); - expect(rootNode.params[0].type).toBe('function'); - expect(rootNode.params[1].value).toBe('10m'); - expect(rootNode.params[3].type).toBe('bool'); + + if (rootNode && rootNode.params) { + expect(rootNode.type).toBe('function'); + expect(rootNode.params[0].type).toBe('function'); + expect(rootNode.params[1].value).toBe('10m'); + expect(rootNode.params[3].type).toBe('bool'); + } }); it('should parse metric expression with ip number segments', () => { const parser = new Parser('5.10.123.5'); const rootNode = parser.getAst(); - expect(rootNode.segments[0].value).toBe('5'); - expect(rootNode.segments[1].value).toBe('10'); - expect(rootNode.segments[2].value).toBe('123'); - expect(rootNode.segments[3].value).toBe('5'); + + if (rootNode && rootNode.segments) { + expect(rootNode.segments[0].value).toBe('5'); + expect(rootNode.segments[1].value).toBe('10'); + expect(rootNode.segments[2].value).toBe('123'); + expect(rootNode.segments[3].value).toBe('5'); + } }); });