From f4f7ac42ec164af87044e745229c79134a5c4f6a Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 22 Mar 2024 11:40:44 +0000 Subject: [PATCH] Adding test cases for interpolation of SQL, confirming that the context correctly gets cleaned up before passing into bindings. --- .../server/src/api/controllers/query/index.ts | 8 ++ .../routes/tests/queries/generic-sql.spec.ts | 85 +++++++++++++++++-- .../api/routes/tests/queries/mongodb.spec.ts | 2 +- .../server/src/integrations/queries/sql.ts | 15 +++- .../server/src/sdk/app/queries/queries.ts | 18 +++- .../server/src/tests/utilities/api/query.ts | 2 +- packages/server/src/threads/query.ts | 29 +++++-- 7 files changed, 137 insertions(+), 22 deletions(-) diff --git a/packages/server/src/api/controllers/query/index.ts b/packages/server/src/api/controllers/query/index.ts index b05b4b1222..03fcdb714a 100644 --- a/packages/server/src/api/controllers/query/index.ts +++ b/packages/server/src/api/controllers/query/index.ts @@ -105,6 +105,12 @@ export async function save(ctx: UserCtx) { query.nullDefaultSupport = true eventFn = () => events.query.created(datasource, query) } else { + // check if flag has previously been set, don't let it change + // allow it to be explicitly set to false via API incase this is ever needed + const existingQuery = await db.get(query._id) + if (existingQuery.nullDefaultSupport && query.nullDefaultSupport == null) { + query.nullDefaultSupport = true + } eventFn = () => events.query.updated(datasource, query) } const response = await db.put(query) @@ -285,6 +291,7 @@ export async function preview( parameters: enrichParameters(query), transformer: query.transformer, schema: query.schema, + nullDefaultSupport: query.nullDefaultSupport, queryId, datasource, // have to pass down to the thread runner - can't put into context now @@ -353,6 +360,7 @@ async function execute( queryId: ctx.params.queryId, // have to pass down to the thread runner - can't put into context now environmentVariables: envVars, + nullDefaultSupport: query.nullDefaultSupport, ctx: { user: ctx.user, auth: { ...authConfigCtx }, diff --git a/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts b/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts index 1fc0ecb382..ac90efa71c 100644 --- a/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts +++ b/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts @@ -12,31 +12,51 @@ const createTableSQL: Record = { CREATE TABLE test_table ( id serial PRIMARY KEY, name VARCHAR ( 50 ) NOT NULL, - birthday TIMESTAMP + birthday TIMESTAMP, + number INT );`, [SourceName.MYSQL]: ` CREATE TABLE test_table ( id INT AUTO_INCREMENT PRIMARY KEY, name VARCHAR(50) NOT NULL, - birthday TIMESTAMP + birthday TIMESTAMP, + number INT );`, [SourceName.SQL_SERVER]: ` CREATE TABLE test_table ( id INT IDENTITY(1,1) PRIMARY KEY, name NVARCHAR(50) NOT NULL, - birthday DATETIME + birthday DATETIME, + number INT );`, } const insertSQL = `INSERT INTO test_table (name) VALUES ('one'), ('two'), ('three'), ('four'), ('five')` const dropTableSQL = `DROP TABLE test_table;` +const POSTGRES_SPECIFICS = { + nullError: 'invalid input syntax for type integer: ""', +} + +const MYSQL_SPECIFICS = { + nullError: "Incorrect integer value: '' for column 'number' at row 1", +} + +const MSSQL_SPECIFICS = { + nullError: "Cannot convert undefined or null to object", +} + +const MARIADB_SPECIFICS = { + nullError: + "Incorrect integer value: '' for column `mysql`.`test_table`.`number` at row 1", +} + describe.each([ - ["postgres", databaseTestProviders.postgres], - ["mysql", databaseTestProviders.mysql], - ["mssql", databaseTestProviders.mssql], - ["mariadb", databaseTestProviders.mariadb], -])("queries (%s)", (__, dsProvider) => { + ["postgres", databaseTestProviders.postgres, POSTGRES_SPECIFICS], + ["mysql", databaseTestProviders.mysql, MYSQL_SPECIFICS], + ["mssql", databaseTestProviders.mssql, MSSQL_SPECIFICS], + ["mariadb", databaseTestProviders.mariadb, MARIADB_SPECIFICS], +])("queries (%s)", (__, dsProvider, testSpecificInformation) => { const config = setup.getConfig() let datasource: Datasource @@ -51,7 +71,7 @@ describe.each([ transformer: "return data", readable: true, } - return await config.api.query.create({ ...defaultQuery, ...query }) + return await config.api.query.save({ ...defaultQuery, ...query }) } async function rawQuery(sql: string): Promise { @@ -398,4 +418,51 @@ describe.each([ expect(rows).toHaveLength(0) }) }) + + // this parameter really only impacts SQL queries + describe("confirm nullDefaultSupport", () => { + const queryParams = { + fields: { + sql: "INSERT INTO test_table (name, number) VALUES ({{ bindingName }}, {{ bindingNumber }})", + }, + parameters: [ + { + name: "bindingName", + default: "", + }, + { + name: "bindingNumber", + default: "", + }, + ], + queryVerb: "create", + } + + it("should error for old queries", async () => { + const query = await createQuery(queryParams) + await config.api.query.save({ ...query, nullDefaultSupport: false }) + let error: string | undefined + try { + await config.api.query.execute(query._id!, { + parameters: { + bindingName: "testing", + }, + }) + } catch (err: any) { + error = err.message + } + expect(error).toBeDefined() + expect(error).toContain(testSpecificInformation.nullError) + }) + + it("should not error for new queries", async () => { + const query = await createQuery(queryParams) + const results = await config.api.query.execute(query._id!, { + parameters: { + bindingName: "testing", + }, + }) + expect(results).toEqual({ data: [{ created: true }] }) + }) + }) }) diff --git a/packages/server/src/api/routes/tests/queries/mongodb.spec.ts b/packages/server/src/api/routes/tests/queries/mongodb.spec.ts index b61f905bef..3280c37e78 100644 --- a/packages/server/src/api/routes/tests/queries/mongodb.spec.ts +++ b/packages/server/src/api/routes/tests/queries/mongodb.spec.ts @@ -33,7 +33,7 @@ describe("/queries", () => { ) { combinedQuery.fields.extra.collection = collection } - return await config.api.query.create(combinedQuery) + return await config.api.query.save(combinedQuery) } async function withClient( diff --git a/packages/server/src/integrations/queries/sql.ts b/packages/server/src/integrations/queries/sql.ts index 6d42117d7d..fd1363c39a 100644 --- a/packages/server/src/integrations/queries/sql.ts +++ b/packages/server/src/integrations/queries/sql.ts @@ -1,13 +1,15 @@ import { findHBSBlocks } from "@budibase/string-templates" import { DatasourcePlus } from "@budibase/types" import sdk from "../../sdk" +import { enrichArrayContext } from "../../sdk/app/queries/queries" const CONST_CHAR_REGEX = new RegExp("'[^']*'", "g") export async function interpolateSQL( - fields: { [key: string]: any }, + fields: { sql: string; bindings: any[] }, parameters: { [key: string]: any }, - integration: DatasourcePlus + integration: DatasourcePlus, + opts: { nullDefaultSupport: boolean } ) { let sql = fields.sql if (!sql || typeof sql !== "string") { @@ -64,7 +66,14 @@ export async function interpolateSQL( } // replicate the knex structure fields.sql = sql - fields.bindings = await sdk.queries.enrichContext(variables, parameters) + fields.bindings = await sdk.queries.enrichArrayContext(variables, parameters) + if (opts.nullDefaultSupport) { + for (let index in fields.bindings) { + if (fields.bindings[index] === "") { + fields.bindings[index] = null + } + } + } // check for arrays in the data let updated: string[] = [] for (let i = 0; i < variables.length; i++) { diff --git a/packages/server/src/sdk/app/queries/queries.ts b/packages/server/src/sdk/app/queries/queries.ts index 3f967b7198..cf0ed4ec95 100644 --- a/packages/server/src/sdk/app/queries/queries.ts +++ b/packages/server/src/sdk/app/queries/queries.ts @@ -65,11 +65,27 @@ export async function fetch(opts: { enrich: boolean } = { enrich: true }) { return updateSchemas(queries) } +export async function enrichArrayContext( + fields: any[], + inputs = {} +): Promise { + const map: Record = {} + for (let [key, value] of Object.entries(fields)) { + map[key] = value + } + const output = await enrichContext(map, inputs) + const outputArray: any[] = [] + for (let [key, value] of Object.entries(output)) { + outputArray[parseInt(key)] = value + } + return outputArray +} + export async function enrichContext( fields: Record, inputs = {} ): Promise> { - const enrichedQuery: Record = Array.isArray(fields) ? [] : {} + const enrichedQuery: Record = {} if (!fields || !inputs) { return enrichedQuery } diff --git a/packages/server/src/tests/utilities/api/query.ts b/packages/server/src/tests/utilities/api/query.ts index 32866314ff..0dad6d2ad9 100644 --- a/packages/server/src/tests/utilities/api/query.ts +++ b/packages/server/src/tests/utilities/api/query.ts @@ -8,7 +8,7 @@ import { import { TestAPI } from "./base" export class QueryAPI extends TestAPI { - create = async (body: Query): Promise => { + save = async (body: Query): Promise => { return await this._post(`/api/queries`, { body }) } diff --git a/packages/server/src/threads/query.ts b/packages/server/src/threads/query.ts index 9f5e02bf69..dbac76c09a 100644 --- a/packages/server/src/threads/query.ts +++ b/packages/server/src/threads/query.ts @@ -26,10 +26,11 @@ class QueryRunner { fields: any parameters: any pagination: any - transformer: string + transformer: string | null cachedVariables: any[] ctx: any queryResponse: any + nullDefaultSupport: boolean noRecursiveQuery: boolean hasRerun: boolean hasRefreshedOAuth: boolean @@ -45,6 +46,7 @@ class QueryRunner { this.transformer = input.transformer this.queryId = input.queryId! this.schema = input.schema + this.nullDefaultSupport = !!input.nullDefaultSupport this.noRecursiveQuery = flags.noRecursiveQuery this.cachedVariables = [] // Additional context items for enrichment @@ -59,7 +61,14 @@ class QueryRunner { } async execute(): Promise { - let { datasource, fields, queryVerb, transformer, schema } = this + let { + datasource, + fields, + queryVerb, + transformer, + schema, + nullDefaultSupport, + } = this let datasourceClone = cloneDeep(datasource) let fieldsClone = cloneDeep(fields) @@ -103,7 +112,9 @@ class QueryRunner { let query // handle SQL injections by interpolating the variables if (isSQL(datasourceClone)) { - query = await interpolateSQL(fieldsClone, enrichedContext, integration) + query = await interpolateSQL(fieldsClone, enrichedContext, integration, { + nullDefaultSupport, + }) } else { query = await sdk.queries.enrichContext(fieldsClone, enrichedContext) } @@ -137,7 +148,9 @@ class QueryRunner { data: rows, params: enrichedParameters, } - rows = vm.withContext(ctx, () => vm.execute(transformer)) + if (transformer != null) { + rows = vm.withContext(ctx, () => vm.execute(transformer!)) + } } // if the request fails we retry once, invalidating the cached value @@ -191,13 +204,15 @@ class QueryRunner { }) return new QueryRunner( { - datasource, + schema: query.schema, queryVerb: query.queryVerb, fields: query.fields, - parameters, transformer: query.transformer, - queryId, + nullDefaultSupport: query.nullDefaultSupport, ctx: this.ctx, + parameters, + datasource, + queryId, }, { noRecursiveQuery: true } ).execute()