From 602faf1c67477ddfb44a2aee28d982c0f9e1ad09 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 19 Jun 2024 17:52:48 +0100 Subject: [PATCH 01/22] Add test for composite primary keys for external datasource imports. --- packages/backend-core/src/sql/sql.ts | 3 +- .../ExistingTableDataImport.svelte | 20 ++--- .../api/controllers/row/ExternalRequest.ts | 1 + .../src/api/controllers/table/external.ts | 14 +++- .../server/src/api/routes/tests/row.spec.ts | 81 ++++++++++++++++++- 5 files changed, 106 insertions(+), 13 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 3224fc043e..7d52665f39 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -587,7 +587,8 @@ class InternalBuilder { if (!primary) { throw new Error("Primary key is required for upsert") } - return query.insert(parsedBody).onConflict(primary).merge() + const ret = query.insert(parsedBody).onConflict(primary).merge() + return ret } else if (this.client === SqlClient.MS_SQL) { // No upsert or onConflict support in MSSQL yet, see: // https://github.com/knex/knex/pull/6050 diff --git a/packages/builder/src/components/backend/TableNavigator/ExistingTableDataImport.svelte b/packages/builder/src/components/backend/TableNavigator/ExistingTableDataImport.svelte index de56fa8ce5..cd6b5aeb2e 100644 --- a/packages/builder/src/components/backend/TableNavigator/ExistingTableDataImport.svelte +++ b/packages/builder/src/components/backend/TableNavigator/ExistingTableDataImport.svelte @@ -185,20 +185,22 @@ {/each} - {#if tableType === DB_TYPE_INTERNAL} -
- (identifierFields = [])} - thin - text="Update existing rows" - /> - {#if updateExistingRows} +
+ (identifierFields = [])} + thin + text="Update existing rows" + /> + {#if updateExistingRows} + {#if tableType === DB_TYPE_INTERNAL} + {:else} +

Rows will be updated based on the table's primary key.

{/if} {/if} {#if invalidColumns.length > 0} diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index b30c97e289..121a1b6b9b 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -653,6 +653,7 @@ export class ExternalRequest { }, meta: { table, + id: config.id, }, } diff --git a/packages/server/src/api/controllers/table/external.ts b/packages/server/src/api/controllers/table/external.ts index f1b186c233..db4343b405 100644 --- a/packages/server/src/api/controllers/table/external.ts +++ b/packages/server/src/api/controllers/table/external.ts @@ -16,6 +16,7 @@ import { import sdk from "../../../sdk" import { builderSocket } from "../../../websockets" import { inputProcessing } from "../../../utilities/rowProcessor" +import _ from "lodash" function getDatasourceId(table: Table) { if (!table) { @@ -82,9 +83,20 @@ export async function bulkImport( ctx: UserCtx ) { let table = await sdk.tables.getTable(ctx.params.tableId) - const { rows } = ctx.request.body + const { rows, identifierFields } = ctx.request.body const schema = table.schema + if (identifierFields && !_.isEqual(identifierFields, table.primary)) { + // This is becuse we make use of the ON CONFLICT functionality in SQL + // databases, which only triggers when there's a conflict against a unique + // index. The only unique index we can count on atm in Budibase is the + // primary key, so this functionality always uses the primary key. + ctx.throw( + 400, + "Identifier fields are not supported for bulk import into an external datasource." + ) + } + if (!rows || !isRows(rows) || !isSchema(schema)) { ctx.throw(400, "Provided data import information is invalid.") } diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 4d1b4f028b..96eec95921 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1,4 +1,8 @@ -import { DatabaseName, getDatasource } from "../../../integrations/tests/utils" +import { + DatabaseName, + getDatasource, + knexClient, +} from "../../../integrations/tests/utils" import tk from "timekeeper" import emitter from "../../../../src/events" @@ -31,6 +35,7 @@ import { import { generator, mocks } from "@budibase/backend-core/tests" import _, { merge } from "lodash" import * as uuid from "uuid" +import { Knex } from "knex" const timestamp = new Date("2023-01-26T11:48:57.597Z").toISOString() tk.freeze(timestamp) @@ -70,13 +75,16 @@ describe.each([ let table: Table let datasource: Datasource | undefined + let client: Knex | undefined beforeAll(async () => { await config.init() if (dsProvider) { + const rawDatasource = await dsProvider datasource = await config.createDatasource({ - datasource: await dsProvider, + datasource: rawDatasource, }) + client = await knexClient(rawDatasource) } }) @@ -1077,6 +1085,75 @@ describe.each([ const rows = await config.api.row.fetch(table._id!) expect(rows.length).toEqual(3) + rows.sort((a, b) => a.name.localeCompare(b.name)) + expect(rows[0].name).toEqual("Row 1 updated") + expect(rows[0].description).toEqual("Row 1 description updated") + expect(rows[1].name).toEqual("Row 2 updated") + expect(rows[1].description).toEqual("Row 2 description updated") + expect(rows[2].name).toEqual("Row 3") + expect(rows[2].description).toEqual("Row 3 description") + }) + + // Upserting isn't yet supported in MSSQL, see: + // https://github.com/knex/knex/pull/6050 + !isMSSQL && + !isInternal && + it("should be able to update existing rows with composite primary keys with bulkImport", async () => { + const tableName = uuid.v4() + await client?.schema.createTable(tableName, table => { + table.integer("companyId") + table.integer("userId") + table.string("name") + table.string("description") + table.primary(["companyId", "userId"]) + }) + + const resp = await config.api.datasource.fetchSchema({ + datasourceId: datasource!._id!, + }) + const table = resp.datasource.entities![tableName] + + const row1 = await config.api.row.save(table._id!, { + companyId: 1, + userId: 1, + name: "Row 1", + description: "Row 1 description", + }) + + const row2 = await config.api.row.save(table._id!, { + companyId: 1, + userId: 2, + name: "Row 2", + description: "Row 2 description", + }) + + await config.api.row.bulkImport(table._id!, { + identifierFields: ["companyId", "userId"], + rows: [ + { + companyId: 1, + userId: row1.userId, + name: "Row 1 updated", + description: "Row 1 description updated", + }, + { + companyId: 1, + userId: row2.userId, + name: "Row 2 updated", + description: "Row 2 description updated", + }, + { + companyId: 1, + userId: 3, + name: "Row 3", + description: "Row 3 description", + }, + ], + }) + + const rows = await config.api.row.fetch(table._id!) + expect(rows.length).toEqual(3) + rows.sort((a, b) => a.name.localeCompare(b.name)) expect(rows[0].name).toEqual("Row 1 updated") expect(rows[0].description).toEqual("Row 1 description updated") From c9fecbaa776e69d2f25da4d21897fc12153e7f07 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 21 Jun 2024 15:12:02 +0100 Subject: [PATCH 02/22] Fix updating rows in external tables where the primary key is an autocolumn. --- .../api/controllers/row/ExternalRequest.ts | 8 +- .../src/api/controllers/table/external.ts | 8 +- .../server/src/api/controllers/table/utils.ts | 2 +- .../server/src/api/routes/tests/row.spec.ts | 87 +++++++++++++------ .../src/utilities/rowProcessor/index.ts | 1 + packages/server/src/utilities/schema.ts | 15 +++- 6 files changed, 89 insertions(+), 32 deletions(-) diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 0ad55a78c3..c2f51decc2 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -289,7 +289,13 @@ export class ExternalRequest { manyRelationships: ManyRelationship[] = [] for (let [key, field] of Object.entries(table.schema)) { // if set already, or not set just skip it - if (row[key] === undefined || newRow[key] || !isEditableColumn(field)) { + if (row[key] === undefined || newRow[key]) { + continue + } + if ( + !(this.operation === Operation.BULK_UPSERT) && + !isEditableColumn(field) + ) { continue } // parse floats/numbers diff --git a/packages/server/src/api/controllers/table/external.ts b/packages/server/src/api/controllers/table/external.ts index db4343b405..15768d73bb 100644 --- a/packages/server/src/api/controllers/table/external.ts +++ b/packages/server/src/api/controllers/table/external.ts @@ -86,7 +86,11 @@ export async function bulkImport( const { rows, identifierFields } = ctx.request.body const schema = table.schema - if (identifierFields && !_.isEqual(identifierFields, table.primary)) { + if ( + identifierFields && + identifierFields.length > 0 && + !_.isEqual(identifierFields, table.primary) + ) { // This is becuse we make use of the ON CONFLICT functionality in SQL // databases, which only triggers when there's a conflict against a unique // index. The only unique index we can count on atm in Budibase is the @@ -102,7 +106,7 @@ export async function bulkImport( } const parsedRows = [] - for (const row of parse(rows, schema)) { + for (const row of parse(rows, table)) { const processed = await inputProcessing(ctx.user?._id, table, row, { noAutoRelationships: true, }) diff --git a/packages/server/src/api/controllers/table/utils.ts b/packages/server/src/api/controllers/table/utils.ts index a42cfc43c3..0e9a32b294 100644 --- a/packages/server/src/api/controllers/table/utils.ts +++ b/packages/server/src/api/controllers/table/utils.ts @@ -178,7 +178,7 @@ export async function handleDataImport( } const db = context.getAppDB() - const data = parse(importRows, schema) + const data = parse(importRows, table) let finalData: any = await importToRows(data, table, user) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 96eec95921..c5f9262861 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -315,13 +315,13 @@ describe.each([ // as quickly as possible. await Promise.all( sequence.map(async () => { - const attempts = 20 + const attempts = 30 for (let attempt = 0; attempt < attempts; attempt++) { try { await config.api.row.save(table._id!, {}) return } catch (e) { - await new Promise(r => setTimeout(r, Math.random() * 15)) + await new Promise(r => setTimeout(r, Math.random() * 50)) } } throw new Error(`Failed to create row after ${attempts} attempts`) @@ -919,32 +919,21 @@ describe.each([ await assertRowUsage(isInternal ? rowUsage - 1 : rowUsage) }) - it("Should ignore malformed/invalid delete requests", async () => { - const rowUsage = await getRowUsage() + it.each([{ not: "valid" }, { rows: 123 }, "invalid"])( + "Should ignore malformed/invalid delete request: %s", + async (request: any) => { + const rowUsage = await getRowUsage() - await config.api.row.delete(table._id!, { not: "valid" } as any, { - status: 400, - body: { - message: "Invalid delete rows request", - }, - }) + await config.api.row.delete(table._id!, request, { + status: 400, + body: { + message: "Invalid delete rows request", + }, + }) - await config.api.row.delete(table._id!, { rows: 123 } as any, { - status: 400, - body: { - message: "Invalid delete rows request", - }, - }) - - await config.api.row.delete(table._id!, "invalid" as any, { - status: 400, - body: { - message: "Invalid delete rows request", - }, - }) - - await assertRowUsage(rowUsage) - }) + await assertRowUsage(rowUsage) + } + ) }) describe("bulkImport", () => { @@ -1162,6 +1151,52 @@ describe.each([ expect(rows[2].name).toEqual("Row 3") expect(rows[2].description).toEqual("Row 3 description") }) + + // Upserting isn't yet supported in MSSQL, see: + // https://github.com/knex/knex/pull/6050 + !isMSSQL && + !isInternal && + it("should be able to update existing rows an autoID primary key", async () => { + const tableName = uuid.v4() + await client!.schema.createTable(tableName, table => { + table.increments("userId").primary() + table.string("name") + }) + + const resp = await config.api.datasource.fetchSchema({ + datasourceId: datasource!._id!, + }) + const table = resp.datasource.entities![tableName] + + const row1 = await config.api.row.save(table._id!, { + name: "Clare", + }) + + const row2 = await config.api.row.save(table._id!, { + name: "Jeff", + }) + + await config.api.row.bulkImport(table._id!, { + identifierFields: ["userId"], + rows: [ + { + userId: row1.userId, + name: "Clare updated", + }, + { + userId: row2.userId, + name: "Jeff updated", + }, + ], + }) + + const rows = await config.api.row.fetch(table._id!) + expect(rows.length).toEqual(2) + + rows.sort((a, b) => a.name.localeCompare(b.name)) + expect(rows[0].name).toEqual("Clare updated") + expect(rows[1].name).toEqual("Jeff updated") + }) }) describe("enrich", () => { diff --git a/packages/server/src/utilities/rowProcessor/index.ts b/packages/server/src/utilities/rowProcessor/index.ts index 73176af6d8..97b5332dd4 100644 --- a/packages/server/src/utilities/rowProcessor/index.ts +++ b/packages/server/src/utilities/rowProcessor/index.ts @@ -9,6 +9,7 @@ import { Row, RowAttachment, Table, + TableSourceType, } from "@budibase/types" import { cloneDeep } from "lodash/fp" import { diff --git a/packages/server/src/utilities/schema.ts b/packages/server/src/utilities/schema.ts index 8e6cd34c7c..e473675633 100644 --- a/packages/server/src/utilities/schema.ts +++ b/packages/server/src/utilities/schema.ts @@ -4,6 +4,7 @@ import { TableSchema, FieldSchema, Row, + Table, } from "@budibase/types" import { ValidColumnNameRegex, helpers, utils } from "@budibase/shared-core" import { db } from "@budibase/backend-core" @@ -118,16 +119,26 @@ export function validate(rows: Rows, schema: TableSchema): ValidationResults { return results } -export function parse(rows: Rows, schema: TableSchema): Rows { +export function parse(rows: Rows, table: Table): Rows { return rows.map(row => { const parsedRow: Row = {} Object.entries(row).forEach(([columnName, columnData]) => { - if (!(columnName in schema) || schema[columnName]?.autocolumn) { + const schema = table.schema + if (!(columnName in schema)) { // Objects can be present in the row data but not in the schema, so make sure we don't proceed in such a case return } + if ( + schema[columnName].autocolumn && + !table.primary?.includes(columnName) + ) { + // Don't want the user specifying values for autocolumns unless they're updating + // a row through its primary key. + return + } + const columnSchema = schema[columnName] const { type: columnType } = columnSchema if (columnType === FieldType.NUMBER) { From d5481312d0e52ec73e569c8a44b724b499eb9c3a Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 21 Jun 2024 15:31:34 +0100 Subject: [PATCH 03/22] Fix lint, add new update test. --- .../server/src/api/routes/tests/row.spec.ts | 33 ++++++++++++++++++- .../src/utilities/rowProcessor/index.ts | 1 - 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index c5f9262861..b6e3edf5ff 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -606,6 +606,35 @@ describe.each([ expect(res.name).toEqual("Updated Name") await assertRowUsage(rowUsage) }) + + !isInternal && + it("can update a row on an external table with a primary key", async () => { + const tableName = uuid.v4().substring(0, 10) + await client!.schema.createTable(tableName, table => { + table.increments("id").primary() + table.string("name") + }) + + const res = await config.api.datasource.fetchSchema({ + datasourceId: datasource!._id!, + }) + const table = res.datasource.entities![tableName] + + const row = await config.api.row.save(table._id!, { + id: 1, + name: "Row 1", + }) + + const updatedRow = await config.api.row.save(table._id!, { + _id: row._id!, + name: "Row 1 Updated", + }) + + expect(updatedRow.name).toEqual("Row 1 Updated") + + const rows = await config.api.row.fetch(table._id!) + expect(rows).toHaveLength(1) + }) }) describe("patch", () => { @@ -675,6 +704,7 @@ describe.each([ expect(event.oldRow.description).toEqual(beforeRow.description) expect(event.row.description).toEqual(beforeRow.description) }) + it("should throw an error when given improper types", async () => { const existing = await config.api.row.save(table._id!, {}) const rowUsage = await getRowUsage() @@ -766,7 +796,8 @@ describe.each([ }) !isInternal && - // TODO: SQL is having issues creating composite keys + // MSSQL needs a setting called IDENTITY_INSERT to be set to ON to allow writing + // to identity columns. This is not something Budibase does currently. providerType !== DatabaseName.SQL_SERVER && it("should support updating fields that are part of a composite key", async () => { const tableRequest = saveTableRequest({ diff --git a/packages/server/src/utilities/rowProcessor/index.ts b/packages/server/src/utilities/rowProcessor/index.ts index 97b5332dd4..73176af6d8 100644 --- a/packages/server/src/utilities/rowProcessor/index.ts +++ b/packages/server/src/utilities/rowProcessor/index.ts @@ -9,7 +9,6 @@ import { Row, RowAttachment, Table, - TableSourceType, } from "@budibase/types" import { cloneDeep } from "lodash/fp" import { From e5c40c7ecdf00f654cbe133317a119f2ee1808d0 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 21 Jun 2024 16:58:27 +0100 Subject: [PATCH 04/22] Moving some stuff around inside ExternalRequests to make it easier to access parts of the full context. --- .../api/controllers/row/ExternalRequest.ts | 238 ++++++++++-------- 1 file changed, 138 insertions(+), 100 deletions(-) diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 619a1e9548..3209416544 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -72,92 +72,6 @@ export type ExternalRequestReturnType = ? number : { row: Row; table: Table } -function buildFilters( - id: string | undefined | string[], - filters: SearchFilters, - table: Table -) { - const primary = table.primary - // if passed in array need to copy for shifting etc - let idCopy: undefined | string | any[] = cloneDeep(id) - if (filters) { - // need to map over the filters and make sure the _id field isn't present - let prefix = 1 - for (let operator of Object.values(filters)) { - for (let field of Object.keys(operator || {})) { - if (dbCore.removeKeyNumbering(field) === "_id") { - if (primary) { - const parts = breakRowIdField(operator[field]) - for (let field of primary) { - operator[`${prefix}:${field}`] = parts.shift() - } - prefix++ - } - // make sure this field doesn't exist on any filter - delete operator[field] - } - } - } - } - // there is no id, just use the user provided filters - if (!idCopy || !table) { - return filters - } - // if used as URL parameter it will have been joined - if (!Array.isArray(idCopy)) { - idCopy = breakRowIdField(idCopy) - } - const equal: any = {} - if (primary && idCopy) { - for (let field of primary) { - // work through the ID and get the parts - equal[field] = idCopy.shift() - } - } - return { - equal, - } -} - -async function removeManyToManyRelationships( - rowId: string, - table: Table, - colName: string -) { - const tableId = table._id! - const filters = buildFilters(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, - }, - }) - } else { - return [] - } -} - -async function removeOneToManyRelationships(rowId: string, table: Table) { - const tableId = table._id! - const filters = buildFilters(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), - filters, - meta: { - table, - }, - }) - } else { - return [] - } -} - /** * This function checks the incoming parameters to make sure all the inputs are * valid based on on the table schema. The main thing this is looking for is when a @@ -240,6 +154,7 @@ 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 @@ -248,6 +163,117 @@ export class ExternalRequest { if (datasource && datasource.entities) { this.tables = datasource.entities } + this.tableList = Object.values(this.tables) + } + + private prepareFilters( + id: string | undefined | string[], + filters: SearchFilters, + table: Table + ) { + const tables = this.tableList + // replace any relationship columns initially, table names and relationship column names are acceptable + const relationshipColumns = sdk.rows.filters.getRelationshipColumns(table) + filters = sdk.rows.filters.updateFilterKeys( + filters, + relationshipColumns + .map(({ name, definition }) => { + const { tableName } = breakExternalTableId(definition.tableId) + return { + original: name, + updated: tableName!, + } + }) + // don't update table names - include this for context incase a column would be replaced + .concat( + tables.map(table => { + const tableName = table.originalName || table.name + return { + original: tableName, + updated: tableName, + } + }) + ) + ) + const primary = table.primary + // if passed in array need to copy for shifting etc + let idCopy: undefined | string | any[] = cloneDeep(id) + if (filters) { + // need to map over the filters and make sure the _id field isn't present + let prefix = 1 + for (let operator of Object.values(filters)) { + for (let field of Object.keys(operator || {})) { + if (dbCore.removeKeyNumbering(field) === "_id") { + if (primary) { + const parts = breakRowIdField(operator[field]) + for (let field of primary) { + operator[`${prefix}:${field}`] = parts.shift() + } + prefix++ + } + // make sure this field doesn't exist on any filter + delete operator[field] + } + } + } + } + // there is no id, just use the user provided filters + if (!idCopy || !table) { + return filters + } + // if used as URL parameter it will have been joined + if (!Array.isArray(idCopy)) { + idCopy = breakRowIdField(idCopy) + } + const equal: any = {} + if (primary && idCopy) { + for (let field of primary) { + // work through the ID and get the parts + equal[field] = idCopy.shift() + } + } + return { + equal, + } + } + + private async removeManyToManyRelationships( + 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.DELETE), + body: { [colName]: null }, + filters, + meta: { + table, + }, + }) + } else { + return [] + } + } + + private async removeOneToManyRelationships(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.UPDATE), + filters, + meta: { + table, + }, + }) + } else { + return [] + } } getTable(tableId: string | undefined): Table | undefined { @@ -260,10 +286,22 @@ export class ExternalRequest { } } + // seeds the object with table and datasource information + async retrieveMetadata(datasourceId: string) { + if (!this.datasource) { + this.datasource = await sdk.datasources.get(datasourceId) + if (!this.datasource || !this.datasource.entities) { + throw "No tables found, fetch tables before query." + } + this.tables = this.datasource.entities + this.tableList = Object.values(this.tables) + } + } + async getRow(table: Table, rowId: string): Promise { const response = await getDatasourceAndQuery({ endpoint: getEndpoint(table._id!, Operation.READ), - filters: buildFilters(rowId, {}, table), + filters: this.prepareFilters(rowId, {}, table), meta: { table, }, @@ -514,7 +552,7 @@ export class ExternalRequest { endpoint: getEndpoint(tableId, operation), // if we're doing many relationships then we're writing, only one response body, - filters: buildFilters(id, {}, linkTable), + filters: this.prepareFilters(id, {}, linkTable), meta: { table: linkTable, }, @@ -538,8 +576,8 @@ export class ExternalRequest { for (let row of rows) { const rowId = generateIdForRow(row, table) const promise: Promise = isMany - ? removeManyToManyRelationships(rowId, table, colName) - : removeOneToManyRelationships(rowId, table) + ? this.removeManyToManyRelationships(rowId, table, colName) + : this.removeOneToManyRelationships(rowId, table) if (promise) { promises.push(promise) } @@ -562,12 +600,12 @@ export class ExternalRequest { rows.map(row => { const rowId = generateIdForRow(row, table) return isMany - ? removeManyToManyRelationships( + ? this.removeManyToManyRelationships( rowId, table, relationshipColumn.fieldName ) - : removeOneToManyRelationships(rowId, table) + : this.removeOneToManyRelationships(rowId, table) }) ) } @@ -580,14 +618,10 @@ export class ExternalRequest { throw "Unable to run without a table name" } if (!this.datasource) { - this.datasource = await sdk.datasources.get(datasourceId!) - if (!this.datasource || !this.datasource.entities) { - throw "No tables found, fetch tables before query." - } - this.tables = this.datasource.entities + await this.retrieveMetadata(datasourceId!) } const table = this.tables[tableName] - let isSql = isSQL(this.datasource) + let isSql = isSQL(this.datasource!) if (!table) { throw `Unable to process query, table "${tableName}" not defined.` } @@ -612,7 +646,7 @@ export class ExternalRequest { break } } - filters = buildFilters(id, filters || {}, table) + filters = this.prepareFilters(id, filters || {}, table) const relationships = buildExternalRelationships(table, this.tables) const incRelationships = @@ -660,7 +694,11 @@ export class ExternalRequest { body: row || rows, // pass an id filter into extra, purely for mysql/returning extra: { - idFilter: buildFilters(id || generateIdForRow(row, table), {}, table), + idFilter: this.prepareFilters( + id || generateIdForRow(row, table), + {}, + table + ), }, meta: { table, From 6812c2107696c05d35e87367088f3639f71c952a Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 21 Jun 2024 16:58:40 +0100 Subject: [PATCH 05/22] Updating test cases. --- .../src/api/routes/tests/search.spec.ts | 71 +++++++++++-------- 1 file changed, 41 insertions(+), 30 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index cff966ab89..c92738479f 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -76,9 +76,9 @@ describe.each([ } }) - async function createTable(schema: TableSchema) { + async function createTable(schema: TableSchema, name?: string) { return await config.api.table.save( - tableForDatasource(datasource, { schema }) + tableForDatasource(datasource, { schema, name }) ) } @@ -1956,51 +1956,62 @@ describe.each([ // isn't available. !isInMemory && describe("relations", () => { - let otherTable: Table - let otherRows: Row[] + let productCategoryTable: Table, productCatRows: Row[] beforeAll(async () => { - otherTable = await createTable({ - one: { name: "one", type: FieldType.STRING }, - }) - table = await createTable({ - two: { name: "two", type: FieldType.STRING }, - other: { - type: FieldType.LINK, - relationshipType: RelationshipType.ONE_TO_MANY, - name: "other", - fieldName: "other", - tableId: otherTable._id!, - constraints: { - type: "array", + productCategoryTable = await createTable( + { + name: { name: "name", type: FieldType.STRING }, + }, + "productCategory" + ) + table = await createTable( + { + name: { name: "name", type: FieldType.STRING }, + productCat: { + type: FieldType.LINK, + relationshipType: RelationshipType.ONE_TO_MANY, + name: "productCat", + fieldName: "product", + tableId: productCategoryTable._id!, + constraints: { + type: "array", + }, }, }, - }) + "product" + ) - otherRows = await Promise.all([ - config.api.row.save(otherTable._id!, { one: "foo" }), - config.api.row.save(otherTable._id!, { one: "bar" }), + productCatRows = await Promise.all([ + config.api.row.save(productCategoryTable._id!, { name: "foo" }), + config.api.row.save(productCategoryTable._id!, { name: "bar" }), ]) await Promise.all([ config.api.row.save(table._id!, { - two: "foo", - other: [otherRows[0]._id], + name: "foo", + productCat: [productCatRows[0]._id], }), config.api.row.save(table._id!, { - two: "bar", - other: [otherRows[1]._id], + name: "bar", + productCat: [productCatRows[1]._id], }), ]) - - rows = await config.api.row.fetch(table._id!) }) - it("can search through relations", async () => { + it("should be able to filter by relationship using column name", async () => { await expectQuery({ - equal: { [`${otherTable.name}.one`]: "foo" }, + equal: { ["productCat.name"]: "foo" }, }).toContainExactly([ - { two: "foo", other: [{ _id: otherRows[0]._id }] }, + { name: "foo", productCat: [{ _id: productCatRows[0]._id }] }, + ]) + }) + + it("should be able to filter by relationship using table name", async () => { + await expectQuery({ + equal: { ["productCategory.name"]: "foo" }, + }).toContainExactly([ + { name: "foo", productCat: [{ _id: productCatRows[0]._id }] }, ]) }) }) From 28d0d627ce971e029018f2b77e9abaae2bdbccf3 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 21 Jun 2024 17:00:12 +0100 Subject: [PATCH 06/22] Getting functions in place which make it easy to update pats of a filter list by their keys - getting this to work for SQS and external. --- packages/server/src/sdk/app/rows/index.ts | 2 + .../server/src/sdk/app/rows/search/filters.ts | 56 +++++++++++++++++ .../server/src/sdk/app/rows/search/sqs.ts | 60 ++++++++++--------- 3 files changed, 89 insertions(+), 29 deletions(-) create mode 100644 packages/server/src/sdk/app/rows/search/filters.ts diff --git a/packages/server/src/sdk/app/rows/index.ts b/packages/server/src/sdk/app/rows/index.ts index c117941419..fb077509a9 100644 --- a/packages/server/src/sdk/app/rows/index.ts +++ b/packages/server/src/sdk/app/rows/index.ts @@ -3,12 +3,14 @@ import * as rows from "./rows" import * as search from "./search" import * as utils from "./utils" import * as external from "./external" +import * as filters from "./search/filters" import AliasTables from "./sqlAlias" export default { ...attachments, ...rows, ...search, + filters, utils, external, AliasTables, diff --git a/packages/server/src/sdk/app/rows/search/filters.ts b/packages/server/src/sdk/app/rows/search/filters.ts new file mode 100644 index 0000000000..3f8facc78e --- /dev/null +++ b/packages/server/src/sdk/app/rows/search/filters.ts @@ -0,0 +1,56 @@ +import { + FieldType, + RelationshipFieldMetadata, + SearchFilters, + Table, +} from "@budibase/types" + +export function getRelationshipColumns(table: Table): { + name: string + definition: RelationshipFieldMetadata +}[] { + return Object.entries(table.schema) + .filter(entry => entry[1].type === FieldType.LINK) + .map(entry => ({ + name: entry[0], + definition: entry[1] as RelationshipFieldMetadata, + })) +} + +export function getTableIDList( + tables: Table[] +): { name: string; id: string }[] { + return tables + .filter(table => table.originalName) + .map(table => ({ id: table._id!, name: table.originalName! })) +} + +export function updateFilterKeys( + filters: SearchFilters, + updates: { original: string; updated: string }[] +): SearchFilters { + // sort the updates by length first - this is necessary to avoid replacing sub-strings + updates = updates.sort((a, b) => b.original.length - a.original.length) + const makeFilterKeyRegex = (str: string) => + new RegExp(`^${str}.|:${str}.`, "g") + for (let filter of Object.values(filters)) { + if (typeof filter !== "object") { + continue + } + for (let [key, keyFilter] of Object.entries(filter)) { + if (keyFilter === "") { + delete filter[key] + } + const possibleKey = updates.find(({ original }) => + key.match(makeFilterKeyRegex(original)) + ) + if (possibleKey && possibleKey.original !== possibleKey.updated) { + // only replace the first, not replaceAll + filter[key.replace(possibleKey.original, possibleKey.updated)] = + filter[key] + delete filter[key] + } + } + } + return filters +} diff --git a/packages/server/src/sdk/app/rows/search/sqs.ts b/packages/server/src/sdk/app/rows/search/sqs.ts index bb1d62affc..174ecc0e38 100644 --- a/packages/server/src/sdk/app/rows/search/sqs.ts +++ b/packages/server/src/sdk/app/rows/search/sqs.ts @@ -1,4 +1,5 @@ import { + Datasource, DocumentType, FieldType, Operation, @@ -12,7 +13,6 @@ import { SortType, SqlClient, Table, - Datasource, } from "@budibase/types" import { buildInternalRelationships, @@ -30,6 +30,11 @@ import AliasTables from "../sqlAlias" import { outputProcessing } from "../../../../utilities/rowProcessor" import pick from "lodash/pick" import { processRowCountResponse } from "../utils" +import { + updateFilterKeys, + getRelationshipColumns, + getTableIDList, +} from "./filters" const builder = new sql.Sql(SqlClient.SQL_LITE) @@ -60,34 +65,31 @@ function buildInternalFieldList( return fieldList } -function tableNameInFieldRegex(tableName: string) { - return new RegExp(`^${tableName}.|:${tableName}.`, "g") -} - -function cleanupFilters(filters: SearchFilters, tables: Table[]) { - for (let filter of Object.values(filters)) { - if (typeof filter !== "object") { - continue - } - for (let [key, keyFilter] of Object.entries(filter)) { - if (keyFilter === "") { - delete filter[key] - } - - // relationship, switch to table ID - const tableRelated = tables.find( - table => - table.originalName && - key.match(tableNameInFieldRegex(table.originalName)) +function cleanupFilters( + filters: SearchFilters, + table: Table, + allTables: Table[] +) { + // get a list of all relationship columns in the table for updating + const relationshipColumns = getRelationshipColumns(table) + // get table names to ID map for relationships + const tableNameToID = getTableIDList(allTables) + // all should be applied at once + filters = updateFilterKeys( + filters, + relationshipColumns + .map(({ name, definition }) => ({ + original: name, + updated: definition.tableId, + })) + .concat( + tableNameToID.map(({ name, id }) => ({ + original: name, + updated: id, + })) ) - if (tableRelated && tableRelated.originalName) { - // only replace the first, not replaceAll - filter[key.replace(tableRelated.originalName, tableRelated._id!)] = - filter[key] - delete filter[key] - } - } - } + ) + return filters } @@ -176,7 +178,7 @@ export async function search( operation: Operation.READ, }, filters: { - ...cleanupFilters(query, allTables), + ...cleanupFilters(query, table, allTables), documentType: DocumentType.ROW, }, table, From 337584f5b2d4f9d53c6c8ee2c22177ce54cb3c99 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 21 Jun 2024 17:51:02 +0100 Subject: [PATCH 07/22] Updating the regex to correctly find within the filter keys. --- packages/server/src/sdk/app/rows/search/filters.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/server/src/sdk/app/rows/search/filters.ts b/packages/server/src/sdk/app/rows/search/filters.ts index 3f8facc78e..32b8526697 100644 --- a/packages/server/src/sdk/app/rows/search/filters.ts +++ b/packages/server/src/sdk/app/rows/search/filters.ts @@ -29,10 +29,8 @@ export function updateFilterKeys( filters: SearchFilters, updates: { original: string; updated: string }[] ): SearchFilters { - // sort the updates by length first - this is necessary to avoid replacing sub-strings - updates = updates.sort((a, b) => b.original.length - a.original.length) const makeFilterKeyRegex = (str: string) => - new RegExp(`^${str}.|:${str}.`, "g") + new RegExp(`^${str}\\.|:${str}\\.`) for (let filter of Object.values(filters)) { if (typeof filter !== "object") { continue From fcf67f729743db26647a15bb7900c101afaf2f3c Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 21 Jun 2024 19:29:30 +0100 Subject: [PATCH 08/22] Fixing an issue raised by Poirazis around empty relationships coming back as related to themselves. --- .../server/src/api/controllers/row/utils/basic.ts | 2 +- packages/server/src/api/routes/tests/search.spec.ts | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/server/src/api/controllers/row/utils/basic.ts b/packages/server/src/api/controllers/row/utils/basic.ts index afb98d0255..bca2494ac3 100644 --- a/packages/server/src/api/controllers/row/utils/basic.ts +++ b/packages/server/src/api/controllers/row/utils/basic.ts @@ -99,7 +99,7 @@ export function basicProcessing({ row, tableName: table._id!, fieldName: internalColumn, - isLinked: false, + isLinked, }) } } diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index c92738479f..b9c25b98aa 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -1950,10 +1950,7 @@ describe.each([ }) }) - // This will never work for Lucene. !isLucene && - // It also can't work for in-memory searching because the related table name - // isn't available. !isInMemory && describe("relations", () => { let productCategoryTable: Table, productCatRows: Row[] @@ -1996,6 +1993,10 @@ describe.each([ name: "bar", productCat: [productCatRows[1]._id], }), + config.api.row.save(table._id!, { + name: "baz", + productCat: [], + }), ]) }) @@ -2014,6 +2015,12 @@ describe.each([ { name: "foo", productCat: [{ _id: productCatRows[0]._id }] }, ]) }) + + it("shouldn't return any relationship for last row", async () => { + await expectQuery({ + equal: { ["name"]: "baz" }, + }).toContainExactly([{ name: "baz", productCat: undefined }]) + }) }) // lucene can't count the total rows From 05ea231d203323b769474c6d3617ef2fef39e7eb Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 24 Jun 2024 11:53:02 +0100 Subject: [PATCH 09/22] Adding back missing comments. --- packages/server/src/api/routes/tests/search.spec.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index b9c25b98aa..8ff35e7232 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -1950,8 +1950,10 @@ describe.each([ }) }) + // This will never work for Lucene. !isLucene && - !isInMemory && + // It also can't work for in-memory searching because the related table name + // isn't available. describe("relations", () => { let productCategoryTable: Table, productCatRows: Row[] From 965725d022b360fd8b5e1b80d6b269514ec7be23 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 24 Jun 2024 12:43:26 +0100 Subject: [PATCH 10/22] First PR comments. --- .../api/controllers/row/ExternalRequest.ts | 25 ++++++------------- .../src/api/routes/tests/search.spec.ts | 1 + .../server/src/sdk/app/rows/search/filters.ts | 24 ++++++++++++------ 3 files changed, 24 insertions(+), 26 deletions(-) diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 3209416544..75d3da6b03 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -176,24 +176,13 @@ export class ExternalRequest { const relationshipColumns = sdk.rows.filters.getRelationshipColumns(table) filters = sdk.rows.filters.updateFilterKeys( filters, - relationshipColumns - .map(({ name, definition }) => { - const { tableName } = breakExternalTableId(definition.tableId) - return { - original: name, - updated: tableName!, - } - }) - // don't update table names - include this for context incase a column would be replaced - .concat( - tables.map(table => { - const tableName = table.originalName || table.name - return { - original: tableName, - updated: tableName, - } - }) - ) + relationshipColumns.map(({ name, definition }) => { + const { tableName } = breakExternalTableId(definition.tableId) + return { + original: name, + updated: tableName!, + } + }) ) const primary = table.primary // if passed in array need to copy for shifting etc diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 8ff35e7232..23c2ca819c 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -1954,6 +1954,7 @@ describe.each([ !isLucene && // It also can't work for in-memory searching because the related table name // isn't available. + !isInMemory && describe("relations", () => { let productCategoryTable: Table, productCatRows: Row[] diff --git a/packages/server/src/sdk/app/rows/search/filters.ts b/packages/server/src/sdk/app/rows/search/filters.ts index 32b8526697..ccce0ab86a 100644 --- a/packages/server/src/sdk/app/rows/search/filters.ts +++ b/packages/server/src/sdk/app/rows/search/filters.ts @@ -4,24 +4,32 @@ import { SearchFilters, Table, } from "@budibase/types" +import { isPlainObject } from "lodash" export function getRelationshipColumns(table: Table): { name: string definition: RelationshipFieldMetadata }[] { - return Object.entries(table.schema) - .filter(entry => entry[1].type === FieldType.LINK) - .map(entry => ({ - name: entry[0], - definition: entry[1] as RelationshipFieldMetadata, - })) + // performing this with a for loop rather than an array filter improves + // type guarding, as no casts are required + const linkEntries: [string, RelationshipFieldMetadata][] = [] + for (let entry of Object.entries(table.schema)) { + if (entry[1].type === FieldType.LINK) { + const linkColumn: RelationshipFieldMetadata = entry[1] + linkEntries.push([entry[0], linkColumn]) + } + } + return linkEntries.map(entry => ({ + name: entry[0], + definition: entry[1], + })) } export function getTableIDList( tables: Table[] ): { name: string; id: string }[] { return tables - .filter(table => table.originalName) + .filter(table => table.originalName && table._id) .map(table => ({ id: table._id!, name: table.originalName! })) } @@ -32,7 +40,7 @@ export function updateFilterKeys( const makeFilterKeyRegex = (str: string) => new RegExp(`^${str}\\.|:${str}\\.`) for (let filter of Object.values(filters)) { - if (typeof filter !== "object") { + if (!isPlainObject(filter)) { continue } for (let [key, keyFilter] of Object.entries(filter)) { From 1402716f5cd9c37a5dc84830000822c083be4e4a Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 24 Jun 2024 13:10:30 +0100 Subject: [PATCH 11/22] Some type updates. --- packages/backend-core/src/sql/sqlTable.ts | 6 ++-- packages/backend-core/src/sql/utils.ts | 8 ++--- .../api/controllers/row/ExternalRequest.ts | 31 ++++++++----------- .../src/api/controllers/row/external.ts | 7 ++--- .../src/api/controllers/row/utils/sqlUtils.ts | 26 +++++++++------- .../src/api/controllers/table/external.ts | 4 +-- .../src/sdk/app/rows/search/external.ts | 10 +++--- packages/server/src/sdk/app/tables/getters.ts | 6 ++-- packages/server/src/sdk/app/views/external.ts | 24 +++++++------- 9 files changed, 60 insertions(+), 62 deletions(-) diff --git a/packages/backend-core/src/sql/sqlTable.ts b/packages/backend-core/src/sql/sqlTable.ts index 09f9908baa..bdc8a3dd69 100644 --- a/packages/backend-core/src/sql/sqlTable.ts +++ b/packages/backend-core/src/sql/sqlTable.ts @@ -109,8 +109,10 @@ function generateSchema( const { tableName } = breakExternalTableId(column.tableId) // @ts-ignore const relatedTable = tables[tableName] - if (!relatedTable) { - throw new Error("Referenced table doesn't exist") + if (!relatedTable || !relatedTable.primary) { + throw new Error( + "Referenced table doesn't exist or has no primary keys" + ) } const relatedPrimary = relatedTable.primary[0] const externalType = relatedTable.schema[relatedPrimary].externalType diff --git a/packages/backend-core/src/sql/utils.ts b/packages/backend-core/src/sql/utils.ts index 2d9b289417..45ab510948 100644 --- a/packages/backend-core/src/sql/utils.ts +++ b/packages/backend-core/src/sql/utils.ts @@ -55,10 +55,7 @@ export function buildExternalTableId(datasourceId: string, tableName: string) { return `${datasourceId}${DOUBLE_SEPARATOR}${tableName}` } -export function breakExternalTableId(tableId: string | undefined) { - if (!tableId) { - return {} - } +export function breakExternalTableId(tableId: string) { const parts = tableId.split(DOUBLE_SEPARATOR) let datasourceId = parts.shift() // if they need joined @@ -67,6 +64,9 @@ export function breakExternalTableId(tableId: string | undefined) { if (tableName.includes(ENCODED_SPACE)) { tableName = decodeURIComponent(tableName) } + if (!datasourceId || !tableName) { + throw new Error("Unable to get datasource/table name from table ID") + } return { datasourceId, tableName } } diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 75d3da6b03..7cbe023fd2 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -126,8 +126,8 @@ function getEndpoint(tableId: string | undefined, operation: string) { } const { datasourceId, tableName } = breakExternalTableId(tableId) return { - datasourceId: datasourceId!, - entityId: tableName!, + datasourceId: datasourceId, + entityId: tableName, operation: operation as Operation, } } @@ -180,7 +180,7 @@ export class ExternalRequest { const { tableName } = breakExternalTableId(definition.tableId) return { original: name, - updated: tableName!, + updated: tableName, } }) ) @@ -267,12 +267,10 @@ export class ExternalRequest { getTable(tableId: string | undefined): Table | undefined { if (!tableId) { - throw "Table ID is unknown, cannot find table" + throw new Error("Table ID is unknown, cannot find table") } const { tableName } = breakExternalTableId(tableId) - if (tableName) { - return this.tables[tableName] - } + return this.tables[tableName] } // seeds the object with table and datasource information @@ -323,9 +321,7 @@ export class ExternalRequest { if (field.type === FieldType.NUMBER && !isNaN(parseFloat(row[key]))) { newRow[key] = parseFloat(row[key]) } else if (field.type === FieldType.LINK) { - const { tableName: linkTableName } = breakExternalTableId( - field?.tableId - ) + const { tableName: linkTableName } = breakExternalTableId(field.tableId) // table has to exist for many to many if (!linkTableName || !this.tables[linkTableName]) { continue @@ -406,9 +402,6 @@ export class ExternalRequest { [key: string]: { rows: Row[]; isMany: boolean; tableId: string } } = {} const { tableName } = breakExternalTableId(tableId) - if (!tableName) { - return related - } const table = this.tables[tableName] // @ts-ignore const primaryKey = table.primary[0] @@ -602,17 +595,19 @@ export class ExternalRequest { async run(config: RunConfig): Promise> { const { operation, tableId } = this - let { datasourceId, tableName } = breakExternalTableId(tableId) - if (!tableName) { - throw "Unable to run without a table name" + if (!tableId) { + throw new Error("Unable to run without a table ID") } + let { datasourceId, tableName } = breakExternalTableId(tableId) if (!this.datasource) { - await this.retrieveMetadata(datasourceId!) + await this.retrieveMetadata(datasourceId) } const table = this.tables[tableName] let isSql = isSQL(this.datasource!) if (!table) { - throw `Unable to process query, table "${tableName}" not defined.` + throw new Error( + `Unable to process query, table "${tableName}" not defined.` + ) } // look for specific components of config which may not be considered acceptable let { id, row, filters, sort, paginate, rows } = cleanupConfig( diff --git a/packages/server/src/api/controllers/row/external.ts b/packages/server/src/api/controllers/row/external.ts index 5b12b5c207..126b11d0c1 100644 --- a/packages/server/src/api/controllers/row/external.ts +++ b/packages/server/src/api/controllers/row/external.ts @@ -136,10 +136,7 @@ export async function fetchEnrichedRow(ctx: UserCtx) { const id = ctx.params.rowId const tableId = utils.getTableId(ctx) const { datasourceId, tableName } = breakExternalTableId(tableId) - const datasource: Datasource = await sdk.datasources.get(datasourceId!) - if (!tableName) { - ctx.throw(400, "Unable to find table.") - } + const datasource: Datasource = await sdk.datasources.get(datasourceId) if (!datasource || !datasource.entities) { ctx.throw(400, "Datasource has not been configured for plus API.") } @@ -163,7 +160,7 @@ export async function fetchEnrichedRow(ctx: UserCtx) { } const links = row[fieldName] const linkedTableId = field.tableId - const linkedTableName = breakExternalTableId(linkedTableId).tableName! + const linkedTableName = breakExternalTableId(linkedTableId).tableName const linkedTable = tables[linkedTableName] // don't support composite keys right now const linkedIds = links.map((link: Row) => breakRowIdField(link._id!)[0]) diff --git a/packages/server/src/api/controllers/row/utils/sqlUtils.ts b/packages/server/src/api/controllers/row/utils/sqlUtils.ts index 6f7bdc7335..767916616c 100644 --- a/packages/server/src/api/controllers/row/utils/sqlUtils.ts +++ b/packages/server/src/api/controllers/row/utils/sqlUtils.ts @@ -2,6 +2,8 @@ import { DatasourcePlusQueryResponse, DSPlusOperation, FieldType, + isManyToOne, + isOneToMany, ManyToManyRelationshipFieldMetadata, RelationshipFieldMetadata, RelationshipsJson, @@ -93,12 +95,12 @@ export function buildExternalRelationships( ): RelationshipsJson[] { const relationships = [] for (let [fieldName, field] of Object.entries(table.schema)) { - if (field.type !== FieldType.LINK) { + if (field.type !== FieldType.LINK || !field.tableId) { continue } const { tableName: linkTableName } = breakExternalTableId(field.tableId) // no table to link to, this is not a valid relationships - if (!linkTableName || !tables[linkTableName]) { + if (!tables[linkTableName]) { continue } const linkTable = tables[linkTableName] @@ -110,7 +112,7 @@ export function buildExternalRelationships( // need to specify where to put this back into column: fieldName, } - if (isManyToMany(field)) { + if (isManyToMany(field) && field.through) { const { tableName: throughTableName } = breakExternalTableId( field.through ) @@ -120,7 +122,7 @@ export function buildExternalRelationships( definition.to = field.throughFrom || linkTable.primary[0] definition.fromPrimary = table.primary[0] definition.toPrimary = linkTable.primary[0] - } else { + } else if (isManyToOne(field) || isOneToMany(field)) { // if no foreign key specified then use the name of the field in other table definition.from = field.foreignKey || table.primary[0] definition.to = field.fieldName @@ -180,16 +182,18 @@ export function buildSqlFieldList( } let fields = extractRealFields(table) for (let field of Object.values(table.schema)) { - if (field.type !== FieldType.LINK || !opts?.relationships) { + if ( + field.type !== FieldType.LINK || + !opts?.relationships || + !field.tableId + ) { continue } const { tableName: linkTableName } = breakExternalTableId(field.tableId) - if (linkTableName) { - const linkTable = tables[linkTableName] - if (linkTable) { - const linkedFields = extractRealFields(linkTable, fields) - fields = fields.concat(linkedFields) - } + const linkTable = tables[linkTableName] + if (linkTable) { + const linkedFields = extractRealFields(linkTable, fields) + fields = fields.concat(linkedFields) } } return fields diff --git a/packages/server/src/api/controllers/table/external.ts b/packages/server/src/api/controllers/table/external.ts index f1b186c233..fabc4d195a 100644 --- a/packages/server/src/api/controllers/table/external.ts +++ b/packages/server/src/api/controllers/table/external.ts @@ -18,8 +18,8 @@ import { builderSocket } from "../../../websockets" import { inputProcessing } from "../../../utilities/rowProcessor" function getDatasourceId(table: Table) { - if (!table) { - throw "No table supplied" + if (!table || !table._id) { + throw new Error("No table/table ID supplied") } if (table.sourceId) { return table.sourceId diff --git a/packages/server/src/sdk/app/rows/search/external.ts b/packages/server/src/sdk/app/rows/search/external.ts index 9fc3487f62..93c46d8cc3 100644 --- a/packages/server/src/sdk/app/rows/search/external.ts +++ b/packages/server/src/sdk/app/rows/search/external.ts @@ -145,6 +145,10 @@ export async function exportRows( delimiter, customHeaders, } = options + + if (!tableId) { + throw new HTTPError("No table ID for search provided.", 400) + } const { datasourceId, tableName } = breakExternalTableId(tableId) let requestQuery: SearchFilters = {} @@ -167,7 +171,7 @@ export async function exportRows( requestQuery = query || {} } - const datasource = await sdk.datasources.get(datasourceId!) + const datasource = await sdk.datasources.get(datasourceId) const table = await sdk.tables.getTable(tableId) if (!datasource || !datasource.entities) { throw new HTTPError("Datasource has not been configured for plus API.", 400) @@ -180,10 +184,6 @@ export async function exportRows( let rows: Row[] = [] let headers - if (!tableName) { - throw new HTTPError("Could not find table name.", 400) - } - // Filter data to only specified columns if required if (columns && columns.length) { for (let i = 0; i < result.rows.length; i++) { diff --git a/packages/server/src/sdk/app/tables/getters.ts b/packages/server/src/sdk/app/tables/getters.ts index 355493579d..738e57eff8 100644 --- a/packages/server/src/sdk/app/tables/getters.ts +++ b/packages/server/src/sdk/app/tables/getters.ts @@ -90,10 +90,10 @@ export async function getExternalTable( export async function getTable(tableId: string): Promise { const db = context.getAppDB() let output: Table - if (isExternalTableID(tableId)) { + if (tableId && isExternalTableID(tableId)) { let { datasourceId, tableName } = breakExternalTableId(tableId) - const datasource = await datasources.get(datasourceId!) - const table = await getExternalTable(datasourceId!, tableName!) + const datasource = await datasources.get(datasourceId) + const table = await getExternalTable(datasourceId, tableName) output = { ...table, sql: isSQL(datasource) } } else { output = await db.get
(tableId) diff --git a/packages/server/src/sdk/app/views/external.ts b/packages/server/src/sdk/app/views/external.ts index 0f96bcc061..2b3e271597 100644 --- a/packages/server/src/sdk/app/views/external.ts +++ b/packages/server/src/sdk/app/views/external.ts @@ -10,9 +10,9 @@ export async function get(viewId: string): Promise { const { tableId } = utils.extractViewInfoFromID(viewId) const { datasourceId, tableName } = breakExternalTableId(tableId) - const ds = await sdk.datasources.get(datasourceId!) + const ds = await sdk.datasources.get(datasourceId) - const table = ds.entities![tableName!] + const table = ds.entities![tableName] const views = Object.values(table.views!).filter(isV2) const found = views.find(v => v.id === viewId) if (!found) { @@ -25,9 +25,9 @@ export async function getEnriched(viewId: string): Promise { const { tableId } = utils.extractViewInfoFromID(viewId) const { datasourceId, tableName } = breakExternalTableId(tableId) - const ds = await sdk.datasources.get(datasourceId!) + const ds = await sdk.datasources.get(datasourceId) - const table = ds.entities![tableName!] + const table = ds.entities![tableName] const views = Object.values(table.views!).filter(isV2) const found = views.find(v => v.id === viewId) if (!found) { @@ -49,9 +49,9 @@ export async function create( const db = context.getAppDB() const { datasourceId, tableName } = breakExternalTableId(tableId) - const ds = await sdk.datasources.get(datasourceId!) - ds.entities![tableName!].views ??= {} - ds.entities![tableName!].views![view.name] = view + const ds = await sdk.datasources.get(datasourceId) + ds.entities![tableName].views ??= {} + ds.entities![tableName].views![view.name] = view await db.put(ds) return view } @@ -60,9 +60,9 @@ export async function update(tableId: string, view: ViewV2): Promise { const db = context.getAppDB() const { datasourceId, tableName } = breakExternalTableId(tableId) - const ds = await sdk.datasources.get(datasourceId!) - ds.entities![tableName!].views ??= {} - const views = ds.entities![tableName!].views! + const ds = await sdk.datasources.get(datasourceId) + ds.entities![tableName].views ??= {} + const views = ds.entities![tableName].views! const existingView = Object.values(views).find( v => isV2(v) && v.id === view.id @@ -87,9 +87,9 @@ export async function remove(viewId: string): Promise { } const { datasourceId, tableName } = breakExternalTableId(view.tableId) - const ds = await sdk.datasources.get(datasourceId!) + const ds = await sdk.datasources.get(datasourceId) - delete ds.entities![tableName!].views![view?.name] + delete ds.entities![tableName].views![view?.name] await db.put(ds) return view } From 75cee3c4fda301ef9fe6a1b3b5953270c34b48e8 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 24 Jun 2024 13:28:13 +0100 Subject: [PATCH 12/22] Quick type improvement. --- .../src/api/controllers/row/ExternalRequest.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 7cbe023fd2..1ce8d29e5f 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -170,8 +170,7 @@ export class ExternalRequest { id: string | undefined | string[], filters: SearchFilters, table: Table - ) { - const tables = this.tableList + ): SearchFilters { // replace any relationship columns initially, table names and relationship column names are acceptable const relationshipColumns = sdk.rows.filters.getRelationshipColumns(table) filters = sdk.rows.filters.updateFilterKeys( @@ -214,7 +213,7 @@ export class ExternalRequest { if (!Array.isArray(idCopy)) { idCopy = breakRowIdField(idCopy) } - const equal: any = {} + const equal: SearchFilters["equal"] = {} if (primary && idCopy) { for (let field of primary) { // work through the ID and get the parts @@ -274,7 +273,9 @@ export class ExternalRequest { } // seeds the object with table and datasource information - async retrieveMetadata(datasourceId: string) { + async retrieveMetadata( + datasourceId: string + ): Promise<{ tables: Record; datasource: Datasource }> { if (!this.datasource) { this.datasource = await sdk.datasources.get(datasourceId) if (!this.datasource || !this.datasource.entities) { @@ -283,6 +284,7 @@ export class ExternalRequest { this.tables = this.datasource.entities this.tableList = Object.values(this.tables) } + return { tables: this.tables, datasource: this.datasource } } async getRow(table: Table, rowId: string): Promise { @@ -599,11 +601,13 @@ export class ExternalRequest { throw new Error("Unable to run without a table ID") } let { datasourceId, tableName } = breakExternalTableId(tableId) - if (!this.datasource) { - await this.retrieveMetadata(datasourceId) + let datasource = this.datasource + if (!datasource) { + const { datasource: ds } = await this.retrieveMetadata(datasourceId) + datasource = ds } const table = this.tables[tableName] - let isSql = isSQL(this.datasource!) + let isSql = isSQL(datasource) if (!table) { throw new Error( `Unable to process query, table "${tableName}" not defined.` From b597bd3dbec1e8a5f2ba35da348fd7eb21609dae Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 24 Jun 2024 13:30:18 +0100 Subject: [PATCH 13/22] Fixing an issue detected by tests. --- packages/server/src/api/controllers/table/external.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/server/src/api/controllers/table/external.ts b/packages/server/src/api/controllers/table/external.ts index fabc4d195a..6ca8cdd82c 100644 --- a/packages/server/src/api/controllers/table/external.ts +++ b/packages/server/src/api/controllers/table/external.ts @@ -18,12 +18,15 @@ import { builderSocket } from "../../../websockets" import { inputProcessing } from "../../../utilities/rowProcessor" function getDatasourceId(table: Table) { - if (!table || !table._id) { - throw new Error("No table/table ID supplied") + if (!table) { + throw new Error("No table supplied") } if (table.sourceId) { return table.sourceId } + if (!table._id) { + throw new Error("No table ID supplied") + } return breakExternalTableId(table._id).datasourceId } From aefe46b253e02a2a7b456c38088e7a634a06ee93 Mon Sep 17 00:00:00 2001 From: Peter Clement Date: Mon, 24 Jun 2024 14:31:27 +0100 Subject: [PATCH 14/22] Adds _id and _rev back to internal datasource filter options (#13977) * Adds _id and _rev back to internal datasource filter options * add bb default datasource const into shared-core * re-export var from shared-core --- packages/backend-core/src/constants/db.ts | 2 +- .../src/components/FilterBuilder.svelte | 21 ++++++++++++++++--- packages/frontend-core/src/constants.js | 6 +++++- packages/shared-core/src/constants/index.ts | 2 ++ 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/packages/backend-core/src/constants/db.ts b/packages/backend-core/src/constants/db.ts index 2fd713119b..3085b91ef1 100644 --- a/packages/backend-core/src/constants/db.ts +++ b/packages/backend-core/src/constants/db.ts @@ -72,4 +72,4 @@ export const DEFAULT_JOBS_TABLE_ID = "ta_bb_jobs" export const DEFAULT_INVENTORY_TABLE_ID = "ta_bb_inventory" export const DEFAULT_EXPENSES_TABLE_ID = "ta_bb_expenses" export const DEFAULT_EMPLOYEE_TABLE_ID = "ta_bb_employee" -export const DEFAULT_BB_DATASOURCE_ID = "datasource_internal_bb_default" +export { DEFAULT_BB_DATASOURCE_ID } from "@budibase/shared-core" diff --git a/packages/frontend-core/src/components/FilterBuilder.svelte b/packages/frontend-core/src/components/FilterBuilder.svelte index 0d254186f2..6d1e1fa502 100644 --- a/packages/frontend-core/src/components/FilterBuilder.svelte +++ b/packages/frontend-core/src/components/FilterBuilder.svelte @@ -18,7 +18,7 @@ import FilterUsers from "./FilterUsers.svelte" import { getFields } from "../utils/searchFields" - const { OperatorOptions } = Constants + const { OperatorOptions, DEFAULT_BB_DATASOURCE_ID } = Constants export let schemaFields export let filters = [] @@ -28,6 +28,23 @@ export let allowBindings = false export let filtersLabel = "Filters" + $: { + if ( + tables.find( + table => + table._id === datasource.tableId && + table.sourceId === DEFAULT_BB_DATASOURCE_ID + ) && + !schemaFields.some(field => field.name === "_id") + ) { + schemaFields = [ + ...schemaFields, + { name: "_id", type: "string" }, + { name: "_rev", type: "string" }, + ] + } + } + $: matchAny = filters?.find(filter => filter.operator === "allOr") != null $: onEmptyFilter = filters?.find(filter => filter.onEmptyFilter)?.onEmptyFilter ?? "all" @@ -35,7 +52,6 @@ $: fieldFilters = filters.filter( filter => filter.operator !== "allOr" && !filter.onEmptyFilter ) - const behaviourOptions = [ { value: "and", label: "Match all filters" }, { value: "or", label: "Match any filter" }, @@ -44,7 +60,6 @@ { value: "all", label: "Return all table rows" }, { value: "none", label: "Return no rows" }, ] - const context = getContext("context") $: fieldOptions = getFields(tables, schemaFields || [], { diff --git a/packages/frontend-core/src/constants.js b/packages/frontend-core/src/constants.js index 0d6261f5f8..e5869a3b98 100644 --- a/packages/frontend-core/src/constants.js +++ b/packages/frontend-core/src/constants.js @@ -1,7 +1,11 @@ /** * Operator options for lucene queries */ -export { OperatorOptions, SqlNumberTypeRangeMap } from "@budibase/shared-core" +export { + OperatorOptions, + SqlNumberTypeRangeMap, + DEFAULT_BB_DATASOURCE_ID, +} from "@budibase/shared-core" export { Feature as Features } from "@budibase/types" import { BpmCorrelationKey } from "@budibase/shared-core" import { FieldType, BBReferenceFieldSubType } from "@budibase/types" diff --git a/packages/shared-core/src/constants/index.ts b/packages/shared-core/src/constants/index.ts index 084bc8fe9e..c9d1a8fc8f 100644 --- a/packages/shared-core/src/constants/index.ts +++ b/packages/shared-core/src/constants/index.ts @@ -180,3 +180,5 @@ export enum BpmStatusValue { VERIFYING_EMAIL = "verifying_email", COMPLETED = "completed", } + +export const DEFAULT_BB_DATASOURCE_ID = "datasource_internal_bb_default" From 2458259093b37fc2c4bd4809c57694d5a58d99a0 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 24 Jun 2024 15:50:09 +0100 Subject: [PATCH 15/22] Respond to PR feedback. --- .../ExistingTableDataImport.svelte | 25 +++++++++++++------ .../integrations/mssql/data/Dockerfile | 2 +- .../src/api/controllers/table/external.ts | 4 +-- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/packages/builder/src/components/backend/TableNavigator/ExistingTableDataImport.svelte b/packages/builder/src/components/backend/TableNavigator/ExistingTableDataImport.svelte index cd6b5aeb2e..80655d1099 100644 --- a/packages/builder/src/components/backend/TableNavigator/ExistingTableDataImport.svelte +++ b/packages/builder/src/components/backend/TableNavigator/ExistingTableDataImport.svelte @@ -1,9 +1,14 @@