From aea9cda8f5408fe4eab068d89c2b9d1b50cdf761 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 17 Jul 2024 15:45:35 +0100 Subject: [PATCH 1/5] wip --- packages/backend-core/src/sql/sql.ts | 101 ++++++++++-------- packages/backend-core/src/sql/sqlTable.ts | 6 +- .../src/api/routes/tests/search.spec.ts | 35 ++++-- 3 files changed, 90 insertions(+), 52 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 4936e4da68..161c2a7488 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -42,27 +42,28 @@ const envLimit = environment.SQL_MAX_ROWS : null const BASE_LIMIT = envLimit || 5000 -function likeKey(client: string | string[], key: string): string { - let start: string, end: string +// Takes a string like foo and returns a quoted string like [foo] for SQL Server +// and "foo" for Postgres. +function quote(client: SqlClient, str: string) { switch (client) { - case SqlClient.MY_SQL: - start = end = "`" - break case SqlClient.SQL_LITE: case SqlClient.ORACLE: case SqlClient.POSTGRES: - start = end = '"' - break + return `"${str}"` case SqlClient.MS_SQL: - start = "[" - end = "]" - break + return `[${str}]` default: - throw new Error("Unknown client generating like key") + return `\`${str}\`` } - const parts = key.split(".") - key = parts.map(part => `${start}${part}${end}`).join(".") +} + +// Takes a string like a.b.c and returns a quoted identifier like [a].[b].[c] +// for SQL Server and `a`.`b`.`c` for MySQL. +function quotedIdentifier(client: SqlClient, key: string): string { return key + .split(".") + .map(part => quote(client, part)) + .join(".") } function parse(input: any) { @@ -113,34 +114,37 @@ function generateSelectStatement( knex: Knex ): (string | Knex.Raw)[] | "*" { const { resource, meta } = json + const client = knex.client.config.client as SqlClient if (!resource || !resource.fields || resource.fields.length === 0) { return "*" } - const schema = meta?.table?.schema + const schema = meta.table.schema return resource.fields.map(field => { - const fieldNames = field.split(/\./g) - const tableName = fieldNames[0] - const columnName = fieldNames[1] - const columnSchema = schema?.[columnName] - if (columnSchema && knex.client.config.client === SqlClient.POSTGRES) { - const externalType = schema[columnName].externalType - if (externalType?.includes("money")) { - return knex.raw( - `"${tableName}"."${columnName}"::money::numeric as "${field}"` - ) - } + const [table, column, ..._rest] = field.split(/\./g) + if ( + client === SqlClient.POSTGRES && + schema[column].externalType?.includes("money") + ) { + return knex.raw(`"${table}"."${column}"::money::numeric as "${field}"`) } if ( - knex.client.config.client === SqlClient.MS_SQL && - columnSchema?.type === FieldType.DATETIME && - columnSchema.timeOnly + client === SqlClient.MS_SQL && + schema[column]?.type === FieldType.DATETIME && + schema[column].timeOnly ) { - // Time gets returned as timestamp from mssql, not matching the expected HH:mm format + // Time gets returned as timestamp from mssql, not matching the expected + // HH:mm format return knex.raw(`CONVERT(varchar, ${field}, 108) as "${field}"`) } return `${field} as ${field}` + // return knex.raw( + // `${quote(client, table)}.${quote(client, column)} as ${quote( + // client, + // field + // )}` + // ) }) } @@ -173,9 +177,9 @@ function convertBooleans(query: SqlQuery | SqlQuery[]): SqlQuery | SqlQuery[] { } class InternalBuilder { - private readonly client: string + private readonly client: SqlClient - constructor(client: string) { + constructor(client: SqlClient) { this.client = client } @@ -250,9 +254,10 @@ class InternalBuilder { } else { const rawFnc = `${fnc}Raw` // @ts-ignore - query = query[rawFnc](`LOWER(${likeKey(this.client, key)}) LIKE ?`, [ - `%${value.toLowerCase()}%`, - ]) + query = query[rawFnc]( + `LOWER(${quotedIdentifier(this.client, key)}) LIKE ?`, + [`%${value.toLowerCase()}%`] + ) } } @@ -302,7 +307,10 @@ class InternalBuilder { } statement += (statement ? andOr : "") + - `COALESCE(LOWER(${likeKey(this.client, key)}), '') LIKE ?` + `COALESCE(LOWER(${quotedIdentifier( + this.client, + key + )}), '') LIKE ?` } if (statement === "") { @@ -336,9 +344,10 @@ class InternalBuilder { } else { const rawFnc = `${fnc}Raw` // @ts-ignore - query = query[rawFnc](`LOWER(${likeKey(this.client, key)}) LIKE ?`, [ - `${value.toLowerCase()}%`, - ]) + query = query[rawFnc]( + `LOWER(${quotedIdentifier(this.client, key)}) LIKE ?`, + [`${value.toLowerCase()}%`] + ) } }) } @@ -376,12 +385,15 @@ class InternalBuilder { const fnc = allOr ? "orWhereRaw" : "whereRaw" if (this.client === SqlClient.MS_SQL) { query = query[fnc]( - `CASE WHEN ${likeKey(this.client, key)} = ? THEN 1 ELSE 0 END = 1`, + `CASE WHEN ${quotedIdentifier( + this.client, + key + )} = ? THEN 1 ELSE 0 END = 1`, [value] ) } else { query = query[fnc]( - `COALESCE(${likeKey(this.client, key)} = ?, FALSE)`, + `COALESCE(${quotedIdentifier(this.client, key)} = ?, FALSE)`, [value] ) } @@ -392,12 +404,15 @@ class InternalBuilder { const fnc = allOr ? "orWhereRaw" : "whereRaw" if (this.client === SqlClient.MS_SQL) { query = query[fnc]( - `CASE WHEN ${likeKey(this.client, key)} = ? THEN 1 ELSE 0 END = 0`, + `CASE WHEN ${quotedIdentifier( + this.client, + key + )} = ? THEN 1 ELSE 0 END = 0`, [value] ) } else { query = query[fnc]( - `COALESCE(${likeKey(this.client, key)} != ?, TRUE)`, + `COALESCE(${quotedIdentifier(this.client, key)} != ?, TRUE)`, [value] ) } @@ -769,7 +784,7 @@ class SqlQueryBuilder extends SqlTableQueryBuilder { private readonly limit: number // pass through client to get flavour of SQL - constructor(client: string, limit: number = BASE_LIMIT) { + constructor(client: SqlClient, limit: number = BASE_LIMIT) { super(client) this.limit = limit } diff --git a/packages/backend-core/src/sql/sqlTable.ts b/packages/backend-core/src/sql/sqlTable.ts index bdc8a3dd69..02acc8af85 100644 --- a/packages/backend-core/src/sql/sqlTable.ts +++ b/packages/backend-core/src/sql/sqlTable.ts @@ -195,14 +195,14 @@ function buildDeleteTable(knex: SchemaBuilder, table: Table): SchemaBuilder { } class SqlTableQueryBuilder { - private readonly sqlClient: string + private readonly sqlClient: SqlClient // pass through client to get flavour of SQL - constructor(client: string) { + constructor(client: SqlClient) { this.sqlClient = client } - getSqlClient(): string { + getSqlClient(): SqlClient { return this.sqlClient } diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index ae35c4c5eb..24197462ee 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -38,13 +38,13 @@ import { structures } from "@budibase/backend-core/tests" import { DEFAULT_EMPLOYEE_TABLE_SCHEMA } from "../../../db/defaultData/datasource_bb_default" describe.each([ - ["in-memory", undefined], - ["lucene", undefined], + // ["in-memory", undefined], + // ["lucene", undefined], ["sqs", undefined], - [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + // [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], + // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], ])("search (%s)", (name, dsProvider) => { const isSqs = name === "sqs" const isLucene = name === "lucene" @@ -735,6 +735,29 @@ describe.each([ query: {}, }).toHaveLength(1) }) + + isInternal && + describe("space at end of column name", () => { + beforeAll(async () => { + table = await createTable({ + "name ": { + name: "name ", + type: FieldType.STRING, + }, + }) + await createRows([{ ["name "]: "foo" }, { ["name "]: "bar" }]) + }) + + it("should be able to query a column that starts with a space", async () => { + await expectSearch({ + query: { + string: { + "1:name ": "foo", + }, + }, + }).toContainExactly([{ ["name "]: "foo" }]) + }) + }) }) describe("equal", () => { From 0b2a5162a4698530f0d5fde2e2c29a33ec29d675 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 18 Jul 2024 11:00:10 +0100 Subject: [PATCH 2/5] Fix the problem, and the tests. --- packages/backend-core/src/sql/sql.ts | 24 ++-- .../src/api/routes/tests/search.spec.ts | 127 +++++++++++++----- .../server/src/sdk/app/rows/search/sqs.ts | 1 - 3 files changed, 107 insertions(+), 45 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 161c2a7488..25c9ead191 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -122,7 +122,7 @@ function generateSelectStatement( const schema = meta.table.schema return resource.fields.map(field => { - const [table, column, ..._rest] = field.split(/\./g) + const [table, column, ...rest] = field.split(/\./g) if ( client === SqlClient.POSTGRES && schema[column].externalType?.includes("money") @@ -138,13 +138,21 @@ function generateSelectStatement( // HH:mm format return knex.raw(`CONVERT(varchar, ${field}, 108) as "${field}"`) } - return `${field} as ${field}` - // return knex.raw( - // `${quote(client, table)}.${quote(client, column)} as ${quote( - // client, - // field - // )}` - // ) + + // There's at least two edge cases being handled in the expression below. + // 1. The column name could start/end with a space, and in that case we + // want to preseve that space. + // 2. Almost all column names are specified in the form table.column, except + // in the case of relationships, where it's table.doc1.column. In that + // case, we want to split it into `table`.`doc1.column` for reasons that + // aren't actually clear to me, but `table`.`doc1` breaks things with the + // sample data tests. + return knex.raw( + `${quote(client, table)}.${quote( + client, + [column, ...rest].join(".") + )} as ${quote(client, field)}` + ) }) } diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 24197462ee..cbfa05e343 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -38,18 +38,18 @@ import { structures } from "@budibase/backend-core/tests" import { DEFAULT_EMPLOYEE_TABLE_SCHEMA } from "../../../db/defaultData/datasource_bb_default" describe.each([ - // ["in-memory", undefined], - // ["lucene", undefined], + ["in-memory", undefined], + ["lucene", undefined], ["sqs", undefined], - // [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], + [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], ])("search (%s)", (name, dsProvider) => { const isSqs = name === "sqs" const isLucene = name === "lucene" const isInMemory = name === "in-memory" - const isInternal = isSqs || isLucene + const isInternal = isSqs || isLucene || isInMemory const config = setup.getConfig() let envCleanup: (() => void) | undefined @@ -115,10 +115,7 @@ describe.each([ if (isInMemory) { return dataFilters.search(_.cloneDeep(rows), this.query) } else { - return config.api.row.search(table._id!, { - ...this.query, - tableId: table._id!, - }) + return config.api.row.search(this.query.tableId, this.query) } } @@ -187,6 +184,15 @@ describe.each([ return row } + // By default, the global `table` variable is used to search against. If you + // need to, though, you can specify a different table to search against. You + // may wish to do this in a nested describe block that defines a different + // table. + againstTable(table: Table) { + this.query.tableId = table._id! + return this + } + // Asserts that the query returns rows matching exactly the set of rows // passed in. The order of the rows matters. Rows returned in an order // different to the one passed in will cause the assertion to fail. Extra @@ -735,29 +741,6 @@ describe.each([ query: {}, }).toHaveLength(1) }) - - isInternal && - describe("space at end of column name", () => { - beforeAll(async () => { - table = await createTable({ - "name ": { - name: "name ", - type: FieldType.STRING, - }, - }) - await createRows([{ ["name "]: "foo" }, { ["name "]: "bar" }]) - }) - - it("should be able to query a column that starts with a space", async () => { - await expectSearch({ - query: { - string: { - "1:name ": "foo", - }, - }, - }).toContainExactly([{ ["name "]: "foo" }]) - }) - }) }) describe("equal", () => { @@ -2205,8 +2188,7 @@ describe.each([ }).toContainExactly([{ name: "baz", productCat: undefined }]) }) }) - - isInternal && + ;(isSqs || isLucene) && describe("relations to same table", () => { let relatedTable: Table, relatedRows: Row[] @@ -2394,6 +2376,7 @@ describe.each([ beforeAll(async () => { await config.api.application.addSampleData(config.appId!) table = DEFAULT_EMPLOYEE_TABLE_SCHEMA + rows = await config.api.row.fetch(table._id!) }) it("should be able to search sample data", async () => { @@ -2478,4 +2461,76 @@ describe.each([ }).toContainExactly([{ [name]: "a" }]) }) }) + + // This is currently not supported in external datasources, it produces SQL + // errors at time of writing. We supported it (potentially by accident) in + // Lucene, though, so we need to make sure it's supported in SQS as well. We + // found real cases in production of column names ending in a space. + isInternal && + describe("space at end of column name", () => { + beforeAll(async () => { + table = await createTable({ + "name ": { + name: "name ", + type: FieldType.STRING, + }, + }) + await createRows([{ ["name "]: "foo" }, { ["name "]: "bar" }]) + }) + + it("should be able to query a column that ends with a space", async () => { + await expectSearch({ + query: { + string: { + "name ": "foo", + }, + }, + }).toContainExactly([{ ["name "]: "foo" }]) + }) + + it("should be able to query a column that ends with a space using numeric notation", async () => { + await expectSearch({ + query: { + string: { + "1:name ": "foo", + }, + }, + }).toContainExactly([{ ["name "]: "foo" }]) + }) + }) + + // This was never actually supported in Lucene but SQS does support it, so may + // as well have a test for it. + ;(isSqs || isInMemory) && + describe("space at start of column name", () => { + beforeAll(async () => { + table = await createTable({ + " name": { + name: " name", + type: FieldType.STRING, + }, + }) + await createRows([{ [" name"]: "foo" }, { [" name"]: "bar" }]) + }) + + it("should be able to query a column that starts with a space", async () => { + await expectSearch({ + query: { + string: { + " name": "foo", + }, + }, + }).toContainExactly([{ [" name"]: "foo" }]) + }) + + it("should be able to query a column that starts with a space using numeric notation", async () => { + await expectSearch({ + query: { + string: { + "1: name": "foo", + }, + }, + }).toContainExactly([{ [" name"]: "foo" }]) + }) + }) }) diff --git a/packages/server/src/sdk/app/rows/search/sqs.ts b/packages/server/src/sdk/app/rows/search/sqs.ts index 7042d6fa2c..dada90b1be 100644 --- a/packages/server/src/sdk/app/rows/search/sqs.ts +++ b/packages/server/src/sdk/app/rows/search/sqs.ts @@ -210,7 +210,6 @@ async function runSqlQuery( let bindings = query.bindings // quick hack for docIds - const fixJunctionDocs = (field: string) => ["doc1", "doc2"].forEach(doc => { sql = sql.replaceAll(`\`${doc}\`.\`${field}\``, `\`${doc}.${field}\``) From 5bbdcc129853ae5cbb8a0f8bb23238f2114c461f Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 18 Jul 2024 11:08:42 +0100 Subject: [PATCH 3/5] Remove unused function. --- packages/server/src/api/routes/tests/search.spec.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index cbfa05e343..68f61ba28d 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -184,15 +184,6 @@ describe.each([ return row } - // By default, the global `table` variable is used to search against. If you - // need to, though, you can specify a different table to search against. You - // may wish to do this in a nested describe block that defines a different - // table. - againstTable(table: Table) { - this.query.tableId = table._id! - return this - } - // Asserts that the query returns rows matching exactly the set of rows // passed in. The order of the rows matters. Rows returned in an order // different to the one passed in will cause the assertion to fail. Extra From 481bf9a8b8d9ecf6842b049e904ad6d49601ad7a Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 18 Jul 2024 11:40:44 +0100 Subject: [PATCH 4/5] Fix generic-sql.spec.ts --- packages/backend-core/src/sql/sql.ts | 58 ++++++++++++++++++----- packages/server/src/integrations/mysql.ts | 4 +- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 25c9ead191..65f4f8733e 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -122,17 +122,49 @@ function generateSelectStatement( const schema = meta.table.schema return resource.fields.map(field => { - const [table, column, ...rest] = field.split(/\./g) + const parts = field.split(/\./g) + let table: string | undefined = undefined + let column: string | undefined = undefined + + // Just a column name, e.g.: "column" + if (parts.length === 1) { + column = parts[0] + } + + // A table name and a column name, e.g.: "table.column" + if (parts.length === 2) { + table = parts[0] + column = parts[1] + } + + // A link doc, e.g.: "table.doc1.fieldName" + if (parts.length > 2) { + table = parts[0] + column = parts.slice(1).join(".") + } + + if (!column) { + throw new Error(`Invalid field name: ${field}`) + } + + const columnSchema = schema[column] + if ( client === SqlClient.POSTGRES && - schema[column].externalType?.includes("money") + columnSchema?.externalType?.includes("money") ) { - return knex.raw(`"${table}"."${column}"::money::numeric as "${field}"`) + return knex.raw( + `${quotedIdentifier( + client, + [table, column].join(".") + )}::money::numeric as ${quote(client, field)}` + ) } + if ( client === SqlClient.MS_SQL && - schema[column]?.type === FieldType.DATETIME && - schema[column].timeOnly + columnSchema?.type === FieldType.DATETIME && + columnSchema.timeOnly ) { // Time gets returned as timestamp from mssql, not matching the expected // HH:mm format @@ -147,12 +179,16 @@ function generateSelectStatement( // case, we want to split it into `table`.`doc1.column` for reasons that // aren't actually clear to me, but `table`.`doc1` breaks things with the // sample data tests. - return knex.raw( - `${quote(client, table)}.${quote( - client, - [column, ...rest].join(".") - )} as ${quote(client, field)}` - ) + if (table) { + return knex.raw( + `${quote(client, table)}.${quote(client, column)} as ${quote( + client, + field + )}` + ) + } else { + return knex.raw(`${quote(client, field)} as ${quote(client, field)}`) + } }) } diff --git a/packages/server/src/integrations/mysql.ts b/packages/server/src/integrations/mysql.ts index ecb3c07fa4..f5b575adb8 100644 --- a/packages/server/src/integrations/mysql.ts +++ b/packages/server/src/integrations/mysql.ts @@ -272,9 +272,9 @@ class MySQLIntegration extends Sql implements DatasourcePlus { } catch (err: any) { let readableMessage = getReadableErrorMessage(SourceName.MYSQL, err.errno) if (readableMessage) { - throw new Error(readableMessage) + throw new Error(readableMessage, { cause: err }) } else { - throw new Error(err.message as string) + throw err } } finally { if (opts?.connect && this.client) { From 669692e72286d0d06c0caead02f3b4041a594385 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 18 Jul 2024 11:41:37 +0100 Subject: [PATCH 5/5] Make switch exhaustive. --- packages/backend-core/src/sql/sql.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 65f4f8733e..a4b924bf54 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -44,7 +44,7 @@ const BASE_LIMIT = envLimit || 5000 // Takes a string like foo and returns a quoted string like [foo] for SQL Server // and "foo" for Postgres. -function quote(client: SqlClient, str: string) { +function quote(client: SqlClient, str: string): string { switch (client) { case SqlClient.SQL_LITE: case SqlClient.ORACLE: @@ -52,7 +52,7 @@ function quote(client: SqlClient, str: string) { return `"${str}"` case SqlClient.MS_SQL: return `[${str}]` - default: + case SqlClient.MY_SQL: return `\`${str}\`` } }