From 76273ff860fe7a71b5f3b3a7d0043cfa849dadd1 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 6 Sep 2024 16:47:43 +0100 Subject: [PATCH 1/7] PR comments. --- packages/backend-core/src/sql/sql.ts | 78 ++++--------------- .../src/api/controllers/row/utils/basic.ts | 45 +++++------ .../routes/tests/queries/generic-sql.spec.ts | 13 ++-- 3 files changed, 45 insertions(+), 91 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index b1a2bc060a..d433cca504 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -855,6 +855,21 @@ class InternalBuilder { return withSchema } + private buildJsonField(field: string): string { + const parts = field.split(".") + let tableField: string, unaliased: string + if (parts.length > 1) { + const alias = parts.shift()! + unaliased = parts.join(".") + tableField = `${this.quote(alias)}.${this.quote(unaliased)}` + } else { + unaliased = parts.join(".") + tableField = this.quote(unaliased) + } + const separator = this.client === SqlClient.ORACLE ? " VALUE " : "," + return `'${unaliased}'${separator}${tableField}` + } + addJsonRelationships( query: Knex.QueryBuilder, fromTable: string, @@ -864,27 +879,6 @@ class InternalBuilder { const knex = this.knex const { resource, tableAliases: aliases, endpoint } = this.query const fields = resource?.fields || [] - const jsonField = (field: string) => { - const parts = field.split(".") - let tableField: string, unaliased: string - if (parts.length > 1) { - const alias = parts.shift()! - unaliased = parts.join(".") - tableField = `${this.quote(alias)}.${this.quote(unaliased)}` - } else { - unaliased = parts.join(".") - tableField = this.quote(unaliased) - } - let separator = "," - switch (sqlClient) { - case SqlClient.ORACLE: - separator = " VALUE " - break - case SqlClient.MS_SQL: - separator = ":" - } - return `'${unaliased}'${separator}${tableField}` - } for (let relationship of relationships) { const { tableName: toTable, @@ -914,7 +908,7 @@ class InternalBuilder { ) } const fieldList: string = relationshipFields - .map(field => jsonField(field)) + .map(field => this.buildJsonField(field)) .join(",") // SQL Server uses TOP - which performs a little differently to the normal LIMIT syntax // it reduces the result set rather than limiting how much data it filters over @@ -1066,43 +1060,6 @@ class InternalBuilder { return query } - addRelationships( - query: Knex.QueryBuilder, - fromTable: string, - relationships: RelationshipsJson[] - ): Knex.QueryBuilder { - const tableSets: Record = {} - // aggregate into table sets (all the same to tables) - for (let relationship of relationships) { - const keyObj: { toTable: string; throughTable: string | undefined } = { - toTable: relationship.tableName, - throughTable: undefined, - } - if (relationship.through) { - keyObj.throughTable = relationship.through - } - const key = JSON.stringify(keyObj) - if (tableSets[key]) { - tableSets[key].push(relationship) - } else { - tableSets[key] = [relationship] - } - } - for (let [key, relationships] of Object.entries(tableSets)) { - const { toTable, throughTable } = JSON.parse(key) - query = this.addJoin( - query, - { - from: fromTable, - to: toTable, - through: throughTable, - }, - relationships - ) - } - return query - } - qualifiedKnex(opts?: { alias?: string | boolean }): Knex.QueryBuilder { let alias = this.query.tableAliases?.[this.query.endpoint.entityId] if (opts?.alias === false) { @@ -1186,8 +1143,7 @@ class InternalBuilder { if (!primary) { throw new Error("Primary key is required for upsert") } - const ret = query.insert(parsedBody).onConflict(primary).merge() - return ret + return query.insert(parsedBody).onConflict(primary).merge() } else if ( this.client === SqlClient.MS_SQL || this.client === SqlClient.ORACLE diff --git a/packages/server/src/api/controllers/row/utils/basic.ts b/packages/server/src/api/controllers/row/utils/basic.ts index b754e288ed..289e56f36f 100644 --- a/packages/server/src/api/controllers/row/utils/basic.ts +++ b/packages/server/src/api/controllers/row/utils/basic.ts @@ -129,32 +129,29 @@ export function basicProcessing({ : typeof value === "string" ? JSON.parse(value) : undefined - if (array) { + if (array && Array.isArray(array)) { thisRow[col] = array // make sure all of them have an _id - if (Array.isArray(thisRow[col])) { - const sortField = - relatedTable.primaryDisplay || relatedTable.primary![0]! - thisRow[col] = (thisRow[col] as Row[]) - .map(relatedRow => { - relatedRow._id = relatedRow._id - ? relatedRow._id - : generateIdForRow(relatedRow, relatedTable) - return relatedRow - }) - .sort((a, b) => { - const aField = a?.[sortField], - bField = b?.[sortField] - if (!aField) { - return 1 - } else if (!bField) { - return -1 - } - return aField.localeCompare - ? aField.localeCompare(bField) - : aField - bField - }) - } + const sortField = relatedTable.primaryDisplay || relatedTable.primary![0]! + thisRow[col] = (thisRow[col] as Row[]) + .map(relatedRow => { + relatedRow._id = relatedRow._id + ? relatedRow._id + : generateIdForRow(relatedRow, relatedTable) + return relatedRow + }) + .sort((a, b) => { + const aField = a?.[sortField], + bField = b?.[sortField] + if (!aField) { + return 1 + } else if (!bField) { + return -1 + } + return aField.localeCompare + ? aField.localeCompare(bField) + : aField - bField + }) } } return thisRow diff --git a/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts b/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts index 4bd1951d67..e6da4693eb 100644 --- a/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts +++ b/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts @@ -832,12 +832,13 @@ describe.each( }, }) expect(res).toHaveLength(1) - expect(res[0]).toEqual( - expect.objectContaining({ - id: 2, - name: "two", - }) - ) + expect(res[0]).toEqual({ + id: 2, + name: "two", + // the use of table.* introduces the possibility of nulls being returned + birthday: null, + number: null, + }) }) // this parameter really only impacts SQL queries From 157e75b9a6224bf69489ca4d894cff973cab2c3d Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 6 Sep 2024 19:34:02 +0100 Subject: [PATCH 2/7] Using a CTE for the main query, then adding the JSON aggregation on afterwards - fixing issue with offset pagination applying the JSON aggregation to all rows before hand. --- packages/backend-core/src/sql/sql.ts | 31 +++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index d433cca504..05c9b036ca 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -1202,12 +1202,33 @@ class InternalBuilder { if (!counting) { query = this.addSorting(query) } - // handle joins - if (relationships) { - query = this.addJsonRelationships(query, tableName, relationships) - } - return this.addFilters(query, filters, { relationship: true }) + query = this.addFilters(query, filters, { relationship: true }) + + // SQLite (SQS) cannot use the WITH statement yet + if (relationships?.length && this.client === SqlClient.SQL_LITE) { + return this.addJsonRelationships(query, tableName, relationships) + } + // handle relationships with a CTE for all others + else if (relationships?.length) { + const mainTable = + this.query.tableAliases?.[this.query.endpoint.entityId] || + this.query.endpoint.entityId + const cte = this.addSorting( + this.knex + .with("paginated", query) + .select(this.generateSelectStatement()) + .from({ + [mainTable]: "paginated", + }) + ) + // add JSON aggregations attached to the CTE + return this.addJsonRelationships(cte, tableName, relationships) + } + // no relationships found - return query + else { + return query + } } update(opts: QueryOptions): Knex.QueryBuilder { From c9b64e3591278fec844c601a5d56c86e5bd3c107 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 11 Sep 2024 13:41:54 +0100 Subject: [PATCH 3/7] SQLite uses CTE with SQS 2.1.1. --- packages/backend-core/src/sql/sql.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 6757bf8f50..2b5243f856 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -1212,12 +1212,8 @@ class InternalBuilder { query = this.addFilters(query, filters, { relationship: true }) - // SQLite (SQS) cannot use the WITH statement yet - if (relationships?.length && this.client === SqlClient.SQL_LITE) { - return this.addJsonRelationships(query, tableName, relationships) - } // handle relationships with a CTE for all others - else if (relationships?.length) { + if (relationships?.length) { const mainTable = this.query.tableAliases?.[this.query.endpoint.entityId] || this.query.endpoint.entityId From 48784d59e6b78df64f51c4ab9ada2915e389a60c Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 11 Sep 2024 13:42:24 +0100 Subject: [PATCH 4/7] Adding development tag. --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index f3cbd75836..582e35180e 100644 --- a/package.json +++ b/package.json @@ -76,7 +76,7 @@ "build:docker:single": "./scripts/build-single-image.sh", "build:docker:single:sqs": "./scripts/build-single-image-sqs.sh", "build:docker:dependencies": "docker build -f hosting/dependencies/Dockerfile -t budibase/dependencies:latest ./hosting", - "publish:docker:couch": "docker buildx build --platform linux/arm64,linux/amd64 -f hosting/couchdb/Dockerfile -t budibase/couchdb:latest -t budibase/couchdb:v3.3.3 --push ./hosting/couchdb", + "publish:docker:couch": "docker buildx build --platform linux/arm64,linux/amd64 -f hosting/couchdb/Dockerfile -t budibase/couchdb:latest -t budibase/couchdb:v3.3.3 -t budibase/couchdb:v3.3.3-sqs-v2.1.1 --push ./hosting/couchdb", "publish:docker:dependencies": "docker buildx build --platform linux/arm64,linux/amd64 -f hosting/dependencies/Dockerfile -t budibase/dependencies:latest -t budibase/dependencies:v3.2.1 --push ./hosting", "release:helm": "node scripts/releaseHelmChart", "env:multi:enable": "lerna run --stream env:multi:enable", From db074c0d222d63df9cb49019e0c486fbe69ae9fa Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 11 Sep 2024 13:45:14 +0100 Subject: [PATCH 5/7] Updating dev docker-compose to use specific SQS version. --- hosting/docker-compose.dev.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hosting/docker-compose.dev.yaml b/hosting/docker-compose.dev.yaml index 186751ed59..de310a591d 100644 --- a/hosting/docker-compose.dev.yaml +++ b/hosting/docker-compose.dev.yaml @@ -42,7 +42,7 @@ services: couchdb-service: container_name: budi-couchdb3-dev restart: on-failure - image: budibase/couchdb:v3.3.3 + image: budibase/couchdb:v3.3.3-sqs-v2.1.1 environment: - COUCHDB_PASSWORD=${COUCH_DB_PASSWORD} - COUCHDB_USER=${COUCH_DB_USER} From fc3a865cfbf0485677075e79fc75ee58091d0c95 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 11 Sep 2024 14:26:01 +0100 Subject: [PATCH 6/7] Test fix. --- .../src/integrations/tests/sqlAlias.spec.ts | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/packages/server/src/integrations/tests/sqlAlias.spec.ts b/packages/server/src/integrations/tests/sqlAlias.spec.ts index 528e782543..9548499c65 100644 --- a/packages/server/src/integrations/tests/sqlAlias.spec.ts +++ b/packages/server/src/integrations/tests/sqlAlias.spec.ts @@ -60,7 +60,7 @@ describe("Captures of real examples", () => { queryJson ) expect(query).toEqual({ - bindings: [relationshipLimit, relationshipLimit, primaryLimit], + 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")` @@ -75,11 +75,11 @@ describe("Captures of real examples", () => { queryJson ) expect(query).toEqual({ - bindings: [relationshipLimit, "assembling", primaryLimit], + bindings: ["assembling", primaryLimit, relationshipLimit], sql: expect.stringContaining( multiline( - `where exists (select 1 from "tasks" as "b" inner join "products_tasks" as "c" on "b"."taskid" = "c"."taskid" - where "c"."productid" = "a"."productid" and COALESCE("b"."taskname" = $2, FALSE)` + `where exists (select 1 from "tasks" as "b" inner join "products_tasks" as "c" on "b"."taskid" = "c"."taskid" where "c"."productid" = "a"."productid" + and COALESCE("b"."taskname" = $1, FALSE)` ) ), }) @@ -91,12 +91,13 @@ describe("Captures of real examples", () => { queryJson ) expect(query).toEqual({ - bindings: [relationshipLimit, primaryLimit], + bindings: [primaryLimit, 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")) - 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 $1` + `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")) + 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` ) ), }) @@ -109,12 +110,12 @@ describe("Captures of real examples", () => { queryJson ) expect(query).toEqual({ - bindings: [relationshipLimit, ...filters, relationshipLimit], + bindings: [...filters, relationshipLimit, relationshipLimit], sql: multiline( - `select "a".*, (select json_agg(json_build_object('productname',"b"."productname",'productid',"b"."productid")) + `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")) 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 $1) as "b") as "products" - from "tasks" as "a" where "a"."taskid" in ($2, $3) order by "a"."taskid" asc limit $4` + 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` ), }) }) @@ -132,18 +133,18 @@ describe("Captures of real examples", () => { expect(query).toEqual({ bindings: [ - relationshipLimit, - relationshipLimit, - relationshipLimit, rangeValue.low, rangeValue.high, equalValue, notEqualsValue, primaryLimit, + relationshipLimit, + relationshipLimit, + relationshipLimit, ], sql: expect.stringContaining( multiline( - `where exists (select 1 from "persons" as "c" where "c"."personid" = "a"."executorid" and "c"."year" between $4 and $5)` + `where exists (select 1 from "persons" as "c" where "c"."personid" = "a"."executorid" and "c"."year" between $1 and $2)` ) ), }) From 176c3d4ffd55b7b53d8d1503aa77385de3c3881e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 11 Sep 2024 14:52:09 +0100 Subject: [PATCH 7/7] SQL test update. --- packages/server/src/integrations/tests/sql.spec.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/server/src/integrations/tests/sql.spec.ts b/packages/server/src/integrations/tests/sql.spec.ts index f2bcc0bde0..cf5b88b0fd 100644 --- a/packages/server/src/integrations/tests/sql.spec.ts +++ b/packages/server/src/integrations/tests/sql.spec.ts @@ -161,16 +161,16 @@ describe("SQL query builder", () => { it("should add the schema to the LEFT JOIN", () => { const query = sql._query(generateRelationshipJson({ schema: "production" })) expect(query).toEqual({ - bindings: [relationshipLimit, limit], - sql: `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 $1) as "products") as "products" from "production"."brands" order by "test"."id" asc limit $2`, + 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`, }) }) it("should handle if the schema is not present when doing a LEFT JOIN", () => { const query = sql._query(generateRelationshipJson()) expect(query).toEqual({ - bindings: [relationshipLimit, limit], - sql: `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 $1) as "products") as "products" from "brands" order by "test"."id" asc limit $2`, + 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`, }) }) @@ -179,8 +179,8 @@ describe("SQL query builder", () => { generateManyRelationshipJson({ schema: "production" }) ) expect(query).toEqual({ - bindings: [relationshipLimit, limit], - sql: `select "stores".*, (select json_agg(json_build_object('product_id',"products"."product_id",'product_name',"products"."product_name")) from (select "products".* from "production"."products" as "products" inner join "production"."stocks" as "stocks" on "products"."product_id" = "stocks"."product_id" where "stocks"."store_id" = "stores"."store_id" order by "products"."product_id" asc limit $1) as "products") as "products" from "production"."stores" order by "test"."id" asc limit $2`, + bindings: [limit, relationshipLimit], + sql: `with "paginated" as (select "stores".* from "production"."stores" order by "test"."id" asc limit $1) select "stores".*, (select json_agg(json_build_object('product_id',"products"."product_id",'product_name',"products"."product_name")) from (select "products".* from "production"."products" as "products" inner join "production"."stocks" as "stocks" on "products"."product_id" = "stocks"."product_id" where "stocks"."store_id" = "stores"."store_id" order by "products"."product_id" asc limit $2) as "products") as "products" from "paginated" as "stores" order by "test"."id" asc`, }) })