Skip to content

Commit

Permalink
🥅 Allow fetching historic snapshots with bad json0 paths
Browse files Browse the repository at this point in the history
`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
  • Loading branch information
alecgibson committed Jul 14, 2021
1 parent e982eb6 commit 53a2238
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 9 deletions.
2 changes: 1 addition & 1 deletion lib/backend.js
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,7 @@ Backend.prototype._fetchSnapshotByTimestamp = function(collection, id, timestamp

Backend.prototype._buildSnapshotFromOps = function(id, startingSnapshot, ops, callback) {
var snapshot = startingSnapshot || new Snapshot(id, 0, null, undefined, null);
var error = ot.applyOps(snapshot, ops);
var error = ot.applyOps(snapshot, ops, {_normalizeJson0Paths: true});
callback(error, snapshot);
};

Expand Down
44 changes: 36 additions & 8 deletions lib/ot.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// These functions understand versions and can deal with out of bound create &
// delete operations.

var types = require('./types').map;
var types = require('./types');
var ShareDBError = require('./error');
var util = require('./util');

Expand All @@ -23,7 +23,7 @@ exports.checkOp = function(op) {
if (typeof typeName !== 'string') {
return new ShareDBError(ERROR_CODE.ERR_OT_OP_BADLY_FORMED, 'Missing create type');
}
var type = types[typeName];
var type = types.map[typeName];
if (type == null || typeof type !== 'object') {
return new ShareDBError(ERROR_CODE.ERR_DOC_TYPE_NOT_RECOGNIZED, 'Unknown type');
}
Expand Down Expand Up @@ -53,7 +53,7 @@ exports.checkOp = function(op) {

// Takes in a string (type name or URI) and returns the normalized name (uri)
exports.normalizeType = function(typeName) {
return types[typeName] && types[typeName].uri;
return types.map[typeName] && types.map[typeName].uri;
};

// This is the super apply function that takes in snapshot data (including the
Expand All @@ -72,7 +72,7 @@ exports.apply = function(snapshot, op) {

// The document doesn't exist, although it might have once existed
var create = op.create;
var type = types[create.type];
var type = types.map[create.type];
if (!type) return new ShareDBError(ERROR_CODE.ERR_DOC_TYPE_NOT_RECOGNIZED, 'Unknown type');

try {
Expand Down Expand Up @@ -105,7 +105,7 @@ function applyOpEdit(snapshot, edit) {
if (!snapshot.type) return new ShareDBError(ERROR_CODE.ERR_DOC_DOES_NOT_EXIST, 'Document does not exist');

if (edit === undefined) return new ShareDBError(ERROR_CODE.ERR_OT_OP_NOT_PROVIDED, 'Missing op');
var type = types[snapshot.type];
var type = types.map[snapshot.type];
if (!type) return new ShareDBError(ERROR_CODE.ERR_DOC_TYPE_NOT_RECOGNIZED, 'Unknown type');

try {
Expand Down Expand Up @@ -138,7 +138,7 @@ exports.transform = function(type, op, appliedOp) {
if (!type) return new ShareDBError(ERROR_CODE.ERR_DOC_DOES_NOT_EXIST, 'Document does not exist');

if (typeof type === 'string') {
type = types[type];
type = types.map[type];
if (!type) return new ShareDBError(ERROR_CODE.ERR_DOC_TYPE_NOT_RECOGNIZED, 'Unknown type');
}

Expand All @@ -157,11 +157,14 @@ exports.transform = function(type, op, appliedOp) {
*
* @param snapshot - a Snapshot object which will be mutated by the provided ops
* @param ops - an array of ops to apply to the snapshot
* @param options - options (currently for internal use only)
* @return an error object if applicable
*/
exports.applyOps = function(snapshot, ops) {
exports.applyOps = function(snapshot, ops, options) {
options = options || {};
for (var index = 0; index < ops.length; index++) {
var op = ops[index];
if (options._normalizeJson0Paths) normalizeJson0Paths(snapshot, op);
snapshot.v = op.v;
var error = exports.apply(snapshot, op);
if (error) return error;
Expand All @@ -174,7 +177,7 @@ exports.transformPresence = function(presence, op, isOwnOp) {

var type = presence.t;
if (typeof type === 'string') {
type = types[type];
type = types.map[type];
}
if (!type) return {code: ERROR_CODE.ERR_DOC_TYPE_NOT_RECOGNIZED, message: 'Unknown type'};
if (!util.supportsPresence(type)) {
Expand All @@ -197,3 +200,28 @@ exports.transformPresence = function(presence, op, isOwnOp) {

presence.v++;
};

/**
* json0 had a breaking change in https://github.com/ottypes/json0/pull/40
* The change added stricter type checking, which breaks fetchSnapshot()
* when trying to rebuild a snapshot from old, committed ops that didn't
* have this stricter validation. This method fixes up the op paths to
* pass the stricter validation
*/
function normalizeJson0Paths(snapshot, json0op) {
if (snapshot.type !== types.defaultType.uri) return;
var ops = json0op.op;
if (!ops) return;
for (var i = 0; i < ops.length; i++) {
var path = ops[i].p;
var element = snapshot.data;
for (var j = 0; j < path.length; j++) {
var key = path[j];
// https://github.com/ottypes/json0/blob/73db17e86adc5d801951d1a69453b01382e66c7d/lib/json0.js#L21
if (Object.prototype.toString.call(element) == '[object Array]') path[j] = +key;
// https://github.com/ottypes/json0/blob/73db17e86adc5d801951d1a69453b01382e66c7d/lib/json0.js#L32
else if (element.constructor === Object) path[j] = key.toString();
element = element[key];
}
}
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"coveralls": "^3.0.7",
"eslint": "^6.5.1",
"eslint-config-google": "^0.14.0",
"ot-json0-v2": "ottypes/json0",
"lolex": "^5.1.1",
"mocha": "^8.2.1",
"nyc": "^14.1.1",
Expand Down
48 changes: 48 additions & 0 deletions test/client/snapshot-version-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ var MemoryDb = require('../../lib/db/memory');
var MemoryMilestoneDb = require('../../lib/milestone-db/memory');
var sinon = require('sinon');
var async = require('async');
var json0v2 = require('ot-json0-v2').type;
var types = require('../../lib/types');

describe('SnapshotVersionRequest', function() {
var backend;
Expand Down Expand Up @@ -438,4 +440,50 @@ describe('SnapshotVersionRequest', function() {
], done);
});
});

describe('invalid json0 path', function() {
beforeEach(function(done) {
var doc = backend.connect().get('series', 'his-dark-materials');
doc.create([{title: 'Golden Compass'}], function(error) {
if (error) return done(error);
doc.submitOp({p: ['0', 'title'], od: 'Golden Compass', oi: 'Northern Lights'}, function(error) {
if (error) return done(error);
doc.submitOp({p: ['1'], li: {title: 'Subtle Knife'}}, done);
});
});
});

describe('json0v1', function() {
it('fetches v2 with json0v1', function(done) {
backend.connect().fetchSnapshot('series', 'his-dark-materials', 2, function(error, snapshot) {
if (error) return done(error);
expect(snapshot.data).to.eql([{title: 'Northern Lights'}]);
done();
});
});
});

describe('json0v2', function() {
var defaultType;

beforeEach(function() {
defaultType = types.defaultType;
types.defaultType = json0v2;
types.register(json0v2);
});

afterEach(function() {
types.defaultType = defaultType;
types.register(defaultType);
});

it('fetches v2 with json0v2', function(done) {
backend.connect().fetchSnapshot('series', 'his-dark-materials', 2, function(error, snapshot) {
if (error) return done(error);
expect(snapshot.data).to.eql([{title: 'Northern Lights'}]);
done();
});
});
});
});
});

0 comments on commit 53a2238

Please sign in to comment.