Merges overwrite deletions

[QuentinRoy]

The following test is failing. In practice, I do not think it is possible to implement a compliant merge of merge-patches.

const origin = { a: { a1: 0, a2: 3 } };
const p1 = { a: null };
const p2 = { a: { a1: 8 } };

assert.deepEqual(
  jsonMergePatch.apply(origin, jsonMergePatch.merge(p1, p2)),
  { a: { a1: 8 } }
); // fails: { a: { a1: 8, a2: 3 } }

[kthy]

Isn't this Working As Expected™? You're merging the patches first which applies p2 over p1. Then you're applying the merged patch to origin. Since RFC 7396 doesn't say anything about merging patches it can't be wrong per se, but you might of course find it to be undesirable behaviour.

[QuentinRoy]

You are right. But except specified otherwise I generally consider an operation such as merging to be lossless, i.e. all the information of the two patches is preserved. That is not the case here since the deletion of the a property (in the first patch) is lost. If used without care, it could lead to significant memory leak. I suppose at least a warning would be welcome.

[savetheclocktower]

Yeah, I agree with @QuentinRoy. It seems very strange to me for this library to go outside the bounds of what's specified in RFC 7396 to implement a method whose behavior can only be inferred by its unit tests.

If I'm reading the example unit test right, I don't think that the merge method can ensure this outcome unless it requires an origin parameter. The patch you'd want in that situation is { a: { a2: null } }, but neither patch mentions a2, so there's no way to generate that patch without knowing which properties need explicit nulling-out.

It seems like merge is not meant to produce a patch equivalent to what you'd get if you sent the original record through a gauntlet of successive patches. It seems more like it's meant for situations where you need to combine multiple patches, all of which are applied to the same snapshot, with some simple last-version-wins logic if two patches happen to change the same leaf in a tree.

That's fine, but it definitely needs documenting, and even if I were in a position where I needed that exact behavior, I doubt I'd be confident that it behaved in the exact way that I needed. I know this library is not being actively maintained (which is fine, because it implements a simple RFC and its dependency graph is extremely simple), but it would be nice to drop a warning in the README that clarifies this method's behavior and describes the sorts of situations where it might be useful.

[pierreinglebert]

Thanks for the report.
This method is indeed completely outside the scope of the RFC and its purpose was to merge/squash successive patches on the same entity.
I added a warning on the README.