From af516427fcf43c5760a5338620b75f35bd3e937b Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 3 May 2024 14:14:10 +0100 Subject: [PATCH 1/4] Attempt to fix race condition when creating multiple auto ID columns in parallel. --- .../src/api/controllers/row/staticFormula.ts | 38 ++------------ .../src/sdk/app/rows/tests/internal.spec.ts | 1 + .../src/utilities/rowProcessor/index.ts | 49 +++++++++++++++++-- .../types/src/documents/app/table/schema.ts | 6 +++ packages/types/src/sdk/locks.ts | 1 - 5 files changed, 54 insertions(+), 41 deletions(-) diff --git a/packages/server/src/api/controllers/row/staticFormula.ts b/packages/server/src/api/controllers/row/staticFormula.ts index a75a6cd2cc..a6a02952d3 100644 --- a/packages/server/src/api/controllers/row/staticFormula.ts +++ b/packages/server/src/api/controllers/row/staticFormula.ts @@ -1,20 +1,11 @@ import { getRowParams } from "../../../db/utils" import { outputProcessing, - processAutoColumn, processFormulas, } from "../../../utilities/rowProcessor" -import { context, locks } from "@budibase/backend-core" -import { - Table, - Row, - LockType, - LockName, - FormulaType, - FieldType, -} from "@budibase/types" +import { context } from "@budibase/backend-core" +import { Table, Row, FormulaType, FieldType } from "@budibase/types" import * as linkRows from "../../../db/linkedRows" -import sdk from "../../../sdk" import isEqual from "lodash/isEqual" import { cloneDeep } from "lodash/fp" @@ -151,30 +142,7 @@ export async function finaliseRow( // if another row has been written since processing this will // handle the auto ID clash if (oldTable && !isEqual(oldTable, table)) { - try { - await db.put(table) - } catch (err: any) { - if (err.status === 409) { - // Some conflicts with the autocolumns occurred, we need to refetch the table and recalculate - await locks.doWithLock( - { - type: LockType.AUTO_EXTEND, - name: LockName.PROCESS_AUTO_COLUMNS, - resource: table._id, - }, - async () => { - const latestTable = await sdk.tables.getTable(table._id!) - let response = processAutoColumn(null, latestTable, row, { - reprocessing: true, - }) - await db.put(response.table) - row = response.row - } - ) - } else { - throw err - } - } + await db.put(table) } const response = await db.put(row) // for response, calculate the formulas for the enriched row diff --git a/packages/server/src/sdk/app/rows/tests/internal.spec.ts b/packages/server/src/sdk/app/rows/tests/internal.spec.ts index 877bd1e6dc..9f7f68ca7b 100644 --- a/packages/server/src/sdk/app/rows/tests/internal.spec.ts +++ b/packages/server/src/sdk/app/rows/tests/internal.spec.ts @@ -145,6 +145,7 @@ describe("sdk >> rows >> internal", () => { lastID: 1, }, }, + _rev: expect.stringMatching("2-.*"), }, row: { ...row, diff --git a/packages/server/src/utilities/rowProcessor/index.ts b/packages/server/src/utilities/rowProcessor/index.ts index e69cfa471a..c44326ff42 100644 --- a/packages/server/src/utilities/rowProcessor/index.ts +++ b/packages/server/src/utilities/rowProcessor/index.ts @@ -1,6 +1,6 @@ import * as linkRows from "../../db/linkedRows" import { processFormulas, fixAutoColumnSubType } from "./utils" -import { objectStore, utils } from "@budibase/backend-core" +import { context, objectStore, utils } from "@budibase/backend-core" import { InternalTables } from "../../db/utils" import { TYPE_TRANSFORM_MAP } from "./map" import { @@ -9,6 +9,7 @@ import { Row, RowAttachment, Table, + isAutoColumnField, } from "@budibase/types" import { cloneDeep } from "lodash/fp" import { @@ -25,7 +26,44 @@ type AutoColumnProcessingOpts = { noAutoRelationships?: boolean } -const BASE_AUTO_ID = 1 +// Returns the next auto ID for a column in a table. On success, the table will +// be updated which is why it gets returned. The nextID returned is guaranteed +// to be given only to you, and if you don't use it it's gone forever (a gap +// will be left in the auto ID sequence). +// +// This function can throw if it fails to generate an auto ID after so many +// attempts. +async function getNextAutoId( + table: Table, + column: string +): Promise<{ table: Table; nextID: number }> { + const db = context.getAppDB() + for (let attempt = 0; attempt < 5; attempt++) { + const schema = table.schema[column] + if (!isAutoColumnField(schema)) { + throw new Error(`Column ${column} is not an auto column`) + } + schema.lastID = (schema.lastID || 0) + 1 + try { + const resp = await db.put(table) + table._rev = resp.rev + return { table, nextID: schema.lastID } + } catch (e: any) { + if (e.status !== 409) { + throw e + } + // We wait for a random amount of time before retrying. The randomness + // makes it less likely for multiple requests modifying this table to + // collide. + await new Promise(resolve => + setTimeout(resolve, Math.random() * 1.2 ** attempt * 1000) + ) + table = await db.get(table._id) + } + } + + throw new Error("Failed to generate an auto ID") +} /** * This will update any auto columns that are found on the row/table with the correct information based on @@ -37,7 +75,7 @@ const BASE_AUTO_ID = 1 * @returns The updated row and table, the table may need to be updated * for automatic ID purposes. */ -export function processAutoColumn( +export async function processAutoColumn( userId: string | null | undefined, table: Table, row: Row, @@ -79,8 +117,9 @@ export function processAutoColumn( break case AutoFieldSubType.AUTO_ID: if (creating) { - schema.lastID = !schema.lastID ? BASE_AUTO_ID : schema.lastID + 1 - row[key] = schema.lastID + const { table: newTable, nextID } = await getNextAutoId(table, key) + table = newTable + row[key] = nextID } break } diff --git a/packages/types/src/documents/app/table/schema.ts b/packages/types/src/documents/app/table/schema.ts index 18fbeae719..407199860b 100644 --- a/packages/types/src/documents/app/table/schema.ts +++ b/packages/types/src/documents/app/table/schema.ts @@ -219,3 +219,9 @@ export function isAttachmentField( ): field is AttachmentFieldMetadata { return field.type === FieldType.ATTACHMENTS } + +export function isAutoColumnField( + field: FieldSchema +): field is AutoColumnFieldMetadata { + return field.type === FieldType.AUTO +} diff --git a/packages/types/src/sdk/locks.ts b/packages/types/src/sdk/locks.ts index c7c028a135..d455e95353 100644 --- a/packages/types/src/sdk/locks.ts +++ b/packages/types/src/sdk/locks.ts @@ -21,7 +21,6 @@ export enum LockName { PERSIST_WRITETHROUGH = "persist_writethrough", QUOTA_USAGE_EVENT = "quota_usage_event", APP_MIGRATION = "app_migrations", - PROCESS_AUTO_COLUMNS = "process_auto_columns", PROCESS_USER_INVITE = "process_user_invite", } From 2b52c11b9af101b166740e69f96763b4853bc5c6 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 3 May 2024 14:16:33 +0100 Subject: [PATCH 2/4] Expand the tests slightly. --- .../server/src/sdk/app/rows/tests/internal.spec.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/server/src/sdk/app/rows/tests/internal.spec.ts b/packages/server/src/sdk/app/rows/tests/internal.spec.ts index 9f7f68ca7b..a7c46f73e7 100644 --- a/packages/server/src/sdk/app/rows/tests/internal.spec.ts +++ b/packages/server/src/sdk/app/rows/tests/internal.spec.ts @@ -190,7 +190,6 @@ describe("sdk >> rows >> internal", () => { type: FieldType.AUTO, subtype: AutoFieldSubType.AUTO_ID, autocolumn: true, - lastID: 0, }, }, }) @@ -200,7 +199,7 @@ describe("sdk >> rows >> internal", () => { await internalSdk.save(table._id!, row, config.getUser()._id) } await Promise.all( - makeRows(10).map(row => + makeRows(20).map(row => internalSdk.save(table._id!, row, config.getUser()._id) ) ) @@ -210,19 +209,21 @@ describe("sdk >> rows >> internal", () => { }) const persistedRows = await config.getRows(table._id!) - expect(persistedRows).toHaveLength(20) + expect(persistedRows).toHaveLength(30) expect(persistedRows).toEqual( expect.arrayContaining( - Array.from({ length: 20 }).map((_, i) => + Array.from({ length: 30 }).map((_, i) => expect.objectContaining({ id: i + 1 }) ) ) ) const persistedTable = await config.getTable(table._id) - expect((table.schema.id as AutoColumnFieldMetadata).lastID).toBe(0) + expect( + (table.schema.id as AutoColumnFieldMetadata).lastID + ).toBeUndefined() expect((persistedTable.schema.id as AutoColumnFieldMetadata).lastID).toBe( - 20 + 30 ) }) }) From 99ecefaedf771d4a23d49ca39df92272785506f8 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 3 May 2024 14:37:13 +0100 Subject: [PATCH 3/4] Fix row.spec.ts --- packages/server/src/utilities/rowProcessor/index.ts | 3 ++- packages/types/src/documents/app/table/schema.ts | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/server/src/utilities/rowProcessor/index.ts b/packages/server/src/utilities/rowProcessor/index.ts index c44326ff42..d072802522 100644 --- a/packages/server/src/utilities/rowProcessor/index.ts +++ b/packages/server/src/utilities/rowProcessor/index.ts @@ -10,6 +10,7 @@ import { RowAttachment, Table, isAutoColumnField, + isAutoColumnNumberField, } from "@budibase/types" import { cloneDeep } from "lodash/fp" import { @@ -40,7 +41,7 @@ async function getNextAutoId( const db = context.getAppDB() for (let attempt = 0; attempt < 5; attempt++) { const schema = table.schema[column] - if (!isAutoColumnField(schema)) { + if (!isAutoColumnField(schema) && !isAutoColumnNumberField(schema)) { throw new Error(`Column ${column} is not an auto column`) } schema.lastID = (schema.lastID || 0) + 1 diff --git a/packages/types/src/documents/app/table/schema.ts b/packages/types/src/documents/app/table/schema.ts index 407199860b..d9fb405ff5 100644 --- a/packages/types/src/documents/app/table/schema.ts +++ b/packages/types/src/documents/app/table/schema.ts @@ -225,3 +225,9 @@ export function isAutoColumnField( ): field is AutoColumnFieldMetadata { return field.type === FieldType.AUTO } + +export function isAutoColumnNumberField( + field: FieldSchema +): field is NumberFieldMetadata { + return field.type === FieldType.NUMBER +} From 703092505d2ab8c50a80551feebd1abd53904731 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 3 May 2024 15:08:21 +0100 Subject: [PATCH 4/4] Respond to PR feedback. --- packages/server/src/utilities/rowProcessor/index.ts | 4 +--- packages/types/src/documents/app/table/schema.ts | 12 ------------ 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/packages/server/src/utilities/rowProcessor/index.ts b/packages/server/src/utilities/rowProcessor/index.ts index d072802522..7da61ebcb6 100644 --- a/packages/server/src/utilities/rowProcessor/index.ts +++ b/packages/server/src/utilities/rowProcessor/index.ts @@ -9,8 +9,6 @@ import { Row, RowAttachment, Table, - isAutoColumnField, - isAutoColumnNumberField, } from "@budibase/types" import { cloneDeep } from "lodash/fp" import { @@ -41,7 +39,7 @@ async function getNextAutoId( const db = context.getAppDB() for (let attempt = 0; attempt < 5; attempt++) { const schema = table.schema[column] - if (!isAutoColumnField(schema) && !isAutoColumnNumberField(schema)) { + if (schema.type !== FieldType.NUMBER && schema.type !== FieldType.AUTO) { throw new Error(`Column ${column} is not an auto column`) } schema.lastID = (schema.lastID || 0) + 1 diff --git a/packages/types/src/documents/app/table/schema.ts b/packages/types/src/documents/app/table/schema.ts index d9fb405ff5..18fbeae719 100644 --- a/packages/types/src/documents/app/table/schema.ts +++ b/packages/types/src/documents/app/table/schema.ts @@ -219,15 +219,3 @@ export function isAttachmentField( ): field is AttachmentFieldMetadata { return field.type === FieldType.ATTACHMENTS } - -export function isAutoColumnField( - field: FieldSchema -): field is AutoColumnFieldMetadata { - return field.type === FieldType.AUTO -} - -export function isAutoColumnNumberField( - field: FieldSchema -): field is NumberFieldMetadata { - return field.type === FieldType.NUMBER -}