From 45543cbc038f5c68e55c925d7c79e100bd505eaf Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 27 Oct 2023 15:59:31 +0100 Subject: [PATCH] Catch a few more edge cases with column names, add tests for them. --- packages/backend-core/src/db/constants.ts | 4 + .../grid/controls/MigrationModal.svelte | 3 + .../server/src/api/routes/tests/table.spec.ts | 124 +++++++++++++++--- .../server/src/sdk/app/tables/migration.ts | 9 ++ 4 files changed, 123 insertions(+), 17 deletions(-) diff --git a/packages/backend-core/src/db/constants.ts b/packages/backend-core/src/db/constants.ts index aea485e3e3..bfa7595d62 100644 --- a/packages/backend-core/src/db/constants.ts +++ b/packages/backend-core/src/db/constants.ts @@ -8,3 +8,7 @@ export const CONSTANT_INTERNAL_ROW_COLS = [ ] as const export const CONSTANT_EXTERNAL_ROW_COLS = ["_id", "_rev", "tableId"] as const + +export function isInternalColumnName(name: string): boolean { + return (CONSTANT_INTERNAL_ROW_COLS as readonly string[]).includes(name) +} diff --git a/packages/frontend-core/src/components/grid/controls/MigrationModal.svelte b/packages/frontend-core/src/components/grid/controls/MigrationModal.svelte index 1957c3259f..ecef009fe0 100644 --- a/packages/frontend-core/src/components/grid/controls/MigrationModal.svelte +++ b/packages/frontend-core/src/components/grid/controls/MigrationModal.svelte @@ -17,6 +17,9 @@ $: error = checkNewColumnName(newColumnName) const checkNewColumnName = newColumnName => { + if (newColumnName === "") { + return "Column name can't be empty." + } if (newColumnName in $definition.schema) { return "New column name can't be the same as an existing column name." } diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 3767e5e912..c239c596fe 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -10,11 +10,13 @@ import { SaveTableRequest, Table, TableSourceType, + User, ViewCalculation, } from "@budibase/types" import { checkBuilderEndpoint } from "./utilities/TestFunctions" import * as setup from "./utilities" import sdk from "../../../sdk" +import uuid from "uuid" const { basicTable } = setup.structures @@ -426,13 +428,16 @@ describe("/tables", () => { }) describe("migrate", () => { - it("should successfully migrate a one-to-many user relationship to a user column", async () => { - const users = await Promise.all([ - config.createUser({ email: "1@example.com" }), - config.createUser({ email: "2@example.com" }), - config.createUser({ email: "3@example.com" }), + let users: User[] + beforeAll(async () => { + users = await Promise.all([ + config.createUser({ email: `${uuid.v4()}@example.com` }), + config.createUser({ email: `${uuid.v4()}@example.com` }), + config.createUser({ email: `${uuid.v4()}@example.com` }), ]) + }) + it("should successfully migrate a one-to-many user relationship to a user column", async () => { const table = await config.api.table.create({ name: "table", type: "table", @@ -488,12 +493,6 @@ describe("/tables", () => { }) it("should successfully migrate a many-to-many user relationship to a users column", async () => { - const users = await Promise.all([ - config.createUser({ email: "1@example.com" }), - config.createUser({ email: "2@example.com" }), - config.createUser({ email: "3@example.com" }), - ]) - const table = await config.api.table.create({ name: "table", type: "table", @@ -551,12 +550,6 @@ describe("/tables", () => { }) it("should successfully migrate a many-to-one user relationship to a users column", async () => { - const users = await Promise.all([ - config.createUser({ email: "1@example.com" }), - config.createUser({ email: "2@example.com" }), - config.createUser({ email: "3@example.com" }), - ]) - const table = await config.api.table.create({ name: "table", type: "table", @@ -612,5 +605,102 @@ describe("/tables", () => { users[2]._id, ]) }) + + describe("unhappy paths", () => { + let table: Table + beforeAll(async () => { + table = await config.api.table.create({ + name: "table", + type: "table", + sourceId: INTERNAL_TABLE_SOURCE_ID, + sourceType: TableSourceType.INTERNAL, + schema: { + "user relationship": { + type: FieldType.LINK, + fieldName: "test", + name: "user relationship", + constraints: { + type: "array", + presence: false, + }, + relationshipType: RelationshipType.MANY_TO_ONE, + tableId: InternalTable.USER_METADATA, + }, + num: { + type: FieldType.NUMBER, + name: "num", + constraints: { + type: "number", + presence: false, + }, + }, + }, + }) + }) + + it("should fail if the new column name is blank", async () => { + await config.api.table.migrate( + table._id!, + { + oldColumn: table.schema["user relationship"], + newColumn: { + name: "", + type: FieldType.BB_REFERENCE, + subtype: FieldSubtype.USERS, + }, + }, + { expectStatus: 400 } + ) + }) + + it("should fail if the new column name is a reserved name", async () => { + await config.api.table.migrate( + table._id!, + { + oldColumn: table.schema["user relationship"], + newColumn: { + name: "_id", + type: FieldType.BB_REFERENCE, + subtype: FieldSubtype.USERS, + }, + }, + { expectStatus: 400 } + ) + }) + + it("should fail if the new column name is the same as an existing column", async () => { + await config.api.table.migrate( + table._id!, + { + oldColumn: table.schema["user relationship"], + newColumn: { + name: "num", + type: FieldType.BB_REFERENCE, + subtype: FieldSubtype.USERS, + }, + }, + { expectStatus: 400 } + ) + }) + + it("should fail if the old column name isn't a column in the table", async () => { + await config.api.table.migrate( + table._id!, + { + oldColumn: { + name: "not a column", + type: FieldType.BB_REFERENCE, + subtype: FieldSubtype.USERS, + }, + newColumn: { + name: "new column", + type: FieldType.BB_REFERENCE, + subtype: FieldSubtype.USERS, + }, + }, + { expectStatus: 400 } + ) + }) + }) }) }) diff --git a/packages/server/src/sdk/app/tables/migration.ts b/packages/server/src/sdk/app/tables/migration.ts index 7ab674bbb2..5a6b0c5bc0 100644 --- a/packages/server/src/sdk/app/tables/migration.ts +++ b/packages/server/src/sdk/app/tables/migration.ts @@ -16,6 +16,7 @@ import sdk from "../../../sdk" import { isExternalTableID } from "../../../integrations/utils" import { EventType, updateLinks } from "../../../db/linkedRows" import { cloneDeep } from "lodash" +import { isInternalColumnName } from "@budibase/backend-core/src/db" export interface MigrationResult { tablesUpdated: Table[] @@ -30,6 +31,14 @@ export async function migrate( throw new BadRequestError(`Column "${newColumn.name}" already exists`) } + if (newColumn.name === "") { + throw new BadRequestError(`Column name cannot be empty`) + } + + if (isInternalColumnName(newColumn.name)) { + throw new BadRequestError(`Column name cannot be a reserved column name`) + } + table.schema[newColumn.name] = newColumn table = await sdk.tables.saveTable(table)