diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 4936e4da68..a4b924bf54 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): 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 - default: - throw new Error("Unknown client generating like key") + return `[${str}]` + case SqlClient.MY_SQL: + 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,81 @@ 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 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 ( - knex.client.config.client === SqlClient.MS_SQL && + client === SqlClient.POSTGRES && + columnSchema?.externalType?.includes("money") + ) { + return knex.raw( + `${quotedIdentifier( + client, + [table, column].join(".") + )}::money::numeric as ${quote(client, field)}` + ) + } + + if ( + client === SqlClient.MS_SQL && columnSchema?.type === FieldType.DATETIME && columnSchema.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}` + + // 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. + 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)}`) + } }) } @@ -173,9 +221,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 +298,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 +351,10 @@ class InternalBuilder { } statement += (statement ? andOr : "") + - `COALESCE(LOWER(${likeKey(this.client, key)}), '') LIKE ?` + `COALESCE(LOWER(${quotedIdentifier( + this.client, + key + )}), '') LIKE ?` } if (statement === "") { @@ -336,9 +388,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 +429,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 +448,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 +828,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..68f61ba28d 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -49,7 +49,7 @@ describe.each([ 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) } } @@ -2182,8 +2179,7 @@ describe.each([ }).toContainExactly([{ name: "baz", productCat: undefined }]) }) }) - - isInternal && + ;(isSqs || isLucene) && describe("relations to same table", () => { let relatedTable: Table, relatedRows: Row[] @@ -2371,6 +2367,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 () => { @@ -2455,4 +2452,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/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) { 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}\``)