IntenseWebs/grafana
3
0
Fork 0
mirror of https://github.com/grafana/grafana.git synced 2025-02-25 18:55:37 -06:00
Code Issues Packages Projects Releases Wiki Activity

add bug fix for multiple nested functions as params (#66882)

Browse Source 
* 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
This commit is contained in:
Brendan O'Handley 2023-05-10 14:52:12 -04:00 committed by GitHub
parent 0565c3440f
commit 1d710408df
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 277 additions and 116 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
.betterer.results
View File

@ -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"]

43
public/app/plugins/datasource/graphite/graphite_query.ts
View File

@ -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;
});
}
}

91
public/app/plugins/datasource/graphite/parser.ts
View File

@ -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;
};

45
public/app/plugins/datasource/graphite/specs/graphite_query.test.ts
View File

@ -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(');
});
});
});

206
public/app/plugins/datasource/graphite/specs/parser.test.ts
View File

@ -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');
}
});
});