Skip to content

Commit

Permalink
Enable noUncheckedIndexedAccess for routerlicious (#21487)
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
RishhiB authored Jul 8, 2024
1 parent cb8a88b commit 7edcd04
Show file tree
Hide file tree
Showing 14 changed files with 50 additions and 36 deletions.
5 changes: 1 addition & 4 deletions packages/drivers/routerlicious-driver/src/createNewUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down
11 changes: 7 additions & 4 deletions packages/drivers/routerlicious-driver/src/documentService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,13 @@ export class DocumentService
private async refreshDiscoveryCore(): Promise<void> {
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;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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}]`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions packages/drivers/routerlicious-driver/src/restWrapperBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)}`;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,14 @@ export class ShreddedSummaryDocumentStorageService implements IDocumentStorageSe

public async getSnapshotTree(version?: IVersion): Promise<ISnapshotTreeEx | null> {
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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ export class SummaryTreeUploadManager implements ISummaryUploadManager {
previousFullSnapshot: ISnapshotTreeEx | undefined,
): Promise<string> {
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),
Expand Down Expand Up @@ -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: {
Expand All @@ -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]!);
}
}
6 changes: 4 additions & 2 deletions packages/drivers/routerlicious-driver/src/urlUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,14 @@ export class WholeSummaryDocumentStorageService implements IDocumentStorageServi
// eslint-disable-next-line @rushstack/no-new-null
public async getSnapshotTree(version?: IVersion): Promise<ISnapshotTree | null> {
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(
Expand Down
1 change: 0 additions & 1 deletion packages/drivers/routerlicious-driver/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,5 @@
"rootDir": "./src",
"outDir": "./lib",
"exactOptionalPropertyTypes": false,
"noUncheckedIndexedAccess": false,
},
}
12 changes: 9 additions & 3 deletions packages/drivers/routerlicious-urlResolver/src/urlResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 0 additions & 1 deletion packages/drivers/routerlicious-urlResolver/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,5 @@
"rootDir": "./src",
"outDir": "./lib",
"types": ["node"],
"noUncheckedIndexedAccess": false,
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -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!"
};

0 comments on commit 7edcd04

Please sign in to comment.