Merge pull request #14193 from Budibase/support-spaces-as-last-char-in-column-sqs

Support space as last char in column name in SQS

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 {
+// Takes a string like foo and returns a quoted string like [foo] for SQL Server
-  let start: string, end: string
+// 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 = '"'
+      return `"${str}"`
-      break
     case SqlClient.MS_SQL:
-      start = "["
+      return `[${str}]`
-      end = "]"
+    case SqlClient.MY_SQL:
-      break
+      return `\`${str}\``
-    default:
-      throw new Error("Unknown client generating like key")
   }
-  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 parts = field.split(/\./g)
-    const tableName = fieldNames[0]
+    let table: string | undefined = undefined
-    const columnName = fieldNames[1]
+    let column: string | undefined = undefined
-    const columnSchema = schema?.[columnName]
+
-    if (columnSchema && knex.client.config.client === SqlClient.POSTGRES) {
+    // Just a column name, e.g.: "column"
-      const externalType = schema[columnName].externalType
+    if (parts.length === 1) {
-      if (externalType?.includes("money")) {
+      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 &&
+      columnSchema?.externalType?.includes("money")
+    ) {
       return knex.raw(
-          `"${tableName}"."${columnName}"::money::numeric as "${field}"`
+        `${quotedIdentifier(
+          client,
+          [table, column].join(".")
+        )}::money::numeric as ${quote(client, field)}`
       )
     }
-    }
+
     if (
-      knex.client.config.client === SqlClient.MS_SQL &&
+      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 ?`, [
+        query = query[rawFnc](
-          `%${value.toLowerCase()}%`,
+          `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 ?`, [
+          query = query[rawFnc](
-            `${value.toLowerCase()}%`,
+            `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
   }

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
   }
 

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!, {
+        return config.api.row.search(this.query.tableId, this.query)
-          ...this.query,
-          tableId: table._id!,
-        })
       }
     }
 
@@ -2182,8 +2179,7 @@ describe.each([
         }).toContainExactly([{ name: "baz", productCat: undefined }])
       })
     })
-
+  ;(isSqs || isLucene) &&
-  isInternal &&
     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" }])
+      })
+    })
 })

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) {

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}\``)