From 62fa05a855205f43b0ef929fc051cd91b0fd8f58 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 13:28:28 +0200 Subject: [PATCH 01/21] Type --- packages/server/src/sdk/app/rows/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/sdk/app/rows/utils.ts b/packages/server/src/sdk/app/rows/utils.ts index cd1b663f6a..e463397ad9 100644 --- a/packages/server/src/sdk/app/rows/utils.ts +++ b/packages/server/src/sdk/app/rows/utils.ts @@ -76,7 +76,7 @@ export async function getDatasourceAndQuery( } export function cleanExportRows( - rows: any[], + rows: Row[], schema: TableSchema, format: string, columns?: string[], From fe2b2bb097fb4ee106754630d2419f0f6a8a3cd2 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 13:33:20 +0200 Subject: [PATCH 02/21] Don't export couchdb fields --- .../server/src/sdk/app/rows/search/internal.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/server/src/sdk/app/rows/search/internal.ts b/packages/server/src/sdk/app/rows/search/internal.ts index 46d2cd8c61..f86b041597 100644 --- a/packages/server/src/sdk/app/rows/search/internal.ts +++ b/packages/server/src/sdk/app/rows/search/internal.ts @@ -11,6 +11,7 @@ import { SearchResponse, SortType, Table, + TableSchema, User, } from "@budibase/types" import { getGlobalUsersFromMetadata } from "../../../../utilities/global" @@ -137,6 +138,9 @@ export async function exportRows( let rows: Row[] = [] let schema = table.schema let headers + + result = trimFields(result, schema) + // Filter data to only specified columns if required if (columns && columns.length) { for (let i = 0; i < result.length; i++) { @@ -299,3 +303,13 @@ async function getView(db: Database, viewName: string) { } return viewInfo } + +function trimFields(rows: Row[], schema: TableSchema) { + const allowedFields = ["_id", ...Object.keys(schema)] + const result = rows.map(row => + Object.keys(row) + .filter(key => allowedFields.includes(key)) + .reduce((acc, key) => ({ ...acc, [key]: row[key] }), {} as Row) + ) + return result +} From 543d0e1ce619b7fbaeeb994c0d33c325b2d934eb Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 14:01:38 +0200 Subject: [PATCH 03/21] Add tests --- .../server/src/api/routes/tests/row.spec.ts | 45 ++++++++++++------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 8871841ee7..96a157893f 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1640,23 +1640,38 @@ describe.each([ table = await config.api.table.save(defaultTable()) }) - it("should allow exporting all columns", async () => { - const existing = await config.api.row.save(table._id!, {}) - const res = await config.api.row.exportRows(table._id!, { - rows: [existing._id!], - }) - const results = JSON.parse(res) - expect(results.length).toEqual(1) - const row = results[0] + isInternal && + it("should not export internal couchdb fields", async () => { + const existing = await config.api.row.save(table._id!, { + name: generator.guid(), + description: generator.paragraph(), + }) + const res = await config.api.row.exportRows(table._id!, { + rows: [existing._id!], + }) + const results = JSON.parse(res) + expect(results.length).toEqual(1) + const row = results[0] - // Ensure all original columns were exported - expect(Object.keys(row).length).toBeGreaterThanOrEqual( - Object.keys(existing).length - ) - Object.keys(existing).forEach(key => { - expect(row[key]).toEqual(existing[key]) + expect(Object.keys(row)).toEqual(["_id", "name", "description"]) + }) + + !isInternal && + it("should allow exporting all columns", async () => { + const existing = await config.api.row.save(table._id!, {}) + const res = await config.api.row.exportRows(table._id!, { + rows: [existing._id!], + }) + const results = JSON.parse(res) + expect(results.length).toEqual(1) + const row = results[0] + + // Ensure all original columns were exported + expect(Object.keys(row).length).toBe(Object.keys(existing).length) + Object.keys(existing).forEach(key => { + expect(row[key]).toEqual(existing[key]) + }) }) - }) it("should allow exporting only certain columns", async () => { const existing = await config.api.row.save(table._id!, {}) From 4f65306c4fdf71deb90fe9283585c446a7b6c318 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 25 Jul 2024 17:20:10 +0200 Subject: [PATCH 04/21] Add basic validateNewTableImport test --- .../server/src/api/routes/tests/table.spec.ts | 33 +++++++++++++++++++ .../server/src/tests/utilities/api/table.ts | 22 ++++++++++++- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index a2cff7b395..b32983b8ad 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -10,6 +10,7 @@ import { Row, SaveTableRequest, Table, + TableSchema, TableSourceType, User, ViewCalculation, @@ -1022,4 +1023,36 @@ describe.each([ }) }) }) + + describe("import validation", () => { + const basicSchema: TableSchema = { + id: { + type: FieldType.NUMBER, + name: "id", + }, + name: { + type: FieldType.STRING, + name: "name", + }, + } + + describe("validateNewTableImport", () => { + it("can validate basic imports", async () => { + const result = await config.api.table.validateNewTableImport( + [{ id: generator.natural(), name: generator.first() }], + basicSchema + ) + + expect(result).toEqual({ + allValid: true, + errors: {}, + invalidColumns: [], + schemaValidation: { + id: true, + name: true, + }, + }) + }) + }) + }) }) diff --git a/packages/server/src/tests/utilities/api/table.ts b/packages/server/src/tests/utilities/api/table.ts index d918ba8b9a..c42247dc59 100644 --- a/packages/server/src/tests/utilities/api/table.ts +++ b/packages/server/src/tests/utilities/api/table.ts @@ -3,9 +3,12 @@ import { BulkImportResponse, MigrateRequest, MigrateResponse, + Row, SaveTableRequest, SaveTableResponse, Table, + TableSchema, + ValidateTableImportResponse, } from "@budibase/types" import { Expectations, TestAPI } from "./base" @@ -61,8 +64,25 @@ export class TableAPI extends TestAPI { revId: string, expectations?: Expectations ): Promise => { - return await this._delete(`/api/tables/${tableId}/${revId}`, { + return await this._delete(`/api/tables/${tableId}/${revId}`, { expectations, }) } + + validateNewTableImport = async ( + rows: Row[], + schema: TableSchema, + expectations?: Expectations + ): Promise => { + return await this._post( + `/api/tables/validateNewTableImport`, + { + body: { + rows, + schema, + }, + expectations, + } + ) + } } From 5896e94e56992964064e5cd43d19b7f7a667c608 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 25 Jul 2024 17:32:53 +0200 Subject: [PATCH 05/21] Add basic validateExistingTableImport test --- .../server/src/api/routes/tests/table.spec.ts | 25 +++++++++++++++++++ .../server/src/tests/utilities/api/table.ts | 14 +++++++++++ 2 files changed, 39 insertions(+) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index b32983b8ad..67b1d64ae1 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1054,5 +1054,30 @@ describe.each([ }) }) }) + + describe("validateExistingTableImport", () => { + it("can validate basic imports", async () => { + const table = await config.api.table.save( + tableForDatasource(datasource, { + primary: ["id"], + schema: basicSchema, + }) + ) + const result = await config.api.table.validateExistingTableImport({ + tableId: table._id, + rows: [{ id: generator.natural(), name: generator.first() }], + }) + + expect(result).toEqual({ + allValid: true, + errors: {}, + invalidColumns: [], + schemaValidation: { + id: true, + name: true, + }, + }) + }) + }) }) }) diff --git a/packages/server/src/tests/utilities/api/table.ts b/packages/server/src/tests/utilities/api/table.ts index c42247dc59..9d4a92250a 100644 --- a/packages/server/src/tests/utilities/api/table.ts +++ b/packages/server/src/tests/utilities/api/table.ts @@ -8,6 +8,7 @@ import { SaveTableResponse, Table, TableSchema, + ValidateTableImportRequest, ValidateTableImportResponse, } from "@budibase/types" import { Expectations, TestAPI } from "./base" @@ -85,4 +86,17 @@ export class TableAPI extends TestAPI { } ) } + + validateExistingTableImport = async ( + body: ValidateTableImportRequest, + expectations?: Expectations + ): Promise => { + return await this._post( + `/api/tables/validateExistingTableImport`, + { + body, + expectations, + } + ) + } } From 9d0fdeff68b19d0551498cbc72d2cf53568476bd Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 14:17:51 +0200 Subject: [PATCH 06/21] Add validateExistingTableImport _id support test --- .../server/src/api/routes/tests/table.spec.ts | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 67b1d64ae1..dd40a2420e 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1,4 +1,4 @@ -import { context, events } from "@budibase/backend-core" +import { context, docIds, events } from "@budibase/backend-core" import { AutoFieldSubType, BBReferenceFieldSubType, @@ -1078,6 +1078,37 @@ describe.each([ }, }) }) + + isInternal && + it("can reimport _id fields for internal tables", async () => { + const table = await config.api.table.save( + tableForDatasource(datasource, { + primary: ["id"], + schema: basicSchema, + }) + ) + const result = await config.api.table.validateExistingTableImport({ + tableId: table._id, + rows: [ + { + _id: docIds.generateRowID(table._id!), + id: generator.natural(), + name: generator.first(), + }, + ], + }) + + expect(result).toEqual({ + allValid: true, + errors: {}, + invalidColumns: [], + schemaValidation: { + _id: true, + id: true, + name: true, + }, + }) + }) }) }) }) From b28aaa3a936dfc27c9263efc83f6c6e169ad5d3f Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 14:22:54 +0200 Subject: [PATCH 07/21] Fix --- packages/server/src/api/controllers/table/index.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/server/src/api/controllers/table/index.ts b/packages/server/src/api/controllers/table/index.ts index ba861064bb..a02ecc665e 100644 --- a/packages/server/src/api/controllers/table/index.ts +++ b/packages/server/src/api/controllers/table/index.ts @@ -17,6 +17,7 @@ import { CsvToJsonRequest, CsvToJsonResponse, FetchTablesResponse, + FieldType, MigrateRequest, MigrateResponse, SaveTableRequest, @@ -178,9 +179,17 @@ export async function validateExistingTableImport( const { rows, tableId } = ctx.request.body let schema = null + if (tableId) { const table = await sdk.tables.getTable(tableId) schema = table.schema + + if (!isExternalTable(table)) { + schema._id = { + name: "_id", + type: FieldType.STRING, + } + } } else { ctx.status = 422 return From 24cdfb3443b28ab34f80ca9a7e6ed0db119a3fc0 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 14:55:25 +0200 Subject: [PATCH 08/21] Fix re-importing --- .../src/api/controllers/table/internal.ts | 23 +++++++-- .../server/src/api/controllers/table/utils.ts | 9 ++-- .../server/src/api/routes/tests/row.spec.ts | 50 +++++++++++++++++++ 3 files changed, 73 insertions(+), 9 deletions(-) diff --git a/packages/server/src/api/controllers/table/internal.ts b/packages/server/src/api/controllers/table/internal.ts index 4286d51d3e..6d1c67e800 100644 --- a/packages/server/src/api/controllers/table/internal.ts +++ b/packages/server/src/api/controllers/table/internal.ts @@ -3,6 +3,7 @@ import { handleDataImport } from "./utils" import { BulkImportRequest, BulkImportResponse, + FieldType, RenameColumn, SaveTableRequest, SaveTableResponse, @@ -69,10 +70,22 @@ export async function bulkImport( ) { const table = await sdk.tables.getTable(ctx.params.tableId) const { rows, identifierFields } = ctx.request.body - await handleDataImport(table, { - importRows: rows, - identifierFields, - user: ctx.user, - }) + await handleDataImport( + { + ...table, + schema: { + _id: { + name: "_id", + type: FieldType.STRING, + }, + ...table.schema, + }, + }, + { + importRows: rows, + identifierFields, + user: ctx.user, + } + ) return table } diff --git a/packages/server/src/api/controllers/table/utils.ts b/packages/server/src/api/controllers/table/utils.ts index e2036c8115..51f7b0e589 100644 --- a/packages/server/src/api/controllers/table/utils.ts +++ b/packages/server/src/api/controllers/table/utils.ts @@ -124,11 +124,12 @@ export async function importToRows( table: Table, user?: ContextUser ) { - let originalTable = table - let finalData: any = [] + const originalTable = table + const finalData: Row[] = [] + const keepCouchId = "_id" in table.schema for (let i = 0; i < data.length; i++) { let row = data[i] - row._id = generateRowID(table._id!) + row._id = (keepCouchId && row._id) || generateRowID(table._id!) row.type = "row" row.tableId = table._id @@ -180,7 +181,7 @@ export async function handleDataImport( const db = context.getAppDB() const data = parse(importRows, table) - let finalData: any = await importToRows(data, table, user) + const finalData = await importToRows(data, table, user) //Set IDs of finalData to match existing row if an update is expected if (identifierFields.length > 0) { diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 96a157893f..b448d46e6a 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1298,6 +1298,56 @@ describe.each([ await assertRowUsage(isInternal ? rowUsage + 2 : rowUsage) }) + isInternal && + it("should be able to update existing rows on bulkImport", async () => { + const table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + type: FieldType.STRING, + name: "name", + }, + description: { + type: FieldType.STRING, + name: "description", + }, + }, + }) + ) + + const existingRow = await config.api.row.save(table._id!, { + name: "Existing row", + description: "Existing description", + }) + + + await config.api.row.bulkImport(table._id!, { + rows: [ + { + name: "Row 1", + description: "Row 1 description", + }, + { ...existingRow, name: "Updated existing row" }, + { + name: "Row 2", + description: "Row 2 description", + }, + ], + identifierFields: ["_id"], + }) + + 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") + expect(rows[0].description).toEqual("Row 1 description") + expect(rows[1].name).toEqual("Row 2") + expect(rows[1].description).toEqual("Row 2 description") + expect(rows[2].name).toEqual("Updated existing row") + expect(rows[2].description).toEqual("Existing description") + }) + // Upserting isn't yet supported in MSSQL, see: // https://github.com/knex/knex/pull/6050 !isMSSQL && From f794f84e9035d92817259d63de39f178d3c94b04 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 15:00:30 +0200 Subject: [PATCH 09/21] Fix quote count --- packages/server/src/api/controllers/table/utils.ts | 6 +++++- packages/server/src/api/routes/tests/row.spec.ts | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/controllers/table/utils.ts b/packages/server/src/api/controllers/table/utils.ts index 51f7b0e589..417cb22fe3 100644 --- a/packages/server/src/api/controllers/table/utils.ts +++ b/packages/server/src/api/controllers/table/utils.ts @@ -183,6 +183,8 @@ export async function handleDataImport( const finalData = await importToRows(data, table, user) + let newRowCount = finalData.length + //Set IDs of finalData to match existing row if an update is expected if (identifierFields.length > 0) { const allDocs = await db.allDocs( @@ -204,12 +206,14 @@ export async function handleDataImport( if (match) { finalItem._id = doc._id finalItem._rev = doc._rev + + newRowCount-- } }) }) } - await quotas.addRows(finalData.length, () => db.bulkDocs(finalData), { + await quotas.addRows(newRowCount, () => db.bulkDocs(finalData), { tableId: table._id, }) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index b448d46e6a..c4586263f4 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1320,6 +1320,7 @@ describe.each([ description: "Existing description", }) + const rowUsage = await getRowUsage() await config.api.row.bulkImport(table._id!, { rows: [ @@ -1346,6 +1347,8 @@ describe.each([ expect(rows[1].description).toEqual("Row 2 description") expect(rows[2].name).toEqual("Updated existing row") expect(rows[2].description).toEqual("Existing description") + + await assertRowUsage(rowUsage + 2) }) // Upserting isn't yet supported in MSSQL, see: From a6beb0fa82f125cf43896d02c04480f7ffca53eb Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 15:14:29 +0200 Subject: [PATCH 10/21] Support no updating existing rows --- .../server/src/api/controllers/table/utils.ts | 9 ++-- .../server/src/api/routes/tests/row.spec.ts | 54 +++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/controllers/table/utils.ts b/packages/server/src/api/controllers/table/utils.ts index 417cb22fe3..0e0a83e3b3 100644 --- a/packages/server/src/api/controllers/table/utils.ts +++ b/packages/server/src/api/controllers/table/utils.ts @@ -122,11 +122,12 @@ export function makeSureTableUpToDate(table: Table, tableToSave: Table) { export async function importToRows( data: Row[], table: Table, - user?: ContextUser + user?: ContextUser, + opts?: { keepCouchId: boolean } ) { const originalTable = table const finalData: Row[] = [] - const keepCouchId = "_id" in table.schema + const keepCouchId = !!opts?.keepCouchId for (let i = 0; i < data.length; i++) { let row = data[i] row._id = (keepCouchId && row._id) || generateRowID(table._id!) @@ -181,7 +182,9 @@ export async function handleDataImport( const db = context.getAppDB() const data = parse(importRows, table) - const finalData = await importToRows(data, table, user) + const finalData = await importToRows(data, table, user, { + keepCouchId: identifierFields.includes("_id"), + }) let newRowCount = finalData.length diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index c4586263f4..edceb925d6 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1351,6 +1351,60 @@ describe.each([ await assertRowUsage(rowUsage + 2) }) + isInternal && + it("should create new rows if not identifierFields are provided", async () => { + const table = await config.api.table.save( + saveTableRequest({ + schema: { + name: { + type: FieldType.STRING, + name: "name", + }, + description: { + type: FieldType.STRING, + name: "description", + }, + }, + }) + ) + + const existingRow = await config.api.row.save(table._id!, { + name: "Existing row", + description: "Existing description", + }) + + const rowUsage = await getRowUsage() + + await config.api.row.bulkImport(table._id!, { + rows: [ + { + name: "Row 1", + description: "Row 1 description", + }, + { ...existingRow, name: "Updated existing row" }, + { + name: "Row 2", + description: "Row 2 description", + }, + ], + }) + + const rows = await config.api.row.fetch(table._id!) + expect(rows.length).toEqual(4) + + rows.sort((a, b) => a.name.localeCompare(b.name)) + expect(rows[0].name).toEqual("Existing row") + expect(rows[0].description).toEqual("Existing description") + expect(rows[1].name).toEqual("Row 1") + expect(rows[1].description).toEqual("Row 1 description") + expect(rows[2].name).toEqual("Row 2") + expect(rows[2].description).toEqual("Row 2 description") + expect(rows[3].name).toEqual("Updated existing row") + expect(rows[3].description).toEqual("Existing description") + + await assertRowUsage(rowUsage + 3) + }) + // Upserting isn't yet supported in MSSQL, see: // https://github.com/knex/knex/pull/6050 !isMSSQL && From b74841d99da31ea616f34ab1d5741f4478dd354d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 15:20:06 +0200 Subject: [PATCH 11/21] Fix --- packages/server/src/db/defaultData/datasource_bb_default.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/db/defaultData/datasource_bb_default.ts b/packages/server/src/db/defaultData/datasource_bb_default.ts index a393888e51..6b553e2d36 100644 --- a/packages/server/src/db/defaultData/datasource_bb_default.ts +++ b/packages/server/src/db/defaultData/datasource_bb_default.ts @@ -651,10 +651,10 @@ export async function buildDefaultDocs() { return new LinkDocument( employeeData.table._id!, "Jobs", - employeeData.rows[index]._id, + employeeData.rows[index]._id!, jobData.table._id!, "Assigned", - jobData.rows[index]._id + jobData.rows[index]._id! ) } ) From 38da9012ea658fa6b36031983bd1a305ed7c7502 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 16:56:14 +0200 Subject: [PATCH 12/21] Display error --- .../backend/TableNavigator/modals/CreateTableModal.svelte | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builder/src/components/backend/TableNavigator/modals/CreateTableModal.svelte b/packages/builder/src/components/backend/TableNavigator/modals/CreateTableModal.svelte index 098369d3b4..b62c8af03d 100644 --- a/packages/builder/src/components/backend/TableNavigator/modals/CreateTableModal.svelte +++ b/packages/builder/src/components/backend/TableNavigator/modals/CreateTableModal.svelte @@ -78,7 +78,7 @@ await datasources.fetch() await afterSave(table) } catch (e) { - notifications.error(e) + notifications.error(e.message || e) // reload in case the table was created await tables.fetch() } From 785ab122378278ead729293911c9bfd0c1eb582d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:07:56 +0200 Subject: [PATCH 13/21] Add protected name validation test --- .../server/src/api/routes/tests/table.spec.ts | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index dd40a2420e..b96c08ea53 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1,4 +1,5 @@ import { context, docIds, events } from "@budibase/backend-core" +import { PROTECTED_EXTERNAL_COLUMNS } from "@budibase/shared-core" import { AutoFieldSubType, BBReferenceFieldSubType, @@ -1053,6 +1054,42 @@ describe.each([ }, }) }) + + isInternal && + it.each(PROTECTED_EXTERNAL_COLUMNS)( + "don't allow protected names (%s)", + async columnName => { + const result = await config.api.table.validateNewTableImport( + [ + { + id: generator.natural(), + name: generator.first(), + [columnName]: generator.word(), + }, + ], + { + ...basicSchema, + [columnName]: { + name: columnName, + type: FieldType.STRING, + }, + } + ) + + expect(result).toEqual({ + allValid: false, + errors: { + [columnName]: `${columnName} is a protected name`, + }, + invalidColumns: [], + schemaValidation: { + id: true, + name: true, + [columnName]: false, + }, + }) + } + ) }) describe("validateExistingTableImport", () => { From 73eefa1046e9c24978572ff39014cf8f5b1a2007 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:14:02 +0200 Subject: [PATCH 14/21] Check protected names on validation --- packages/server/src/api/controllers/table/index.ts | 14 +++++++++++--- packages/server/src/utilities/schema.ts | 14 +++++++++++++- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/packages/server/src/api/controllers/table/index.ts b/packages/server/src/api/controllers/table/index.ts index a02ecc665e..ee6947cec8 100644 --- a/packages/server/src/api/controllers/table/index.ts +++ b/packages/server/src/api/controllers/table/index.ts @@ -34,7 +34,11 @@ import sdk from "../../../sdk" import { jsonFromCsvString } from "../../../utilities/csv" import { builderSocket } from "../../../websockets" import { cloneDeep, isEqual } from "lodash" -import { helpers } from "@budibase/shared-core" +import { + helpers, + PROTECTED_EXTERNAL_COLUMNS, + PROTECTED_INTERNAL_COLUMNS, +} from "@budibase/shared-core" function pickApi({ tableId, table }: { tableId?: string; table?: Table }) { if (table && isExternalTable(table)) { @@ -167,7 +171,7 @@ export async function validateNewTableImport( if (isRows(rows) && isSchema(schema)) { ctx.status = 200 - ctx.body = validateSchema(rows, schema) + ctx.body = validateSchema(rows, schema, PROTECTED_INTERNAL_COLUMNS) } else { ctx.status = 422 } @@ -180,6 +184,7 @@ export async function validateExistingTableImport( let schema = null + let protectedColumnNames if (tableId) { const table = await sdk.tables.getTable(tableId) schema = table.schema @@ -189,6 +194,9 @@ export async function validateExistingTableImport( name: "_id", type: FieldType.STRING, } + protectedColumnNames = PROTECTED_INTERNAL_COLUMNS.filter(x => x !== "_id") + } else { + protectedColumnNames = PROTECTED_EXTERNAL_COLUMNS } } else { ctx.status = 422 @@ -197,7 +205,7 @@ export async function validateExistingTableImport( if (tableId && isRows(rows) && isSchema(schema)) { ctx.status = 200 - ctx.body = validateSchema(rows, schema) + ctx.body = validateSchema(rows, schema, protectedColumnNames) } else { ctx.status = 422 } diff --git a/packages/server/src/utilities/schema.ts b/packages/server/src/utilities/schema.ts index 4bd4e8f583..c7a4427dd7 100644 --- a/packages/server/src/utilities/schema.ts +++ b/packages/server/src/utilities/schema.ts @@ -41,7 +41,11 @@ export function isRows(rows: any): rows is Rows { return Array.isArray(rows) && rows.every(row => typeof row === "object") } -export function validate(rows: Rows, schema: TableSchema): ValidationResults { +export function validate( + rows: Rows, + schema: TableSchema, + protectedColumnNames: readonly string[] +): ValidationResults { const results: ValidationResults = { schemaValidation: {}, allValid: false, @@ -49,6 +53,8 @@ export function validate(rows: Rows, schema: TableSchema): ValidationResults { errors: {}, } + protectedColumnNames = protectedColumnNames.map(x => x.toLowerCase()) + rows.forEach(row => { Object.entries(row).forEach(([columnName, columnData]) => { const { @@ -63,6 +69,12 @@ export function validate(rows: Rows, schema: TableSchema): ValidationResults { return } + if (protectedColumnNames.includes(columnName.toLowerCase())) { + results.schemaValidation[columnName] = false + results.errors[columnName] = `${columnName} is a protected name` + return + } + // If the columnType is not a string, then it's not present in the schema, and should be added to the invalid columns array if (typeof columnType !== "string") { results.invalidColumns.push(columnName) From c015f8d192416695e6b9200eb13749597ac475ad Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:18:03 +0200 Subject: [PATCH 15/21] Run for both internal and external --- packages/server/src/api/routes/tests/table.spec.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index b96c08ea53..0d15919db4 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1,5 +1,8 @@ import { context, docIds, events } from "@budibase/backend-core" -import { PROTECTED_EXTERNAL_COLUMNS } from "@budibase/shared-core" +import { + PROTECTED_EXTERNAL_COLUMNS, + PROTECTED_INTERNAL_COLUMNS, +} from "@budibase/shared-core" import { AutoFieldSubType, BBReferenceFieldSubType, @@ -1055,10 +1058,9 @@ describe.each([ }) }) - isInternal && - it.each(PROTECTED_EXTERNAL_COLUMNS)( - "don't allow protected names (%s)", - async columnName => { + it.each( + isInternal ? PROTECTED_INTERNAL_COLUMNS : PROTECTED_EXTERNAL_COLUMNS + )("don't allow protected names (%s)", async columnName => { const result = await config.api.table.validateNewTableImport( [ { From 788a16cf48cbddf82c041f4439a83df13246d4e6 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:26:42 +0200 Subject: [PATCH 16/21] Add safety tests --- .../server/src/api/routes/tests/table.spec.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 0d15919db4..16f7f68550 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -123,6 +123,29 @@ describe.each([ body: basicTable(), }) }) + + it("does not persist the row fields that are not on the table schema", async () => { + const table: SaveTableRequest = basicTable() + table.rows = [ + { + name: "test-name", + description: "test-desc", + nonValid: "test-non-valid", + }, + ] + + const res = await config.api.table.save(table) + + const persistedRows = await config.api.row.search(res._id!) + + expect(persistedRows.rows).toEqual([ + expect.objectContaining({ + name: "test-name", + description: "test-desc", + }), + ]) + expect(persistedRows.rows[0].nonValid).toBeUndefined() + }) }) describe("update", () => { From 3f4484af00dbbef7b4de8e0e3c8e64e2362e7e7c Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:32:19 +0200 Subject: [PATCH 17/21] Add extra tests --- .../server/src/api/routes/tests/table.spec.ts | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 16f7f68550..594f95f9fa 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -146,6 +146,41 @@ describe.each([ ]) expect(persistedRows.rows[0].nonValid).toBeUndefined() }) + + it.each( + isInternal ? PROTECTED_INTERNAL_COLUMNS : PROTECTED_EXTERNAL_COLUMNS + )( + "cannot use protected column names (%s) while importing a table", + async columnName => { + const table: SaveTableRequest = basicTable() + table.rows = [ + { + name: "test-name", + description: "test-desc", + }, + ] + + await config.api.table.save( + { + ...table, + schema: { + ...table.schema, + [columnName]: { + name: columnName, + type: FieldType.STRING, + }, + }, + }, + { + status: 400, + body: { + message: `Column(s) "${columnName}" are duplicated - check for other columns with these name (case in-sensitive)`, + status: 400, + }, + } + ) + } + ) }) describe("update", () => { From ad74eca709b96d4b14027110e1c835b7a59d6af1 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:32:36 +0200 Subject: [PATCH 18/21] Fix --- .../server/src/sdk/app/tables/internal/index.ts | 4 +--- packages/shared-core/src/table.ts | 14 +++++--------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/server/src/sdk/app/tables/internal/index.ts b/packages/server/src/sdk/app/tables/internal/index.ts index 58f172e016..c0beed0db8 100644 --- a/packages/server/src/sdk/app/tables/internal/index.ts +++ b/packages/server/src/sdk/app/tables/internal/index.ts @@ -48,9 +48,7 @@ export async function save( } // check for case sensitivity - we don't want to allow duplicated columns - const duplicateColumn = findDuplicateInternalColumns(table, { - ignoreProtectedColumnNames: !oldTable && !!opts?.isImport, - }) + const duplicateColumn = findDuplicateInternalColumns(table) if (duplicateColumn.length) { throw new Error( `Column(s) "${duplicateColumn.join( diff --git a/packages/shared-core/src/table.ts b/packages/shared-core/src/table.ts index d5fd1dec00..9e7626cb1c 100644 --- a/packages/shared-core/src/table.ts +++ b/packages/shared-core/src/table.ts @@ -53,10 +53,7 @@ export function canBeSortColumn(type: FieldType): boolean { return !!allowSortColumnByType[type] } -export function findDuplicateInternalColumns( - table: Table, - opts?: { ignoreProtectedColumnNames: boolean } -): string[] { +export function findDuplicateInternalColumns(table: Table): string[] { // maintains the case of keys const casedKeys = Object.keys(table.schema) // get the column names @@ -72,11 +69,10 @@ export function findDuplicateInternalColumns( } } } - if (!opts?.ignoreProtectedColumnNames) { - for (let internalColumn of PROTECTED_INTERNAL_COLUMNS) { - if (casedKeys.find(key => key === internalColumn)) { - duplicates.push(internalColumn) - } + + for (let internalColumn of PROTECTED_INTERNAL_COLUMNS) { + if (casedKeys.find(key => key === internalColumn)) { + duplicates.push(internalColumn) } } return duplicates From 8f741ffe6ac1fa561311e6ecc35ea8f9e9b3cdef Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:40:30 +0200 Subject: [PATCH 19/21] More validations --- .../server/src/api/routes/tests/table.spec.ts | 89 ++++++++++++------- packages/server/src/utilities/schema.ts | 9 +- 2 files changed, 67 insertions(+), 31 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 594f95f9fa..f383fed927 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1118,38 +1118,67 @@ describe.each([ it.each( isInternal ? PROTECTED_INTERNAL_COLUMNS : PROTECTED_EXTERNAL_COLUMNS - )("don't allow protected names (%s)", async columnName => { - const result = await config.api.table.validateNewTableImport( - [ - { - id: generator.natural(), - name: generator.first(), - [columnName]: generator.word(), - }, - ], - { - ...basicSchema, - [columnName]: { - name: columnName, - type: FieldType.STRING, - }, - } - ) - - expect(result).toEqual({ - allValid: false, - errors: { - [columnName]: `${columnName} is a protected name`, - }, - invalidColumns: [], - schemaValidation: { - id: true, - name: true, - [columnName]: false, - }, - }) + )("don't allow protected names in schema (%s)", async columnName => { + const result = await config.api.table.validateNewTableImport( + [ + { + id: generator.natural(), + name: generator.first(), + [columnName]: generator.word(), + }, + ], + { + ...basicSchema, } ) + + expect(result).toEqual({ + allValid: false, + errors: { + [columnName]: `${columnName} is a protected column name`, + }, + invalidColumns: [], + schemaValidation: { + id: true, + name: true, + [columnName]: false, + }, + }) + }) + + isInternal && + it.each( + isInternal ? PROTECTED_INTERNAL_COLUMNS : PROTECTED_EXTERNAL_COLUMNS + )("don't allow protected names in the rows (%s)", async columnName => { + const result = await config.api.table.validateNewTableImport( + [ + { + id: generator.natural(), + name: generator.first(), + }, + ], + { + ...basicSchema, + [columnName]: { + name: columnName, + type: FieldType.STRING, + }, + } + ) + + expect(result).toEqual({ + allValid: false, + errors: { + [columnName]: `${columnName} is a protected column name`, + }, + invalidColumns: [], + schemaValidation: { + id: true, + name: true, + [columnName]: false, + }, + }) + }) }) describe("validateExistingTableImport", () => { diff --git a/packages/server/src/utilities/schema.ts b/packages/server/src/utilities/schema.ts index c7a4427dd7..c6b26b55c8 100644 --- a/packages/server/src/utilities/schema.ts +++ b/packages/server/src/utilities/schema.ts @@ -71,7 +71,7 @@ export function validate( if (protectedColumnNames.includes(columnName.toLowerCase())) { results.schemaValidation[columnName] = false - results.errors[columnName] = `${columnName} is a protected name` + results.errors[columnName] = `${columnName} is a protected column name` return } @@ -121,6 +121,13 @@ export function validate( }) }) + for (const schemaField of Object.keys(schema)) { + if (protectedColumnNames.includes(schemaField.toLowerCase())) { + results.schemaValidation[schemaField] = false + results.errors[schemaField] = `${schemaField} is a protected column name` + } + } + results.allValid = Object.values(results.schemaValidation).length > 0 && Object.values(results.schemaValidation).every(column => column) From f4bd3035721b0f778d6ff73d9b4653500d7baa37 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 1 Aug 2024 11:02:21 +0200 Subject: [PATCH 20/21] Handle frontend --- .../backend/TableNavigator/TableDataImport.svelte | 12 ++++++++---- packages/shared-core/src/utils.ts | 10 ++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/packages/builder/src/components/backend/TableNavigator/TableDataImport.svelte b/packages/builder/src/components/backend/TableNavigator/TableDataImport.svelte index 5bc3b9e728..4c9f4dd10f 100644 --- a/packages/builder/src/components/backend/TableNavigator/TableDataImport.svelte +++ b/packages/builder/src/components/backend/TableNavigator/TableDataImport.svelte @@ -1,9 +1,9 @@