Skip to content

Commit 5ac1a00

Browse files
SlachSlach
authored
Merge pull request #848 from Altinity/hotfix_single_quotes
no single quotes after query Template variable
2 parents e4d314c + 49bf9c9 commit 5ac1a00

File tree

4 files changed

+299
-44
lines changed

4 files changed

+299
-44
lines changed

‎docker/grafana/dashboards/conditionalTest_quotes_issue_847.json‎

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
{
2+
"annotations": {
3+
"list": [
4+
{
5+
"builtIn": 1,
6+
"datasource": {
7+
"type": "grafana",
8+
"uid": "-- Grafana --"
9+
},
10+
"enable": true,
11+
"hide": true,
12+
"iconColor": "rgba(0, 211, 255, 1)",
13+
"name": "Annotations & Alerts",
14+
"type": "dashboard"
15+
}
16+
]
17+
},
18+
"description": "look https://github.com/Altinity/clickhouse-grafana/issues/847",
19+
"editable": true,
20+
"fiscalYearStartMonth": 0,
21+
"graphTooltip": 0,
22+
"id": 0,
23+
"links": [],
24+
"panels": [
25+
{
26+
"datasource": {
27+
"type": "vertamedia-clickhouse-datasource",
28+
"uid": "P7E099F39B84EA795"
29+
},
30+
"description": "reproduce https://github.com/Altinity/clickhouse-grafana/issues/847",
31+
"fieldConfig": {
32+
"defaults": {
33+
"color": {
34+
"mode": "palette-classic"
35+
},
36+
"custom": {
37+
"axisBorderShow": false,
38+
"axisCenteredZero": false,
39+
"axisColorMode": "text",
40+
"axisLabel": "",
41+
"axisPlacement": "auto",
42+
"barAlignment": 0,
43+
"barWidthFactor": 0.6,
44+
"drawStyle": "line",
45+
"fillOpacity": 0,
46+
"gradientMode": "none",
47+
"hideFrom": {
48+
"legend": false,
49+
"tooltip": false,
50+
"viz": false
51+
},
52+
"insertNulls": false,
53+
"lineInterpolation": "linear",
54+
"lineWidth": 1,
55+
"pointSize": 5,
56+
"scaleDistribution": {
57+
"type": "linear"
58+
},
59+
"showPoints": "auto",
60+
"showValues": false,
61+
"spanNulls": false,
62+
"stacking": {
63+
"group": "A",
64+
"mode": "none"
65+
},
66+
"thresholdsStyle": {
67+
"mode": "off"
68+
}
69+
},
70+
"mappings": [],
71+
"thresholds": {
72+
"mode": "absolute",
73+
"steps": [
74+
{
75+
"color": "green",
76+
"value": 0
77+
},
78+
{
79+
"color": "red",
80+
"value": 80
81+
}
82+
]
83+
}
84+
},
85+
"overrides": []
86+
},
87+
"gridPos": {
88+
"h": 8,
89+
"w": 12,
90+
"x": 0,
91+
"y": 0
92+
},
93+
"id": 1,
94+
"options": {
95+
"legend": {
96+
"calcs": [],
97+
"displayMode": "list",
98+
"placement": "bottom",
99+
"showLegend": true
100+
},
101+
"tooltip": {
102+
"hideZeros": false,
103+
"mode": "single",
104+
"sort": "none"
105+
}
106+
},
107+
"pluginVersion": "12.3.2",
108+
"targets": [
109+
{
110+
"adHocFilters": [],
111+
"adHocValuesQuery": "",
112+
"add_metadata": true,
113+
"contextWindowSize": "10",
114+
"database": "default",
115+
"datasource": {
116+
"type": "vertamedia-clickhouse-datasource",
117+
"uid": "P7E099F39B84EA795"
118+
},
119+
"dateTimeColDataType": "event_time",
120+
"dateTimeType": "DATETIME",
121+
"editorMode": "sql",
122+
"extrapolate": true,
123+
"format": "table",
124+
"formattedQuery": "/* grafana dashboard=' single quotes reproduce', user=1 */\nwith\ncte2 as (\n select cte2_field,\n blatenant \"Alias\" \n FROM\n cte2_table\n WHERE 1\n AND cte2_tenant in (tenant1) \n)\nselect * EXCEPT (\"excluded_column\")\nFROM cte2",
125+
"interval": "",
126+
"intervalFactor": 1,
127+
"nullifySparse": false,
128+
"query": "with\ncte2 as (\n select cte2_field,\n blatenant \"Alias\" \n FROM\n cte2_table\n WHERE 1\n $conditionalTest(AND cte2_tenant in ($Tenant), $Tenant)\n)\nselect * EXCEPT (\"excluded_column\")\nFROM cte2",
129+
"refId": "A",
130+
"round": "0s",
131+
"showFormattedSQL": true,
132+
"skip_comments": true,
133+
"table": "test_grafana",
134+
"useWindowFuncForMacros": true
135+
}
136+
],
137+
"title": "CTE Alias + EXCEPT",
138+
"type": "timeseries"
139+
}
140+
],
141+
"preload": false,
142+
"schemaVersion": 42,
143+
"tags": [],
144+
"templating": {
145+
"list": [
146+
{
147+
"current": {
148+
"text": "tenant1",
149+
"value": "tenant1"
150+
},
151+
"datasource": {
152+
"type": "vertamedia-clickhouse-datasource",
153+
"uid": "P7E099F39B84EA795"
154+
},
155+
"definition": "SELECT 'tenant1' \nUNION ALL\nSELECT 'tenant2'\n",
156+
"name": "Tenant",
157+
"options": [],
158+
"query": {
159+
"adHocFilters": [],
160+
"adHocValuesQuery": "",
161+
"add_metadata": true,
162+
"contextWindowSize": "10",
163+
"datasourceMode": "Variable",
164+
"editorMode": "sql",
165+
"extrapolate": true,
166+
"format": "time_series",
167+
"formattedQuery": "/* grafana dashboard=' single quotes reproduce', user=1 */\nSELECT 'tenant1' \nUNION ALL\nSELECT 'tenant2'",
168+
"interval": "",
169+
"intervalFactor": 1,
170+
"nullifySparse": false,
171+
"query": "SELECT 'tenant1' \nUNION ALL\nSELECT 'tenant2'\n",
172+
"round": "0s",
173+
"skip_comments": true,
174+
"useWindowFuncForMacros": true
175+
},
176+
"refresh": 1,
177+
"regex": "",
178+
"type": "query"
179+
}
180+
]
181+
},
182+
"time": {
183+
"from": "now-6h",
184+
"to": "now"
185+
},
186+
"timepicker": {},
187+
"timezone": "browser",
188+
"title": "no single quotes in $conditonalTest reproduce ",
189+
"uid": "adrr6gn",
190+
"version": 4
191+
}

‎src/datasource/datasource.ts‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -783,13 +783,15 @@ export class CHDataSource
783783
let expandedQueries = queries;
784784
if (queries && queries.length > 0) {
785785
expandedQueries = queries.map((query: any) => {
786+
// Important: use transformed query for context-aware interpolation (fix for issue #847)
787+
const transformedQuery = conditionalTest(query.query, this.templateSrv);
786788
const expandedQuery = {
787789
...query,
788790
datasource: this.getRef(),
789791
query: this.templateSrv.replace(
790-
conditionalTest(query.query, this.templateSrv),
792+
transformedQuery,
791793
scopedVars,
792-
createContextAwareInterpolation(query.query, this.templateSrv.getVariables())
794+
createContextAwareInterpolation(transformedQuery, this.templateSrv.getVariables())
793795
),
794796
};
795797
return expandedQuery;
@@ -832,10 +834,13 @@ export class CHDataSource
832834
};
833835

834836
// Apply template variable replacements (these don't require backend processing)
837+
// Important: use transformed query for context-aware interpolation (fix for issue #847)
838+
const transformedQuery = conditionalTest(queryData.query, this.templateSrv);
839+
835840
queryData.query = this.templateSrv.replace(
836-
conditionalTest(queryData.query, this.templateSrv),
841+
transformedQuery,
837842
options.scopedVars,
838-
createContextAwareInterpolation(queryData.query, this.templateSrv.getVariables())
843+
createContextAwareInterpolation(transformedQuery, this.templateSrv.getVariables())
839844
);
840845

841846
// SAFE OPTIMIZATION: Batch createQuery + applyAdhocFilters (reduces 3->2 calls)
@@ -848,10 +853,12 @@ export class CHDataSource
848853
let query = queryResult.sql;
849854

850855
// Apply template variable replacements (these don't require backend processing)
856+
// Important: use transformed query for context-aware interpolation (fix for issue #847)
857+
const transformedQueryAfterBackend = conditionalTest(query, this.templateSrv);
851858
query = this.templateSrv.replace(
852-
conditionalTest(query, this.templateSrv),
859+
transformedQueryAfterBackend,
853860
options.scopedVars,
854-
createContextAwareInterpolation(query, this.templateSrv.getVariables())
861+
createContextAwareInterpolation(transformedQueryAfterBackend, this.templateSrv.getVariables())
855862
);
856863

857864
const wildcardChar = '%';
@@ -868,10 +875,12 @@ export class CHDataSource
868875
query = this.templateSrv.replace(query, scopedVars, createContextAwareInterpolation(query, this.templateSrv.getVariables()));
869876
}
870877

878+
// Important: use transformed query for context-aware interpolation (fix for issue #847)
879+
const finalTransformedQuery = conditionalTest(query, this.templateSrv);
871880
const interpolatedQuery = this.templateSrv.replace(
872-
conditionalTest(query, this.templateSrv),
881+
finalTransformedQuery,
873882
scopedVars,
874-
createContextAwareInterpolation(query, this.templateSrv.getVariables())
883+
createContextAwareInterpolation(finalTransformedQuery, this.templateSrv.getVariables())
875884
);
876885

877886
// Extract GROUP BY properties from the FINAL query (after template replacement)

‎src/datasource/helpers/index.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -833,5 +833,37 @@ describe('Variable Interpolation', () => {
833833
expect(resultQuery).toContain("WHERE host LIKE 'telegraf-%'");
834834
expect(resultQuery).not.toContain("''telegraf-'");
835835
});
836+
837+
it('should fix issue #847 - single value in IN clause with multi=false needs quotes', () => {
838+
// Issue #847: $conditionalTest(AND cte2_tenant in ($Tenant), $Tenant)
839+
// After expansion: AND cte2_tenant in ($Tenant)
840+
// Variable $Tenant has multi=false, includeAll=false (query variable default)
841+
// The value must be quoted in the IN clause
842+
const query = 'SELECT * FROM cte2 WHERE cte2_tenant in ($Tenant)';
843+
const variables = [{ name: 'Tenant', current: { value: 'tenant1' } }];
844+
const interpolateFn = interpolateQueryExprWithContext(query, variables);
845+
const variable = { name: 'Tenant', multi: false, includeAll: false };
846+
847+
// Priority 3 in interpolateQueryExprWithContext detects IN clause and quotes the value
848+
expect(interpolateFn('tenant1', variable)).toBe("'tenant1'");
849+
});
850+
851+
it('should fix issue #847 - NOT IN clause also needs quotes for single values', () => {
852+
const query = 'SELECT * FROM table WHERE id NOT IN ($var)';
853+
const variables = [{ name: 'var', current: { value: 'value1' } }];
854+
const interpolateFn = interpolateQueryExprWithContext(query, variables);
855+
const variable = { name: 'var', multi: false, includeAll: false };
856+
857+
expect(interpolateFn('value1', variable)).toBe("'value1'");
858+
});
859+
860+
it('should fix issue #847 - GLOBAL IN clause also needs quotes for single values', () => {
861+
const query = 'SELECT * FROM table WHERE id GLOBAL IN ($var)';
862+
const variables = [{ name: 'var', current: { value: 'value1' } }];
863+
const interpolateFn = interpolateQueryExprWithContext(query, variables);
864+
const variable = { name: 'var', multi: false, includeAll: false };
865+
866+
expect(interpolateFn('value1', variable)).toBe("'value1'");
867+
});
836868
});
837869
});

0 commit comments

Comments
 (0)