From 7edcd0410b7052bfd1e42f98c0b8d3449824135e Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 8 Jul 2024 18:16:48 -0400 Subject: [PATCH] Enable noUncheckedIndexedAccess for routerlicious (#21487) Enable noUncheckedIndexedAccess for routerlicious The property noUncheckedIndexedAccess is being enabled to improve type safety by making the TypeScript compiler assume all arrays are sparse, which requires runtime validation or non-null assertions for indexed access. This aims to catch potential issues with type changes and ensure API semver compliance [AB#8216](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8216) --- .../routerlicious-driver/src/createNewUtils.ts | 5 +---- .../routerlicious-driver/src/deltaStorageService.ts | 2 +- .../routerlicious-driver/src/documentService.ts | 11 +++++++---- .../src/documentServiceFactory.ts | 9 ++++++--- .../routerlicious-driver/src/r11sSnapshotParser.ts | 8 +++++--- .../routerlicious-driver/src/restWrapperBase.ts | 6 +++--- .../src/shreddedSummaryDocumentStorageService.ts | 7 ++++--- .../src/summaryTreeUploadManager.ts | 8 ++++---- .../drivers/routerlicious-driver/src/urlUtils.ts | 6 ++++-- .../src/wholeSummaryDocumentStorageService.ts | 7 ++++--- packages/drivers/routerlicious-driver/tsconfig.json | 1 - .../routerlicious-urlResolver/src/urlResolver.ts | 12 +++++++++--- .../drivers/routerlicious-urlResolver/tsconfig.json | 1 - .../test-runtime-utils/src/assertionShortCodesMap.ts | 3 ++- 14 files changed, 50 insertions(+), 36 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/createNewUtils.ts b/packages/drivers/routerlicious-driver/src/createNewUtils.ts index 37675d3dca95..3de896017101 100644 --- a/packages/drivers/routerlicious-driver/src/createNewUtils.ts +++ b/packages/drivers/routerlicious-driver/src/createNewUtils.ts @@ -12,10 +12,7 @@ import { ISummaryTree, SummaryType } from "@fluidframework/driver-definitions"; * @returns Modified summary tree where the blob contents could be utf8 string only. */ export function convertSummaryToCreateNewSummary(summary: ISummaryTree): ISummaryTree { - const keys = Object.keys(summary.tree); - for (const key of keys) { - const summaryObject = summary.tree[key]; - + for (const [key, summaryObject] of Object.entries(summary.tree)) { switch (summaryObject.type) { case SummaryType.Tree: { summary.tree[key] = convertSummaryToCreateNewSummary(summaryObject); diff --git a/packages/drivers/routerlicious-driver/src/deltaStorageService.ts b/packages/drivers/routerlicious-driver/src/deltaStorageService.ts index 430696579862..26d1b31ebbb9 100644 --- a/packages/drivers/routerlicious-driver/src/deltaStorageService.ts +++ b/packages/drivers/routerlicious-driver/src/deltaStorageService.ts @@ -83,7 +83,7 @@ export class DocumentDeltaStorageService implements IDocumentDeltaStorageService (op) => op.sequenceNumber >= from && op.sequenceNumber < to, ); validateMessages("snapshotOps", messages, from, this.logger, false /* strict */); - if (messages.length > 0 && messages[0].sequenceNumber === from) { + if (messages.length > 0 && messages[0] && messages[0].sequenceNumber === from) { this.snapshotOps = this.snapshotOps.filter((op) => op.sequenceNumber >= to); opsFromSnapshot += messages.length; return { messages, partialResult: true }; diff --git a/packages/drivers/routerlicious-driver/src/documentService.ts b/packages/drivers/routerlicious-driver/src/documentService.ts index 5b77277d8bda..0d029e9904bd 100644 --- a/packages/drivers/routerlicious-driver/src/documentService.ts +++ b/packages/drivers/routerlicious-driver/src/documentService.ts @@ -327,10 +327,13 @@ export class DocumentService private async refreshDiscoveryCore(): Promise { const fluidResolvedUrl = await this.discoverFluidResolvedUrl(); this._resolvedUrl = fluidResolvedUrl; - this.storageUrl = fluidResolvedUrl.endpoints.storageUrl; - this.ordererUrl = fluidResolvedUrl.endpoints.ordererUrl; - this.deltaStorageUrl = fluidResolvedUrl.endpoints.deltaStorageUrl; - this.deltaStreamUrl = fluidResolvedUrl.endpoints.deltaStreamUrl || this.ordererUrl; + // TODO why are we non null asserting here? + this.storageUrl = fluidResolvedUrl.endpoints.storageUrl!; + // TODO why are we non null asserting here? + this.ordererUrl = fluidResolvedUrl.endpoints.ordererUrl!; + // TODO why are we non null asserting here? + this.deltaStorageUrl = fluidResolvedUrl.endpoints.deltaStorageUrl!; + this.deltaStreamUrl = fluidResolvedUrl.endpoints.deltaStreamUrl ?? this.ordererUrl; } /** diff --git a/packages/drivers/routerlicious-driver/src/documentServiceFactory.ts b/packages/drivers/routerlicious-driver/src/documentServiceFactory.ts index 09de266ce826..597b501d94e9 100644 --- a/packages/drivers/routerlicious-driver/src/documentServiceFactory.ts +++ b/packages/drivers/routerlicious-driver/src/documentServiceFactory.ts @@ -113,6 +113,7 @@ export class RouterliciousDocumentServiceFactory implements IDocumentServiceFact throw new Error("Parsed url should contain tenant and doc Id!!"); } const [, tenantId] = parsedUrl.pathname.split("/"); + assert(tenantId !== undefined, 0x9ac /* "Missing tenant ID!" */); if (!isCombinedAppAndProtocolSummary(createNewSummary)) { throw new Error("Protocol and App Summary required in the full summary"); @@ -320,10 +321,12 @@ export class RouterliciousDocumentServiceFactory implements IDocumentServiceFact ? getDiscoveredFluidResolvedUrl(resolvedUrl, session) : await discoverFluidResolvedUrl(); - const storageUrl = fluidResolvedUrl.endpoints.storageUrl; - const ordererUrl = fluidResolvedUrl.endpoints.ordererUrl; + // TODO why are we non null asserting here? + const storageUrl = fluidResolvedUrl.endpoints.storageUrl!; + // TODO why are we non null asserting here? + const ordererUrl = fluidResolvedUrl.endpoints.ordererUrl!; const deltaStorageUrl = fluidResolvedUrl.endpoints.deltaStorageUrl; - const deltaStreamUrl = fluidResolvedUrl.endpoints.deltaStreamUrl || ordererUrl; // backward compatibility + const deltaStreamUrl = fluidResolvedUrl.endpoints.deltaStreamUrl ?? ordererUrl; // backward compatibility if (!ordererUrl || !deltaStorageUrl) { throw new Error( `All endpoints urls must be provided. [ordererUrl:${ordererUrl}][deltaStorageUrl:${deltaStorageUrl}]`, diff --git a/packages/drivers/routerlicious-driver/src/r11sSnapshotParser.ts b/packages/drivers/routerlicious-driver/src/r11sSnapshotParser.ts index 417cc89656aa..bf2009557ae8 100644 --- a/packages/drivers/routerlicious-driver/src/r11sSnapshotParser.ts +++ b/packages/drivers/routerlicious-driver/src/r11sSnapshotParser.ts @@ -36,7 +36,8 @@ function buildHierarchy( const entryPathBase = entryPath.slice(lastIndex + 1); // The flat output is breadth-first so we can assume we see tree nodes prior to their contents - const node = lookup[entryPathDir]; + // TODO why are we non null asserting here? + const node = lookup[entryPathDir]!; // Add in either the blob or tree if (entry.type === "tree") { @@ -75,8 +76,9 @@ export function convertWholeFlatSnapshotToSnapshotTreeAndBlobs( blobs.set(blob.id, stringToBuffer(blob.content, blob.encoding ?? "utf-8")); }); } - const flatSnapshotTree = flatSnapshot.trees?.[0]; - const sequenceNumber = flatSnapshotTree?.sequenceNumber; + // TODO why are we non null asserting here? + const flatSnapshotTree = flatSnapshot.trees[0]!; + const sequenceNumber = flatSnapshotTree.sequenceNumber; const snapshotTree = buildHierarchy(flatSnapshotTree, treePrefixToRemove); return { diff --git a/packages/drivers/routerlicious-driver/src/restWrapperBase.ts b/packages/drivers/routerlicious-driver/src/restWrapperBase.ts index 89e02b6e70c3..758d876a3f2c 100644 --- a/packages/drivers/routerlicious-driver/src/restWrapperBase.ts +++ b/packages/drivers/routerlicious-driver/src/restWrapperBase.ts @@ -133,10 +133,10 @@ export abstract class RestWrapper { */ export function getQueryString(queryParams: QueryStringType): string { let queryString = ""; - for (const key of Object.keys(queryParams)) { - if (queryParams[key] !== undefined) { + for (const [key, value] of Object.entries(queryParams)) { + if (value !== undefined) { const startChar = queryString === "" ? "?" : "&"; - queryString += `${startChar}${key}=${encodeURIComponent(queryParams[key])}`; + queryString += `${startChar}${key}=${encodeURIComponent(value)}`; } } diff --git a/packages/drivers/routerlicious-driver/src/shreddedSummaryDocumentStorageService.ts b/packages/drivers/routerlicious-driver/src/shreddedSummaryDocumentStorageService.ts index 0ca3e1947246..d766cd9fdc63 100644 --- a/packages/drivers/routerlicious-driver/src/shreddedSummaryDocumentStorageService.ts +++ b/packages/drivers/routerlicious-driver/src/shreddedSummaryDocumentStorageService.ts @@ -98,13 +98,14 @@ export class ShreddedSummaryDocumentStorageService implements IDocumentStorageSe public async getSnapshotTree(version?: IVersion): Promise { let requestVersion = version; - if (!requestVersion) { + if (requestVersion === undefined) { const versions = await this.getVersions(this.id, 1); - if (versions.length === 0) { + const firstVersion = versions[0]; + if (firstVersion === undefined) { return null; } - requestVersion = versions[0]; + requestVersion = firstVersion; } const cachedSnapshotTree = await this.snapshotTreeCache?.get( diff --git a/packages/drivers/routerlicious-driver/src/summaryTreeUploadManager.ts b/packages/drivers/routerlicious-driver/src/summaryTreeUploadManager.ts index 78faa9a5b9f2..2758e612fa2e 100644 --- a/packages/drivers/routerlicious-driver/src/summaryTreeUploadManager.ts +++ b/packages/drivers/routerlicious-driver/src/summaryTreeUploadManager.ts @@ -41,8 +41,7 @@ export class SummaryTreeUploadManager implements ISummaryUploadManager { previousFullSnapshot: ISnapshotTreeEx | undefined, ): Promise { const entries = await Promise.all( - Object.keys(summaryTree.tree).map(async (key) => { - const entry = summaryTree.tree[key]; + Object.entries(summaryTree.tree).map(async ([key, entry]) => { const pathHandle = await this.writeSummaryTreeObject(entry, previousFullSnapshot); const treeEntry: IGitCreateTreeEntry = { mode: getGitMode(entry), @@ -123,8 +122,8 @@ export class SummaryTreeUploadManager implements ISummaryUploadManager { /** Previous snapshot, subtree relative to this path part */ previousSnapshot: ISnapshotTreeEx, ): string { - assert(path.length > 0, 0x0b3 /* "Expected at least 1 path part" */); const key = path[0]; + assert(path.length > 0 && key !== undefined, 0x0b3 /* "Expected at least 1 path part" */); if (path.length === 1) { switch (handleType) { case SummaryType.Blob: { @@ -147,6 +146,7 @@ export class SummaryTreeUploadManager implements ISummaryUploadManager { throw Error(`Unexpected handle summary object type: "${handleType}".`); } } - return this.getIdFromPathCore(handleType, path.slice(1), previousSnapshot.trees[key]); + // TODO why are we non null asserting here? + return this.getIdFromPathCore(handleType, path.slice(1), previousSnapshot.trees[key]!); } } diff --git a/packages/drivers/routerlicious-driver/src/urlUtils.ts b/packages/drivers/routerlicious-driver/src/urlUtils.ts index ec47344ae04e..bc2698c97fcc 100644 --- a/packages/drivers/routerlicious-driver/src/urlUtils.ts +++ b/packages/drivers/routerlicious-driver/src/urlUtils.ts @@ -20,11 +20,13 @@ export const getDiscoveredFluidResolvedUrl = ( session: ISession, ): IResolvedUrl => { const discoveredOrdererUrl = new URL(session.ordererUrl); - const deltaStorageUrl = new URL(resolvedUrl.endpoints.deltaStorageUrl); + // TODO why are we non null asserting here? + const deltaStorageUrl = new URL(resolvedUrl.endpoints.deltaStorageUrl!); deltaStorageUrl.host = discoveredOrdererUrl.host; const discoveredStorageUrl = new URL(session.historianUrl); - const storageUrl = new URL(resolvedUrl.endpoints.storageUrl); + // TODO why are we non null asserting here? + const storageUrl = new URL(resolvedUrl.endpoints.storageUrl!); storageUrl.host = discoveredStorageUrl.host; const parsedUrl = new URL(resolvedUrl.url); diff --git a/packages/drivers/routerlicious-driver/src/wholeSummaryDocumentStorageService.ts b/packages/drivers/routerlicious-driver/src/wholeSummaryDocumentStorageService.ts index f722d842a413..85084da5f400 100644 --- a/packages/drivers/routerlicious-driver/src/wholeSummaryDocumentStorageService.ts +++ b/packages/drivers/routerlicious-driver/src/wholeSummaryDocumentStorageService.ts @@ -161,13 +161,14 @@ export class WholeSummaryDocumentStorageService implements IDocumentStorageServi // eslint-disable-next-line @rushstack/no-new-null public async getSnapshotTree(version?: IVersion): Promise { let requestVersion = version; - if (!requestVersion) { + if (requestVersion === undefined) { const versions = await this.getVersions(this.id, 1); - if (versions.length === 0) { + const firstVersion = versions[0]; + if (firstVersion === undefined) { return null; } - requestVersion = versions[0]; + requestVersion = firstVersion; } let normalizedWholeSnapshot = await this.snapshotTreeCache.get( diff --git a/packages/drivers/routerlicious-driver/tsconfig.json b/packages/drivers/routerlicious-driver/tsconfig.json index 679025b46439..9a9e0de4d11b 100644 --- a/packages/drivers/routerlicious-driver/tsconfig.json +++ b/packages/drivers/routerlicious-driver/tsconfig.json @@ -6,6 +6,5 @@ "rootDir": "./src", "outDir": "./lib", "exactOptionalPropertyTypes": false, - "noUncheckedIndexedAccess": false, }, } diff --git a/packages/drivers/routerlicious-urlResolver/src/urlResolver.ts b/packages/drivers/routerlicious-urlResolver/src/urlResolver.ts index ffb017ce5ca8..edc22fa50dab 100644 --- a/packages/drivers/routerlicious-urlResolver/src/urlResolver.ts +++ b/packages/drivers/routerlicious-urlResolver/src/urlResolver.ts @@ -62,11 +62,17 @@ export class RouterliciousUrlResolver implements IUrlResolver { documentId = this.config.documentId; provider = this.config.provider; } else if (path.length >= 4) { - tenantId = path[2]; - documentId = path[3]; + // Non null asserting here because of the length check above + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + tenantId = path[2]!; + // Non null asserting here because of the length check above + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + documentId = path[3]!; } else { tenantId = "fluid"; - documentId = path[2]; + // TODO why are we non null asserting here? + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + documentId = path[2]!; } const token = await this.getToken(); diff --git a/packages/drivers/routerlicious-urlResolver/tsconfig.json b/packages/drivers/routerlicious-urlResolver/tsconfig.json index 7d00bfd90e9a..9ea9ac49d6aa 100644 --- a/packages/drivers/routerlicious-urlResolver/tsconfig.json +++ b/packages/drivers/routerlicious-urlResolver/tsconfig.json @@ -6,6 +6,5 @@ "rootDir": "./src", "outDir": "./lib", "types": ["node"], - "noUncheckedIndexedAccess": false, }, } diff --git a/packages/runtime/test-runtime-utils/src/assertionShortCodesMap.ts b/packages/runtime/test-runtime-utils/src/assertionShortCodesMap.ts index afbde4056c30..437fdaf14aaf 100644 --- a/packages/runtime/test-runtime-utils/src/assertionShortCodesMap.ts +++ b/packages/runtime/test-runtime-utils/src/assertionShortCodesMap.ts @@ -1572,5 +1572,6 @@ export const shortCodeMap = { "0x9a8": "should be in fields mode", "0x9a9": "expected to find a parent commit", "0x9aa": "identifier must be type string", - "0x9ab": "childId is undefined in unpackChildNodesUsedRoutes" + "0x9ab": "childId is undefined in unpackChildNodesUsedRoutes", + "0x9ac": "Missing tenant ID!" };