Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefix trivial issues when enabling no-unchecked-record-access for client packages #23432

Merged
merged 8 commits into from
Jan 6, 2025
Merged
2 changes: 1 addition & 1 deletion experimental/dds/attributable-map/src/mapKernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ export class AttributableMapKernel {
return nextVal.done
? { value: undefined, done: true }
: // Unpack the stored value
{ value: [nextVal.value[0], nextVal.value[1].value], done: false };
{ value: [nextVal.value[0], nextVal.value[1]?.value], done: false };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RishhiB, @alexvy86, @Abe27342, why do we like this change?
First, the type of nextVal.value[1] is ILocalValue in all type evaluation cases. There is no chance of undefined here (assuming typing is accurate).
Second, this changes the runtime behavior. Before we would throw here if nextVal.value[1] was actually undefined; now the undefined element is stored in the result.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that there was a complaint appears to be a failing of the linter rule. I suspect it doesn't recognize tuples.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My review was only for the devtools changes. I agree in this case the change in runtime behavior is reason enough to undo this bit and the similar ones throughout the PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened #23486

},
[Symbol.iterator](): IterableIterator<[string, unknown]> {
return this;
Expand Down
2 changes: 1 addition & 1 deletion packages/dds/map/src/directory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1607,7 +1607,7 @@ class SubDirectory extends TypedEventEmitter<IDirectoryEvents> implements IDirec
const nextVal = localEntriesIterator.next();
return nextVal.done
? { value: undefined, done: true }
: { value: [nextVal.value[0], nextVal.value[1].value], done: false };
: { value: [nextVal.value[0], nextVal.value[1]?.value], done: false };
},
[Symbol.iterator](): IterableIterator<[string, unknown]> {
return this;
Expand Down
2 changes: 1 addition & 1 deletion packages/dds/map/src/mapKernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ export class MapKernel {
return nextVal.done
? { value: undefined, done: true }
: // Unpack the stored value
{ value: [nextVal.value[0], nextVal.value[1].value], done: false };
{ value: [nextVal.value[0], nextVal.value[1]?.value], done: false };
},
[Symbol.iterator](): IterableIterator<[string, unknown]> {
return this;
Expand Down
20 changes: 10 additions & 10 deletions packages/dds/map/src/test/mocha/directory.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,12 @@ function serialize(directory1: ISharedDirectory): string {
assert.strictEqual(summaryObjectKeys.length, 1, "summary tree should only have one blob");
assert.strictEqual(summaryObjectKeys[0], "header", "summary should have a header blob");
assert.strictEqual(
summaryTree.tree.header.type,
summaryTree.tree.header?.type,
SummaryType.Blob,
"header is not of SummaryType.Blob",
);

const content = summaryTree.tree.header.content as string;
const content = summaryTree.tree.header?.content as string;
return JSON.stringify((JSON.parse(content) as IDirectoryNewStorageFormat).content);
}

Expand Down Expand Up @@ -1725,15 +1725,15 @@ describe("Directory", () => {
const fooSubDirIterator = fooSubDir.entries();
const fooSubDirResult1 = fooSubDirIterator.next();
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
assert.equal(fooSubDirResult1.value[0], "testKey");
assert.equal(fooSubDirResult1.value?.[0], "testKey");
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
assert.equal(fooSubDirResult1.value[1], "testValue");
assert.equal(fooSubDirResult1.value?.[1], "testValue");
assert.equal(fooSubDirResult1.done, false);
const fooSubDirResult2 = fooSubDirIterator.next();
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
assert.equal(fooSubDirResult2.value[0], "testKey2");
assert.equal(fooSubDirResult2.value?.[0], "testKey2");
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
assert.equal(fooSubDirResult2.value[1], "testValue2");
assert.equal(fooSubDirResult2.value?.[1], "testValue2");
assert.equal(fooSubDirResult2.done, false);
const fooSubDirResult3 = fooSubDirIterator.next();
assert.equal(fooSubDirResult3.value, undefined);
Expand All @@ -1755,15 +1755,15 @@ describe("Directory", () => {
const fooSubDir2Iterator = fooSubDir2.entries();
const fooSubDir2Result1 = fooSubDir2Iterator.next();
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
assert.equal(fooSubDir2Result1.value[0], "testKey");
assert.equal(fooSubDir2Result1.value?.[0], "testKey");
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
assert.equal(fooSubDir2Result1.value[1], "testValue");
assert.equal(fooSubDir2Result1.value?.[1], "testValue");
assert.equal(fooSubDir2Result1.done, false);
const fooSubDir2Result2 = fooSubDir2Iterator.next();
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
assert.equal(fooSubDir2Result2.value[0], "testKey2");
assert.equal(fooSubDir2Result2.value?.[0], "testKey2");
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
assert.equal(fooSubDir2Result2.value[1], "testValue2");
assert.equal(fooSubDir2Result2.value?.[1], "testValue2");
assert.equal(fooSubDir2Result2.done, false);
const fooSubDir2Result3 = fooSubDir2Iterator.next();
assert.equal(fooSubDir2Result3.value, undefined);
Expand Down
2 changes: 1 addition & 1 deletion packages/dds/merge-tree/src/test/beastTest.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1432,7 +1432,7 @@ export class DocumentTree {
});
this.pos++;
} else {
const trid = docNode.name + this.ids[docNode.name].toString();
const trid = docNode.name + this.ids[docNode.name]!.toString();
docNode.id = trid;
id = this.ids[docNode.name]++;
const props = {
Expand Down
12 changes: 6 additions & 6 deletions packages/dds/sequence/src/revertibles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ export function appendSharedStringDeltaToRevertibles(

revertible.intervals.push({
intervalId: interval.getIntervalId(),
label: interval.properties.referenceRangeLabels[0],
label: interval.properties.referenceRangeLabels?.[0],
Comment on lines 341 to +343
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RishhiB, @alexvy86, @Abe27342, I don't know why TypeScript allows this. label is typed are string. The ? could allow undefined. Perhaps TypeScript is thinking referenceRangeLabels is always defined and then ignoring the ?.
This doesn't look like an okay code pattern. Either we assert with ! (which is removed at transpile) or do something to handle undefined case. Silent storing of undefined for a string appears incorrect.

startOffset: offset,
endOffset,
});
Expand All @@ -350,7 +350,7 @@ export function appendSharedStringDeltaToRevertibles(
endIntervals.forEach(({ interval, offset }) => {
revertible.intervals.push({
intervalId: interval.getIntervalId(),
label: interval.properties.referenceRangeLabels[0],
label: interval.properties.referenceRangeLabels?.[0],
endOffset: offset,
});
});
Expand Down Expand Up @@ -432,7 +432,7 @@ function revertLocalAdd(
revertible: TypedRevertible<typeof IntervalOpType.ADD>,
) {
const id = getUpdatedIdFromInterval(revertible.interval);
const label = revertible.interval.properties.referenceRangeLabels[0];
const label = revertible.interval.properties.referenceRangeLabels?.[0];
string.getIntervalCollection(label).removeIntervalById(id);
}

Expand Down Expand Up @@ -460,7 +460,7 @@ function revertLocalDelete(
string: ISharedString,
revertible: TypedRevertible<typeof IntervalOpType.DELETE>,
) {
const label = revertible.interval.properties.referenceRangeLabels[0];
const label = revertible.interval.properties.referenceRangeLabels?.[0];
const collection = string.getIntervalCollection(label);
const start = string.localReferencePositionToPosition(revertible.start);
const startSlidePos = getSlidePosition(string, revertible.start, start);
Expand Down Expand Up @@ -499,7 +499,7 @@ function revertLocalChange(
string: ISharedString,
revertible: TypedRevertible<typeof IntervalOpType.CHANGE>,
) {
const label = revertible.interval.properties.referenceRangeLabels[0];
const label = revertible.interval.properties.referenceRangeLabels?.[0];
const collection = string.getIntervalCollection(label);
const id = getUpdatedIdFromInterval(revertible.interval);
const start = string.localReferencePositionToPosition(revertible.start);
Expand Down Expand Up @@ -539,7 +539,7 @@ function revertLocalPropertyChanged(
string: ISharedString,
revertible: TypedRevertible<typeof IntervalOpType.PROPERTY_CHANGED>,
) {
const label = revertible.interval.properties.referenceRangeLabels[0];
const label = revertible.interval.properties.referenceRangeLabels?.[0];
const id = getUpdatedIdFromInterval(revertible.interval);
const newProps = revertible.propertyDeltas;
string.getIntervalCollection(label).change(id, { props: newProps });
Expand Down
6 changes: 3 additions & 3 deletions packages/drivers/odsp-driver/src/WriteBufferUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export class WriteBuffer {
let index = 0;
const oldData = this.data;
while (index < length) {
newData[index] = oldData[index];
newData[index] = oldData[index]!;
index++;
}
this.data = newData;
Expand Down Expand Up @@ -233,8 +233,8 @@ function serializeNodeCore(
for (const child of nodeCore.nodes) {
if (child instanceof NodeCore) {
// For a tree node start and end with set/list start and end marker codes.
const startCode = MarkerCodesStart[child.type];
const endCode = MarkerCodesEnd[child.type];
const startCode: MarkerCodesStart | undefined = MarkerCodesStart[child.type];
const endCode: MarkerCodesEnd | undefined = MarkerCodesEnd[child.type];
assert(startCode !== undefined, 0x285 /* "Start code should not undefined" */);
assert(endCode !== undefined, 0x286 /* "End code should not undefined" */);
buffer.write(startCode);
Expand Down
10 changes: 5 additions & 5 deletions packages/drivers/odsp-driver/src/compactSnapshotParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ function readBlobSection(node: NodeTypes): {
const records = getNodeProps(blob);
assertBlobCoreInstance(records.data, "data should be of BlobCore type");
const id = getStringInstance(records.id, "blob id should be string");
blobContents.set(id, records.data.arrayBuffer);
blobContents.set(id, records.data?.arrayBuffer);
}
}
return { blobContents, slowBlobStructureCount };
Expand All @@ -88,15 +88,15 @@ function readOpsSection(node: NodeTypes): ISequencedDocumentMessage[] {
const records = getNodeProps(node);
assertNumberInstance(records.firstSequenceNumber, "Seq number should be a number");
assertNodeCoreInstance(records.deltas, "Deltas should be a Node");
for (let i = 0; i < records.deltas.length; ++i) {
for (let i = 0; i < records.deltas?.length; ++i) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
ops.push(JSON.parse(records.deltas.getString(i)));
ops.push(JSON.parse(records.deltas?.getString(i)));
}
// Due to a bug at service side, in an edge case service was serializing deltas even
// when there are no ops. So just make the code resilient to that bug. Service has also
// fixed that bug.
assert(
ops.length === 0 || records.firstSequenceNumber.valueOf() === ops[0].sequenceNumber,
ops.length === 0 || records.firstSequenceNumber?.valueOf() === ops[0].sequenceNumber,
0x280 /* "Validate first op seq number" */,
);
return ops;
Expand Down Expand Up @@ -244,7 +244,7 @@ function readSnapshotSection(node: NodeTypes): {
const { snapshotTree, slowTreeStructureCount, treeStructureCountWithGroupId } =
readTreeSection(records.treeNodes);
snapshotTree.id = getStringInstance(records.id, "snapshotId should be string");
const sequenceNumber = records.sequenceNumber.valueOf();
const sequenceNumber = records.sequenceNumber?.valueOf();
return {
sequenceNumber,
snapshotTree,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,9 @@ export abstract class OdspDocumentStorageServiceBase implements IDocumentStorage
protected combineProtocolAndAppSnapshotTree(snapshotTree: ISnapshotTree): ISnapshotTree {
// When we upload the container snapshot, we upload appTree in ".app" and protocol tree in ".protocol"
// So when we request the snapshot we get ".app" as tree and not as commit node as in the case just above.
const hierarchicalAppTree = snapshotTree.trees[".app"];
const hierarchicalProtocolTree = snapshotTree.trees[".protocol"];
const hierarchicalAppTree: ISnapshotTree | undefined = snapshotTree.trees[".app"];
const hierarchicalProtocolTree: ISnapshotTree | undefined =
snapshotTree.trees[".protocol"];
const summarySnapshotTree: ISnapshotTree = {
blobs: {
...hierarchicalAppTree.blobs,
Expand Down
2 changes: 1 addition & 1 deletion packages/drivers/odsp-driver/src/odspDriverUrlResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export class OdspDriverUrlResolver implements IUrlResolver {

const searchParams = new URLSearchParams(queryString);
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
const fileName: string = request.headers[DriverHeader.createNew].fileName;
const fileName: string = request.headers[DriverHeader.createNew]?.fileName;
const driveID = searchParams.get("driveId");
const filePath = searchParams.get("path");
const packageName = searchParams.get("containerPackageName");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ import { strict as assert } from "node:assert";

import { bufferToString, fromBase64ToUtf8 } from "@fluid-internal/client-utils";
import { ISummaryTree, SummaryType } from "@fluidframework/driver-definitions";
import { ISnapshot, IDocumentAttributes } from "@fluidframework/driver-definitions/internal";
import {
ISnapshot,
IDocumentAttributes,
type ISnapshotTree,
} from "@fluidframework/driver-definitions/internal";
import {
IFileEntry,
IOdspResolvedUrl,
Expand Down Expand Up @@ -139,18 +143,18 @@ describe("Create New Utils Tests", () => {
);
assert.strictEqual(snapshot.blobContents.size, 2, "2 blobs should be there");

const appTree = snapshotTree.trees[".app"];
const protocolTree = snapshotTree.trees[".protocol"];
const appTree: ISnapshotTree | undefined = snapshotTree.trees[".app"];
const protocolTree: ISnapshotTree | undefined = snapshotTree.trees[".protocol"];
assert(appTree !== undefined, "App tree should be there");
assert(protocolTree !== undefined, "Protocol tree should be there");

const appTreeBlobId = appTree.blobs.attributes;
const appTreeBlobId: string | undefined = appTree.blobs.attributes;
const appTreeBlobValBuffer = snapshot.blobContents.get(appTreeBlobId);
assert(appTreeBlobValBuffer !== undefined, "app blob value should exist");
const appTreeBlobVal = bufferToString(appTreeBlobValBuffer, "utf8");
assert(appTreeBlobVal === blobContent, "Blob content should match");

const docAttributesBlobId = protocolTree.blobs.attributes;
const docAttributesBlobId: string | undefined = protocolTree.blobs.attributes;
const docAttributesBuffer = snapshot.blobContents.get(docAttributesBlobId);
assert(docAttributesBuffer !== undefined, "protocol attributes blob value should exist");
const docAttributesBlobValue = bufferToString(docAttributesBuffer, "utf8");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe("getHeadersWithAuth", () => {
result: { [index: string]: string },
): void => {
assert.strictEqual(
result.Authorization.endsWith(token),
result.Authorization?.endsWith(token),
true,
"Returned header must contain token",
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -626,9 +626,9 @@ describe("Tests for prefetching snapshot", () => {
};
const odspCompactSnapshotWithGroupId = convertToCompactSnapshot(snapshotWithGroupId);
const snapshotTreeWithGroupIdToCompare: ISnapshotTree = {
blobs: { ...snapshotTreeWithGroupId.trees[".app"].blobs },
blobs: { ...snapshotTreeWithGroupId.trees[".app"]?.blobs },
trees: {
...snapshotTreeWithGroupId.trees[".app"].trees,
...snapshotTreeWithGroupId.trees[".app"]?.trees,
".protocol": snapshotTreeWithGroupId.trees[".protocol"],
},
id: "SnapshotId",
Expand Down Expand Up @@ -866,9 +866,9 @@ describe("Tests for prefetching snapshot", () => {
};
const odspCompactSnapshotWithGroupId = convertToCompactSnapshot(snapshotWithGroupId);
const snapshotTreeWithGroupIdToCompare: ISnapshotTree = {
blobs: { ...snapshotTreeWithGroupId.trees[".app"].blobs },
blobs: { ...snapshotTreeWithGroupId.trees[".app"]?.blobs },
trees: {
...snapshotTreeWithGroupId.trees[".app"].trees,
...snapshotTreeWithGroupId.trees[".app"]?.trees,
".protocol": snapshotTreeWithGroupId.trees[".protocol"],
},
id: "SnapshotId",
Expand Down
4 changes: 2 additions & 2 deletions packages/drivers/replay-driver/src/storageImplementations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,9 @@ export class FileSnapshotReader
throw new Error(`Unknown version id: ${versionRequested}`);
}

let snapshotTree = this.trees[versionRequested.id];
let snapshotTree: ISnapshotTree | undefined = this.trees[versionRequested.id];
if (snapshotTree === undefined) {
const tree = this.commits[versionRequested.id];
const tree: ITree | undefined = this.commits[versionRequested.id];
if (tree === undefined) {
throw new Error(`Can't find version ${versionRequested.id}`);
}
Expand Down
6 changes: 3 additions & 3 deletions packages/drivers/routerlicious-driver/src/documentService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,9 +302,9 @@ export class DocumentService
}
const fluidResolvedUrl = response.resolvedUrl;
this._resolvedUrl = fluidResolvedUrl;
this.storageUrl = fluidResolvedUrl.endpoints.storageUrl;
this.ordererUrl = fluidResolvedUrl.endpoints.ordererUrl;
this.deltaStorageUrl = fluidResolvedUrl.endpoints.deltaStorageUrl;
this.storageUrl = fluidResolvedUrl.endpoints?.storageUrl;
this.ordererUrl = fluidResolvedUrl.endpoints?.ordererUrl;
this.deltaStorageUrl = fluidResolvedUrl.endpoints?.deltaStorageUrl;
this.deltaStreamUrl = fluidResolvedUrl.endpoints.deltaStreamUrl ?? this.ordererUrl;
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ export class RouterliciousDocumentServiceFactory implements IDocumentServiceFact
}

parsedUrl.pathname = replaceDocumentIdInPath(parsedUrl.pathname, documentId);
const deltaStorageUrl = resolvedUrl.endpoints.deltaStorageUrl;
const deltaStorageUrl = resolvedUrl.endpoints?.deltaStorageUrl;
if (!deltaStorageUrl) {
throw new Error(
`All endpoints urls must be provided. [deltaStorageUrl:${deltaStorageUrl}]`,
Expand Down Expand Up @@ -303,9 +303,9 @@ export class RouterliciousDocumentServiceFactory implements IDocumentServiceFact
},
);

const storageUrl = fluidResolvedUrl.endpoints.storageUrl;
const ordererUrl = fluidResolvedUrl.endpoints.ordererUrl;
const deltaStorageUrl = fluidResolvedUrl.endpoints.deltaStorageUrl;
const storageUrl = fluidResolvedUrl.endpoints?.storageUrl;
const ordererUrl = fluidResolvedUrl.endpoints?.ordererUrl;
const deltaStorageUrl = fluidResolvedUrl.endpoints?.deltaStorageUrl;
const deltaStreamUrl = fluidResolvedUrl.endpoints.deltaStreamUrl || ordererUrl; // backward compatibility
if (!ordererUrl || !deltaStorageUrl) {
throw new Error(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ function generateSummaryWithContent(contentSize: number) {
".channels"
] as ISummaryTree
).tree["7a99532d-94ec-43ac-8a53-d9f978ad4ae9"] as ISummaryTree
).tree.header;
).tree?.header;
let contentString = "";
while (contentString.length < contentSize) {
if (contentString.length + 10 > contentSize) {
Expand All @@ -91,7 +91,7 @@ function generateSummaryWithBinaryContent(startsWith: number, contentSize: numbe
".channels"
] as ISummaryTree
).tree["7a99532d-94ec-43ac-8a53-d9f978ad4ae9"] as ISummaryTree
).tree.header;
).tree?.header;
const content = new Uint8Array(contentSize);
content[0] = startsWith;
for (let i = 1; i < contentSize; i = i + 10) {
Expand Down Expand Up @@ -621,7 +621,7 @@ function getHeaderContent(summary: ISummaryTree) {
}

function getHeader(summary: ISummaryTree) {
return getHeaderHolder(summary).tree.header;
return getHeaderHolder(summary).tree?.header;
}

function getHeaderHolder(summary: ISummaryTree) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
type FetchSource,
type IDocumentAttributes,
SummaryType,
type SummaryObject,
} from "@fluidframework/driver-definitions/internal";
import {
ISummaryTreeWithStats,
Expand Down Expand Up @@ -2093,7 +2094,7 @@ describe("Runtime", () => {
false,
);
const { summary } = await containerRuntime.summarize({ fullTree: true });
const blob = summary.tree[recentBatchInfoBlobName];
const blob: SummaryObject | undefined = summary.tree[recentBatchInfoBlobName];
assert(blob.type === SummaryType.Blob, "Expected blob");
assert.equal(blob.content, '[[123,"batchId1"]]', "Expected single batchId mapping");

Expand Down Expand Up @@ -2423,7 +2424,7 @@ describe("Runtime", () => {
["missingDataStore"],
);
assert.deepStrictEqual(
snapshotTree.trees[".channels"].trees.missingDataStore,
snapshotTree.trees[".channels"]?.trees.missingDataStore,
snapshot.snapshotTree,
"snapshot should be equal",
);
Expand Down
Loading
Loading