Bugfix for applyChangeSet. String changes maintain value and oldValue

[pswillcock]

applyChangeSet is incorrectly applying changes to string properties when combining reversible changeSets (i.e. with value and oldValue) causing oldValue for the secondary changeSet to be lost on application.

Behaviour

const cs1 = {
  String: {
    x: {
      oldValue: "a",
      value: "b"
    }
  }
};
const cs2 = {
  String: {
    y: {
      oldValue: "c",
      value: "d"
    }
  }
};

applyChangeSet (Before fix)

{
  String: {
    x: {
      oldValue: "a",
      value: "b"
    }
    y: "d",
  },
}

applyChangeSet (After fix)

{
  String: {
    x: {
      oldValue: "a",
      value: "b"
    }
    y:  {
      oldValue: "c",
      value: "d"
    }
  },
}

[ruiterr]

Where did you encounter this problem? Where are these changesets from?

The underlying problem here is, that the original changeset is non-reversible and the applied changeSet is reversible. So those two changesets cannot result in a reversible changeset, since the original changeset already lacked the reversibility information.

It should not happen that a non-reversible and a reversible changeSet are squashed with each other. In such a case, the reversible changeset should be stripped before squashing to get a final non-reversible changeset. Another place where this could happen is, when squashing an insert operation with a reversible changeset, but in such a case the result should again be an insert and thus there should be no old_states in there.

[ruiterr]

The new example indeed does look like a bug.

Could you explain, in which way the code change does fix the problem. It looks to me more or less equivalent to the old code. The only difference seems to be that appliedChanges is not overwritten. Is it used later (at a place that is not shown in the PR) and that causes the problem?

One other small thing: could you add a unit test that reproduces the problem, to prevent future regressions? A simple test with just your example from above should suffice.

[dstanesc]

@pswillcock @ruiterr @nedalhy Was this defect already addressed elsewhere?

[nedalhy]

@dstanesc It would be great if you can introduce a simple reproducible test in the PR as @ruiterr requested, then we can merge it.

[milanro]

@pswillcock @ruiterr @nedalhy @dstanesc I wonder whether something was fixed here. I do not have the tests from @pswillcock . I tried to reproduce but it looks to work. Here is the code which I used to reproduce

function testApplyChangeSet(workspace: BoundWorkspace){
    const rootProp: NodeProperty = workspace.workspace.rootProperty;
    const prop1Name = "prop1";
    const prop2Name = "prop2";
    const prop1: StringProperty = rootProp.resolvePath(prop1Name) as StringProperty;
    prop1.value = "p1";
    const ch1 = rootProp.getPendingChanges();
    ch1._toReversibleChangeSet(workspace.workspace.tree.tipView);
    workspace.workspace.commit();
    const prop2: StringProperty = rootProp.resolvePath(prop2Name) as StringProperty;
    prop2.value = "p2";
    const ch2: ChangeSet = rootProp.getPendingChanges();
    ch2._toReversibleChangeSet(workspace.workspace.tree.tipView);
    console.log("CH1: " + JSON.stringify(ch1));
    console.log("CH2: " + JSON.stringify(ch2));
    ch1.applyChangeSet(ch2);
    console.log("CH1-CH2: " + JSON.stringify(ch1));
}

The output is following :

CH1: {"_changes":{"modify":{"String":{"prop1":{"value":"p1","oldValue":""}}}},"_isNormalized":false}

CH2: {"_changes":{"modify":{"String":{"prop2":{"value":"p2","oldValue":""}}}},"_isNormalized":false}

CH1-CH2: {"_changes":{"modify":{"String":{"prop1":{"value":"p1","oldValue":""},"prop2":{"value":"p2","oldValue":""}}}},"_isNormalized":false}

The oldValue is present also after applyChangeSet