From d62d5b7043af74f015c654c78c014f849e96428e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 9 Oct 2024 15:09:38 +0100 Subject: [PATCH 1/3] Fixing an issue with removing relationships from the many side of a table in SQL, this was not correctly updating the other table. --- packages/backend-core/src/sql/sql.ts | 25 +++++++++++++------ .../api/controllers/row/ExternalRequest.ts | 22 ++++++++-------- 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index ed8dc929d6..05b38db309 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -325,14 +325,18 @@ class InternalBuilder { return input } - private parseBody(body: any) { - for (let [key, value] of Object.entries(body)) { - const { column } = this.splitter.run(key) - const schema = this.table.schema[column] - if (!schema) { - continue + private parseBody(body: Record) { + try { + for (let [key, value] of Object.entries(body)) { + const { column } = this.splitter.run(key) + const schema = this.table.schema[column] + if (!schema) { + continue + } + body[key] = this.parse(value, schema) } - body[key] = this.parse(value, schema) + } catch (err) { + console.log(err) } return body } @@ -1259,6 +1263,10 @@ class InternalBuilder { create(opts: QueryOptions): Knex.QueryBuilder { const { body } = this.query + if (!body) { + throw new Error("Cannot create without row body") + } + let query = this.qualifiedKnex({ alias: false }) const parsedBody = this.parseBody(body) @@ -1417,6 +1425,9 @@ class InternalBuilder { update(opts: QueryOptions): Knex.QueryBuilder { const { body, filters } = this.query + if (!body) { + throw new Error("Cannot update without row body") + } let query = this.qualifiedKnex() const parsedBody = this.parseBody(body) query = this.addFilters(query, filters) diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index ed8836626c..62ceedb56b 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -269,18 +269,13 @@ export class ExternalRequest { } } - private async removeManyToManyRelationships( - rowId: string, - table: Table, - colName: string - ) { + private async removeManyToManyRelationships(rowId: string, table: Table) { const tableId = table._id! const filters = this.prepareFilters(rowId, {}, table) // safety check, if there are no filters on deletion bad things happen if (Object.keys(filters).length !== 0) { return getDatasourceAndQuery({ endpoint: getEndpoint(tableId, Operation.DELETE), - body: { [colName]: null }, filters, meta: { table, @@ -291,13 +286,18 @@ export class ExternalRequest { } } - private async removeOneToManyRelationships(rowId: string, table: Table) { + private async removeOneToManyRelationships( + rowId: string, + table: Table, + colName: string + ) { const tableId = table._id! const filters = this.prepareFilters(rowId, {}, table) // safety check, if there are no filters on deletion bad things happen if (Object.keys(filters).length !== 0) { return getDatasourceAndQuery({ endpoint: getEndpoint(tableId, Operation.UPDATE), + body: { [colName]: null }, filters, meta: { table, @@ -595,8 +595,8 @@ export class ExternalRequest { for (let row of rows) { const rowId = generateIdForRow(row, table) const promise: Promise = isMany - ? this.removeManyToManyRelationships(rowId, table, colName) - : this.removeOneToManyRelationships(rowId, table) + ? this.removeManyToManyRelationships(rowId, table) + : this.removeOneToManyRelationships(rowId, table, colName) if (promise) { promises.push(promise) } @@ -619,12 +619,12 @@ export class ExternalRequest { rows.map(row => { const rowId = generateIdForRow(row, table) return isMany - ? this.removeManyToManyRelationships( + ? this.removeManyToManyRelationships(rowId, table) + : this.removeOneToManyRelationships( rowId, table, relationshipColumn.fieldName ) - : this.removeOneToManyRelationships(rowId, table) }) ) } From 70ab14319d8061fee391d0472e5f9a325d902698 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 9 Oct 2024 16:51:11 +0100 Subject: [PATCH 2/3] Adding test case for removing from many side of relationships in SQL. --- .../api/controllers/row/ExternalRequest.ts | 22 ++++++++++++---- .../server/src/api/routes/tests/row.spec.ts | 26 +++++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 62ceedb56b..3510b5ed75 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -4,6 +4,7 @@ import { AutoFieldSubType, AutoReason, Datasource, + DatasourcePlusQueryResponse, FieldSchema, FieldType, FilterType, @@ -557,8 +558,9 @@ export class ExternalRequest { return matchesPrimaryLink } - const matchesSecondayLink = row[linkSecondary] === body?.[linkSecondary] - return matchesPrimaryLink && matchesSecondayLink + const matchesSecondaryLink = + row[linkSecondary] === body?.[linkSecondary] + return matchesPrimaryLink && matchesSecondaryLink } const existingRelationship = rows.find((row: { [key: string]: any }) => @@ -743,9 +745,19 @@ export class ExternalRequest { // aliasing can be disabled fully if desired const aliasing = new sdk.rows.AliasTables(Object.keys(this.tables)) - let response = env.SQL_ALIASING_DISABLE - ? await getDatasourceAndQuery(json) - : await aliasing.queryWithAliasing(json, makeExternalQuery) + let response: DatasourcePlusQueryResponse + // there's a chance after input processing nothing needs updated, so pass over the call + // we might still need to perform other operations like updating the foreign keys on other rows + if ( + this.operation === Operation.UPDATE && + Object.keys(row || {}).length === 0 + ) { + response = [config.row!] + } else { + response = env.SQL_ALIASING_DISABLE + ? await getDatasourceAndQuery(json) + : await aliasing.queryWithAliasing(json, makeExternalQuery) + } // if it's a counting operation there will be no more processing, just return the number if (this.operation === Operation.COUNT) { diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 0995bd3824..6f56916ddc 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1114,6 +1114,32 @@ describe.each([ expect(getResp.user2[0]._id).toEqual(user2._id) }) + it("should be able to remove a relationship from many side", async () => { + const row = await config.api.row.save(otherTable._id!, { + name: "test", + description: "test", + }) + const row2 = await config.api.row.save(otherTable._id!, { + name: "test", + description: "test", + }) + const { _id } = await config.api.row.save(table._id!, { + relationship: [{ _id: row._id }, { _id: row2._id }], + }) + const relatedRow = await config.api.row.get(table._id!, _id!, { + status: 200, + }) + expect(relatedRow.relationship.length).toEqual(2) + await config.api.row.save(table._id!, { + ...relatedRow, + relationship: [{ _id: row._id }], + }) + const afterRelatedRow = await config.api.row.get(table._id!, _id!, { + status: 200, + }) + expect(afterRelatedRow.relationship.length).toEqual(1) + }) + it("should be able to update relationships when both columns are same name", async () => { let row = await config.api.row.save(table._id!, { name: "test", From 00048a2d3e797e710ef640d15b77289035026631 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 9 Oct 2024 17:04:27 +0100 Subject: [PATCH 3/3] Addressing PR comments. --- packages/backend-core/src/sql/sql.ts | 16 ++++++---------- .../src/api/controllers/row/ExternalRequest.ts | 6 ++++-- packages/server/src/api/routes/tests/row.spec.ts | 1 + 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 05b38db309..f122ad1c41 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -326,17 +326,13 @@ class InternalBuilder { } private parseBody(body: Record) { - try { - for (let [key, value] of Object.entries(body)) { - const { column } = this.splitter.run(key) - const schema = this.table.schema[column] - if (!schema) { - continue - } - body[key] = this.parse(value, schema) + for (let [key, value] of Object.entries(body)) { + const { column } = this.splitter.run(key) + const schema = this.table.schema[column] + if (!schema) { + continue } - } catch (err) { - console.log(err) + body[key] = this.parse(value, schema) } return body } diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 3510b5ed75..56522acb33 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -671,6 +671,7 @@ export class ExternalRequest { config.includeSqlRelationships === IncludeRelationship.INCLUDE // clean up row on ingress using schema + const unprocessedRow = config.row const processed = this.inputProcessing(row, table) row = processed.row let manyRelationships = processed.manyRelationships @@ -750,9 +751,10 @@ export class ExternalRequest { // we might still need to perform other operations like updating the foreign keys on other rows if ( this.operation === Operation.UPDATE && - Object.keys(row || {}).length === 0 + Object.keys(row || {}).length === 0 && + unprocessedRow ) { - response = [config.row!] + response = [unprocessedRow] } else { response = env.SQL_ALIASING_DISABLE ? await getDatasourceAndQuery(json) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 6f56916ddc..b86ec38d08 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1138,6 +1138,7 @@ describe.each([ status: 200, }) expect(afterRelatedRow.relationship.length).toEqual(1) + expect(afterRelatedRow.relationship[0]._id).toEqual(row._id) }) it("should be able to update relationships when both columns are same name", async () => {