refactor(tree): Misc. cleanup in table schema APIs (#25333)

Removes a few private methods that really aren't needed, clears up some
semantics regarding node id resolution, and removes nested transactions
in column deletion.

Author: Josmithr

packages/dds/tree/src/tableSchema.ts

@@ -3,7 +3,7 @@
  * Licensed under the MIT License.
  */
 
-import { assert, fail } from "@fluidframework/core-utils/internal";
+import { fail } from "@fluidframework/core-utils/internal";
 import { UsageError } from "@fluidframework/telemetry-utils/internal";
 
 import { Tree, TreeAlpha } from "./shared-tree/index.js";
@@ -752,51 +752,38 @@ export namespace System_TableSchema {
 					});
 					return removedColumns ?? fail("Transaction did not complete.");
 				} else {
-					const columnsToRemove = indexOrColumns;
-
 					// If there are no columns to remove, do nothing
-					if (columnsToRemove.length === 0) {
+					if (indexOrColumns.length === 0) {
 						return [];
 					}
 
-					// Ensure the specified columns exists before starting transaction.
-					// Improves user-facing error experience.
-					for (const columnToRemove of columnsToRemove) {
-						this._assertContainsColumn(columnToRemove);
+					// Resolve any IDs to actual nodes.
+					// This validates that all of the rows exist before starting transaction.
+					// This improves user-facing error experience.
+					const columnsToRemove: ColumnValueType[] = [];
+					for (const columnOrIdToRemove of indexOrColumns) {
+						columnsToRemove.push(this._getColumn(columnOrIdToRemove));
 					}
 
-					const removedColumns: ColumnValueType[] = [];
 					this._applyEditsInBatch(() => {
 						// Note, throwing an error within a transaction will abort the entire transaction.
 						// So if we throw an error here for any column, no columns will be removed.
 						for (const columnToRemove of columnsToRemove) {
-							const removedColumn = this._removeColumn(columnToRemove);
-							removedColumns.push(removedColumn);
+							// Remove the corresponding cell from all rows.
+							for (const row of this.rows) {
+								// TypeScript is unable to narrow the row type correctly here, hence the cast.
+								// See: https://github.com/microsoft/TypeScript/issues/52144
+								(row as RowValueType).removeCell(columnToRemove);
+							}
+
+							// We have already validated that all of the columns exist above, so this is safe.
+							this.columns.removeAt(this.columns.indexOf(columnToRemove));
 						}
 					});
-					return removedColumns;
+					return columnsToRemove;
 				}
 			}
 
-			private _removeColumn(columnOrId: string | ColumnValueType): ColumnValueType {
-				const column = this._getColumn(columnOrId);
-				const index = this.columns.indexOf(column);
-				assert(index !== -1, "Column should exist");
-
-				this._applyEditsInBatch(() => {
-					// Remove the corresponding cell from all rows.
-					for (const row of this.rows) {
-						// TypeScript is unable to narrow the row type correctly here, hence the cast.
-						// See: https://github.com/microsoft/TypeScript/issues/52144
-						(row as RowValueType).removeCell(column);
-					}
-
-					this.columns.removeAt(index);
-				});
-
-				return column;
-			}
-
 			public removeRows(
 				indexOrRows: number | undefined | readonly string[] | readonly RowValueType[],
 				count?: number | undefined,
@@ -819,39 +806,28 @@ export namespace System_TableSchema {
 					);
 				}
 
-				const rowsToRemove = indexOrRows;
-
 				// If there are no rows to remove, do nothing
-				if (rowsToRemove.length === 0) {
+				if (indexOrRows.length === 0) {
 					return [];
 				}
 
-				// Ensure the specified rows exists before starting transaction.
-				// Improves user-facing error experience.
-				for (const rowToRemove of rowsToRemove) {
-					this._assertContainsRow(rowToRemove);
+				// Resolve any IDs to actual nodes.
+				// This validates that all of the rows exist before starting transaction.
+				// This improves user-facing error experience.
+				const rowsToRemove: RowValueType[] = [];
+				for (const rowToRemove of indexOrRows) {
+					rowsToRemove.push(this._getRow(rowToRemove));
 				}
 
-				const removedRows: RowValueType[] = [];
 				this._applyEditsInBatch(() => {
 					// Note, throwing an error within a transaction will abort the entire transaction.
 					// So if we throw an error here for any row, no rows will be removed.
 					for (const rowToRemove of rowsToRemove) {
-						const removedRow = this._removeRow(rowToRemove);
-						removedRows.push(removedRow);
+						// We have already validated that all of the rows exist above, so this is safe.
+						this.rows.removeAt(this.rows.indexOf(rowToRemove));
 					}
 				});
-				return removedRows;
-			}
-
-			private _removeRow(rowOrId: string | RowValueType): RowValueType {
-				const rowToRemove = this._getRow(rowOrId);
-
-				const index = this.rows.indexOf(rowToRemove);
-				assert(index !== -1, "Row should exist");
-
-				this.rows.removeAt(index);
-				return rowToRemove;
+				return rowsToRemove;
 			}
 
 			public removeCell(
@@ -928,24 +904,24 @@ export namespace System_TableSchema {
 			}
 
 			/**
-			 * Attempts to resolve a Column node or ID to a Column node.
-			 * If a node is provided, it is returned as-is.
-			 * If an ID is provided, we check the table for the corresponding Column node and return it if it exists, otherwise undefined.
+			 * Attempts to resolve the provided Column node or ID to a Column node in the table.
+			 * Returns `undefined` if there is no match.
+			 * @remarks Searches for a match based strictly on the ID and returns that result.
 			 */
 			private _tryGetColumn(
 				columnOrId: string | ColumnValueType,
 			): ColumnValueType | undefined {
-				return typeof columnOrId === "string" ? this.getColumn(columnOrId) : columnOrId;
+				const columnId = this._getColumnId(columnOrId);
+				return this.getColumn(columnId);
 			}
 
 			/**
-			 * Attempts to resolve a Column node or ID to a Column node.
-			 * If a node is provided, it is returned as-is.
-			 * If an ID is provided, we check the table for the corresponding Column node and return it if it exists, otherwise we throw an exception.
+			 * Attempts to resolve the provided Column node or ID to a Column node in the table.
+			 * @throws Throws a `UsageError` if there is no match.
+			 * @remarks Searches for a match based strictly on the ID and returns that result.
 			 */
 			private _getColumn(columnOrId: string | ColumnValueType): ColumnValueType {
-				const column =
-					typeof columnOrId === "string" ? this.getColumn(columnOrId) : columnOrId;
+				const column = this._tryGetColumn(columnOrId);
 				if (column === undefined) {
 					this._throwMissingColumnError(this._getColumnId(columnOrId));
 				}
@@ -961,36 +937,11 @@ export namespace System_TableSchema {
 				return typeof columnOrId === "string" ? columnOrId : columnOrId.id;
 			}
 
-			/**
-			 * Attempts to resolve a Column node or ID to its index in the table.
-			 * Returns undefined if the Column does not exist in the table.
-			 */
-			private _getColumnIndex(columnOrId: string | ColumnValueType): number | undefined {
-				const column = this._tryGetColumn(columnOrId);
-				if (column === undefined) {
-					return undefined;
-				}
-
-				const index = this.columns.indexOf(column);
-				return index === -1 ? undefined : index;
-			}
-
 			/**
 			 * Checks if a Column with the specified ID exists in the table.
 			 */
 			private _containsColumnWithId(columnId: string): boolean {
-				return this._getColumnIndex(columnId) !== undefined;
-			}
-
-			/**
-			 * Assert that the provided Column exists in the table.
-			 * @throws Throws a `UsageError` if the Column does not exist.
-			 */
-			private _assertContainsColumn(columnOrId: ColumnValueType | string): void {
-				const columnId = this._getColumnId(columnOrId);
-				if (!this._containsColumnWithId(columnId)) {
-					this._throwMissingColumnError(columnId);
-				}
+				return this._tryGetColumn(columnId) !== undefined;
 			}
 
 			/**
@@ -1001,21 +952,22 @@ export namespace System_TableSchema {
 			}
 
 			/**
-			 * Attempts to resolve a Row node or ID to a Row node.
-			 * If a node is provided, it is returned as-is.
-			 * If an ID is provided, we check the table for the corresponding Row node and return it if it exists, otherwise undefined.
+			 * Attempts to resolve the provided Row node or ID to a Row node in the table.
+			 * Returns `undefined` if there is no match.
+			 * @remarks Searches for a match based strictly on the ID and returns that result.
 			 */
 			private _tryGetRow(rowOrId: string | RowValueType): RowValueType | undefined {
-				return typeof rowOrId === "string" ? this.getRow(rowOrId) : rowOrId;
+				const rowId = this._getRowId(rowOrId);
+				return this.getRow(rowId);
 			}
 
 			/**
-			 * Attempts to resolve a Row node or ID to a Row node.
-			 * If a node is provided, it is returned as-is.
-			 * If an ID is provided, we check the table for the corresponding Row node and return it if it exists, otherwise we throw an exception.
+			 * Attempts to resolve the provided Row node or ID to a Row node in the table.
+			 * @throws Throws a `UsageError` if there is no match.
+			 * @remarks Searches for a match based strictly on the ID and returns that result.
 			 */
 			private _getRow(rowOrId: string | RowValueType): RowValueType {
-				const row = typeof rowOrId === "string" ? this.getRow(rowOrId) : rowOrId;
+				const row = this._tryGetRow(rowOrId);
 				if (row === undefined) {
 					this._throwMissingRowError(this._getRowId(rowOrId));
 				}
@@ -1031,37 +983,6 @@ export namespace System_TableSchema {
 				return typeof rowOrId === "string" ? rowOrId : rowOrId.id;
 			}
 
-			/**
-			 * Attempts to resolve a Row node or ID to its index in the table.
-			 * Returns undefined if the Row does not exist in the table.
-			 */
-			private _getRowIndex(rowOrId: string | RowValueType): number | undefined {
-				const row = this._tryGetRow(rowOrId);
-				if (row === undefined) {
-					return undefined;
-				}
-				const index = this.rows.indexOf(row);
-				return index === -1 ? undefined : index;
-			}
-
-			/**
-			 * Checks if a Column with the specified ID exists in the table.
-			 */
-			private _containsRowWithId(rowId: string): boolean {
-				return this._getRowIndex(rowId) !== undefined;
-			}
-
-			/**
-			 * Assert that the provided Row exists in the table.
-			 * @throws Throws a `UsageError` if the Row does not exist.
-			 */
-			private _assertContainsRow(rowOrId: RowValueType | string): void {
-				const rowId = this._getRowId(rowOrId);
-				if (!this._containsRowWithId(rowId)) {
-					this._throwMissingRowError(rowId);
-				}
-			}
-
 			/**
 			 * Throw a `UsageError` for a missing Row by its ID.
 			 */