From 0b2a5162a4698530f0d5fde2e2c29a33ec29d675 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 18 Jul 2024 11:00:10 +0100 Subject: [PATCH] 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}\``)