Merge pull request #13645 from Budibase/fix/disable-external-auto-columns

Disable autocolumn addition to external tables (API validation)

packages/server/src/api/routes/tests/table.spec.ts

@@ -1,8 +1,8 @@
 import { context, events } from "@budibase/backend-core"
 import {
   AutoFieldSubType,
-  Datasource,
   BBReferenceFieldSubType,
+  Datasource,
   FieldType,
   INTERNAL_TABLE_SOURCE_ID,
   InternalTable,
@@ -149,58 +149,59 @@ describe.each([
       expect(res.name).toBeUndefined()
     })
 
-    it("updates only the passed fields", async () => {
+    isInternal &&
-      await timekeeper.withFreeze(new Date(2021, 1, 1), async () => {
+      it("updates only the passed fields", async () => {
-        const table = await config.api.table.save(
+        await timekeeper.withFreeze(new Date(2021, 1, 1), async () => {
-          tableForDatasource(datasource, {
+          const table = await config.api.table.save(
-            schema: {
+            tableForDatasource(datasource, {
-              autoId: {
+              schema: {
-                name: "id",
+                autoId: {
-                type: FieldType.NUMBER,
+                  name: "id",
-                subtype: AutoFieldSubType.AUTO_ID,
+                  type: FieldType.NUMBER,
-                autocolumn: true,
+                  subtype: AutoFieldSubType.AUTO_ID,
-                constraints: {
+                  autocolumn: true,
-                  type: "number",
+                  constraints: {
-                  presence: false,
+                    type: "number",
+                    presence: false,
+                  },
                 },
               },
-            },
+            })
+          )
+
+          const newName = generator.guid()
+
+          const updatedTable = await config.api.table.save({
+            ...table,
+            name: newName,
           })
-        )
 
-        const newName = generator.guid()
+          let expected: Table = {
+            ...table,
+            name: newName,
+            _id: expect.any(String),
+          }
+          if (isInternal) {
+            expected._rev = expect.stringMatching(/^2-.+/)
+          }
 
-        const updatedTable = await config.api.table.save({
+          expect(updatedTable).toEqual(expected)
-          ...table,
+
-          name: newName,
+          const persistedTable = await config.api.table.get(updatedTable._id!)
+          expected = {
+            ...table,
+            name: newName,
+            _id: updatedTable._id,
+          }
+          if (datasource?.isSQL) {
+            expected.sql = true
+          }
+          if (isInternal) {
+            expected._rev = expect.stringMatching(/^2-.+/)
+          }
+          expect(persistedTable).toEqual(expected)
         })
-
-        let expected: Table = {
-          ...table,
-          name: newName,
-          _id: expect.any(String),
-        }
-        if (isInternal) {
-          expected._rev = expect.stringMatching(/^2-.+/)
-        }
-
-        expect(updatedTable).toEqual(expected)
-
-        const persistedTable = await config.api.table.get(updatedTable._id!)
-        expected = {
-          ...table,
-          name: newName,
-          _id: updatedTable._id,
-        }
-        if (datasource?.isSQL) {
-          expected.sql = true
-        }
-        if (isInternal) {
-          expected._rev = expect.stringMatching(/^2-.+/)
-        }
-        expect(persistedTable).toEqual(expected)
       })
-    })
 
     describe("user table", () => {
       isInternal &&
@@ -214,6 +215,57 @@ describe.each([
         })
     })
 
+    describe("external table validation", () => {
+      !isInternal &&
+        it("should error if column is of type auto", async () => {
+          const table = basicTable(datasource)
+          await config.api.table.save(
+            {
+              ...table,
+              schema: {
+                ...table.schema,
+                auto: {
+                  name: "auto",
+                  autocolumn: true,
+                  type: FieldType.AUTO,
+                },
+              },
+            },
+            {
+              status: 400,
+              body: {
+                message: `Column "auto" has type "${FieldType.AUTO}" - this is not supported.`,
+              },
+            }
+          )
+        })
+
+      !isInternal &&
+        it("should error if column has auto subtype", async () => {
+          const table = basicTable(datasource)
+          await config.api.table.save(
+            {
+              ...table,
+              schema: {
+                ...table.schema,
+                auto: {
+                  name: "auto",
+                  autocolumn: true,
+                  type: FieldType.NUMBER,
+                  subtype: AutoFieldSubType.AUTO_ID,
+                },
+              },
+            },
+            {
+              status: 400,
+              body: {
+                message: `Column "auto" has subtype "${AutoFieldSubType.AUTO_ID}" - this is not supported.`,
+              },
+            }
+          )
+        })
+    })
+
     it("should add a new column for an internal DB table", async () => {
       const saveTableRequest: SaveTableRequest = {
         ...basicTable(),

packages/server/src/sdk/app/tables/external/index.ts

@@ -6,6 +6,7 @@ import {
   Table,
   TableRequest,
   ViewV2,
+  AutoFieldSubType,
 } from "@budibase/types"
 import { context } from "@budibase/backend-core"
 import { buildExternalTableId } from "../../../../integrations/utils"
@@ -29,6 +30,52 @@ import { populateExternalTableSchemas } from "../validation"
 import datasourceSdk from "../../datasources"
 import * as viewSdk from "../../views"
 
+const DEFAULT_PRIMARY_COLUMN = "id"
+
+function noPrimaryKey(table: Table) {
+  return table.primary == null || table.primary.length === 0
+}
+
+function validate(table: Table, oldTable?: Table) {
+  if (
+    !oldTable &&
+    table.schema[DEFAULT_PRIMARY_COLUMN] &&
+    noPrimaryKey(table)
+  ) {
+    throw new Error(
+      "External tables with no `primary` column set will define an `id` column, but we found an `id` column in the supplied schema. Either set a `primary` column or remove the `id` column."
+    )
+  }
+
+  if (hasTypeChanged(table, oldTable)) {
+    throw new Error("A column type has changed.")
+  }
+
+  const autoSubTypes = Object.values(AutoFieldSubType)
+  // check for auto columns, they are not allowed
+  for (let [key, column] of Object.entries(table.schema)) {
+    // this column is a special case, do not validate it
+    if (key === DEFAULT_PRIMARY_COLUMN) {
+      continue
+    }
+    // the auto-column type should never be used
+    if (column.type === FieldType.AUTO) {
+      throw new Error(
+        `Column "${key}" has type "${FieldType.AUTO}" - this is not supported.`
+      )
+    }
+
+    if (
+      column.subtype &&
+      autoSubTypes.includes(column.subtype as AutoFieldSubType)
+    ) {
+      throw new Error(
+        `Column "${key}" has subtype "${column.subtype}" - this is not supported.`
+      )
+    }
+  }
+}
+
 export async function save(
   datasourceId: string,
   update: Table,
@@ -47,28 +94,18 @@ export async function save(
     oldTable = await getTable(tableId)
   }
 
-  if (
+  // this will throw an error if something is wrong
-    !oldTable &&
+  validate(tableToSave, oldTable)
-    (tableToSave.primary == null || tableToSave.primary.length === 0)
-  ) {
-    if (tableToSave.schema.id) {
-      throw new Error(
-        "External tables with no `primary` column set will define an `id` column, but we found an `id` column in the supplied schema. Either set a `primary` column or remove the `id` column."
-      )
-    }
 
-    tableToSave.primary = ["id"]
+  if (!oldTable && noPrimaryKey(tableToSave)) {
-    tableToSave.schema.id = {
+    tableToSave.primary = [DEFAULT_PRIMARY_COLUMN]
+    tableToSave.schema[DEFAULT_PRIMARY_COLUMN] = {
       type: FieldType.NUMBER,
       autocolumn: true,
-      name: "id",
+      name: DEFAULT_PRIMARY_COLUMN,
     }
   }
 
-  if (hasTypeChanged(tableToSave, oldTable)) {
-    throw new Error("A column type has changed.")
-  }
-
   for (let view in tableToSave.views) {
     const tableView = tableToSave.views[view]
     if (!tableView || !viewSdk.isV2(tableView)) continue