Fix for attachment deletion when updating table column name, as well as refactoring to make the code a bit easier to follow.

packages/builder/src/components/backend/DataTable/modals/CreateEditColumn.svelte

@@ -171,7 +171,7 @@
         }
       }
     }
-    if (!savingColumn) {
+    if (!savingColumn && !originalName) {
       let highestNumber = 0
       Object.keys(table.schema).forEach(columnName => {
         const columnNumber = extractColumnNumber(columnName)

packages/server/src/api/controllers/row/internal.ts

@@ -2,7 +2,7 @@ import * as linkRows from "../../../db/linkedRows"
 import { generateRowID, InternalTables } from "../../../db/utils"
 import * as userController from "../user"
 import {
-  cleanupAttachments,
+  AttachmentCleanup,
   inputProcessing,
   outputProcessing,
 } from "../../../utilities/rowProcessor"
@@ -79,7 +79,7 @@ export async function patch(ctx: UserCtx<PatchRowRequest, PatchRowResponse>) {
     table,
   })) as Row
   // check if any attachments removed
-  await cleanupAttachments(table, { oldRow, row })
+  await AttachmentCleanup.rowUpdate(table, row, oldRow)
 
   if (isUserTable) {
     // the row has been updated, need to put it into the ctx
@@ -119,7 +119,7 @@ export async function save(ctx: UserCtx) {
     throw { validation: validateResult.errors }
   }
 
-  // make sure link rows are up to date
+  // make sure link rows are up-to-date
   row = (await linkRows.updateLinks({
     eventType: linkRows.EventType.ROW_SAVE,
     row,
@@ -165,7 +165,7 @@ export async function destroy(ctx: UserCtx) {
     tableId,
   })
   // remove any attachments that were on the row from object storage
-  await cleanupAttachments(table, { row })
+  await AttachmentCleanup.rowDelete(table, [row])
   // remove any static formula
   await updateRelatedFormula(table, row)
 
@@ -216,7 +216,7 @@ export async function bulkDestroy(ctx: UserCtx) {
     await db.bulkDocs(processedRows.map(row => ({ ...row, _deleted: true })))
   }
   // remove any attachments that were on the rows from object storage
-  await cleanupAttachments(table, { rows: processedRows })
+  await AttachmentCleanup.rowDelete(table, processedRows)
   await updateRelatedFormula(table, processedRows)
   await Promise.all(updates)
   return { response: { ok: true }, rows: processedRows }

packages/server/src/api/controllers/table/utils.ts

@@ -11,7 +11,7 @@ import {
 } from "../../../constants"
 import {
   inputProcessing,
-  cleanupAttachments,
+  AttachmentCleanup,
 } from "../../../utilities/rowProcessor"
 import { getViews, saveView } from "../view/utils"
 import viewTemplate from "../view/viewBuilder"
@@ -82,7 +82,10 @@ export async function checkForColumnUpdates(
     })
 
     // cleanup any attachments from object storage for deleted attachment columns
-    await cleanupAttachments(updatedTable, { oldTable, rows: rawRows })
+    await AttachmentCleanup.tableUpdate(updatedTable, rawRows, {
+      oldTable,
+      rename: columnRename,
+    })
     // Update views
     await checkForViewUpdates(updatedTable, deletedColumns, columnRename)
   }

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

@@ -19,11 +19,10 @@ import { context } from "@budibase/backend-core"
 import { getTable } from "../getters"
 import { checkAutoColumns } from "./utils"
 import * as viewsSdk from "../../views"
-import sdk from "../../../index"
 import { getRowParams } from "../../../../db/utils"
 import { quotas } from "@budibase/pro"
 import env from "../../../../environment"
-import { cleanupAttachments } from "../../../../utilities/rowProcessor"
+import { AttachmentCleanup } from "../../../../utilities/rowProcessor"
 
 export async function save(
   table: Table,
@@ -164,9 +163,10 @@ export async function destroy(table: Table) {
   await runStaticFormulaChecks(table, {
     deletion: true,
   })
-  await cleanupAttachments(table, {
-    rows: rowsData.rows.map((row: any) => row.doc),
-  })
+  await AttachmentCleanup.tableDelete(
+    table,
+    rowsData.rows.map((row: any) => row.doc)
+  )
 
   return { table }
 }

packages/server/src/utilities/rowProcessor/attachments.ts

@@ -0,0 +1,97 @@
+import { FieldTypes, ObjectStoreBuckets } from "../../constants"
+import { context, db as dbCore, objectStore } from "@budibase/backend-core"
+import { RenameColumn, Row, RowAttachment, Table } from "@budibase/types"
+
+export class AttachmentCleanup {
+  static async coreCleanup(fileListFn: () => string[]): Promise<void> {
+    const appId = context.getAppId()
+    if (!dbCore.isProdAppID(appId)) {
+      const prodAppId = dbCore.getProdAppID(appId!)
+      // if prod exists, then don't allow deleting
+      const exists = await dbCore.dbExists(prodAppId)
+      if (exists) {
+        return
+      }
+    }
+    const files = fileListFn()
+    if (files.length > 0) {
+      await objectStore.deleteFiles(ObjectStoreBuckets.APPS, files)
+    }
+  }
+
+  static async tableDelete(table: Table, rows: Row[]) {
+    return AttachmentCleanup.tableUpdate(table, rows, { deleting: true })
+  }
+
+  /**
+   * Cleanup attachments when updating or deleting a table.
+   */
+  static async tableUpdate(
+    table: Table,
+    rows: Row[],
+    opts: { oldTable?: Table; rename?: RenameColumn; deleting?: boolean }
+  ) {
+    return AttachmentCleanup.coreCleanup(() => {
+      let files: string[] = []
+      const tableSchema = opts.oldTable?.schema || table.schema
+      for (let [key, schema] of Object.entries(tableSchema)) {
+        if (schema.type !== FieldTypes.ATTACHMENT) {
+          continue
+        }
+        const columnRemoved = opts.oldTable && !table.schema[key]
+        const renaming = opts.rename?.old === key
+        // old table had this column, new table doesn't - delete it
+        if ((columnRemoved && !renaming) || opts.deleting) {
+          rows.forEach(row => {
+            files = files.concat(
+              row[key].map((attachment: any) => attachment.key)
+            )
+          })
+        }
+      }
+      return files
+    })
+  }
+
+  /**
+   * Cleanup attachments when deleting rows.
+   */
+  static async rowDelete(table: Table, rows: Row[]) {
+    return AttachmentCleanup.coreCleanup(() => {
+      let files: string[] = []
+      for (let [key, schema] of Object.entries(table.schema)) {
+        if (schema.type !== FieldTypes.ATTACHMENT) {
+          continue
+        }
+        rows.forEach(row => {
+          files = files.concat(
+            row[key].map((attachment: any) => attachment.key)
+          )
+        })
+      }
+      return files
+    })
+  }
+
+  /**
+   * Remove attachments when updating a row, if new row doesn't have the attachments.
+   */
+  static rowUpdate(table: Table, row: Row, oldRow: Row) {
+    return AttachmentCleanup.coreCleanup(() => {
+      let files: string[] = []
+      for (let [key, schema] of Object.entries(table.schema)) {
+        if (schema.type !== FieldTypes.ATTACHMENT) {
+          continue
+        }
+        const oldKeys =
+          oldRow[key]?.map((attachment: RowAttachment) => attachment.key) || []
+        const newKeys =
+          row[key]?.map((attachment: RowAttachment) => attachment.key) || []
+        files = files.concat(
+          oldKeys.filter((key: string) => newKeys.indexOf(key) === -1)
+        )
+      }
+      return files
+    })
+  }
+}

packages/server/src/utilities/rowProcessor/index.ts

@@ -1,16 +1,7 @@
 import * as linkRows from "../../db/linkedRows"
-import {
-  FieldTypes,
-  AutoFieldSubTypes,
-  ObjectStoreBuckets,
-} from "../../constants"
+import { FieldTypes, AutoFieldSubTypes } from "../../constants"
 import { processFormulas, fixAutoColumnSubType } from "./utils"
-import {
-  context,
-  db as dbCore,
-  objectStore,
-  utils,
-} from "@budibase/backend-core"
+import { objectStore, utils } from "@budibase/backend-core"
 import { InternalTables } from "../../db/utils"
 import { TYPE_TRANSFORM_MAP } from "./map"
 import { FieldSubtype, Row, RowAttachment, Table } from "@budibase/types"
@@ -22,6 +13,7 @@ import {
 import { isExternalTableID } from "../../integrations/utils"
 
 export * from "./utils"
+export * from "./attachments"
 
 type AutoColumnProcessingOpts = {
   reprocessing?: boolean
@@ -30,27 +22,6 @@ type AutoColumnProcessingOpts = {
 
 const BASE_AUTO_ID = 1
 
-/**
- * Given the old state of the row and the new one after an update, this will
- * find the keys that have been removed in the updated row.
- */
-function getRemovedAttachmentKeys(
-  oldRow: Row,
-  row: Row,
-  attachmentKey: string
-) {
-  if (!oldRow[attachmentKey]) {
-    return []
-  }
-  const oldKeys = oldRow[attachmentKey].map((attachment: any) => attachment.key)
-  // no attachments in new row, all removed
-  if (!row[attachmentKey]) {
-    return oldKeys
-  }
-  const newKeys = row[attachmentKey].map((attachment: any) => attachment.key)
-  return oldKeys.filter((key: string) => newKeys.indexOf(key) === -1)
-}
-
 /**
  * This will update any auto columns that are found on the row/table with the correct information based on
  * time now and the current logged in user making the request.
@@ -288,59 +259,3 @@ export async function outputProcessing<T extends Row[] | Row>(
   }
   return (wasArray ? enriched : enriched[0]) as T
 }
-
-/**
- * Clean up any attachments that were attached to a row.
- * @param table The table from which a row is being removed.
- * @param row optional - the row being removed.
- * @param rows optional - if multiple rows being deleted can do this in bulk.
- * @param oldRow optional - if updating a row this will determine the difference.
- * @param oldTable optional - if updating a table, can supply the old table to look for
- * deleted attachment columns.
- * @return When all attachments have been removed this will return.
- */
-export async function cleanupAttachments(
-  table: Table,
-  {
-    row,
-    rows,
-    oldRow,
-    oldTable,
-  }: { row?: Row; rows?: Row[]; oldRow?: Row; oldTable?: Table }
-): Promise<any> {
-  const appId = context.getAppId()
-  if (!dbCore.isProdAppID(appId)) {
-    const prodAppId = dbCore.getProdAppID(appId!)
-    // if prod exists, then don't allow deleting
-    const exists = await dbCore.dbExists(prodAppId)
-    if (exists) {
-      return
-    }
-  }
-  let files: string[] = []
-  function addFiles(row: Row, key: string) {
-    if (row[key]) {
-      files = files.concat(row[key].map((attachment: any) => attachment.key))
-    }
-  }
-  const schemaToUse = oldTable ? oldTable.schema : table.schema
-  for (let [key, schema] of Object.entries(schemaToUse)) {
-    if (schema.type !== FieldTypes.ATTACHMENT) {
-      continue
-    }
-    // old table had this column, new table doesn't - delete it
-    if (rows && oldTable && !table.schema[key]) {
-      rows.forEach(row => addFiles(row, key))
-    } else if (oldRow && row) {
-      // if updating, need to manage the differences
-      files = files.concat(getRemovedAttachmentKeys(oldRow, row, key))
-    } else if (row) {
-      addFiles(row, key)
-    } else if (rows) {
-      rows.forEach(row => addFiles(row, key))
-    }
-  }
-  if (files.length > 0) {
-    await objectStore.deleteFiles(ObjectStoreBuckets.APPS, files)
-  }
-}