From 1777f1f8fe45b52715edea5ac38d7d1e32df4615 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 3 May 2024 17:29:20 +0100 Subject: [PATCH 1/7] Fix boolean searching for Lucene/SQS --- packages/backend-core/src/db/lucene.ts | 4 +- .../src/api/controllers/row/utils/utils.ts | 21 +++- .../src/api/routes/tests/search.spec.ts | 100 +++++++++++++++--- .../src/sdk/app/rows/search/internalSearch.ts | 0 .../server/src/sdk/app/rows/search/sqs.ts | 9 +- 5 files changed, 115 insertions(+), 19 deletions(-) delete mode 100644 packages/server/src/sdk/app/rows/search/internalSearch.ts diff --git a/packages/backend-core/src/db/lucene.ts b/packages/backend-core/src/db/lucene.ts index d9dddd0097..f60ef5a978 100644 --- a/packages/backend-core/src/db/lucene.ts +++ b/packages/backend-core/src/db/lucene.ts @@ -283,7 +283,7 @@ export class QueryBuilder { const equal = (key: string, value: any) => { // 0 evaluates to false, which means we would return all rows if we don't check it - if (!value && value !== 0) { + if (value === null || value === undefined) { return null } return `${key}:${builder.preprocess(value, allPreProcessingOpts)}` @@ -421,7 +421,7 @@ export class QueryBuilder { } if (this.#query.notEqual) { build(this.#query.notEqual, (key: string, value: any) => { - if (!value) { + if (value === null || value === undefined) { return null } if (typeof value === "boolean") { diff --git a/packages/server/src/api/controllers/row/utils/utils.ts b/packages/server/src/api/controllers/row/utils/utils.ts index bf9ede6fe3..d6113d8b7e 100644 --- a/packages/server/src/api/controllers/row/utils/utils.ts +++ b/packages/server/src/api/controllers/row/utils/utils.ts @@ -117,6 +117,19 @@ export async function validate( }) } +function fixBooleanFields({ row, table }: { row: Row; table: Table }) { + for (let col of Object.values(table.schema)) { + if (col.type === FieldType.BOOLEAN) { + if (row[col.name] === 1) { + row[col.name] = true + } else if (row[col.name] === 0) { + row[col.name] = false + } + } + } + return row +} + export async function sqlOutputProcessing( rows: DatasourcePlusQueryResponse, table: Table, @@ -161,7 +174,13 @@ export async function sqlOutputProcessing( if (thisRow._id == null) { throw new Error("Unable to generate row ID for SQL rows") } - finalRows[thisRow._id] = thisRow + + if (opts?.sqs) { + finalRows[thisRow._id] = fixBooleanFields({ row: thisRow, table }) + } else { + finalRows[thisRow._id] = thisRow + } + // do this at end once its been added to the final rows finalRows = await updateRelationshipColumns( table, diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 6ab7ac61d4..b38f187404 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -15,16 +15,17 @@ import { TableSchema, } from "@budibase/types" import _ from "lodash" +import exp from "constants" jest.unmock("mssql") describe.each([ - ["lucene", 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)], ])("/api/:sourceId/search (%s)", (name, dsProvider) => { const isSqs = name === "sqs" const isLucene = name === "lucene" @@ -67,6 +68,22 @@ describe.each([ class SearchAssertion { constructor(private readonly query: RowSearchParams) {} + private findRow(expectedRow: any, foundRows: any[]) { + const row = foundRows.find(foundRow => _.isMatch(foundRow, expectedRow)) + if (!row) { + const fields = Object.keys(expectedRow) + // To make the error message more readable, we only include the fields + // that are present in the expected row. + const searchedObjects = foundRows.map(row => _.pick(row, fields)) + throw new Error( + `Failed to find row: ${JSON.stringify( + expectedRow + )} in ${JSON.stringify(searchedObjects)}` + ) + } + return row + } + // 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 @@ -82,9 +99,7 @@ describe.each([ // eslint-disable-next-line jest/no-standalone-expect expect(foundRows).toEqual( expectedRows.map((expectedRow: any) => - expect.objectContaining( - foundRows.find(foundRow => _.isMatch(foundRow, expectedRow)) - ) + expect.objectContaining(this.findRow(expectedRow, foundRows)) ) ) } @@ -104,9 +119,7 @@ describe.each([ expect(foundRows).toEqual( expect.arrayContaining( expectedRows.map((expectedRow: any) => - expect.objectContaining( - foundRows.find(foundRow => _.isMatch(foundRow, expectedRow)) - ) + expect.objectContaining(this.findRow(expectedRow, foundRows)) ) ) ) @@ -125,9 +138,7 @@ describe.each([ expect(foundRows).toEqual( expect.arrayContaining( expectedRows.map((expectedRow: any) => - expect.objectContaining( - foundRows.find(foundRow => _.isMatch(foundRow, expectedRow)) - ) + expect.objectContaining(this.findRow(expectedRow, foundRows)) ) ) ) @@ -156,6 +167,67 @@ describe.each([ return expectSearch({ query }) } + describe.only("boolean", () => { + beforeAll(async () => { + await createTable({ + isTrue: { name: "isTrue", type: FieldType.BOOLEAN }, + }) + await createRows([{ isTrue: true }, { isTrue: false }]) + }) + + describe("equal", () => { + it("successfully finds true row", () => + expectQuery({ equal: { isTrue: true } }).toMatchExactly([ + { isTrue: true }, + ])) + + it("successfully finds false row", () => + expectQuery({ equal: { isTrue: false } }).toMatchExactly([ + { isTrue: false }, + ])) + }) + + describe("notEqual", () => { + it("successfully finds false row", () => + expectQuery({ notEqual: { isTrue: true } }).toContainExactly([ + { isTrue: false }, + ])) + + it("successfully finds true row", () => + expectQuery({ notEqual: { isTrue: false } }).toContainExactly([ + { isTrue: true }, + ])) + }) + + describe("oneOf", () => { + it("successfully finds true row", () => + expectQuery({ oneOf: { isTrue: [true] } }).toContainExactly([ + { isTrue: true }, + ])) + + it("successfully finds false row", () => + expectQuery({ oneOf: { isTrue: [false] } }).toContainExactly([ + { isTrue: false }, + ])) + }) + + describe("sort", () => { + it("sorts ascending", () => + expectSearch({ + query: {}, + sort: "isTrue", + sortOrder: SortOrder.ASCENDING, + }).toMatchExactly([{ isTrue: false }, { isTrue: true }])) + + it("sorts descending", () => + expectSearch({ + query: {}, + sort: "isTrue", + sortOrder: SortOrder.DESCENDING, + }).toMatchExactly([{ isTrue: true }, { isTrue: false }])) + }) + }) + describe.each([FieldType.STRING, FieldType.LONGFORM])("%s", () => { beforeAll(async () => { await createTable({ diff --git a/packages/server/src/sdk/app/rows/search/internalSearch.ts b/packages/server/src/sdk/app/rows/search/internalSearch.ts deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/packages/server/src/sdk/app/rows/search/sqs.ts b/packages/server/src/sdk/app/rows/search/sqs.ts index dabccc4b55..f6b79ce968 100644 --- a/packages/server/src/sdk/app/rows/search/sqs.ts +++ b/packages/server/src/sdk/app/rows/search/sqs.ts @@ -164,8 +164,13 @@ export async function search( throw new Error("SQS cannot currently handle multiple queries") } - let sql = query.sql, - bindings = query.bindings + let sql = query.sql + let bindings = query.bindings?.map(b => { + if (typeof b === "boolean") { + return b ? 1 : 0 + } + return b + }) // quick hack for docIds sql = sql.replace(/`doc1`.`rowId`/g, "`doc1.rowId`") From 1edc525d9b548da6fd58e8a17b45b29ce4c62104 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 3 May 2024 17:31:44 +0100 Subject: [PATCH 2/7] Fix boolean searching for external datasources. --- .../server/src/api/controllers/row/utils/utils.ts | 6 +----- packages/server/src/api/routes/tests/search.spec.ts | 13 ++++++------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/packages/server/src/api/controllers/row/utils/utils.ts b/packages/server/src/api/controllers/row/utils/utils.ts index d6113d8b7e..afee5a9475 100644 --- a/packages/server/src/api/controllers/row/utils/utils.ts +++ b/packages/server/src/api/controllers/row/utils/utils.ts @@ -175,11 +175,7 @@ export async function sqlOutputProcessing( throw new Error("Unable to generate row ID for SQL rows") } - if (opts?.sqs) { - finalRows[thisRow._id] = fixBooleanFields({ row: thisRow, table }) - } else { - finalRows[thisRow._id] = thisRow - } + finalRows[thisRow._id] = fixBooleanFields({ row: thisRow, table }) // do this at end once its been added to the final rows finalRows = await updateRelationshipColumns( diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index b38f187404..6b926c9a4e 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -15,17 +15,16 @@ import { TableSchema, } from "@budibase/types" import _ from "lodash" -import exp from "constants" jest.unmock("mssql") describe.each([ - // ["lucene", 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)], ])("/api/:sourceId/search (%s)", (name, dsProvider) => { const isSqs = name === "sqs" const isLucene = name === "lucene" @@ -167,7 +166,7 @@ describe.each([ return expectSearch({ query }) } - describe.only("boolean", () => { + describe("boolean", () => { beforeAll(async () => { await createTable({ isTrue: { name: "isTrue", type: FieldType.BOOLEAN }, From 27e68f79f52dc280ce1f2a5c4a6f8d91fe763237 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 3 May 2024 17:34:03 +0100 Subject: [PATCH 3/7] Remove extraneous comment. --- packages/backend-core/src/db/lucene.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/backend-core/src/db/lucene.ts b/packages/backend-core/src/db/lucene.ts index f60ef5a978..4dbb7d56e2 100644 --- a/packages/backend-core/src/db/lucene.ts +++ b/packages/backend-core/src/db/lucene.ts @@ -282,7 +282,6 @@ export class QueryBuilder { } const equal = (key: string, value: any) => { - // 0 evaluates to false, which means we would return all rows if we don't check it if (value === null || value === undefined) { return null } From 32bd505ef9cf354d5a9f1c4ddacbe9de5d67711d Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 7 May 2024 10:38:47 +0100 Subject: [PATCH 4/7] Respond to PR feedback. --- packages/backend-core/src/db/lucene.ts | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/packages/backend-core/src/db/lucene.ts b/packages/backend-core/src/db/lucene.ts index 4dbb7d56e2..14d07636ca 100644 --- a/packages/backend-core/src/db/lucene.ts +++ b/packages/backend-core/src/db/lucene.ts @@ -12,6 +12,14 @@ import { dataFilters } from "@budibase/shared-core" export const removeKeyNumbering = dataFilters.removeKeyNumbering +function isEmpty(value: any) { + return ( + value == null || + value === "" || + (Array.isArray(value) && value.length === 0) + ) +} + /** * Class to build lucene query URLs. * Optionally takes a base lucene query object. @@ -282,14 +290,14 @@ export class QueryBuilder { } const equal = (key: string, value: any) => { - if (value === null || value === undefined) { + if (isEmpty(value)) { return null } return `${key}:${builder.preprocess(value, allPreProcessingOpts)}` } const contains = (key: string, value: any, mode = "AND") => { - if (!value || (Array.isArray(value) && value.length === 0)) { + if (isEmpty(value)) { return null } if (!Array.isArray(value)) { @@ -305,7 +313,7 @@ export class QueryBuilder { } const fuzzy = (key: string, value: any) => { - if (!value) { + if (isEmpty(value)) { return null } value = builder.preprocess(value, { @@ -327,7 +335,7 @@ export class QueryBuilder { } const oneOf = (key: string, value: any) => { - if (!value) { + if (isEmpty(value)) { return `*:*` } if (!Array.isArray(value)) { @@ -385,7 +393,7 @@ export class QueryBuilder { // Construct the actual lucene search query string from JSON structure if (this.#query.string) { build(this.#query.string, (key: string, value: any) => { - if (!value) { + if (isEmpty(value)) { return null } value = builder.preprocess(value, { @@ -398,7 +406,7 @@ export class QueryBuilder { } if (this.#query.range) { build(this.#query.range, (key: string, value: any) => { - if (!value) { + if (isEmpty(value)) { return null } if (value.low == null || value.low === "") { @@ -420,7 +428,7 @@ export class QueryBuilder { } if (this.#query.notEqual) { build(this.#query.notEqual, (key: string, value: any) => { - if (value === null || value === undefined) { + if (isEmpty(value)) { return null } if (typeof value === "boolean") { From 4722fd1cab96574ad88b9f579433aa1fc0504476 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 7 May 2024 17:16:47 +0100 Subject: [PATCH 5/7] Fix queryRows.spec.ts --- .../src/automations/tests/queryRows.spec.ts | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/packages/server/src/automations/tests/queryRows.spec.ts b/packages/server/src/automations/tests/queryRows.spec.ts index bee90df08e..6b9113a309 100644 --- a/packages/server/src/automations/tests/queryRows.spec.ts +++ b/packages/server/src/automations/tests/queryRows.spec.ts @@ -1,14 +1,4 @@ -// lucene searching not supported in test due to use of PouchDB -let rows: Row[] = [] -jest.mock("../../sdk/app/rows/search/internalSearch", () => ({ - fullSearch: jest.fn(() => { - return { - rows, - } - }), - paginatedSearch: jest.fn(), -})) -import { Row, Table } from "@budibase/types" +import { Table } from "@budibase/types" import * as setup from "./utilities" const NAME = "Test" @@ -25,8 +15,8 @@ describe("Test a query step automation", () => { description: "original description", tableId: table._id, } - rows.push(await config.createRow(row)) - rows.push(await config.createRow(row)) + await config.createRow(row) + await config.createRow(row) }) afterAll(setup.afterAll) From 39f8727830e67228ce483c65129e1a580602cfb8 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 7 May 2024 17:41:43 +0100 Subject: [PATCH 6/7] Move boolean coversion down a layer in the stack so it's not tied so directly to search. --- packages/server/src/integrations/base/sql.ts | 22 ++++++++++++++++++- .../server/src/sdk/app/rows/search/sqs.ts | 7 +----- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/packages/server/src/integrations/base/sql.ts b/packages/server/src/integrations/base/sql.ts index ae3e8e8aea..8ea5fee0c9 100644 --- a/packages/server/src/integrations/base/sql.ts +++ b/packages/server/src/integrations/base/sql.ts @@ -150,6 +150,22 @@ function getTableName(table?: Table): string | undefined { } } +function convertBooleans(query: SqlQuery | SqlQuery[]): SqlQuery | SqlQuery[] { + if (Array.isArray(query)) { + return query.map((q: SqlQuery) => convertBooleans(q) as SqlQuery) + } else { + if (query.bindings) { + query.bindings = query.bindings.map(binding => { + if (typeof binding === "boolean") { + return binding ? 1 : 0 + } + return binding + }) + } + } + return query +} + class InternalBuilder { private readonly client: string @@ -654,7 +670,11 @@ class SqlQueryBuilder extends SqlTableQueryBuilder { if (opts?.disableBindings) { return { sql: query.toString() } } else { - return getNativeSql(query) + let native = getNativeSql(query) + if (sqlClient === SqlClient.SQL_LITE) { + native = convertBooleans(native) + } + return native } } diff --git a/packages/server/src/sdk/app/rows/search/sqs.ts b/packages/server/src/sdk/app/rows/search/sqs.ts index f6b79ce968..05b1a3bd96 100644 --- a/packages/server/src/sdk/app/rows/search/sqs.ts +++ b/packages/server/src/sdk/app/rows/search/sqs.ts @@ -165,12 +165,7 @@ export async function search( } let sql = query.sql - let bindings = query.bindings?.map(b => { - if (typeof b === "boolean") { - return b ? 1 : 0 - } - return b - }) + let bindings = query.bindings // quick hack for docIds sql = sql.replace(/`doc1`.`rowId`/g, "`doc1.rowId`") From 5783ee790fecbd722e5bfc18a1b59ba8c6e220f8 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 8 May 2024 15:36:26 +0100 Subject: [PATCH 7/7] Fix Lucene tests. --- packages/backend-core/src/db/lucene.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/backend-core/src/db/lucene.ts b/packages/backend-core/src/db/lucene.ts index 76ba257d59..f5ad7e6433 100644 --- a/packages/backend-core/src/db/lucene.ts +++ b/packages/backend-core/src/db/lucene.ts @@ -13,11 +13,7 @@ import { dataFilters } from "@budibase/shared-core" export const removeKeyNumbering = dataFilters.removeKeyNumbering function isEmpty(value: any) { - return ( - value == null || - value === "" || - (Array.isArray(value) && value.length === 0) - ) + return value == null || value === "" } /**