Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,100 @@
*/

import { bufferToString } from "@fluid-internal/client-utils";
import { assert } from "@fluidframework/core-utils/internal";
import type { IChannelStorageService } from "@fluidframework/datastore-definitions/internal";
import type {
IExperimentalIncrementalSummaryContext,
ISummaryTreeWithStats,
ITelemetryContext,
MinimumVersionForCollab,
} from "@fluidframework/runtime-definitions/internal";
import { createSingleBlobSummary } from "@fluidframework/shared-object-base/internal";
import { SummaryTreeBuilder } from "@fluidframework/runtime-utils/internal";

import type { DetachedFieldIndex } from "../core/index.js";
import type {
Summarizable,
SummaryElementParser,
SummaryElementStringifier,
} from "../shared-tree-core/index.js";
import type { JsonCompatibleReadOnly } from "../util/index.js";
import {
brand,
readAndParseSnapshotBlob,
type Brand,
type JsonCompatibleReadOnly,
} from "../util/index.js";
import { FluidClientVersion } from "../codec/index.js";

/**
* The storage key for the blob in the summary containing schema data
*/
const detachedFieldIndexBlobKey = "DetachedFieldIndexBlob";

/**
* The storage key for the blob containing metadata for the detached field index's summary.
*/
export const detachedFieldIndexMetadataKey = ".metadata";
Copy link
Contributor

Choose a reason for hiding this comment

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

This starting with a "." is interesting.

If this is indented to mean something, maybe establish a pattern that "." keys are part of the top level summary not the summary items or something, it should be documented somewhere central, and this should link to it.


/**
* The versions for the detached field index summary.
*/
export const DetachedFieldIndexSummaryVersion = {
/**
* Version 0 represents summaries before versioning was added. This version is not written.
* It is only used to avoid undefined checks.
*/
v0: 0,
/**
* Version 1 adds metadata to the detached field index summary.
*/
v1: 1,
/**
* The latest version of the detached field index summary. Must be updated when a new version is added.
*/
vLatest: 1,
} as const;
export type DetachedFieldIndexSummaryVersion = Brand<
(typeof DetachedFieldIndexSummaryVersion)[keyof typeof DetachedFieldIndexSummaryVersion],
"DetachedFieldIndexSummaryVersion"
>;
Comment on lines +41 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a worse version of what could be an enum.

You have type branding, but the actual values like DetachedFieldIndexSummaryVersion.v1 are not branded so you can't use them as a DetachedFieldIndexSummaryVersion.

Is there a reason to not just use an enum here?

If you really want to not use enums and use branding, then you should define a branded type, then brand the constants as members of that type (for example using the newish brandConst utility).


/**
* The type for the metadata in the detached field index's summary.
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably link the key for this.

* Using type definition instead of interface to make this compatible with JsonCompatible.
*/
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not follow our linter's recommendations? Did you read the documentation for that rule and decide why its suggestion should apply to our codebase, but not to this specific instance? If so, please include that reason as a comment here. If not, go read it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that I'm pretty sure I know what it recommends and I agree with it here for a better developer experience, but I should have to know that from memory to review this: the linter is supposed to help tell people things when relevant.

Note 2: its way easier to get to that rule documentation from in the IDE when looking at the warning (click the link in the warning IIRC) than for a reviewer here. That's another reason to explain why this suppression is ok despite our choice to enable this lint inline here.

export type DetachedFieldIndexSummaryMetadata = {
/** The version of the detached field index summary. */
readonly version: DetachedFieldIndexSummaryVersion;
};

/**
* Returns the summary version to use as per the given minimum version for collab.
*/
function minVersionToDetachedFieldIndexSummaryVersion(
version: MinimumVersionForCollab,
): DetachedFieldIndexSummaryVersion {
return version < FluidClientVersion.v2_73
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment on the declaration of FluidClientVersion.v2_73 in src\codec\codec.ts that explains the format implications of using v2_73.

? brand(DetachedFieldIndexSummaryVersion.v0)
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 we have to brand our own constants to their intended type is removing a lot of the value of having named constants and a branded type. See my other comments about fixing this.

: brand(DetachedFieldIndexSummaryVersion.v1);
}

/**
* Provides methods for summarizing and loading a tree index.
*/
export class DetachedFieldIndexSummarizer implements Summarizable {
public readonly key = "DetachedFieldIndex";

public constructor(private readonly detachedFieldIndex: DetachedFieldIndex) {}
// The summary version to write in the metadata for the detached field index summary.
private readonly summaryWriteVersion: DetachedFieldIndexSummaryVersion;

public constructor(
private readonly detachedFieldIndex: DetachedFieldIndex,
minVersionForCollab: MinimumVersionForCollab,
) {
this.summaryWriteVersion =
minVersionToDetachedFieldIndexSummaryVersion(minVersionForCollab);
}

public summarize(props: {
stringify: SummaryElementStringifier;
Expand All @@ -41,13 +107,35 @@ export class DetachedFieldIndexSummarizer implements Summarizable {
incrementalSummaryContext?: IExperimentalIncrementalSummaryContext;
}): ISummaryTreeWithStats {
const data = this.detachedFieldIndex.encode();
return createSingleBlobSummary(detachedFieldIndexBlobKey, props.stringify(data));
const builder = new SummaryTreeBuilder();
builder.addBlob(detachedFieldIndexBlobKey, props.stringify(data));

if (this.summaryWriteVersion >= DetachedFieldIndexSummaryVersion.v1) {
const metadata: DetachedFieldIndexSummaryMetadata = {
version: this.summaryWriteVersion,
};
builder.addBlob(detachedFieldIndexMetadataKey, JSON.stringify(metadata));
}

return builder.getSummaryTree();
}

public async load(
services: IChannelStorageService,
parse: SummaryElementParser,
): Promise<void> {
if (await services.contains(detachedFieldIndexMetadataKey)) {
const metadata = await readAndParseSnapshotBlob<DetachedFieldIndexSummaryMetadata>(
detachedFieldIndexMetadataKey,
services,
parse,
);
assert(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure an "assert" is the right error type for this. When encountering a document too new for the current code, that indicates a usage error by the application team (either bad rollout schedule, bad min version for collab) or a decision to simply not support this client.

In none of those cases is this error a bug in the fluid framework code, which is what asserts indicate.

If there was some upstream checking which was supposed to ensure such a case would never make it to this point, then an assert here would be valid. If that is the case, can you add a comment explaining what check would catch this and give a usage effort before we get here? If not, replace this with a usage error with useful customer facing details (maybe we should have a shared utility for throwing such forwards compat errors?)

metadata.version === DetachedFieldIndexSummaryVersion.v1,
"Unsupported detached field index summary",
);
}

if (await services.contains(detachedFieldIndexBlobKey)) {
const detachedFieldIndexBuffer = await services.readBlob(detachedFieldIndexBlobKey);
const treeBufferString = bufferToString(detachedFieldIndexBuffer, "utf8");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* Licensed under the MIT License.
*/

import { bufferToString } from "@fluid-internal/client-utils";
import { assert } from "@fluidframework/core-utils/internal";
import type { IChannelStorageService } from "@fluidframework/datastore-definitions/internal";
import type { IIdCompressor } from "@fluidframework/id-compressor";
Expand Down Expand Up @@ -32,7 +31,7 @@ import type {
SummaryElementParser,
SummaryElementStringifier,
} from "../../shared-tree-core/index.js";
import { idAllocatorFromMaxId, type JsonCompatible } from "../../util/index.js";
import { idAllocatorFromMaxId, readAndParseSnapshotBlob } from "../../util/index.js";
// eslint-disable-next-line import-x/no-internal-modules
import { chunkFieldSingle, defaultChunkPolicy } from "../chunked-forest/chunkTree.js";
import {
Expand All @@ -46,17 +45,16 @@ import { type ForestCodec, makeForestSummarizerCodec } from "./codec.js";
import {
ForestIncrementalSummaryBehavior,
ForestIncrementalSummaryBuilder,
forestSummaryContentKey,
} from "./incrementalSummaryBuilder.js";
import { TreeCompressionStrategyExtended } from "../treeCompressionUtils.js";
import type { IFluidHandle } from "@fluidframework/core-interfaces";

/**
* The key for the tree that contains the overall forest's summary tree.
* This tree is added by the parent of the forest summarizer.
* See {@link ForestIncrementalSummaryBuilder} for details on the summary structure.
*/
export const forestSummaryKey = "Forest";
import {
forestSummaryContentKey,
forestSummaryKey,
forestSummaryMetadataKey,
ForestSummaryVersion,
minVersionToForestSummaryVersion,
type ForestSummaryMetadata,
} from "./summaryTypes.js";

/**
* Provides methods for summarizing and loading a forest.
Expand Down Expand Up @@ -89,6 +87,7 @@ export class ForestSummarizer implements Summarizable {
(cursor: ITreeCursorSynchronous) => this.forest.chunkField(cursor),
shouldEncodeIncrementally,
initialSequenceNumber,
minVersionToForestSummaryVersion(options.minVersionForCollab),
);
}

Expand Down Expand Up @@ -164,24 +163,30 @@ export class ForestSummarizer implements Summarizable {
0xc21 /* Forest summary content missing in snapshot */,
);

const readAndParseBlob = async <T extends JsonCompatible<IFluidHandle>>(
id: string,
): Promise<T> => {
const treeBuffer = await services.readBlob(id);
const treeBufferString = bufferToString(treeBuffer, "utf8");
return parse(treeBufferString) as T;
};
if (await services.contains(forestSummaryMetadataKey)) {
const metadata = await readAndParseSnapshotBlob<ForestSummaryMetadata>(
forestSummaryMetadataKey,
services,
parse,
);
assert(metadata.version === ForestSummaryVersion.v1, "Unsupported forest summary");
Copy link
Contributor

Choose a reason for hiding this comment

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

}

// Load the incremental summary builder so that it can download any incremental chunks in the
// snapshot.
await this.incrementalSummaryBuilder.load(services, readAndParseBlob);
await this.incrementalSummaryBuilder.load(services, async (id: string) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

there are a lot of "id"s in this code. Could you use a more specific name or type? Just looking at this code I don't even know what this async function is (can't see the parameter name for it from the call site).

It seems a bit odd to be adding this async function as a parameter here in this PR: is this part of the versioning logic somehow?

readAndParseSnapshotBlob(id, services, parse),
);

// TODO: this code is parsing data without an optional validator, this should be defined in a typebox schema as part of the
// forest summary format.
const fields = this.codec.decode(await readAndParseBlob(forestSummaryContentKey), {
...this.encoderContext,
incrementalEncoderDecoder: this.incrementalSummaryBuilder,
});
const fields = this.codec.decode(
await readAndParseSnapshotBlob(forestSummaryContentKey, services, parse),
{
...this.encoderContext,
incrementalEncoderDecoder: this.incrementalSummaryBuilder,
},
);
const allocator = idAllocatorFromMaxId();
const fieldChanges: [FieldKey, DeltaFieldChanges][] = [];
const build: DeltaDetachedNodeBuild[] = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,13 @@ import type { ISnapshotTree } from "@fluidframework/driver-definitions/internal"
import { LoggingError } from "@fluidframework/telemetry-utils/internal";
import type { IFluidHandle } from "@fluidframework/core-interfaces";
import type { SummaryElementStringifier } from "../../shared-tree-core/index.js";

/**
* The key for the blob under ForestSummarizer's root.
* This blob contains the ForestCodec's output.
* See {@link ForestIncrementalSummaryBuilder} for details on the summary structure.
*/
export const forestSummaryContentKey = "ForestTree";

/**
* The contents of an incremental chunk is under a summary tree node with its {@link ChunkReferenceId} as the key.
* The inline portion of the chunk content is encoded with the forest codec is stored in a blob with this key.
* The rest of the chunk contents is stored in the summary tree under the summary tree node.
* See the summary format in {@link ForestIncrementalSummaryBuilder} for more details.
*/
const chunkContentsBlobKey = "contents";
import {
chunkContentsBlobKey,
forestSummaryContentKey,
forestSummaryMetadataKey,
ForestSummaryVersion,
type ForestSummaryMetadata,
} from "./summaryTypes.js";

/**
* State that tells whether a summary is currently being tracked.
Expand Down Expand Up @@ -291,6 +283,8 @@ export class ForestIncrementalSummaryBuilder implements IncrementalEncoderDecode
private readonly getChunkAtCursor: (cursor: ITreeCursorSynchronous) => TreeChunk[],
public readonly shouldEncodeIncrementally: IncrementalEncodingPolicy,
private readonly initialSequenceNumber: number,
// The summary version to write in the metadata for the detached field index summary.
private readonly summaryWriteVersion: ForestSummaryVersion,
) {}

/**
Expand Down Expand Up @@ -462,6 +456,15 @@ export class ForestIncrementalSummaryBuilder implements IncrementalEncoderDecode
return chunkReferenceIds;
}

private maybeAddMetadataToSummary(summaryBuilder: SummaryTreeBuilder): void {
if (this.summaryWriteVersion >= ForestSummaryVersion.v1) {
const metadata: ForestSummaryMetadata = {
version: this.summaryWriteVersion,
};
summaryBuilder.addBlob(forestSummaryMetadataKey, JSON.stringify(metadata));
}
}

/**
* Must be called after summary generation is complete to finish tracking the summary.
* It clears any tracking state and deletes the tracking properties for summaries that are older than the
Expand All @@ -479,10 +482,12 @@ export class ForestIncrementalSummaryBuilder implements IncrementalEncoderDecode
if (!this.enableIncrementalSummary || incrementalSummaryContext === undefined) {
const summaryBuilder = new SummaryTreeBuilder();
summaryBuilder.addBlob(forestSummaryContentKey, forestSummaryContent);
this.maybeAddMetadataToSummary(summaryBuilder);
return summaryBuilder.getSummaryTree();
}

validateTrackingSummary(this.forestSummaryState, this.trackedSummaryProperties);
this.maybeAddMetadataToSummary(this.trackedSummaryProperties.parentSummaryBuilder);

this.trackedSummaryProperties.parentSummaryBuilder.addBlob(
forestSummaryContentKey,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Licensed under the MIT License.
*/

export { forestSummaryKey, ForestSummarizer } from "./forestSummarizer.js";
export { ForestSummarizer } from "./forestSummarizer.js";
export { getCodecTreeForForestFormat } from "./codec.js";
export { ForestFormatVersion } from "./format.js";
export { forestSummaryKey } from "./summaryTypes.js";
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

import type { MinimumVersionForCollab } from "@fluidframework/runtime-definitions/internal";
import { FluidClientVersion } from "../../codec/index.js";
import { brand, type Brand } from "../../util/index.js";

/**
* The key for the tree that contains the overall forest's summary tree.
* This tree is added by the parent of the forest summarizer.
* See {@link ForestIncrementalSummaryBuilder} for details on the summary structure.
*/
export const forestSummaryKey = "Forest";

/**
* The key for the blob under ForestSummarizer's root.
* This blob contains the ForestCodec's output.
* See {@link ForestIncrementalSummaryBuilder} for details on the summary structure.
*/
export const forestSummaryContentKey = "ForestTree";

/**
* The storage key for the blob containing metadata for the forest's summary.
*/
export const forestSummaryMetadataKey = ".metadata";

/**
* The contents of an incremental chunk is under a summary tree node with its {@link ChunkReferenceId} as the key.
* The inline portion of the chunk content is encoded with the forest codec is stored in a blob with this key.
* The rest of the chunk contents is stored in the summary tree under the summary tree node.
* See the summary format in {@link ForestIncrementalSummaryBuilder} for more details.
*/
export const chunkContentsBlobKey = "contents";

/**
* The versions for the forest summary.
*/
export const ForestSummaryVersion = {
/**
* Version 0 represents summaries before versioning was added. This version is not written.
* It is only used to avoid undefined checks.
*/
v0: 0,
/**
* Version 1 adds metadata to the forest summary.
*/
v1: 1,
/**
* The latest version of the forest summary. Must be updated when a new version is added.
*/
vLatest: 1,
} as const;
export type ForestSummaryVersion = Brand<
(typeof ForestSummaryVersion)[keyof typeof ForestSummaryVersion],
"ForestSummaryVersion"
>;

/**
* The type for the metadata in forest's summary.
* Using type definition instead of interface to make this compatible with JsonCompatible.
*/
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
export type ForestSummaryMetadata = {
/** The version of the forest summary. */
readonly version: ForestSummaryVersion;
};

/**
* Returns the summary version to use as per the given minimum version for collab.
*/
export function minVersionToForestSummaryVersion(
version: MinimumVersionForCollab,
): ForestSummaryVersion {
return version < FluidClientVersion.v2_73
? brand(ForestSummaryVersion.v0)
: brand(ForestSummaryVersion.v1);
}
Loading
Loading