Merge pull request #14758 from Budibase/budi-8717-if-you-delete-a-table-that-has-a-row-action-attached-it

Properly clean up row actions on table deletion.

packages/backend-core/src/db/couch/DatabaseImpl.ts

@@ -211,6 +211,17 @@ export class DatabaseImpl implements Database {
     })
   }
 
+  async tryGet<T extends Document>(id?: string): Promise<T | undefined> {
+    try {
+      return await this.get<T>(id)
+    } catch (err: any) {
+      if (err.statusCode === 404) {
+        return undefined
+      }
+      throw err
+    }
+  }
+
   async getMultiple<T extends Document>(
     ids: string[],
     opts?: { allowMissing?: boolean; excludeDocs?: boolean }

packages/backend-core/src/db/instrumentation.ts

@@ -42,6 +42,13 @@ export class DDInstrumentedDatabase implements Database {
     })
   }
 
+  tryGet<T extends Document>(id?: string | undefined): Promise<T | undefined> {
+    return tracer.trace("db.tryGet", span => {
+      span?.addTags({ db_name: this.name, doc_id: id })
+      return this.db.tryGet(id)
+    })
+  }
+
   getMultiple<T extends Document>(
     ids: string[],
     opts?: { allowMissing?: boolean | undefined } | undefined

packages/server/src/api/controllers/rowAction/crud.ts

@@ -21,14 +21,15 @@ export async function find(ctx: Ctx<void, RowActionsResponse>) {
   const table = await getTable(ctx)
   const tableId = table._id!
 
-  if (!(await sdk.rowActions.docExists(tableId))) {
+  const rowActions = await sdk.rowActions.getAll(tableId)
+  if (!rowActions) {
     ctx.body = {
       actions: {},
     }
     return
   }
 
-  const { actions } = await sdk.rowActions.getAll(tableId)
+  const { actions } = rowActions
   const result: RowActionsResponse = {
     actions: Object.entries(actions).reduce<Record<string, RowActionResponse>>(
       (acc, [key, action]) => ({

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

@@ -139,6 +139,7 @@ export async function save(ctx: UserCtx<SaveTableRequest, SaveTableResponse>) {
 export async function destroy(ctx: UserCtx) {
   const appId = ctx.appId
   const tableId = ctx.params.tableId
+  await sdk.rowActions.deleteAll(tableId)
   const deletedTable = await pickApi({ tableId }).destroy(ctx)
   await events.table.deleted(deletedTable)
   ctx.eventEmitter &&

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

@@ -1041,4 +1041,44 @@ describe("/rowsActions", () => {
       )
     })
   })
+
+  describe("scenarios", () => {
+    // https://linear.app/budibase/issue/BUDI-8717/
+    it("should not brick the app when deleting a table with row actions", async () => {
+      const view = await config.api.viewV2.create({
+        tableId,
+        name: generator.guid(),
+        schema: {
+          name: { visible: true },
+        },
+      })
+
+      const rowAction = await config.api.rowAction.save(tableId, {
+        name: generator.guid(),
+      })
+
+      await config.api.rowAction.setViewPermission(
+        tableId,
+        view.id,
+        rowAction.id
+      )
+
+      let actionsResp = await config.api.rowAction.find(tableId)
+      expect(actionsResp.actions[rowAction.id]).toEqual({
+        ...rowAction,
+        allowedSources: [tableId, view.id],
+      })
+
+      const table = await config.api.table.get(tableId)
+      await config.api.table.destroy(table._id!, table._rev!)
+
+      // In the bug reported by Conor, when a delete got deleted its row action
+      // document was not being cleaned up. This meant there existed code paths
+      // that would find it and try to reference the tables within it, resulting
+      // in errors.
+      await config.api.automation.fetchEnriched({
+        status: 200,
+      })
+    })
+  })
 })

packages/server/src/sdk/app/automations/utils.ts

@@ -26,13 +26,13 @@ export async function getBuilderData(
     return tableNameCache[tableId]
   }
 
-  const rowActionNameCache: Record<string, TableRowActions> = {}
+  const rowActionNameCache: Record<string, TableRowActions | undefined> = {}
   async function getRowActionName(tableId: string, rowActionId: string) {
     if (!rowActionNameCache[tableId]) {
       rowActionNameCache[tableId] = await sdk.rowActions.getAll(tableId)
     }
 
-    return rowActionNameCache[tableId].actions[rowActionId]?.name
+    return rowActionNameCache[tableId]?.actions[rowActionId]?.name
   }
 
   const result: Record<string, AutomationBuilderData> = {}
@@ -51,6 +51,10 @@ export async function getBuilderData(
     const tableName = await getTableName(tableId)
     const rowActionName = await getRowActionName(tableId, rowActionId)
 
+    if (!rowActionName) {
+      throw new Error(`Row action not found: ${rowActionId}`)
+    }
+
     result[automation._id!] = {
       displayName: `${tableName}: ${automation.name}`,
       triggerInfo: {

packages/server/src/sdk/app/rowActions.ts

@@ -1,5 +1,6 @@
 import { context, docIds, HTTPError, utils } from "@budibase/backend-core"
 import {
+  Automation,
   AutomationTriggerStepId,
   SEPARATOR,
   TableRowActions,
@@ -102,7 +103,25 @@ export async function get(tableId: string, rowActionId: string) {
 export async function getAll(tableId: string) {
   const db = context.getAppDB()
   const rowActionsId = generateRowActionsID(tableId)
-  return await db.get<TableRowActions>(rowActionsId)
+  return await db.tryGet<TableRowActions>(rowActionsId)
+}
+
+export async function deleteAll(tableId: string) {
+  const db = context.getAppDB()
+
+  const doc = await getAll(tableId)
+  if (!doc) {
+    return
+  }
+
+  const automationIds = Object.values(doc.actions).map(a => a.automationId)
+  const automations = await db.getMultiple<Automation>(automationIds)
+
+  for (const automation of automations) {
+    await sdk.automations.remove(automation._id!, automation._rev!)
+  }
+
+  await db.remove(doc)
 }
 
 export async function docExists(tableId: string) {
@@ -223,9 +242,8 @@ export async function run(tableId: any, rowActionId: any, rowId: string) {
     throw new HTTPError("Table not found", 404)
   }
 
-  const { actions } = await getAll(tableId)
-
-  const rowAction = actions[rowActionId]
+  const rowActions = await getAll(tableId)
+  const rowAction = rowActions?.actions[rowActionId]
   if (!rowAction) {
     throw new HTTPError("Row action not found", 404)
   }

packages/server/src/tests/utilities/api/automation.ts

@@ -1,4 +1,4 @@
-import { Automation } from "@budibase/types"
+import { Automation, FetchAutomationResponse } from "@budibase/types"
 import { Expectations, TestAPI } from "./base"
 
 export class AutomationAPI extends TestAPI {
@@ -14,6 +14,26 @@ export class AutomationAPI extends TestAPI {
     )
     return result
   }
+
+  fetch = async (
+    expectations?: Expectations
+  ): Promise<FetchAutomationResponse> => {
+    return await this._get<FetchAutomationResponse>(`/api/automations`, {
+      expectations,
+    })
+  }
+
+  fetchEnriched = async (
+    expectations?: Expectations
+  ): Promise<FetchAutomationResponse> => {
+    return await this._get<FetchAutomationResponse>(
+      `/api/automations?enrich=true`,
+      {
+        expectations,
+      }
+    )
+  }
+
   post = async (
     body: Automation,
     expectations?: Expectations

packages/types/src/sdk/db.ts

@@ -129,7 +129,12 @@ export interface Database {
   name: string
 
   exists(): Promise<boolean>
+  /**
+   * @deprecated the plan is to get everything using `tryGet` instead, then rename
+   * `tryGet` to `get`.
+   */
   get<T extends Document>(id?: string): Promise<T>
+  tryGet<T extends Document>(id?: string): Promise<T | undefined>
   exists(docId: string): Promise<boolean>
   getMultiple<T extends Document>(
     ids: string[],