🥅 Allow fetching historic snapshots with bad json0 paths
#496
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`json0` recently merged a [breaking change][1] which enforces some stricter type checking. Apart from this stricter type checking, no other changes were made, so any client that only ever submits well-formed ops should be able to upgrade directly without any trouble. However, if clients submitted some bad ops (that are no longer allowed under the stricter checking), then `fetchSnapshot()` will fail when trying to apply these ops to rebuild the snapshot. This failure can be surprising if the only "bad" thing about the ops was that they had [bad path types][2], because at the time when they were submitted, the snapshot would have been correctly updated. This change rescues from this particular failure by coercing paths into the correct type: `number` for arrays, and `string` for objects. Note that we use the exact same check for arrays and objects as `json0` to ensure consistency. Also note that we don't attempt to rescue new ops being submitted, because these should correctly be rejected. [1]: ottypes/json0#40 [2]: ottypes/json0#37
ericyhwang
approved these changes
Jul 20, 2021
alecgibson
added a commit
that referenced
this pull request
Aug 9, 2021
We recently [added a shim][1] to let consumers bump to a breaking version of `json0`. This shim normalized the `json0` path to allow older, less strict ops to be applied using the stricter version of `json0`. However, that change also included [stricter checking for `lm`][2]. This change updates our shim to also allow for these older ops. We also rename our internal methods to be more reflective of the fact that we're normalizing ops (rather than just their paths). [1]: #496 [2]: https://github.com/ottypes/json0/pull/40/files#diff-188db14d4925d79e94b18d68782b7df264ff95d95a26812b5b2c07c90f5640afR222-R223
Merged
alecgibson
added a commit
that referenced
this pull request
Aug 9, 2021
We recently [added a shim][1] to let consumers bump to a breaking version of `json0`. This shim normalized the `json0` path to allow older, less strict ops to be applied using the stricter version of `json0`. However, that change also included [stricter checking for `lm`][2]. This change updates our shim to also allow for these older ops. We also rename our internal methods to be more reflective of the fact that we're normalizing ops (rather than just their paths). [1]: #496 [2]: https://github.com/ottypes/json0/pull/40/files#diff-188db14d4925d79e94b18d68782b7df264ff95d95a26812b5b2c07c90f5640afR222-R223
alecgibson
added a commit
that referenced
this pull request
Jan 12, 2022
We added some code for backwards-compatibility with breaking `json0` changes in #496 However, this code doesn't correctly handle ops with multiple components since later components can be affected by earlier components. For example, consider this op: ```js [ {p: ['path'], oi: ''}, {p: ['path', 0], si: 'foo'}, ] ``` Assuming an empty starting document `{}`, then if we **don't** update the snapshot between op components, then the second component will attempt to access an invalid path (since `path` doesn't yet exist on the document). This change fixes this case by calling `defaultType.apply()` between op components, so that later components can correctly check their paths. This unfortunately duplicates some work (since normally the op components are iterated [within `json0` itself][1], so we can't insert logic into this loop). However, the work duplication is limited to: - fetching historic snapshots - there is no impact on "live" documents - `json0` ops with multiple components [1]: https://github.com/ottypes/json0/blob/90a3ae26364c4fa3b19b6df34dad46707a704421/lib/json0.js#L145
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
json0recently merged a breaking change which enforces somestricter type checking.
Apart from this stricter type checking, no other changes were made, so
any client that only ever submits well-formed ops should be able to
upgrade directly without any trouble.
However, if clients submitted some bad ops (that are no longer allowed
under the stricter checking), then
fetchSnapshot()will fail whentrying to apply these ops to rebuild the snapshot.
This failure can be surprising if the only "bad" thing about the ops
was that they had bad path types, because at the time when they
were submitted, the snapshot would have been correctly updated.
This change rescues from this particular failure by coercing paths into
the correct type:
numberfor arrays, andstringfor objects. Notethat we use the exact same check for arrays and objects as
json0toensure consistency.
Also note that we don't attempt to rescue new ops being submitted,
because these should correctly be rejected.