Fix the problem, and the tests.

This commit is contained in:
Sam Rose 2024-07-18 11:00:10 +01:00
parent aea9cda8f5
commit 0b2a5162a4
No known key found for this signature in database
3 changed files with 107 additions and 45 deletions

View File

@ -122,7 +122,7 @@ function generateSelectStatement(
const schema = meta.table.schema const schema = meta.table.schema
return resource.fields.map(field => { return resource.fields.map(field => {
const [table, column, ..._rest] = field.split(/\./g) const [table, column, ...rest] = field.split(/\./g)
if ( if (
client === SqlClient.POSTGRES && client === SqlClient.POSTGRES &&
schema[column].externalType?.includes("money") schema[column].externalType?.includes("money")
@ -138,13 +138,21 @@ function generateSelectStatement(
// HH:mm format // HH:mm format
return knex.raw(`CONVERT(varchar, ${field}, 108) as "${field}"`) return knex.raw(`CONVERT(varchar, ${field}, 108) as "${field}"`)
} }
return `${field} as ${field}`
// return knex.raw( // There's at least two edge cases being handled in the expression below.
// `${quote(client, table)}.${quote(client, column)} as ${quote( // 1. The column name could start/end with a space, and in that case we
// client, // want to preseve that space.
// field // 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)}`
)
}) })
} }

View File

@ -38,18 +38,18 @@ import { structures } from "@budibase/backend-core/tests"
import { DEFAULT_EMPLOYEE_TABLE_SCHEMA } from "../../../db/defaultData/datasource_bb_default" import { DEFAULT_EMPLOYEE_TABLE_SCHEMA } from "../../../db/defaultData/datasource_bb_default"
describe.each([ describe.each([
// ["in-memory", undefined], ["in-memory", undefined],
// ["lucene", undefined], ["lucene", undefined],
["sqs", undefined], ["sqs", undefined],
// [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)],
// [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)],
// [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)],
// [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)],
])("search (%s)", (name, dsProvider) => { ])("search (%s)", (name, dsProvider) => {
const isSqs = name === "sqs" const isSqs = name === "sqs"
const isLucene = name === "lucene" const isLucene = name === "lucene"
const isInMemory = name === "in-memory" const isInMemory = name === "in-memory"
const isInternal = isSqs || isLucene const isInternal = isSqs || isLucene || isInMemory
const config = setup.getConfig() const config = setup.getConfig()
let envCleanup: (() => void) | undefined let envCleanup: (() => void) | undefined
@ -115,10 +115,7 @@ describe.each([
if (isInMemory) { if (isInMemory) {
return dataFilters.search(_.cloneDeep(rows), this.query) return dataFilters.search(_.cloneDeep(rows), this.query)
} else { } else {
return config.api.row.search(table._id!, { return config.api.row.search(this.query.tableId, this.query)
...this.query,
tableId: table._id!,
})
} }
} }
@ -187,6 +184,15 @@ describe.each([
return row 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 // 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 // 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 // different to the one passed in will cause the assertion to fail. Extra
@ -735,29 +741,6 @@ describe.each([
query: {}, query: {},
}).toHaveLength(1) }).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", () => { describe("equal", () => {
@ -2205,8 +2188,7 @@ describe.each([
}).toContainExactly([{ name: "baz", productCat: undefined }]) }).toContainExactly([{ name: "baz", productCat: undefined }])
}) })
}) })
;(isSqs || isLucene) &&
isInternal &&
describe("relations to same table", () => { describe("relations to same table", () => {
let relatedTable: Table, relatedRows: Row[] let relatedTable: Table, relatedRows: Row[]
@ -2394,6 +2376,7 @@ describe.each([
beforeAll(async () => { beforeAll(async () => {
await config.api.application.addSampleData(config.appId!) await config.api.application.addSampleData(config.appId!)
table = DEFAULT_EMPLOYEE_TABLE_SCHEMA table = DEFAULT_EMPLOYEE_TABLE_SCHEMA
rows = await config.api.row.fetch(table._id!)
}) })
it("should be able to search sample data", async () => { it("should be able to search sample data", async () => {
@ -2478,4 +2461,76 @@ describe.each([
}).toContainExactly([{ [name]: "a" }]) }).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" }])
})
})
}) })

View File

@ -210,7 +210,6 @@ async function runSqlQuery(
let bindings = query.bindings let bindings = query.bindings
// quick hack for docIds // quick hack for docIds
const fixJunctionDocs = (field: string) => const fixJunctionDocs = (field: string) =>
["doc1", "doc2"].forEach(doc => { ["doc1", "doc2"].forEach(doc => {
sql = sql.replaceAll(`\`${doc}\`.\`${field}\``, `\`${doc}.${field}\``) sql = sql.replaceAll(`\`${doc}\`.\`${field}\``, `\`${doc}.${field}\``)