Merge pull request #14758 from Budibase/budi-8717-if-you-delete-a-tab…

…le-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[],