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

Special handling of legacy ops #494

Closed
alecgibson opened this issue Jul 12, 2021 · 3 comments
Closed

Special handling of legacy ops #494

alecgibson opened this issue Jul 12, 2021 · 3 comments

Comments

@alecgibson
Copy link
Collaborator

alecgibson commented Jul 12, 2021

This issue is a downstream effect of this json0 issue: ottypes/json0#37

The problem we're facing is this:

  • we previously submitted some "sort of bad" ops: eg [{p: ['foo', '0', 'bar'], oi: 'baz'}] (see above issue for more details on why this is "sort of bad")
  • these ops have now been committed to our database
  • in order to prevent more of these ops being submitted, we've now added stricter checking: 🥅 Stricter types in apply() ottypes/json0#40
  • however, now when we call json0.apply() on old (bad) ops, the apply fails. The main place this is an issue is calling fetchSnapshot(), where we try to fetch old ops and apply them

We'd like — somehow — to keep strict checking on new ops, but be more permissive when building snapshots from old ops.

There are a few ways we could potentially attack this, and I'm not entirely sure which is best:

  • Add middleware for ot.apply() — this would possibly be the most flexible option; here, we have access to the op and to the snapshot, so we can "fix" the op by checking the path against the snapshot. Note that we'd probably need additional context to distinguish between "old" and "new" ops
  • Add middleware for buildSnapshotFromOps() — this would allow us to target just this particular case of rebuilding from old ops, and could let us manipulate the ops. However, sanitizing the ops is easiest when applying each one of them (since we need the snapshot for context). We could potentially also do something hacky like re-register a less strict type for the duration of this (synchronous) operation
  • Allow different types to be registered and used when reading historic ops — this would be the least work for the consumer, and possibly the most semantic, but also feels a little bit dirty to include as an "official" solution
  • Somehow think about semver on types...? If we could mark the stricter type as a new major version, we could register two versions of the type, and have the old ops still work through the less strict version, but new documents are created with the new type, with stricter safety (and docs can be "migrated" by deleting and recreating them with the same contents, but with an updated type). In theory I guess we can do this without any changes to ShareDB...? (You'd just have two completely separate types registered) But it feels a bit "scary", and is hard work for consumers
@alecgibson
Copy link
Collaborator Author

One issue with the delete-create approach is that old clients will probably have their old ops dropped (because I think transforming anything by a deletion just no-ops it, right?).

@alecgibson
Copy link
Collaborator Author

As discussed with @ericyhwang :

  • rename ot.applyOps() to ot.applyHistoricOps() to avoid confusion
  • ot.applyHistoricOps() should then shim the string vs number path issue by checking array vs object (and then maybe in future also check ld, od if we throw when mismatching)

@alecgibson
Copy link
Collaborator Author

Addressed in #496

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

1 participant