Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ottypes/docs spec is not supported correctly wrt (de)serialisation #214

Open
gkubisa opened this issue Jun 12, 2018 · 2 comments
Open

ottypes/docs spec is not supported correctly wrt (de)serialisation #214

gkubisa opened this issue Jun 12, 2018 · 2 comments

Comments

@gkubisa
Copy link
Contributor

gkubisa commented Jun 12, 2018

Spec: https://github.com/ottypes/docs#standard-properties

First of all, according to the spec, initialData, snapshot and data may all be completely different data types in the following function specs create([initialData]) -> snapshot, serialize(snapshot) -> data, deserialize(data) -> snapshot. text-tp2 is an example of such a type. Please note also that create([initialData]) -> snapshot returns a non-serialized snapshot.

Secondly, apply(snapshot, op) -> snapshot' expects a non-serialized snapshot as the first parameter. Again, text-tp2 is an example of a type which relies on the first param being a non-serialized snapshot.

I found a number of problems in ShareDB related to the above:

  1. ShareDB applies an operation to initialData, instead of a snapshot, in tryCompose. I have already opened a PR with a fix.
  2. ShareDB passes a non-serialized snapshot to deserialize here.
  3. ShareDB uses createDeserialized, which is not in the spec and is not needed because the standard create already returns a non-serialized snapshot.
  4. ShareDB never calls serialize and relies on the OT types to define toJSON instead. This is not documented anywhere and violates the spec.
  5. ShareDB applies operations to serialized snapshots on the server-side because it does not deserialize the data coming from the database.
@DetweilerRyan
Copy link

The contract for custom (de)serialization of ot types in ShareDB differs from the spec you reference. This was done for good reason and it is "documented" here #108.

I'm always glad to hear of new use cases for custom ot types. At Retrium we have been using custom ot types with custom serialization for about three years in production and I assure you the current implementation in sharedb works without fail.

@gkubisa
Copy link
Contributor Author

gkubisa commented Jun 15, 2018

The contract for custom (de)serialization of ot types in ShareDB differs from the spec you reference.

From reading ShareDB source code, I see the differences affect the following functions: create, createDeserialized, apply and serialize. The biggest issue for me is that those differences are not documented clearly and in a place where users would actually find out about them.

This was done for good reason and it is "documented" here #108.

As I understand, the reason is performance, which is a good reason indeed. :-) However, couldn't the same benefits be achieved while maintaining compatibility with ottypes/docs? For example, add a new optional function to the spec: applySerialized(serializedSnapshot, op) -> serializedSnapshot' and use it only on the server as follows:

  • if applySerialized is available, call applySerialized. (fast for types with custom (de)serialization)
  • else if serialize and deserialize are available, call deserialize -> apply -> serialize. (MAY be slow for types with custom (de)serialization but compatible with ottypes/docs)
  • else, call apply. (fast for types without custom serialization)

I assure you the current implementation in sharedb works without fail.

I never questioned that it works. I just pointed out that it implements a different spec than the one referenced in the readme file.

IMHO, the written ottypes/docs spec is clearer and simpler than the implied spec implemented by ShareDB, so I'd still argue for maintaining compatibility with ottypes/docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants