diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts index 2e1e6b2ef2a..41942cd7a5c 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts @@ -199,6 +199,20 @@ describe("WorkflowGraph", () => { expect(operator.operatorProperties).toEqual(testProperty); }); + // The property panel clears a field by sending the properties without it, so a + // key left out has to be dropped here. Retaining it leaves the operator running + // on a value the user removed and the panel no longer shows. + it("should drop a property the new object leaves out", () => { + workflowGraph.addOperator(mockScanPredicate); + workflowGraph.setOperatorProperty(mockScanPredicate.operatorID, { tableName: "testTable", limit: 5 }); + + workflowGraph.setOperatorProperty(mockScanPredicate.operatorID, { tableName: "testTable" }); + + expect(workflowGraph.getOperator(mockScanPredicate.operatorID).operatorProperties).toEqual({ + tableName: "testTable", + }); + }); + it("should throw an error when trying to set the property of an nonexist operator", () => { expect(() => { const testProperty = { tableName: "testTable" }; diff --git a/frontend/src/app/workspace/types/shared-editing.interface.spec.ts b/frontend/src/app/workspace/types/shared-editing.interface.spec.ts index 40141d00060..83af7d2fda4 100644 --- a/frontend/src/app/workspace/types/shared-editing.interface.spec.ts +++ b/frontend/src/app/workspace/types/shared-editing.interface.spec.ts @@ -142,7 +142,7 @@ describe("updateYTypeFromObject", () => { expect(originalText.toJSON()).toBe("world"); }); - it("adds new object keys but leaves keys absent from the new object in place", () => { + it("adds new object keys and removes the ones the new object no longer carries", () => { const doc = new Y.Doc(); const yMap = attach(doc, "m", createYTypeFromObject({ keep: 1, drop: 2 })); @@ -153,8 +153,21 @@ describe("updateYTypeFromObject", () => { // A key present only in the new object is added. expect(asMap.has("add")).toBe(true); expect(asMap.get("add")).toBe(3); - // A key omitted from the new object is retained (an undefined new value never deletes). - expect(asMap.get("drop")).toBe(2); + // A key the new object has dropped is deleted rather than left behind. Clearing + // an operator property is exactly this: the editor sends the properties without + // it, and anything retained here is a value the user removed and can no longer see. + expect(asMap.has("drop")).toBe(false); + }); + + it("keeps a key whose new value is explicitly undefined", () => { + const doc = new Y.Doc(); + const yMap = attach(doc, "m", createYTypeFromObject({ a: 1 })); + + updateYTypeFromObject(yMap, { a: undefined } as any); + + // Carrying the key with an undefined value is not the same as dropping it, and + // only the latter means removal. + expect((yMap as unknown as Y.Map).get("a")).toBe(1); }); it("appends new items to a Y.Array in place", () => { diff --git a/frontend/src/app/workspace/types/shared-editing.interface.ts b/frontend/src/app/workspace/types/shared-editing.interface.ts index fc7843992dc..f68beef1e45 100644 --- a/frontend/src/app/workspace/types/shared-editing.interface.ts +++ b/frontend/src/app/workspace/types/shared-editing.interface.ts @@ -246,11 +246,21 @@ export function updateYTypeFromObject(oldYObj: YType, newOb const keySet = new Set([...Object.keys(oldObj), ...Object.keys(newObj)]); keySet.forEach((k: string) => { const newValue = newObj[k as keyof T] as any; - if (!_.isEqual(oldObj[k as keyof T], newValue)) { - if (!updateYTypeFromObject(oldYObjAsYMap.get(k), newValue)) { - if (newValue !== undefined) { - oldYObjAsYMap.set(k, createYTypeFromObject(newValue)); - } + if (_.isEqual(oldObj[k as keyof T], newValue)) { + return; + } + // A key the new object does not carry is one the caller removed, and deleting + // it is the only way that removal can reach the shared type: the update below + // changes a value or adds one, and for a key that is simply gone it did + // neither, leaving the old value in place. `in` rather than an undefined check, + // so a key explicitly carrying undefined still takes the path below. + if (!(k in newObj)) { + oldYObjAsYMap.delete(k); + return; + } + if (!updateYTypeFromObject(oldYObjAsYMap.get(k), newValue)) { + if (newValue !== undefined) { + oldYObjAsYMap.set(k, createYTypeFromObject(newValue)); } } });