From 987a24fabc85d76bb3332e364d877a4349083692 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 1 Oct 2024 11:48:14 +0100 Subject: [PATCH 1/7] wip --- packages/backend-core/src/sql/sql.ts | 87 ++++++++++++------- .../src/api/routes/tests/viewV2.spec.ts | 39 +++++++-- 2 files changed, 88 insertions(+), 38 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index b7bf5bc102..54ca5a0135 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -71,18 +71,6 @@ function prioritisedArraySort(toSort: string[], priorities: string[]) { }) } -function getTableName(table?: Table): string | undefined { - // SQS uses the table ID rather than the table name - if ( - table?.sourceType === TableSourceType.INTERNAL || - table?.sourceId === INTERNAL_TABLE_SOURCE_ID - ) { - return table?._id - } else { - return table?.name - } -} - function convertBooleans(query: SqlQuery | SqlQuery[]): SqlQuery | SqlQuery[] { if (Array.isArray(query)) { return query.map((q: SqlQuery) => convertBooleans(q) as SqlQuery) @@ -180,15 +168,13 @@ class InternalBuilder { } private generateSelectStatement(): (string | Knex.Raw)[] | "*" { - const { meta, endpoint, resource, tableAliases } = this.query + const { meta, endpoint, resource } = this.query if (!resource || !resource.fields || resource.fields.length === 0) { return "*" } - const alias = tableAliases?.[endpoint.entityId] - ? tableAliases?.[endpoint.entityId] - : endpoint.entityId + const alias = this.getTableName(endpoint.entityId) const schema = meta.table.schema if (!this.isFullSelectStatementRequired()) { return [this.knex.raw(`${this.quote(alias)}.*`)] @@ -813,17 +799,39 @@ class InternalBuilder { return query } + getTableName(t?: Table | string): string { + let table: Table + if (typeof t === "string") { + if (!this.query.meta.tables?.[t]) { + throw new Error(`Table ${t} not found`) + } + table = this.query.meta.tables[t] + } else if (t) { + table = t + } else { + table = this.table + } + + let name = table.name + if ( + (table.sourceType === TableSourceType.INTERNAL || + table.sourceId === INTERNAL_TABLE_SOURCE_ID) && + table._id + ) { + // SQS uses the table ID rather than the table name + name = table._id + } + const aliases = this.query.tableAliases || {} + return aliases[name] ? aliases[name] : name + } + addDistinctCount(query: Knex.QueryBuilder): Knex.QueryBuilder { - const primary = this.table.primary - const aliases = this.query.tableAliases - const aliased = - this.table.name && aliases?.[this.table.name] - ? aliases[this.table.name] - : this.table.name - if (!primary) { + if (!this.table.primary) { throw new Error("SQL counting requires primary key to be supplied") } - return query.countDistinct(`${aliased}.${primary[0]} as total`) + return query.countDistinct( + `${this.getTableName(this.table)}.${this.table.primary[0]} as total` + ) } addAggregations( @@ -831,8 +839,9 @@ class InternalBuilder { aggregations: Aggregation[] ): Knex.QueryBuilder { const fields = this.query.resource?.fields || [] + const tableName = this.getTableName() if (fields.length > 0) { - query = query.groupBy(fields.map(field => `${this.table.name}.${field}`)) + query = query.groupBy(fields.map(field => `${tableName}.${field}`)) } for (const aggregation of aggregations) { const op = aggregation.calculationType @@ -861,10 +870,7 @@ class InternalBuilder { addSorting(query: Knex.QueryBuilder): Knex.QueryBuilder { let { sort, resource } = this.query const primaryKey = this.table.primary - const tableName = getTableName(this.table) - const aliases = this.query.tableAliases - const aliased = - tableName && aliases?.[tableName] ? aliases[tableName] : this.table?.name + const aliased = this.getTableName() if (!Array.isArray(primaryKey)) { throw new Error("Sorting requires primary key to be specified for table") } @@ -1508,18 +1514,35 @@ class SqlQueryBuilder extends SqlTableQueryBuilder { return results.length ? results : [{ [operation.toLowerCase()]: true }] } + private getTableName( + table: Table, + aliases?: Record + ): string | undefined { + let name = table.name + if ( + table.sourceType === TableSourceType.INTERNAL || + table.sourceId === INTERNAL_TABLE_SOURCE_ID + ) { + if (!table._id) { + return + } + // SQS uses the table ID rather than the table name + name = table._id + } + return aliases?.[name] ? aliases[name] : name + } + convertJsonStringColumns>( table: Table, results: T[], aliases?: Record ): T[] { - const tableName = getTableName(table) + const tableName = this.getTableName(table, aliases) for (const [name, field] of Object.entries(table.schema)) { if (!this._isJsonColumn(field)) { continue } - const aliasedTableName = (tableName && aliases?.[tableName]) || tableName - const fullName = `${aliasedTableName}.${name}` + const fullName = `${tableName}.${name}` for (let row of results) { if (typeof row[fullName as keyof T] === "string") { row[fullName as keyof T] = JSON.parse(row[fullName]) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 09273abdce..0f4e6c961c 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -39,13 +39,13 @@ import { } from "@budibase/backend-core" describe.each([ - ["lucene", undefined], - ["sqs", 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.ORACLE, getDatasource(DatabaseName.ORACLE)], + // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + // [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("/v2/views (%s)", (name, dsProvider) => { const config = setup.getConfig() const isSqs = name === "sqs" @@ -2458,6 +2458,33 @@ describe.each([ expect("_id" in row).toBe(false) } }) + + it.only("should be able to group by a basic field", async () => { + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + quantity: { + visible: true, + field: "quantity", + }, + "Total Price": { + visible: true, + calculationType: CalculationType.SUM, + field: "price", + }, + }, + }) + + const response = await config.api.viewV2.search(view.id, { + query: {}, + }) + + for (const row of response.rows) { + expect(row["quantity"]).toBeGreaterThan(0) + expect(row["Total Price"]).toBeGreaterThan(0) + } + }) }) }) From ae4f7ae4b45ab4e30778eddff0938017924a22d0 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 1 Oct 2024 15:04:01 +0100 Subject: [PATCH 2/7] Implement group by and add a test for it. --- packages/backend-core/src/sql/sql.ts | 19 +++++++++------ .../src/api/routes/tests/viewV2.spec.ts | 23 +++++++++++-------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 54ca5a0135..14e32623e3 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -799,6 +799,14 @@ class InternalBuilder { return query } + isSqs(t?: Table): boolean { + const table = t || this.table + return ( + table.sourceType === TableSourceType.INTERNAL || + table.sourceId === INTERNAL_TABLE_SOURCE_ID + ) + } + getTableName(t?: Table | string): string { let table: Table if (typeof t === "string") { @@ -813,11 +821,7 @@ class InternalBuilder { } let name = table.name - if ( - (table.sourceType === TableSourceType.INTERNAL || - table.sourceId === INTERNAL_TABLE_SOURCE_ID) && - table._id - ) { + if (this.isSqs(table) && table._id) { // SQS uses the table ID rather than the table name name = table._id } @@ -830,7 +834,7 @@ class InternalBuilder { throw new Error("SQL counting requires primary key to be supplied") } return query.countDistinct( - `${this.getTableName(this.table)}.${this.table.primary[0]} as total` + `${this.getTableName()}.${this.table.primary[0]} as total` ) } @@ -842,10 +846,11 @@ class InternalBuilder { const tableName = this.getTableName() if (fields.length > 0) { query = query.groupBy(fields.map(field => `${tableName}.${field}`)) + query = query.select(fields.map(field => `${tableName}.${field}`)) } for (const aggregation of aggregations) { const op = aggregation.calculationType - const field = `${this.table.name}.${aggregation.field} as ${aggregation.name}` + const field = `${tableName}.${aggregation.field} as ${aggregation.name}` switch (op) { case CalculationType.COUNT: query = query.count(field) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 0f4e6c961c..b03c445b78 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -39,13 +39,13 @@ import { } from "@budibase/backend-core" describe.each([ - // ["lucene", undefined], - // ["sqs", 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.ORACLE, getDatasource(DatabaseName.ORACLE)], + [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("/v2/views (%s)", (name, dsProvider) => { const config = setup.getConfig() const isSqs = name === "sqs" @@ -2459,7 +2459,7 @@ describe.each([ } }) - it.only("should be able to group by a basic field", async () => { + it("should be able to group by a basic field", async () => { const view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), @@ -2480,9 +2480,14 @@ describe.each([ query: {}, }) + const priceByQuantity: Record = {} + for (const row of rows) { + priceByQuantity[row.quantity] ??= 0 + priceByQuantity[row.quantity] += row.price + } + for (const row of response.rows) { - expect(row["quantity"]).toBeGreaterThan(0) - expect(row["Total Price"]).toBeGreaterThan(0) + expect(row["Total Price"]).toEqual(priceByQuantity[row.quantity]) } }) }) From addd54a8e8baab79811e6f71aab728f32239577b Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 1 Oct 2024 15:39:33 +0100 Subject: [PATCH 3/7] Fix generic-sql.spec.ts --- packages/backend-core/src/sql/sql.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 14e32623e3..0f72eea96e 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -811,7 +811,10 @@ class InternalBuilder { let table: Table if (typeof t === "string") { if (!this.query.meta.tables?.[t]) { - throw new Error(`Table ${t} not found`) + // This can legitimately happen in custom queries, where the user is + // querying against a table that may not have been imported into + // Budibase. + return t } table = this.query.meta.tables[t] } else if (t) { @@ -1547,12 +1550,12 @@ class SqlQueryBuilder extends SqlTableQueryBuilder { if (!this._isJsonColumn(field)) { continue } - const fullName = `${tableName}.${name}` + const fullName = `${tableName}.${name}` as keyof T for (let row of results) { - if (typeof row[fullName as keyof T] === "string") { - row[fullName as keyof T] = JSON.parse(row[fullName]) + if (typeof row[fullName] === "string") { + row[fullName] = JSON.parse(row[fullName]) } - if (typeof row[name as keyof T] === "string") { + if (typeof row[name] === "string") { row[name as keyof T] = JSON.parse(row[name]) } } From 7cee1509aa3ee47677e3116121beab3b57a6deef Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 1 Oct 2024 16:17:11 +0100 Subject: [PATCH 4/7] Fix sqlAlias.spec.ts --- packages/backend-core/src/sql/sql.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 0f72eea96e..105116e828 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -810,13 +810,18 @@ class InternalBuilder { getTableName(t?: Table | string): string { let table: Table if (typeof t === "string") { - if (!this.query.meta.tables?.[t]) { + if (this.query.table?.name === t) { + table = this.query.table + } else if (this.query.meta.table?.name === t) { + table = this.query.meta.table + } else if (!this.query.meta.tables?.[t]) { // This can legitimately happen in custom queries, where the user is // querying against a table that may not have been imported into // Budibase. return t + } else { + table = this.query.meta.tables[t] } - table = this.query.meta.tables[t] } else if (t) { table = t } else { From 08f1c4dadc668db7fe645f01336832341546828e Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 2 Oct 2024 09:35:15 +0100 Subject: [PATCH 5/7] Update packages/backend-core/src/sql/sql.ts Co-authored-by: Adria Navarro --- packages/backend-core/src/sql/sql.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 105116e828..e55524a67e 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -1542,7 +1542,7 @@ class SqlQueryBuilder extends SqlTableQueryBuilder { // SQS uses the table ID rather than the table name name = table._id } - return aliases?.[name] ? aliases[name] : name + return aliases?.[name] || name } convertJsonStringColumns>( From ddd229062c20e37f2860c8a07583688f67c0ed1c Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 2 Oct 2024 09:39:54 +0100 Subject: [PATCH 6/7] Rename total field when doing row counts. --- packages/backend-core/src/sql/sql.ts | 2 +- packages/server/src/sdk/app/rows/utils.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index e55524a67e..701797329f 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -842,7 +842,7 @@ class InternalBuilder { throw new Error("SQL counting requires primary key to be supplied") } return query.countDistinct( - `${this.getTableName()}.${this.table.primary[0]} as total` + `${this.getTableName()}.${this.table.primary[0]} as __bb_total` ) } diff --git a/packages/server/src/sdk/app/rows/utils.ts b/packages/server/src/sdk/app/rows/utils.ts index d5c0560d9b..3d6bf39d3f 100644 --- a/packages/server/src/sdk/app/rows/utils.ts +++ b/packages/server/src/sdk/app/rows/utils.ts @@ -57,8 +57,8 @@ export function getSQLClient(datasource: Datasource): SqlClient { export function processRowCountResponse( response: DatasourcePlusQueryResponse ): number { - if (response && response.length === 1 && "total" in response[0]) { - const total = response[0].total + if (response && response.length === 1 && "__bb_total" in response[0]) { + const total = response[0].__bb_total return typeof total === "number" ? total : parseInt(total) } else { throw new Error("Unable to count rows in query - no count response") From 7b9af81fd510f2aa87c757173abd028c068cebba Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 2 Oct 2024 09:44:20 +0100 Subject: [PATCH 7/7] Clean up params and isSqs --- packages/backend-core/src/sql/sql.ts | 36 +++++++++++++++------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 701797329f..627be039ca 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -87,6 +87,13 @@ function convertBooleans(query: SqlQuery | SqlQuery[]): SqlQuery | SqlQuery[] { return query } +function isSqs(table: Table): boolean { + return ( + table.sourceType === TableSourceType.INTERNAL || + table.sourceId === INTERNAL_TABLE_SOURCE_ID + ) +} + class InternalBuilder { private readonly client: SqlClient private readonly query: QueryJson @@ -799,37 +806,34 @@ class InternalBuilder { return query } - isSqs(t?: Table): boolean { - const table = t || this.table - return ( - table.sourceType === TableSourceType.INTERNAL || - table.sourceId === INTERNAL_TABLE_SOURCE_ID - ) + isSqs(): boolean { + return isSqs(this.table) } - getTableName(t?: Table | string): string { + getTableName(tableOrName?: Table | string): string { let table: Table - if (typeof t === "string") { - if (this.query.table?.name === t) { + if (typeof tableOrName === "string") { + const name = tableOrName + if (this.query.table?.name === name) { table = this.query.table - } else if (this.query.meta.table?.name === t) { + } else if (this.query.meta.table?.name === name) { table = this.query.meta.table - } else if (!this.query.meta.tables?.[t]) { + } else if (!this.query.meta.tables?.[name]) { // This can legitimately happen in custom queries, where the user is // querying against a table that may not have been imported into // Budibase. - return t + return name } else { - table = this.query.meta.tables[t] + table = this.query.meta.tables[name] } - } else if (t) { - table = t + } else if (tableOrName) { + table = tableOrName } else { table = this.table } let name = table.name - if (this.isSqs(table) && table._id) { + if (isSqs(table) && table._id) { // SQS uses the table ID rather than the table name name = table._id }