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.

Sign up for GitHub

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

eslint(container-loader): Prefix container-loader before enabling no-unchecked-record-access #23423

Merged
merged 4 commits into from
Jan 6, 2025
Merged

eslint(container-loader): Prefix container-loader before enabling no-unchecked-record-access #23423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
84a7b59
eslint(container-loader): Prefix container-loader before enabling no-…
RishhiB Jan 2, 2025
9d80bfe
Merge branch 'main' into preFixContainerLoader
RishhiB Jan 2, 2025
0704f0d
format
RishhiB Jan 2, 2025
3d51d5e
change back to warn
RishhiB Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/loader/container-loader/src/containerStorageAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ export class ContainerStorageAdapter
}

public async readBlob(id: string): Promise<ArrayBufferLike> {
const maybeBlob = this.blobContents[id];
const maybeBlob: string | ArrayBufferLike | undefined = this.blobContents[id];
if (maybeBlob !== undefined) {
if (typeof maybeBlob === "string") {
const blob = stringToBuffer(maybeBlob, "utf8");
Expand Down Expand Up @@ -361,7 +361,7 @@ async function getBlobManagerTreeFromTree(
blobs: ISerializableBlobContents,
storage: Pick<IDocumentStorageService, "readBlob">,
): Promise<void> {
const id = tree.blobs[redirectTableBlobName];
const id: string | undefined = tree.blobs[redirectTableBlobName];
assert(id !== undefined, 0x9ce /* id is undefined in getBlobManagerTreeFromTree */);
const blob = await storage.readBlob(id);
// ArrayBufferLike will not survive JSON.stringify()
Expand Down Expand Up @@ -404,7 +404,7 @@ function getBlobManagerTreeFromTreeWithBlobContents(
tree: ISnapshotTreeWithBlobContents,
blobs: ISerializableBlobContents,
): void {
const id = tree.blobs[redirectTableBlobName];
const id: string | undefined = tree.blobs[redirectTableBlobName];
assert(
id !== undefined,
0x9cf /* id is undefined in getBlobManagerTreeFromTreeWithBlobContents */,
Expand Down
5 changes: 3 additions & 2 deletions packages/loader/container-loader/src/test/serializedStateManager.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,9 @@ const getAttributesFromPendingState = (
if (pendingState.baseSnapshot === undefined) {
throw new Error("base snapshot should be valid");
}
const attributesId = pendingState.baseSnapshot.trees[".protocol"].blobs.attributes;
const attributes = pendingState.snapshotBlobs[attributesId];
const attributesId: string | undefined =
pendingState.baseSnapshot.trees[".protocol"]?.blobs.attributes;
const attributes: string | undefined = pendingState.snapshotBlobs[attributesId];
return JSON.parse(attributes) as IDocumentAttributes;
Comment on lines -157 to 160
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this pattern isn't actually working for us.
JSON.parse does not accept undefined. It only accepts string.
If you hover on attributes on line 160 in VS Code, it will say the type is string. TypeScript appears to ignore the explicit type when it thinks it can successfully narrow the type to something more specific.
It doesn't look like we can use this pattern to be safe. (It might be okay if the linter followed the uses of the variables to double check typing. I would guess that is pretty hard.)

};

Expand Down
20 changes: 11 additions & 9 deletions packages/loader/container-loader/src/test/snapshotConversionTest.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,15 @@ describe("Dehydrate Container", () => {

assert.strictEqual(Object.keys(baseSnapshot.trees).length, 2, "2 trees should be there");
assert.strictEqual(
Object.keys(baseSnapshot.trees[".protocol"].blobs).length,
Object.keys(baseSnapshot.trees[".protocol"]?.blobs).length,
2,
"2 protocol blobs should be there.",
);

// Validate the ".component" blob.
const defaultDataStoreBlobId = baseSnapshot.trees.default.blobs[".component"];
const defaultDataStoreBlob = snapshotBlobs[defaultDataStoreBlobId];
const defaultDataStoreBlobId: string | undefined =
baseSnapshot.trees.default?.blobs[".component"];
const defaultDataStoreBlob: string | undefined = snapshotBlobs[defaultDataStoreBlobId];
assert.strict(defaultDataStoreBlob, "defaultDataStoreBlob undefined");
assert.strictEqual(
JSON.parse(defaultDataStoreBlob),
Expand All @@ -83,37 +84,38 @@ describe("Dehydrate Container", () => {
);

// Validate "root" sub-tree.
const rootAttributesBlobId = baseSnapshot.trees.default.trees.root.blobs.attributes;
const rootAttributesBlob = snapshotBlobs[rootAttributesBlobId];
const rootAttributesBlobId: string | undefined =
baseSnapshot.trees.default.trees.root?.blobs.attributes;
const rootAttributesBlob: string | undefined = snapshotBlobs[rootAttributesBlobId];
assert.strict(rootAttributesBlob, "rootAttributesBlob undefined");
assert.strictEqual(
JSON.parse(rootAttributesBlob),
"rootattributes",
"The root sub-tree's content is incorrect",
);
assert.strictEqual(
baseSnapshot.trees.default.trees.root.unreferenced,
baseSnapshot.trees.default?.trees.root?.unreferenced,
Comment on lines -95 to +97
Copy link
Contributor

Choose a reason for hiding this comment

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

@RishhiB, what do this and other ? additions have to do with no-unchecked-record-access rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe its because baseSnapshot.trees is of type [path: string]: ISnapshotTree, so default or root record access may not exist

undefined,
"The root sub-tree should not be marked as unreferenced",
);

// Validate "unref" sub-tree.
assert.strictEqual(
baseSnapshot.trees.default.trees.unref.unreferenced,
baseSnapshot.trees.default?.trees.unref?.unreferenced,
true,
"The unref sub-tree should be marked as unreferenced",
);

// Validate "groupId" sub-tree.
assert.strictEqual(
baseSnapshot.trees.default.trees.groupId.groupId,
baseSnapshot.trees.default?.trees.groupId?.groupId,
"group",
"The groupId sub-tree should have a groupId",
);

// Validate "groupId" sub-tree.
assert.strictEqual(
baseSnapshot.trees.default.trees.groupId.groupId,
baseSnapshot.trees.default?.trees.groupId?.groupId,
"group",
"The groupId sub-tree should have a groupId",
);
Expand Down
5 changes: 1 addition & 4 deletions packages/loader/container-loader/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,7 @@ function convertSummaryToSnapshotAndBlobs(summary: ISummaryTree): SnapshotWithBl
unreferenced: summary.unreferenced,
groupId: summary.groupId,
};
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: {
const innerSnapshot = convertSummaryToSnapshotAndBlobs(summaryObject);
Expand Down
Loading