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

Enhance reliability of obtaining colletion from interval #16251

Merged
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
18 changes: 18 additions & 0 deletions packages/dds/sequence/src/intervalCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1367,6 +1367,17 @@ export class LocalIntervalCollection<TInterval extends ISerializableInterval> {
}

if (props) {
// This check is intended to prevent scenarios where a random interval is created and then
// inserted into a collection. The aim is to ensure that the collection is created first
// then the user can create/add intervals based on the collection
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think our error message here needs to be particularly friendly. asserting here and below might give better bundle size. not a big deal though.

props[reservedRangeLabelsKey] !== undefined &&
props[reservedRangeLabelsKey][0] !== this.label
) {
throw new LoggingError(
"Adding an interval that belongs to another interval collection is not permitted",
);
}
interval.addProperties(props);
}
interval.properties[reservedIntervalIdKey] ??= uuid();
Expand Down Expand Up @@ -2228,6 +2239,13 @@ export class IntervalCollection<TInterval extends ISerializableInterval>
if (!props) {
throw new LoggingError("changeProperties should be called with a property set");
}
// prevent the overwriting of an interval label, it should remain unchanged
// once it has been inserted into the collection.
if (props[reservedRangeLabelsKey] !== undefined) {
throw new LoggingError(
"The label property should not be modified once inserted to the collection",
);
}

const interval = this.getIntervalById(id);
if (interval) {
Expand Down
1 change: 0 additions & 1 deletion packages/dds/sequence/src/revertibles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,6 @@ export function discardSharedStringRevertibles(
});
}

// Uses of referenceRangeLabels will be removed once AB#4081 is completed.
function revertLocalAdd(
string: SharedString,
revertible: TypedRevertible<typeof IntervalOpType.ADD>,
Expand Down
40 changes: 39 additions & 1 deletion packages/dds/sequence/src/test/intervalCollection.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@

import { strict as assert } from "assert";
import { IChannelServices } from "@fluidframework/datastore-definitions";
import { ReferenceType, SlidingPreference } from "@fluidframework/merge-tree";
import {
ReferenceType,
SlidingPreference,
reservedRangeLabelsKey,
} from "@fluidframework/merge-tree";
import {
MockFluidDataStoreRuntime,
MockContainerRuntimeFactory,
Expand All @@ -14,6 +18,7 @@ import {
MockStorage,
MockEmptyDeltaConnection,
} from "@fluidframework/test-runtime-utils";
import { LoggingError } from "@fluidframework/telemetry-utils";
import { SharedString } from "../sharedString";
import { SharedStringFactory } from "../sequenceFactory";
import {
Expand Down Expand Up @@ -1732,4 +1737,37 @@ describe("SharedString interval collections", () => {
});
});
});

describe("maintain consistency between the collection label and that in interval properties", () => {
let collection;

beforeEach(() => {
sharedString.initializeLocal();
collection = sharedString.getIntervalCollection("test");
sharedString.insertText(0, "xyz");
});

it("can not insert the interval which does not belong to this collection", () => {
assert.throws(
() => {
collection.add(1, 1, IntervalType.SlideOnRemove, {
[reservedRangeLabelsKey]: ["test2"],
});
},
LoggingError,
"The collection is unable to add an interval which does not belong to it",
);
});

it("can not modify the interval's label after it has been inserted to the collection", () => {
const id = collection.add(1, 1, IntervalType.SlideOnRemove).getIntervalId();
assert.throws(
() => {
collection.changeProperties(id, { [reservedRangeLabelsKey]: ["test2"] });
},
LoggingError,
"The label property of an interval should not be modified once inserted to the collection",
);
});
});
});