Add preventInvertInRollback option

[antoinelyset]

My try at adding an option to improve support for JSON1. #442

I heavily relied on a previous spec, so it may be possible to simplify it.

[coveralls]

Coverage increased (+0.0008%) to 97.387% when pulling 9d9031b on antoinelyset:prevent-invert-in-rollback-option into fa9179d on share:master.

[alecgibson]

@antoinelyset thanks for the Pull Request!

We've discussed this, and we believe this conceptually live at the JSON1 level. The thing is that the OT Type interface should act as a guarantee. If a type has implemented an optional method like invert, then ShareDB should be able to call that method and assume it exhibits the expected behaviour. If invert doesn't work properly, this is really a bug with the type. We largely don't want to set the precedent of addressing type-level bugs at the ShareDB level, since this is a mix of concerns, and ShareDB patches may drift against type issues.

Our suggestions for moving forwards with this issue:

• Get invert fixed on JSON1 (or apply pressure to get it fixed)
• If that can't be achieved (for whatever reason), push to get a flag added to the JSON1 library to disable inversion (if it's broken, then it should be disabled at that level)
• If you've tried both of those things, let us know (with links to discussions)

That obviously doesn't fix your issue right now. In the meantime we suggest:

• Monkey-patching the type to remove the invert method (and ShareDB should work fine without it)
• Forking the type and removing the invert method

[antoinelyset]

Thanks for your quick answer. I will ask what we can do in the JSON1 repo.

[josephg]

Hi! json1 and sharedb original author here. I think can do better than those options.

The problem isn't that json1 is broken. The problem is that json1 has both invertible and non-invertible operations. Non-invertible operations come from two sources:

1. Creating remove / replace operations which don't specify the removed / replaced content. Eg json1.removeOp(['a']) produces a non-invertible operation, while json1.removeOp(['a'], "deleted content") produces an invertible operation. (When inverted, it will insert "deleted content" into doc.a)
2. Calling transform will remove invertibility information in many cases. This is unfortunately unavoidable. Consider op1 moves doc.a to doc.b.c, and op2 deletes doc.b. transform(op1, op2) will produce an operation that deletes doc.a, but the transform function does not know the original contents of doc.a, and thus can't make the result invertible. There used to be a bug where compose would produce non-invertible operations as well - I think thats been fixed but its a long time since I looked at the code. Issues / patches welcome!

To make matter worse, there's no flag in an operation which specifies whether or not its invertible. This is a flaw in the design.

json1 has two methods to help here:

• json1.makeInvertible(op, doc) -> op adds the missing information back into an operation
• invertWithDoc(op, doc) -> doc - which inverts an operation given the document state before the operation was originally applied. (Now I think about it isn't super useful because if you knew that, you probably don't need to call invert.) invertWithDoc is just a wrapper is just a wrapper around invert(makeInvertible(op, doc)).

I had sharedb in mind when I wrote json1 (and if it existed when sharedb was written, we would have used it). Its been a long time since I looked at the code, but sharejs should be able to use the makeInvertible method in json1 to restore invertibility information in the undo / rollback stack, since the state is known by sharedb when the operation is created.

> doc = {a:'hi'}
> op = json1.removeOp(['a'])
[ 'a', { r: true } ]
> json1.makeInvertible(op, doc)
[ 'a', { r: 'hi' } ]

[alecgibson]

All in all, it sounds like inversion is a bit "unreliable" from the perspective of ShareDB consuming an API — that is ShareDB has no idea of whether or not it should be able to apply an inversion to any given op. This is also true of json0.

@josephg we could potentially track an in-memory undo stack (this would incidentally make rich-text psuedo-invertible, too, since it also implements a method that can invert an op with the doc). However, I feel like that would be a lot of effort to go to for error recovery (when we could just hard rollback instead, which — in my opinion — would be more robust anyway).

In an ideal world, my suggestion would be to default to hard rollback, and make invertible fallback opt-in. However, that would be a breaking change, so we probably can't go down that route.

Perhaps this PR is then the most pragmatic solution. @ericyhwang had raised the concern that it's a bit awkward to use as a consumer: you have to set whether you want an inverted rollback on each document. However, on reflection, I think it's possible that some consumers might use different documents/collections in different ways (perhaps they use invertible ops on one, but not on another?). It's a bit awkward, but applying it at this level gives consumers the greatest granularity of control.

My only other thought is that we could perhaps offer an event before rollback (which might also be nice for logging). This would also let consumers decide per-op if they want to soft rollback or not. A potential API might look like:

doc.on('before rollback', (event) => {
  if (consumerNotWantInvert(event.op)) {
    event.hardRollback();
  }
});

[josephg]

Sure - happy to defer to your opinion on what sharedb should do. The rollback infrastructure is from after my time, and I wasn't in those design meetings.

Otherwise if there's any changes you'd like to see in json1, let me know / file an issue.

[curran]

Minor name change suggestions.

Does this change effectively implement Default to hard rollback #451?

Is there anything missing here for addressing #451 or is this PR good to go?

[curran]

I can't help but notice a grammar issue. Suggest to simplify this name.

Suggested change

	var invertHaveBeenCalled = false;
	var invertCalled = false;

[curran]

Suggested change

	invertHaveBeenCalled = true;
	invertCalled = true;

[curran]

Suggested change

	expect(invertHaveBeenCalled).to.eql(!preventInvertInRollback);
	expect(invertCalled).to.eql(!preventInvertInRollback);