From 75e0a0eb939ec176b91ba8a6d91224c44b46db67 Mon Sep 17 00:00:00 2001 From: "grafana-delivery-bot[bot]" <132647405+grafana-delivery-bot[bot]@users.noreply.github.com> Date: Thu, 4 Dec 2025 21:29:05 +0100 Subject: [PATCH] [release-12.3.1] Postgresql: Fix variable interpolation logic when the variable has multiple values (#114876) Postgresql: Fix variable interpolation logic when the variable has multiple values (#114058) * fix the variable interpolation * add jest config to grafana-sql * fix broken tests * add variable interpolation tests * lint * apply fix only to postresql datasource (cherry picked from commit 0291f6d1e71715fb020b16e6e4dd9a66d9093136) Co-authored-by: ismail simsek --- packages/grafana-sql/jest.config.js | 5 + .../datasource/variable-interpolation.test.ts | 169 ++++++++++++++++++ .../datasource.test.ts | 8 +- .../datasource.ts | 24 ++- 4 files changed, 201 insertions(+), 5 deletions(-) create mode 100644 packages/grafana-sql/jest.config.js create mode 100644 packages/grafana-sql/src/datasource/variable-interpolation.test.ts diff --git a/packages/grafana-sql/jest.config.js b/packages/grafana-sql/jest.config.js new file mode 100644 index 00000000000..93995be84c2 --- /dev/null +++ b/packages/grafana-sql/jest.config.js @@ -0,0 +1,5 @@ +const sharedConfig = require('../../jest.config.js'); +module.exports = { + ...sharedConfig, + rootDir: '../../', +}; diff --git a/packages/grafana-sql/src/datasource/variable-interpolation.test.ts b/packages/grafana-sql/src/datasource/variable-interpolation.test.ts new file mode 100644 index 00000000000..606d2b0f6bb --- /dev/null +++ b/packages/grafana-sql/src/datasource/variable-interpolation.test.ts @@ -0,0 +1,169 @@ +import { DataSourceInstanceSettings } from '@grafana/data'; + +import { DB, SQLOptions, SqlQueryModel } from '../types'; +import { makeVariable } from '../utils/testHelpers'; + +import { SqlDatasource } from './SqlDatasource'; + +// Minimal test implementation of SqlDatasource +class TestSqlDatasource extends SqlDatasource { + getDB(): DB { + return {} as DB; + } + + getQueryModel(): SqlQueryModel { + return { + quoteLiteral: (value: string) => `'${value.replace(/'/g, "''")}'`, + } as SqlQueryModel; + } +} + +describe('SqlDatasource - Variable Interpolation', () => { + const instanceSettings = { + jsonData: { + defaultProject: 'testproject', + }, + } as unknown as DataSourceInstanceSettings; + + let ds: TestSqlDatasource; + + beforeEach(() => { + ds = new TestSqlDatasource(instanceSettings); + }); + + describe('Case 1: Multi-value enabled, single value selected', () => { + it('should escape single quotes in string value', () => { + const variable = makeVariable('id1', 'name1', { multi: true }); + // When we apply the general fix for all SQL data sources these should be uncommented + // expect(ds.interpolateVariable('value1', variable)).toEqual('value1'); + // expect(ds.interpolateVariable("O'Brien", variable)).toEqual("O''Brien"); + expect(ds.interpolateVariable('value1', variable)).toEqual(`'value1'`); + expect(ds.interpolateVariable("O'Brien", variable)).toEqual(`'O''Brien'`); + }); + + it('should handle numeric value', () => { + const variable = makeVariable('id1', 'name1', { multi: true }); + expect(ds.interpolateVariable(42 as unknown as string, variable)).toEqual(42); + }); + }); + + describe('Case 2: Multi-value enabled, multiple values selected', () => { + it('should return quoted, comma-separated values', () => { + const variable = makeVariable('id1', 'name1', { multi: true }); + expect(ds.interpolateVariable(['value1', 'value2', 'value3'], variable)).toEqual("'value1','value2','value3'"); + }); + + it('should escape single quotes in array values', () => { + const variable = makeVariable('id1', 'name1', { multi: true }); + expect(ds.interpolateVariable(["O'Brien", 'Smith', "D'Angelo"], variable)).toEqual( + "'O''Brien','Smith','D''Angelo'" + ); + }); + + it('should handle empty array', () => { + const variable = makeVariable('id1', 'name1', { multi: true }); + expect(ds.interpolateVariable([], variable)).toEqual(''); + }); + }); + + describe('Case 3: Include all enabled, single value selected', () => { + it('should escape single quotes in string value', () => { + const variable = makeVariable('id1', 'name1', { includeAll: true }); + // When we apply the general fix for all SQL data sources these should be uncommented + // expect(ds.interpolateVariable('value1', variable)).toEqual('value1'); + // expect(ds.interpolateVariable("O'Brien", variable)).toEqual("O''Brien"); + expect(ds.interpolateVariable('value1', variable)).toEqual(`'value1'`); + expect(ds.interpolateVariable("O'Brien", variable)).toEqual(`'O''Brien'`); + }); + + it('should handle numeric value', () => { + const variable = makeVariable('id1', 'name1', { includeAll: true }); + expect(ds.interpolateVariable(123 as unknown as string, variable)).toEqual(123); + }); + }); + + describe('Case 4: Include all enabled, "All" value selected', () => { + it('should handle All option as array', () => { + const variable = makeVariable('id1', 'name1', { includeAll: true }); + expect(ds.interpolateVariable(['value1', 'value2', 'value3'], variable)).toEqual("'value1','value2','value3'"); + }); + + it('should handle All option with special characters', () => { + const variable = makeVariable('id1', 'name1', { includeAll: true }); + expect(ds.interpolateVariable(["test'1", 'test2', "test'3"], variable)).toEqual("'test''1','test2','test''3'"); + }); + }); + + describe('Case 5: No include all, no multi-value, single value selected', () => { + it('should escape single quotes in string value', () => { + const variable = makeVariable('id1', 'name1', { multi: false, includeAll: false }); + expect(ds.interpolateVariable('value1', variable)).toEqual('value1'); + expect(ds.interpolateVariable("O'Brien", variable)).toEqual("O''Brien"); + }); + + it('should handle numeric value', () => { + const variable = makeVariable('id1', 'name1', { multi: false, includeAll: false }); + expect(ds.interpolateVariable(999 as unknown as string, variable)).toEqual(999); + }); + + it('should handle empty string', () => { + const variable = makeVariable('id1', 'name1', { multi: false, includeAll: false }); + expect(ds.interpolateVariable('', variable)).toEqual(''); + }); + }); + + describe('Case 6: Both include all and multi-value enabled, single value selected', () => { + it('should escape single quotes in string value', () => { + const variable = makeVariable('id1', 'name1', { multi: true, includeAll: true }); + // When we apply the general fix for all SQL data sources these should be uncommented + // expect(ds.interpolateVariable('value1', variable)).toEqual('value1'); + // expect(ds.interpolateVariable("O'Brien", variable)).toEqual("O''Brien"); + expect(ds.interpolateVariable('value1', variable)).toEqual(`'value1'`); + expect(ds.interpolateVariable("O'Brien", variable)).toEqual(`'O''Brien'`); + }); + + it('should handle numeric value', () => { + const variable = makeVariable('id1', 'name1', { multi: true, includeAll: true }); + expect(ds.interpolateVariable(456 as unknown as string, variable)).toEqual(456); + }); + }); + + describe('Case 7: Both include all and multi-value enabled, "All" value selected', () => { + it('should handle All option as array', () => { + const variable = makeVariable('id1', 'name1', { multi: true, includeAll: true }); + expect(ds.interpolateVariable(['value1', 'value2', 'value3'], variable)).toEqual("'value1','value2','value3'"); + }); + + it('should handle All option with mixed values', () => { + const variable = makeVariable('id1', 'name1', { multi: true, includeAll: true }); + expect(ds.interpolateVariable(['alpha', 'beta', 'gamma'], variable)).toEqual("'alpha','beta','gamma'"); + }); + + it('should handle All option with special characters', () => { + const variable = makeVariable('id1', 'name1', { multi: true, includeAll: true }); + expect(ds.interpolateVariable(["it's", "can't", "won't"], variable)).toEqual("'it''s','can''t','won''t'"); + }); + }); + + describe('Case 8: Both include all and multi-value enabled, multiple values selected', () => { + it('should return quoted, comma-separated values', () => { + const variable = makeVariable('id1', 'name1', { multi: true, includeAll: true }); + expect(ds.interpolateVariable(['value1', 'value2'], variable)).toEqual("'value1','value2'"); + }); + + it('should escape single quotes in array values', () => { + const variable = makeVariable('id1', 'name1', { multi: true, includeAll: true }); + expect(ds.interpolateVariable(["O'Brien", "D'Angelo"], variable)).toEqual("'O''Brien','D''Angelo'"); + }); + + it('should handle single item array', () => { + const variable = makeVariable('id1', 'name1', { multi: true, includeAll: true }); + expect(ds.interpolateVariable(['value1'], variable)).toEqual("'value1'"); + }); + + it('should handle array with single quote escaping', () => { + const variable = makeVariable('id1', 'name1', { multi: true, includeAll: true }); + expect(ds.interpolateVariable(['a', "b'c", 'd'], variable)).toEqual("'a','b''c','d'"); + }); + }); +}); diff --git a/public/app/plugins/datasource/grafana-postgresql-datasource/datasource.test.ts b/public/app/plugins/datasource/grafana-postgresql-datasource/datasource.test.ts index 207991a495a..9067ab7f138 100644 --- a/public/app/plugins/datasource/grafana-postgresql-datasource/datasource.test.ts +++ b/public/app/plugins/datasource/grafana-postgresql-datasource/datasource.test.ts @@ -714,7 +714,7 @@ describe('PostgreSQLDatasource', () => { it('should return a quoted value', () => { const { ds, variable } = setupTestContext({}); variable.multi = true; - expect(ds.interpolateVariable('abc', variable)).toEqual("'abc'"); + expect(ds.interpolateVariable('abc', variable)).toEqual('abc'); }); }); @@ -722,8 +722,8 @@ describe('PostgreSQLDatasource', () => { it('should return a quoted value', () => { const { ds, variable } = setupTestContext({}); variable.multi = true; - expect(ds.interpolateVariable("a'bc", variable)).toEqual("'a''bc'"); - expect(ds.interpolateVariable("a'b'c", variable)).toEqual("'a''b''c'"); + expect(ds.interpolateVariable("a'bc", variable)).toEqual("a''bc"); + expect(ds.interpolateVariable("a'b'c", variable)).toEqual("a''b''c"); }); }); @@ -731,7 +731,7 @@ describe('PostgreSQLDatasource', () => { it('should return a quoted value', () => { const { ds, variable } = setupTestContext({}); variable.includeAll = true; - expect(ds.interpolateVariable('abc', variable)).toEqual("'abc'"); + expect(ds.interpolateVariable('abc', variable)).toEqual('abc'); }); }); }); diff --git a/public/app/plugins/datasource/grafana-postgresql-datasource/datasource.ts b/public/app/plugins/datasource/grafana-postgresql-datasource/datasource.ts index 3a79e700fb7..9ea4498db62 100644 --- a/public/app/plugins/datasource/grafana-postgresql-datasource/datasource.ts +++ b/public/app/plugins/datasource/grafana-postgresql-datasource/datasource.ts @@ -1,6 +1,6 @@ import { v4 as uuidv4 } from 'uuid'; -import { DataSourceInstanceSettings, ScopedVars } from '@grafana/data'; +import { DataSourceInstanceSettings, ScopedVars, VariableWithMultiSupport } from '@grafana/data'; import { LanguageDefinition } from '@grafana/plugin-ui'; import { TemplateSrv } from '@grafana/runtime'; import { @@ -31,6 +31,28 @@ export class PostgresDatasource extends SqlDatasource { return new PostgresQueryModel(target, templateSrv, scopedVars); } + interpolateVariable = (value: string | string[] | number, variable: VariableWithMultiSupport) => { + if (typeof value === 'string') { + // For single string values, just escape quotes (don't add outer quotes) + // The quotes are provided by the query template: WHERE x = '$var' + // We only escape internal single quotes: O'Brien -> O''Brien + return String(value).replace(/'/g, "''"); + } + + if (typeof value === 'number') { + return value; + } + + if (Array.isArray(value)) { + // For arrays, quote each value individually and join with comma + // Used in: WHERE x IN ($var) -> WHERE x IN ('val1','val2','val3') + const quotedValues = value.map((v) => this.getQueryModel().quoteLiteral(v)); + return quotedValues.join(','); + } + + return value; + }; + async getVersion(): Promise { const value = await this.runSql<{ version: number }>(getVersion()); const results = value.fields.version?.values;