Skip to content

Commit

Permalink
[Port 2.0] Cherry pick move validation bugs (#21674)
Browse files Browse the repository at this point in the history 
## Description

This PR fixes a bug (introduced in [this
PR](bd66baf))
where the validation for the destination index was being done against
the source array. Also renames `index` to `destinationIndex`, and
`field` to `sourceField` and `destinationField` for clarity

Also fixes the type validation for moved content.
  • Loading branch information
@daesunp
daesunp authored Jun 27, 2024
1 parent 4ebaae3 commit bb0ffd2
Show file tree
Hide file tree
Showing 2 changed files with 193 additions and 58 deletions.
52 changes: 28 additions & 24 deletions packages/dds/tree/src/simple-tree/arrayNode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -774,22 +774,27 @@ abstract class CustomArrayNodeBase<const T extends ImplicitAllowedTypes>
}
public moveToStart(sourceIndex: number, source?: TreeArrayNode): void {
const sourceArray = source ?? this;
const field = getSequenceField(sourceArray);
validateIndex(sourceIndex, field, "moveToStart");
const sourceField = getSequenceField(sourceArray);
validateIndex(sourceIndex, sourceField, "moveToStart");
this.moveRangeToIndex(0, sourceIndex, sourceIndex + 1, source);
}
public moveToEnd(sourceIndex: number, source?: TreeArrayNode): void {
const sourceArray = source ?? this;
const field = getSequenceField(sourceArray);
validateIndex(sourceIndex, field, "moveToEnd");
const sourceField = getSequenceField(sourceArray);
validateIndex(sourceIndex, sourceField, "moveToEnd");
this.moveRangeToIndex(this.length, sourceIndex, sourceIndex + 1, source);
}
public moveToIndex(index: number, sourceIndex: number, source?: TreeArrayNode): void {
public moveToIndex(
destinationIndex: number,
sourceIndex: number,
source?: TreeArrayNode,
): void {
const sourceArray = source ?? this;
const field = getSequenceField(sourceArray);
validateIndex(index, field, "moveToIndex", true);
validateIndex(sourceIndex, field, "moveToIndex");
this.moveRangeToIndex(index, sourceIndex, sourceIndex + 1, source);
const sourceField = getSequenceField(sourceArray);
const destinationField = getSequenceField(this);
validateIndex(destinationIndex, destinationField, "moveToIndex", true);
validateIndex(sourceIndex, sourceField, "moveToIndex");
this.moveRangeToIndex(destinationIndex, sourceIndex, sourceIndex + 1, source);
}
public moveRangeToStart(
sourceStart: number,
Expand All @@ -814,41 +819,40 @@ abstract class CustomArrayNodeBase<const T extends ImplicitAllowedTypes>
this.moveRangeToIndex(this.length, sourceStart, sourceEnd, source);
}
public moveRangeToIndex(
index: number,
destinationIndex: number,
sourceStart: number,
sourceEnd: number,
source?: TreeArrayNode,
): void {
const field = getSequenceField(this);
validateIndex(index, field, "moveRangeToIndex", true);
validateIndexRange(sourceStart, sourceEnd, source ?? field, "moveRangeToIndex");
const destinationField = getSequenceField(this);
validateIndex(destinationIndex, destinationField, "moveRangeToIndex", true);
validateIndexRange(sourceStart, sourceEnd, source ?? destinationField, "moveRangeToIndex");
const sourceField =
source !== undefined
? field.isSameAs(getSequenceField(source))
? field
? destinationField.isSameAs(getSequenceField(source))
? destinationField
: getSequenceField(source)
: field;
: destinationField;

// TODO: determine support for move across different sequence types
if (field.schema.types !== undefined && sourceField !== field) {
if (destinationField.schema.types !== undefined && sourceField !== destinationField) {
for (let i = sourceStart; i < sourceEnd; i++) {
const sourceNode =
sourceField.boxedAt(sourceStart) ?? fail("impossible out of bounds index");
if (!field.schema.types.has(sourceNode.schema.name)) {
throw new Error("Type in source sequence is not allowed in destination.");
const sourceNode = sourceField.boxedAt(i) ?? fail("impossible out of bounds index");
if (!destinationField.schema.types.has(sourceNode.schema.name)) {
throw new UsageError("Type in source sequence is not allowed in destination.");
}
}
}
const movedCount = sourceEnd - sourceStart;
const sourceFieldPath = sourceField.getFieldPath();

const destinationFieldPath = field.getFieldPath();
field.context.checkout.editor.move(
const destinationFieldPath = destinationField.getFieldPath();
destinationField.context.checkout.editor.move(
sourceFieldPath,
sourceStart,
movedCount,
destinationFieldPath,
index,
destinationIndex,
);
}
}
Expand Down
199 changes: 165 additions & 34 deletions packages/dds/tree/src/test/simple-tree/arrayNode.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,43 +211,160 @@ describe("ArrayNode", () => {
assert.deepEqual([...array2], [1, 3]);
});

it("move within field", () => {
const array = hydrate(schemaType, [1, 2, 3]);
array.moveToIndex(0, 1);
assert.deepEqual([...array], [2, 1, 3]);
});
for (const specifySource of [false, true]) {
describe(`move within field ${
specifySource ? "(specified source)" : "(implicit source)"
}`, () => {
it("moves node to the destination index when valid", () => {
const initialState = [0, 1, 2];
for (let sourceIndex = 0; sourceIndex < initialState.length; sourceIndex += 1) {
const movedValue = initialState[sourceIndex];
for (
let destinationIndex = 0;
destinationIndex < initialState.length;
destinationIndex += 1
) {
const array = hydrate(schemaType, initialState);
if (specifySource) {
array.moveToIndex(destinationIndex, sourceIndex, array);
} else {
array.moveToIndex(destinationIndex, sourceIndex);
}
const actual = [...array];
const expected =
sourceIndex < destinationIndex
? [
...initialState.slice(0, sourceIndex),
...initialState.slice(sourceIndex + 1, destinationIndex),
movedValue,
...initialState.slice(destinationIndex),
]
: [
...initialState.slice(0, destinationIndex),
movedValue,
...initialState.slice(destinationIndex, sourceIndex),
...initialState.slice(sourceIndex + 1),
];
assert.deepEqual(actual, expected);
}
}
});

it("throws when the source index is invalid", () => {
const array = hydrate(schemaType, [1, 2, 3]);
// Destination index too large
assert.throws(
() => array.moveToIndex(4, 0),
validateUsageError(
/Index value passed to TreeArrayNode.moveToIndex is out of bounds./,
),
);
// Source index too large
assert.throws(
() => array.moveToIndex(0, 4),
validateUsageError(
/Index value passed to TreeArrayNode.moveToIndex is out of bounds./,
),
);
// Destination index is negative
assert.throws(
() => array.moveToIndex(-1, 0),
validateUsageError(/Expected non-negative index, got -1./),
);
// Source index is negative
assert.throws(
() => array.moveToIndex(0, -1),
validateUsageError(/Expected non-negative index, got -1./),
);
});
});
}

it("cross-field move", () => {
const schema = schemaFactory.object("parent", {
array1: schemaFactory.array(schemaFactory.number),
array2: schemaFactory.array(schemaFactory.number),
describe("move across fields", () => {
it("moves node to the destination index when valid", () => {
const schema = schemaFactory.object("parent", {
source: schemaFactory.array(schemaFactory.number),
destination: schemaFactory.array(schemaFactory.number),
});
for (const [initialSourceState, initialDestinationState] of [
[[1, 2, 3], []],
[
[1, 2, 3],
[4, 5],
],
[
[1, 2],
[3, 4, 5],
],
]) {
for (
let sourceIndex = 0;
sourceIndex < initialSourceState.length;
sourceIndex += 1
) {
const movedValue = initialSourceState[sourceIndex];
for (
let destinationIndex = 0;
destinationIndex < initialDestinationState.length;
destinationIndex += 1
) {
const { source, destination } = hydrate(schema, {
source: initialSourceState,
destination: initialDestinationState,
});
destination.moveToIndex(destinationIndex, sourceIndex, source);
const actualSource = [...source];
const actualDestination = [...destination];
const expectedSource = [
...initialSourceState.slice(0, sourceIndex),
...initialSourceState.slice(sourceIndex + 1),
];
const expectedDestination = [
...initialDestinationState.slice(0, destinationIndex),
movedValue,
...initialDestinationState.slice(destinationIndex),
];
assert.deepEqual(actualSource, expectedSource);
assert.deepEqual(actualDestination, expectedDestination);
}
}
}
});
const { array1, array2 } = hydrate(schema, { array1: [1, 2], array2: [1, 2] });
array1.moveToIndex(1, 0, array2);
assert.deepEqual([...array1], [1, 1, 2]);
});

it("invalid index", () => {
const array = hydrate(schemaType, [1, 2, 3]);
// Destination index too large
assert.throws(
() => array.moveToIndex(4, 0),
validateUsageError(
/Index value passed to TreeArrayNode.moveToIndex is out of bounds./,
),
);
// Source index too large
assert.throws(
() => array.moveToIndex(0, 4),
validateUsageError(
/Index value passed to TreeArrayNode.moveToIndex is out of bounds./,
),
);
// Index is negative
assert.throws(
() => array.moveToIndex(-1, 0),
validateUsageError(/Expected non-negative index, got -1./),
);
it("throws when the source index is invalid", () => {
const schema = schemaFactory.object("parent", {
source: schemaFactory.array(schemaFactory.number),
destination: schemaFactory.array(schemaFactory.number),
});
const { source, destination } = hydrate(schema, {
source: [1, 2, 3],
destination: [4, 5, 6, 7],
});
// Destination index too large
assert.throws(
() => destination.moveToIndex(5, 0, source),
validateUsageError(
/Index value passed to TreeArrayNode.moveToIndex is out of bounds./,
),
);
// Source index too large
assert.throws(
() => destination.moveToIndex(0, 4, source),
validateUsageError(
/Index value passed to TreeArrayNode.moveToIndex is out of bounds./,
),
);
// Destination index is negative
assert.throws(
() => destination.moveToIndex(-1, 0, source),
validateUsageError(/Expected non-negative index, got -1./),
);
// Source index is negative
assert.throws(
() => destination.moveToIndex(0, -1, source),
validateUsageError(/Expected non-negative index, got -1./),
);
});
});
});

Expand Down Expand Up @@ -368,6 +485,20 @@ describe("ArrayNode", () => {
assert.deepEqual([...array], []);
});

it("invalid content type", () => {
const schema = schemaFactory.object("parent", {
array1: schemaFactory.array([schemaFactory.number, schemaFactory.string]),
array2: schemaFactory.array(schemaFactory.number),
});
const { array1, array2 } = hydrate(schema, { array1: [1, "bad", 2], array2: [] });
const expected = validateUsageError(
/Type in source sequence is not allowed in destination./,
);
assert.throws(() => array2.moveRangeToIndex(0, 1, 3, array1), expected);
assert.throws(() => array2.moveRangeToIndex(0, 0, 2, array1), expected);
assert.throws(() => array2.moveRangeToIndex(0, 0, 3, array1), expected);
});

it("invalid index", () => {
const array = hydrate(schemaType, [1, 2, 3]);
// Destination index too large
Expand Down

0 comments on commit bb0ffd2

Please sign in to comment.