Skip to content

Commit 8dadddf

Browse files
arturminchukovarturminchukov
authored
Fix grafana testing comments (#429)
* sanitize URLs in debug section * ensure proper handling of gzip responses and close readers * add regex operator replacement functionality and improve error message for variable usage * fix all values (*) interpolation in adhoc filter
1 parent 8d9b1e9 commit 8dadddf

File tree

8 files changed

+197
-89
lines changed

8 files changed

+197
-89
lines changed

‎pkg/plugin/datasource.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -573,21 +573,23 @@ func (d *Datasource) VLAPIQuery(rw http.ResponseWriter, req *http.Request) {
573573
return
574574
}
575575
}
576-
reader := io.Reader(resp.Body)
576+
defer resp.Body.Close()
577+
578+
reader := resp.Body
577579
if resp.Header.Get("Content-Encoding") == "gzip" {
578580
reader, err = gzip.NewReader(reader)
579581
if err != nil {
580582
writeError(rw, http.StatusBadRequest, fmt.Errorf("failed to create gzip reader: %w", err))
581583
return
582584
}
585+
defer reader.Close()
583586
}
584587

585588
bodyBytes, err := io.ReadAll(reader)
586589
if err != nil {
587590
writeError(rw, http.StatusBadRequest, fmt.Errorf("failed to read http response body: %w", err))
588591
return
589592
}
590-
defer resp.Body.Close()
591593

592594
rw.Header().Add("Content-Type", "application/json")
593595
rw.WriteHeader(http.StatusOK)

‎src/LogsQL/regExpOperator.test.ts‎

Lines changed: 141 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -1,83 +1,143 @@
1-
import { getQueryExprVariableRegExp } from './regExpOperator';
2-
3-
describe('getQueryExprVariableRegExp', () => {
4-
it('should fing fieldName:~$var', () => {
5-
const result = getQueryExprVariableRegExp(' fieldName:~$var ');
6-
expect(result?.[0]).toEqual(' fieldName:~$var');
7-
});
8-
9-
it('should find fieldName:~$var without spaces', () => {
10-
const result = getQueryExprVariableRegExp('fieldName:~$var');
11-
expect(result?.[0]).toEqual('fieldName:~$var');
12-
});
13-
14-
it('should find fieldName:~$var at the start of string', () => {
15-
const result = getQueryExprVariableRegExp('fieldName:~$var and more');
16-
expect(result?.[0]).toEqual('fieldName:~$var');
17-
});
18-
19-
it('should find fieldName:~$var at the end of string', () => {
20-
const result = getQueryExprVariableRegExp('some query fieldName:~$var');
21-
expect(result?.[0]).toEqual(' fieldName:~$var');
22-
});
23-
24-
it('should find variable with underscores', () => {
25-
const result = getQueryExprVariableRegExp('fieldName:~$my_var_name');
26-
expect(result?.[0]).toEqual('fieldName:~$my_var_name');
27-
});
28-
29-
it('should find variable with numbers', () => {
30-
const result = getQueryExprVariableRegExp('fieldName:~$var123');
31-
expect(result?.[0]).toEqual('fieldName:~$var123');
32-
});
33-
34-
it('should find complex field name with variable', () => {
35-
const result = getQueryExprVariableRegExp('complex_field_name:~$variable');
36-
expect(result?.[0]).toEqual('complex_field_name:~$variable');
37-
});
38-
39-
it('should return null for query without variables', () => {
40-
const result = getQueryExprVariableRegExp('fieldName:value');
41-
expect(result).toBeNull();
42-
});
43-
44-
it('should return null for empty string', () => {
45-
const result = getQueryExprVariableRegExp('');
46-
expect(result).toBeNull();
47-
});
48-
49-
it('should return null for string with only spaces', () => {
50-
const result = getQueryExprVariableRegExp(' ');
51-
expect(result).toBeNull();
52-
});
53-
54-
it('should find first variable when multiple variables exist', () => {
55-
const result = getQueryExprVariableRegExp('field1:~$var1 field2:~$var2');
56-
expect(result?.[0]).toEqual('field1:~$var1');
57-
});
58-
59-
it('should find variable with special characters in field name', () => {
60-
const result = getQueryExprVariableRegExp('field-name:~$var');
61-
expect(result?.[0]).toEqual('field-name:~$var');
62-
});
63-
64-
it('should not handle variable without field name prefix', () => {
65-
const result = getQueryExprVariableRegExp('~$var');
66-
expect(result).toBeNull();
67-
});
68-
69-
it('should handle multiple spaces around variable', () => {
70-
const result = getQueryExprVariableRegExp(' fieldName:~$var ');
71-
expect(result?.[0]).toEqual(' fieldName:~$var');
72-
});
73-
74-
it('should find variable with mixed case field name', () => {
75-
const result = getQueryExprVariableRegExp('fieldName:~$MyVariable');
76-
expect(result?.[0]).toEqual('fieldName:~$MyVariable');
77-
});
78-
79-
it('should find variable with dot separated field name', () => {
80-
const result = getQueryExprVariableRegExp('someField: Some value | field.name.with.some.dot.separated:~$MyVariable');
81-
expect(result?.[0]).toEqual(' field.name.with.some.dot.separated:~$MyVariable');
1+
import { getQueryExprVariableRegExp, replaceRegExpOperatorToOperator } from './regExpOperator';
2+
3+
4+
describe('regExpOperator', () => {
5+
describe('getQueryExprVariableRegExp', () => {
6+
it('should fing fieldName:~$var', () => {
7+
const result = getQueryExprVariableRegExp(' fieldName:~$var ');
8+
expect(result?.[0]).toEqual(' fieldName:~$var');
9+
});
10+
11+
it('should find fieldName:~$var without spaces', () => {
12+
const result = getQueryExprVariableRegExp('fieldName:~$var');
13+
expect(result?.[0]).toEqual('fieldName:~$var');
14+
});
15+
16+
it('should find fieldName:~$var at the start of string', () => {
17+
const result = getQueryExprVariableRegExp('fieldName:~$var and more');
18+
expect(result?.[0]).toEqual('fieldName:~$var');
19+
});
20+
21+
it('should find fieldName:~$var at the end of string', () => {
22+
const result = getQueryExprVariableRegExp('some query fieldName:~$var');
23+
expect(result?.[0]).toEqual(' fieldName:~$var');
24+
});
25+
26+
it('should find variable with underscores', () => {
27+
const result = getQueryExprVariableRegExp('fieldName:~$my_var_name');
28+
expect(result?.[0]).toEqual('fieldName:~$my_var_name');
29+
});
30+
31+
it('should find variable with numbers', () => {
32+
const result = getQueryExprVariableRegExp('fieldName:~$var123');
33+
expect(result?.[0]).toEqual('fieldName:~$var123');
34+
});
35+
36+
it('should find complex field name with variable', () => {
37+
const result = getQueryExprVariableRegExp('complex_field_name:~$variable');
38+
expect(result?.[0]).toEqual('complex_field_name:~$variable');
39+
});
40+
41+
it('should return null for query without variables', () => {
42+
const result = getQueryExprVariableRegExp('fieldName:value');
43+
expect(result).toBeNull();
44+
});
45+
46+
it('should return null for empty string', () => {
47+
const result = getQueryExprVariableRegExp('');
48+
expect(result).toBeNull();
49+
});
50+
51+
it('should return null for string with only spaces', () => {
52+
const result = getQueryExprVariableRegExp(' ');
53+
expect(result).toBeNull();
54+
});
55+
56+
it('should find first variable when multiple variables exist', () => {
57+
const result = getQueryExprVariableRegExp('field1:~$var1 field2:~$var2');
58+
expect(result?.[0]).toEqual('field1:~$var1');
59+
});
60+
61+
it('should find variable with special characters in field name', () => {
62+
const result = getQueryExprVariableRegExp('field-name:~$var');
63+
expect(result?.[0]).toEqual('field-name:~$var');
64+
});
65+
66+
it('should not handle variable without field name prefix', () => {
67+
const result = getQueryExprVariableRegExp('~$var');
68+
expect(result).toBeNull();
69+
});
70+
71+
it('should handle multiple spaces around variable', () => {
72+
const result = getQueryExprVariableRegExp(' fieldName:~$var ');
73+
expect(result?.[0]).toEqual(' fieldName:~$var');
74+
});
75+
76+
it('should find variable with mixed case field name', () => {
77+
const result = getQueryExprVariableRegExp('fieldName:~$MyVariable');
78+
expect(result?.[0]).toEqual('fieldName:~$MyVariable');
79+
});
80+
81+
it('should find variable with dot separated field name', () => {
82+
const result = getQueryExprVariableRegExp('someField: Some value | field.name.with.some.dot.separated:~$MyVariable');
83+
expect(result?.[0]).toEqual(' field.name.with.some.dot.separated:~$MyVariable');
84+
});
85+
});
86+
87+
describe('replaceRegExpOperatorToOperator', () => {
88+
it('should replace :~ with :', () => {
89+
const result = replaceRegExpOperatorToOperator('fieldName:~$var');
90+
expect(result).toEqual('fieldName:$var');
91+
});
92+
93+
it('should replace :~ with a custom operator', () => {
94+
const result = replaceRegExpOperatorToOperator('fieldName:~$var', '!=');
95+
expect(result).toEqual('fieldName!=$var');
96+
});
97+
98+
it('should ignore queries without :~', () => {
99+
const result = replaceRegExpOperatorToOperator('fieldName:value');
100+
expect(result).toEqual('fieldName:value');
101+
});
102+
103+
it('should handle queries with spaces around :~', () => {
104+
const result = replaceRegExpOperatorToOperator('fieldName : ~ $var');
105+
expect(result).toEqual('fieldName:$var');
106+
});
107+
108+
it('should return the same string if no match is found', () => {
109+
const result = replaceRegExpOperatorToOperator('');
110+
expect(result).toEqual('');
111+
});
112+
113+
it('should handle multiple spaces in the query string', () => {
114+
const result = replaceRegExpOperatorToOperator(' fieldName :~ $var ');
115+
expect(result).toEqual(' fieldName:$var ');
116+
});
117+
118+
it('should replace variable with underscores', () => {
119+
const result = replaceRegExpOperatorToOperator('fieldName:~$my_var_name');
120+
expect(result).toEqual('fieldName:$my_var_name');
121+
});
122+
123+
it('should replace variables with numbers', () => {
124+
const result = replaceRegExpOperatorToOperator('fieldName:~$var123');
125+
expect(result).toEqual('fieldName:$var123');
126+
});
127+
128+
it('should replace complex field names with a variable', () => {
129+
const result = replaceRegExpOperatorToOperator('complex_field_name:~$variable');
130+
expect(result).toEqual('complex_field_name:$variable');
131+
});
132+
133+
it('should handle fields with special characters', () => {
134+
const result = replaceRegExpOperatorToOperator('field-name:~$var');
135+
expect(result).toEqual('field-name:$var');
136+
});
137+
138+
it('should replace dot-separated field names with variables', () => {
139+
const result = replaceRegExpOperatorToOperator('field.name.with.dots:~$variable');
140+
expect(result).toEqual('field.name.with.dots:$variable');
141+
});
82142
});
83143
});

‎src/LogsQL/regExpOperator.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
11
/**
22
* Regular expression for matching variables in a query expression -> filterName:~"$VariableName"
33
* */
4-
const variableRegExp = /(^|\||\s)([^\s:~]+)\s*:\s*~\s*(?:"\$([A-Za-z0-9_.-]+)"|\$([A-Za-z0-9_.-]+)|"([^"]+)"|([^\s|]+))(?=\s|\||$)/;
4+
const variableRegExp = /(^|\||\s)([^\s:~]+)\s*:\s*~\s*(?:"(\$[A-Za-z0-9_.-]+)"|(\$[A-Za-z0-9_.-]+)|"([^"]+)"|([^\s|]+))(?=\s|\||$)/;
55

66
export function getQueryExprVariableRegExp(queryExpr: string) {
77
return variableRegExp.exec(queryExpr);
88
}
9+
10+
export function replaceRegExpOperatorToOperator(queryExpr: string, operator = ':') {
11+
return queryExpr.replace(variableRegExp, `$1$2${operator}$4`);
12+
}

‎src/components/QueryEditor/QueryEditorVariableRegexpError.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import React from 'react';
44
import { GrafanaTheme2 } from "@grafana/data";
55
import { Badge, useTheme2 } from "@grafana/ui";
66

7+
import { replaceRegExpOperatorToOperator } from "../../LogsQL/regExpOperator";
8+
79

810
interface Props {
911
regExp: string;
@@ -12,10 +14,11 @@ interface Props {
1214
const QueryEditorVariableRegexpError = ({ regExp }: Props) => {
1315
const theme = useTheme2();
1416
const styles = getStyles(theme);
17+
const fixedFilter = replaceRegExpOperatorToOperator(regExp);
1518

1619
const text = (
1720
<div>
18-
Regexp operator (~) cannot be used with variables in {regExp}. Use exact match operator (:) or use non variable in regexp instead.
21+
Regexp operator `~` cannot be used with variables in `{regExp}`. Use exact match operator `:` (e.g. `{fixedFilter}`) or use non variable in regexp instead.
1922
</div>
2023
)
2124

‎src/configuration/DebugSection.tsx‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import React, { ReactNode, useState } from 'react';
22

3+
import { textUtil } from "@grafana/data";
34
import { getTemplateSrv } from '@grafana/runtime';
45
import { InlineField, TextArea } from '@grafana/ui';
56

@@ -53,7 +54,7 @@ const DebugFields = ({ fields }: DebugFieldItemProps) => {
5354
if (field.error && field.error instanceof Error) {
5455
value = field.error.message;
5556
} else if (field.href) {
56-
value = <a href={field.href}>{value}</a>;
57+
value = <a href={textUtil.sanitizeUrl(field.href)}>{value}</a>;
5758
}
5859
return (
5960
<tr key={`${field.name}=${field.value}`}>
@@ -93,6 +94,7 @@ function makeDebugFields(derivedFields: DerivedFieldConfig[], debugText: string)
9394
text: 'Raw value',
9495
},
9596
});
97+
href = textUtil.sanitizeUrl(href);
9698
}
9799
const debugFiled: DebugField = {
98100
name: field.name,

‎src/constants.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
export const VARIABLE_ALL_VALUE = "$__all";
2+
export const TEXT_FILTER_ALL_VALUE = "*";

‎src/datasource.test.ts‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { AdHocVariableFilter } from '@grafana/data';
22
import { TemplateSrv } from "@grafana/runtime";
33

44
import { createDatasource } from "./__mocks__/datasource";
5-
import { VARIABLE_ALL_VALUE } from "./constants";
5+
import { TEXT_FILTER_ALL_VALUE, VARIABLE_ALL_VALUE } from "./constants";
66
import { VictoriaLogsDatasource } from "./datasource";
77

88
const replaceMock = jest.fn().mockImplementation((a: string) => a);
@@ -266,5 +266,36 @@ describe('VictoriaLogsDatasource', () => {
266266
const result = ds.interpolateString('foo: $var1 bar: $var2', scopedVars);
267267
expect(result).toStrictEqual('foo: in(\"foo\",\"bar\") bar: in(*)');
268268
})
269+
270+
it('should not escape the * in the text filter ', () => {
271+
const scopedVars = {};
272+
const variables = [
273+
{
274+
name: 'var1',
275+
current: [{ value: "foo" }, { value: "bar" }],
276+
multi: true,
277+
type: "query",
278+
query: {
279+
type: "fieldValue"
280+
}
281+
}, {
282+
name: 'var2',
283+
current: { value: VARIABLE_ALL_VALUE },
284+
multi: false,
285+
},
286+
{
287+
name: 'var3',
288+
current: { value: TEXT_FILTER_ALL_VALUE },
289+
multi: false,
290+
}
291+
];
292+
const templateSrvMock = {
293+
replace: jest.fn((a) => a), // return the input string
294+
getVariables: jest.fn().mockReturnValue(variables),
295+
} as unknown as TemplateSrv;
296+
const ds = createDatasource(templateSrvMock);
297+
const result = ds.interpolateString('foo: $var1 bar: $var2 | $var3', scopedVars);
298+
expect(result).toStrictEqual('foo:in($var1) bar:in(*) | *');
299+
})
269300
});
270301
});

‎src/datasource.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ import { DataQuery } from "@grafana/schema";
3535
import { transformBackendResult } from "./backendResultTransformer";
3636
import QueryEditor from "./components/QueryEditor/QueryEditor";
3737
import { LogLevelRule } from "./configuration/LogLevelRules/types";
38-
import { VARIABLE_ALL_VALUE } from "./constants";
38+
import { TEXT_FILTER_ALL_VALUE, VARIABLE_ALL_VALUE } from "./constants";
3939
import { escapeLabelValueInSelector } from "./languageUtils";
4040
import LogsQlLanguageProvider from "./language_provider";
4141
import { LOGS_VOLUME_BARS, queryLogsVolume } from "./logsVolumeLegacy";
@@ -278,7 +278,12 @@ export class VictoriaLogsDatasource
278278
if (!value) {
279279
return false;
280280
}
281-
return Array.isArray(value) ? value.includes(VARIABLE_ALL_VALUE) : value === VARIABLE_ALL_VALUE;
281+
282+
if (typeof value === 'string') {
283+
return value === VARIABLE_ALL_VALUE || value === TEXT_FILTER_ALL_VALUE;
284+
}
285+
286+
return Array.isArray(value) ? value.includes(VARIABLE_ALL_VALUE) : false;
282287
}
283288

284289
replaceOperatorsToInForMultiQueryVariables(expr: string,) {

0 commit comments

Comments
 (0)