From 9d6fc54a992e2ee0fe389a519d6d158d88f5995d Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 16 Sep 2024 16:12:07 +0100 Subject: [PATCH 1/7] Adding function parameter limit control for different SQL DBs, every DB has different limits with Postgres being the lowest at 100. We need to fix for wide tables which are related. --- packages/backend-core/src/sql/sql.ts | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 2b5243f856..b0e7b30067 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -40,7 +40,6 @@ import { dataFilters, helpers } from "@budibase/shared-core" import { cloneDeep } from "lodash" type QueryFunction = (query: SqlQuery | SqlQuery[], operation: Operation) => any -const MAX_SQS_RELATIONSHIP_FIELDS = 63 function getBaseLimit() { const envLimit = environment.SQL_MAX_ROWS @@ -877,6 +876,22 @@ class InternalBuilder { return `'${unaliased}'${separator}${tableField}` } + maxFunctionParameters() { + // functions like say json_build_object() in SQL have a limit as to how many can be performed + // before a limit is met, this limit exists in Postgres/SQLite. This can be very important, such as + // for JSON column building as part of relationships. We also have a default limit to avoid very complex + // functions being built - it is likely this is not necessary or the best way to do it. + switch (this.client) { + case SqlClient.SQL_LITE: + return 127 + case SqlClient.POSTGRES: + return 100 + // other DBs don't have a limit, but set some sort of limit + default: + return 200 + } + } + addJsonRelationships( query: Knex.QueryBuilder, fromTable: string, @@ -908,12 +923,10 @@ class InternalBuilder { let relationshipFields = fields.filter( field => field.split(".")[0] === toAlias ) - if (this.client === SqlClient.SQL_LITE) { - relationshipFields = relationshipFields.slice( - 0, - MAX_SQS_RELATIONSHIP_FIELDS - ) - } + relationshipFields = relationshipFields.slice( + 0, + Math.floor(this.maxFunctionParameters() / 2) + ) const fieldList: string = relationshipFields .map(field => this.buildJsonField(field)) .join(",") From 26ad987072e13b4956dca68ee44c681aa8e2aee3 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 16 Sep 2024 16:15:09 +0100 Subject: [PATCH 2/7] Fix Google Sheets pagination. --- .../server/src/integrations/googlesheets.ts | 15 +++++--- .../integrations/tests/googlesheets.spec.ts | 37 +++++++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/packages/server/src/integrations/googlesheets.ts b/packages/server/src/integrations/googlesheets.ts index 0d766ac1ef..dd9bef84ab 100644 --- a/packages/server/src/integrations/googlesheets.ts +++ b/packages/server/src/integrations/googlesheets.ts @@ -551,11 +551,16 @@ export class GoogleSheetsIntegration implements DatasourcePlus { await this.connect() const hasFilters = dataFilters.hasFilters(query.filters) const limit = query.paginate?.limit || 100 - const page: number = - typeof query.paginate?.page === "number" - ? query.paginate.page - : parseInt(query.paginate?.page || "1") - const offset = (page - 1) * limit + let offset = query.paginate?.offset || 0 + + let page = query.paginate?.page + if (typeof page === "string") { + page = parseInt(page) + } + if (page !== undefined) { + offset = page * limit + } + const sheet = this.client.sheetsByTitle[query.sheet] let rows: GoogleSpreadsheetRow[] = [] if (query.paginate && !hasFilters) { diff --git a/packages/server/src/integrations/tests/googlesheets.spec.ts b/packages/server/src/integrations/tests/googlesheets.spec.ts index 079e418f3b..a1fabc2c04 100644 --- a/packages/server/src/integrations/tests/googlesheets.spec.ts +++ b/packages/server/src/integrations/tests/googlesheets.spec.ts @@ -5,6 +5,7 @@ import TestConfiguration from "../../tests/utilities/TestConfiguration" import { Datasource, FieldType, + Row, SourceName, Table, TableSourceType, @@ -208,6 +209,42 @@ describe("Google Sheets Integration", () => { expect(row2.name).toEqual("Test Contact 2") expect(row2.description).toEqual("original description 2") }) + + it("can paginate correctly", async () => { + await config.api.row.bulkImport(table._id!, { + rows: Array.from({ length: 248 }, (_, i) => ({ + name: `${i}`, + description: "", + })), + }) + + let resp = await config.api.row.search(table._id!, { + tableId: table._id!, + query: {}, + paginate: true, + limit: 10, + }) + let rows = resp.rows + + while (resp.hasNextPage) { + resp = await config.api.row.search(table._id!, { + tableId: table._id!, + query: {}, + paginate: true, + limit: 10, + bookmark: resp.bookmark, + }) + rows = rows.concat(resp.rows) + if (rows.length > 250) { + throw new Error("Too many rows returned") + } + } + + expect(rows.length).toEqual(250) + expect(rows.map(row => row.name)).toEqual( + expect.arrayContaining(Array.from({ length: 248 }, (_, i) => `${i}`)) + ) + }) }) describe("update", () => { From 9f21dc88b016ff631f132f1239381ee8c88d0a62 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 16 Sep 2024 16:18:57 +0100 Subject: [PATCH 3/7] Fix lint. --- packages/server/src/integrations/tests/googlesheets.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/server/src/integrations/tests/googlesheets.spec.ts b/packages/server/src/integrations/tests/googlesheets.spec.ts index a1fabc2c04..62d56bb2c2 100644 --- a/packages/server/src/integrations/tests/googlesheets.spec.ts +++ b/packages/server/src/integrations/tests/googlesheets.spec.ts @@ -5,7 +5,6 @@ import TestConfiguration from "../../tests/utilities/TestConfiguration" import { Datasource, FieldType, - Row, SourceName, Table, TableSourceType, From 68a710699d6ab8fa978d9ee46c0020814b966845 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 16 Sep 2024 18:09:01 +0100 Subject: [PATCH 4/7] Getting external DBs to correctly handle when too many fields. --- packages/backend-core/src/sql/sql.ts | 15 +++++-- .../api/controllers/row/ExternalRequest.ts | 13 +++--- .../src/api/routes/tests/search.spec.ts | 42 +++++++++++++++++++ 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index b0e7b30067..ff32026814 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -899,7 +899,7 @@ class InternalBuilder { ): Knex.QueryBuilder { const sqlClient = this.client const knex = this.knex - const { resource, tableAliases: aliases, endpoint } = this.query + const { resource, tableAliases: aliases, endpoint, meta } = this.query const fields = resource?.fields || [] for (let relationship of relationships) { const { @@ -914,15 +914,22 @@ class InternalBuilder { if (!toTable || !fromTable) { continue } + const relatedTable = meta.tables?.[toTable] const toAlias = aliases?.[toTable] || toTable, fromAlias = aliases?.[fromTable] || fromTable let toTableWithSchema = this.tableNameWithSchema(toTable, { alias: toAlias, schema: endpoint.schema, }) - let relationshipFields = fields.filter( - field => field.split(".")[0] === toAlias - ) + const requiredFields = [ + ...(relatedTable?.primary || []), + relatedTable?.primaryDisplay, + ] + let relationshipFields = fields + .filter(field => field.split(".")[0] === toAlias) + // sort the required fields to first in the list + .sort(field => (requiredFields.includes(field) ? 1 : -1)) + relationshipFields = relationshipFields.slice( 0, Math.floor(this.maxFunctionParameters() / 2) diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index ac2d1e8c39..9c9bd0b284 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -12,6 +12,7 @@ import { OneToManyRelationshipFieldMetadata, Operation, PaginationJson, + QueryJson, RelationshipFieldMetadata, Row, SearchFilters, @@ -161,7 +162,6 @@ export class ExternalRequest { private readonly tableId: string private datasource?: Datasource private tables: { [key: string]: Table } = {} - private tableList: Table[] constructor(operation: T, tableId: string, datasource?: Datasource) { this.operation = operation @@ -170,7 +170,6 @@ export class ExternalRequest { if (datasource && datasource.entities) { this.tables = datasource.entities } - this.tableList = Object.values(this.tables) } private prepareFilters( @@ -301,7 +300,6 @@ export class ExternalRequest { throw "No tables found, fetch tables before query." } this.tables = this.datasource.entities - this.tableList = Object.values(this.tables) } return { tables: this.tables, datasource: this.datasource } } @@ -463,7 +461,7 @@ export class ExternalRequest { breakExternalTableId(relatedTableId) // @ts-ignore const linkPrimaryKey = this.tables[relatedTableName].primary[0] - if (!lookupField || !row[lookupField]) { + if (!lookupField || !row?.[lookupField] == null) { continue } const endpoint = getEndpoint(relatedTableId, Operation.READ) @@ -631,7 +629,8 @@ export class ExternalRequest { const { datasource: ds } = await this.retrieveMetadata(datasourceId) datasource = ds } - const table = this.tables[tableName] + const tables = this.tables + const table = tables[tableName] let isSql = isSQL(datasource) if (!table) { throw new Error( @@ -686,7 +685,7 @@ export class ExternalRequest { ) { throw "Deletion must be filtered" } - let json = { + let json: QueryJson = { endpoint: { datasourceId: datasourceId!, entityId: tableName, @@ -715,7 +714,7 @@ export class ExternalRequest { }, meta: { table, - id: config.id, + tables: tables, }, } diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index c9e7f4c3c5..0b0802bab2 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -3080,4 +3080,46 @@ describe.each([ }).toHaveLength(4) }) }) + + isSql && + describe("max related columns", () => { + let relatedRows: Row[] + + beforeAll(async () => { + const relatedSchema: TableSchema = {} + const row: Row = {} + for (let i = 0; i < 100; i++) { + const name = `column${i}` + relatedSchema[name] = { name, type: FieldType.NUMBER } + row[name] = i + } + const relatedTable = await createTable(relatedSchema) + table = await createTable({ + name: { name: "name", type: FieldType.STRING }, + related1: { + type: FieldType.LINK, + name: "related1", + fieldName: "main1", + tableId: relatedTable._id!, + relationshipType: RelationshipType.MANY_TO_MANY, + }, + }) + relatedRows = await Promise.all([ + config.api.row.save(relatedTable._id!, row), + ]) + await config.api.row.save(table._id!, { + name: "foo", + related1: [relatedRows[0]._id], + }) + }) + + it("retrieve the row with relationships", async () => { + await expectQuery({}).toContainExactly([ + { + name: "foo", + related1: [{ _id: relatedRows[0]._id }], + }, + ]) + }) + }) }) From 63c0d9afb8cf6eb21a5b84734aa2c5b97534b883 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 16 Sep 2024 18:27:53 +0100 Subject: [PATCH 5/7] Sorting the field list to make sure we have the important fields at the top (if known). --- packages/backend-core/src/sql/sql.ts | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index ff32026814..55f71d76b0 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -55,6 +55,20 @@ function getRelationshipLimit() { return envLimit || 500 } +function prioritisedArraySort(toSort: string[], priorities: string[]) { + return toSort.sort((a, b) => { + const aPriority = priorities.find(field => field && a.endsWith(field)) + const bPriority = priorities.find(field => field && b.endsWith(field)) + if (aPriority && !bPriority) { + return -1 + } + if (!aPriority && bPriority) { + return 1 + } + return a.localeCompare(b) + }) +} + function getTableName(table?: Table): string | undefined { // SQS uses the table ID rather than the table name if ( @@ -924,11 +938,12 @@ class InternalBuilder { const requiredFields = [ ...(relatedTable?.primary || []), relatedTable?.primaryDisplay, - ] - let relationshipFields = fields - .filter(field => field.split(".")[0] === toAlias) - // sort the required fields to first in the list - .sort(field => (requiredFields.includes(field) ? 1 : -1)) + ].filter(field => field) as string[] + // sort the required fields to first in the list, so they don't get sliced out + let relationshipFields = prioritisedArraySort( + fields.filter(field => field.split(".")[0] === toAlias), + requiredFields + ) relationshipFields = relationshipFields.slice( 0, From ec400dee6f73b510d5b96378abb66f7877cda5cb Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 16 Sep 2024 19:20:58 +0100 Subject: [PATCH 6/7] Fixing test cases. --- packages/server/src/integrations/tests/sql.spec.ts | 4 ++-- packages/server/src/integrations/tests/sqlAlias.spec.ts | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/server/src/integrations/tests/sql.spec.ts b/packages/server/src/integrations/tests/sql.spec.ts index cf5b88b0fd..5a505fbb40 100644 --- a/packages/server/src/integrations/tests/sql.spec.ts +++ b/packages/server/src/integrations/tests/sql.spec.ts @@ -162,7 +162,7 @@ describe("SQL query builder", () => { const query = sql._query(generateRelationshipJson({ schema: "production" })) expect(query).toEqual({ bindings: [limit, relationshipLimit], - sql: `with "paginated" as (select "brands".* from "production"."brands" order by "test"."id" asc limit $1) select "brands".*, (select json_agg(json_build_object('product_id',"products"."product_id",'product_name',"products"."product_name",'brand_id',"products"."brand_id")) from (select "products".* from "production"."products" as "products" where "products"."brand_id" = "brands"."brand_id" order by "products"."brand_id" asc limit $2) as "products") as "products" from "paginated" as "brands" order by "test"."id" asc`, + sql: `with "paginated" as (select "brands".* from "production"."brands" order by "test"."id" asc limit $1) select "brands".*, (select json_agg(json_build_object('brand_id',"products"."brand_id",'product_id',"products"."product_id",'product_name',"products"."product_name")) from (select "products".* from "production"."products" as "products" where "products"."brand_id" = "brands"."brand_id" order by "products"."brand_id" asc limit $2) as "products") as "products" from "paginated" as "brands" order by "test"."id" asc`, }) }) @@ -170,7 +170,7 @@ describe("SQL query builder", () => { const query = sql._query(generateRelationshipJson()) expect(query).toEqual({ bindings: [limit, relationshipLimit], - sql: `with "paginated" as (select "brands".* from "brands" order by "test"."id" asc limit $1) select "brands".*, (select json_agg(json_build_object('product_id',"products"."product_id",'product_name',"products"."product_name",'brand_id',"products"."brand_id")) from (select "products".* from "products" as "products" where "products"."brand_id" = "brands"."brand_id" order by "products"."brand_id" asc limit $2) as "products") as "products" from "paginated" as "brands" order by "test"."id" asc`, + sql: `with "paginated" as (select "brands".* from "brands" order by "test"."id" asc limit $1) select "brands".*, (select json_agg(json_build_object('brand_id',"products"."brand_id",'product_id',"products"."product_id",'product_name',"products"."product_name")) from (select "products".* from "products" as "products" where "products"."brand_id" = "brands"."brand_id" order by "products"."brand_id" asc limit $2) as "products") as "products" from "paginated" as "brands" order by "test"."id" asc`, }) }) diff --git a/packages/server/src/integrations/tests/sqlAlias.spec.ts b/packages/server/src/integrations/tests/sqlAlias.spec.ts index 9548499c65..fc5af4238c 100644 --- a/packages/server/src/integrations/tests/sqlAlias.spec.ts +++ b/packages/server/src/integrations/tests/sqlAlias.spec.ts @@ -63,7 +63,7 @@ describe("Captures of real examples", () => { bindings: [primaryLimit, relationshipLimit, relationshipLimit], sql: expect.stringContaining( multiline( - `select json_agg(json_build_object('executorid',"b"."executorid",'taskname',"b"."taskname",'taskid',"b"."taskid",'completed',"b"."completed",'qaid',"b"."qaid",'executorid',"b"."executorid",'taskname',"b"."taskname",'taskid',"b"."taskid",'completed',"b"."completed",'qaid',"b"."qaid")` + `select json_agg(json_build_object('completed',"b"."completed",'completed',"b"."completed",'executorid',"b"."executorid",'executorid',"b"."executorid",'qaid',"b"."qaid",'qaid',"b"."qaid",'taskid',"b"."taskid",'taskid',"b"."taskid",'taskname',"b"."taskname",'taskname',"b"."taskname")` ) ), }) @@ -95,7 +95,7 @@ describe("Captures of real examples", () => { sql: expect.stringContaining( multiline( `with "paginated" as (select "a".* from "products" as "a" order by "a"."productname" asc nulls first, "a"."productid" asc limit $1) - select "a".*, (select json_agg(json_build_object('executorid',"b"."executorid",'taskname',"b"."taskname",'taskid',"b"."taskid",'completed',"b"."completed",'qaid',"b"."qaid")) + select "a".*, (select json_agg(json_build_object('completed',"b"."completed",'executorid',"b"."executorid",'qaid',"b"."qaid",'taskid',"b"."taskid",'taskname',"b"."taskname")) from (select "b".* from "tasks" as "b" inner join "products_tasks" as "c" on "b"."taskid" = "c"."taskid" where "c"."productid" = "a"."productid" order by "b"."taskid" asc limit $2) as "b") as "tasks" from "paginated" as "a" order by "a"."productname" asc nulls first, "a"."productid" asc` ) @@ -113,7 +113,7 @@ describe("Captures of real examples", () => { bindings: [...filters, relationshipLimit, relationshipLimit], sql: multiline( `with "paginated" as (select "a".* from "tasks" as "a" where "a"."taskid" in ($1, $2) order by "a"."taskid" asc limit $3) - select "a".*, (select json_agg(json_build_object('productname',"b"."productname",'productid',"b"."productid")) + select "a".*, (select json_agg(json_build_object('productid',"b"."productid",'productname',"b"."productname")) from (select "b".* from "products" as "b" inner join "products_tasks" as "c" on "b"."productid" = "c"."productid" where "c"."taskid" = "a"."taskid" order by "b"."productid" asc limit $4) as "b") as "products" from "paginated" as "a" order by "a"."taskid" asc` ), From 614132eb0462a071cbbcc6f0c867e589e1502108 Mon Sep 17 00:00:00 2001 From: Budibase Staging Release Bot <> Date: Mon, 16 Sep 2024 20:40:02 +0000 Subject: [PATCH 7/7] Bump version to 2.32.5 --- lerna.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lerna.json b/lerna.json index a9c50ea4b5..c710d888c7 100644 --- a/lerna.json +++ b/lerna.json @@ -1,6 +1,6 @@ { "$schema": "node_modules/lerna/schemas/lerna-schema.json", - "version": "2.32.4", + "version": "2.32.5", "npmClient": "yarn", "packages": [ "packages/*",