From e58fb0bce3e9a764b072b610c6f9eeaed00220f8 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 25 Oct 2021 06:07:51 -0400 Subject: [PATCH] Changeset: Move changeset logic to a new `Changeset` class --- src/node/handler/PadMessageHandler.js | 5 +- src/node/utils/padDiff.js | 2 +- src/static/js/Changeset.js | 242 +++++++++++++++----------- src/static/js/ace2_inner.js | 6 +- src/static/js/changesettracker.js | 2 +- src/tests/frontend/specs/easysync.js | 49 ++++-- 6 files changed, 181 insertions(+), 125 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 4f7a999e5e9..f5f5b0e4102 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -572,8 +572,7 @@ const handleUserChanges = async (socket, message) => { // create the changeset try { try { - // Verify that the changeset has valid syntax and is in canonical form - Changeset.checkRep(changeset); + const cs = Changeset.unpack(changeset).validate(); // Verify that the attribute indexes used in the changeset are all // defined in the accompanying attribute pool. @@ -584,7 +583,7 @@ const handleUserChanges = async (socket, message) => { }); // Validate all added 'author' attribs to be the same value as the current user - for (const op of Changeset.deserializeOps(Changeset.unpack(changeset).ops)) { + for (const op of Changeset.deserializeOps(cs.ops)) { // + can add text with attribs // = can change or add attribs // - can have attribs, but they are discarded and don't show up in the attribs - diff --git a/src/node/utils/padDiff.js b/src/node/utils/padDiff.js index 9c9eef0e6df..8087cb6fe63 100644 --- a/src/node/utils/padDiff.js +++ b/src/node/utils/padDiff.js @@ -461,7 +461,7 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) { } } - return Changeset.checkRep(builder.toString()); + return builder.build().validate().toString(); }; // export the constructor diff --git a/src/static/js/Changeset.js b/src/static/js/Changeset.js index d78610e57ad..39578500cda 100644 --- a/src/static/js/Changeset.js +++ b/src/static/js/Changeset.js @@ -157,14 +157,123 @@ exports.Op = Op; /** * Describes changes to apply to a document. Does not include the attribute pool or the original * document. - * - * @typedef {object} Changeset - * @property {number} oldLen - The length of the base document. - * @property {number} newLen - The length of the document after applying the changeset. - * @property {string} ops - Serialized sequence of operations. Use `deserializeOps` to parse this - * string. - * @property {string} charBank - Characters inserted by insert operations. */ +class Changeset { + /** + * Parses an encoded changeset. + * + * @param {string} cs - Encoded changeset. + * @returns {Changeset} + */ + static unpack(cs) { + const headerRegex = /Z:([0-9a-z]+)([><])([0-9a-z]+)|/; + const headerMatch = headerRegex.exec(cs); + if ((!headerMatch) || (!headerMatch[0])) { + error(`Not a exports: ${cs}`); + } + const oldLen = exports.parseNum(headerMatch[1]); + const changeSign = (headerMatch[2] === '>') ? 1 : -1; + const changeMag = exports.parseNum(headerMatch[3]); + const newLen = oldLen + changeSign * changeMag; + const opsStart = headerMatch[0].length; + let opsEnd = cs.indexOf('$'); + if (opsEnd < 0) opsEnd = cs.length; + return new Changeset(oldLen, newLen, cs.substring(opsStart, opsEnd), cs.substring(opsEnd + 1)); + } + + /** + * @param {number} oldLen - Initial value of the `oldLen` property. + * @param {number} newLen - Initial value of the `newLen` property. + * @param {string} ops - Initial value of the `ops` property. + * @param {string} charBank - Initial value of the `charBank` property. + */ + constructor(oldLen, newLen, ops, charBank) { + /** + * The length of the base document. + * + * @type {number} + * @public + */ + this.oldLen = oldLen; + + /** + * The length of the document after applying the changeset. + * + * @type {number} + * @public + */ + this.newLen = newLen; + + /** + * Serialized sequence of operations. Use `deserializeOps` to parse this string. + * + * @type {string} + * @public + */ + this.ops = ops; + + /** + * Characters inserted by insert operations. + * + * @type {string} + * @public + */ + this.charBank = charBank; + } + + /** + * @returns {string} The encoded changeset. + */ + toString() { + const lenDiff = this.newLen - this.oldLen; + const lenDiffStr = lenDiff >= 0 + ? `>${exports.numToString(lenDiff)}` + : `<${exports.numToString(-lenDiff)}`; + const a = []; + a.push('Z:', exports.numToString(this.oldLen), lenDiffStr, this.ops, '$', this.charBank); + return a.join(''); + } + + /** + * Check that this Changeset is valid. This method does not check things that require access to + * the attribute pool (e.g., attribute order) or original text (e.g., newline positions). + * + * @returns {Changeset} this (for chaining) + */ + validate() { + let oldPos = 0; + let calcNewLen = 0; + let numInserted = 0; + const cs = this.toString(); + const ops = (function* () { + for (const o of exports.deserializeOps(this.ops)) { + switch (o.opcode) { + case '=': + oldPos += o.chars; + calcNewLen += o.chars; + break; + case '-': + oldPos += o.chars; + assert(oldPos <= this.oldLen, `${oldPos} > ${this.oldLen} in ${cs}`); + break; + case '+': + calcNewLen += o.chars; + numInserted += o.chars; + assert(calcNewLen <= this.newLen, `${calcNewLen} > ${this.newLen} in ${cs}`); + break; + } + yield o; + } + })(); + const serializedOps = exports.serializeOps(exports.canonicalizeOps(ops, true)); + calcNewLen += this.oldLen - oldPos; + let charBank = this.charBank.substring(0, numInserted); + while (charBank.length < numInserted) charBank += '?'; + const normalized = new Changeset(this.oldLen, calcNewLen, serializedOps, charBank).toString(); + assert(normalized === cs, 'Invalid changeset'); + return this; + } +} /** * Returns the required length of the text before changeset can be applied. @@ -172,7 +281,7 @@ exports.Op = Op; * @param {string} cs - String representation of the Changeset * @returns {number} oldLen property */ -exports.oldLen = (cs) => exports.unpack(cs).oldLen; +exports.oldLen = (cs) => Changeset.unpack(cs).oldLen; /** * Returns the length of the text after changeset is applied. @@ -180,7 +289,7 @@ exports.oldLen = (cs) => exports.unpack(cs).oldLen; * @param {string} cs - String representation of the Changeset * @returns {number} newLen property */ -exports.newLen = (cs) => exports.unpack(cs).newLen; +exports.newLen = (cs) => Changeset.unpack(cs).newLen; /** * Parses a string of serialized changeset operations. @@ -594,49 +703,12 @@ class SmartOpAssembler { * Used to check if a Changeset is valid. This function does not check things that require access to * the attribute pool (e.g., attribute order) or original text (e.g., newline positions). * + * @deprecated Use `Changeset.unpack(cs).validate()` instead. * @param {string} cs - Changeset to check * @returns {string} the checked Changeset */ exports.checkRep = (cs) => { - const unpacked = exports.unpack(cs); - const oldLen = unpacked.oldLen; - const newLen = unpacked.newLen; - let charBank = unpacked.charBank; - - let oldPos = 0; - let calcNewLen = 0; - let numInserted = 0; - const ops = (function* () { - for (const o of exports.deserializeOps(unpacked.ops)) { - switch (o.opcode) { - case '=': - oldPos += o.chars; - calcNewLen += o.chars; - break; - case '-': - oldPos += o.chars; - assert(oldPos <= oldLen, `${oldPos} > ${oldLen} in ${cs}`); - break; - case '+': - calcNewLen += o.chars; - numInserted += o.chars; - assert(calcNewLen <= newLen, `${calcNewLen} > ${newLen} in ${cs}`); - break; - } - yield o; - } - })(); - const serializedOps = exports.serializeOps(exports.canonicalizeOps(ops, true)); - - calcNewLen += oldLen - oldPos; - charBank = charBank.substring(0, numInserted); - while (charBank.length < numInserted) { - charBank += '?'; - } - - const normalized = exports.pack(oldLen, calcNewLen, serializedOps, charBank); - assert(normalized === cs, 'Invalid changeset (checkRep failed)'); - + Changeset.unpack(cs).validate(); return cs; }; @@ -1113,26 +1185,7 @@ const applyZip = (in1, in2, func) => { * @param {string} cs - The encoded changeset. * @returns {Changeset} */ -exports.unpack = (cs) => { - const headerRegex = /Z:([0-9a-z]+)([><])([0-9a-z]+)|/; - const headerMatch = headerRegex.exec(cs); - if ((!headerMatch) || (!headerMatch[0])) { - error(`Not a exports: ${cs}`); - } - const oldLen = exports.parseNum(headerMatch[1]); - const changeSign = (headerMatch[2] === '>') ? 1 : -1; - const changeMag = exports.parseNum(headerMatch[3]); - const newLen = oldLen + changeSign * changeMag; - const opsStart = headerMatch[0].length; - let opsEnd = cs.indexOf('$'); - if (opsEnd < 0) opsEnd = cs.length; - return { - oldLen, - newLen, - ops: cs.substring(opsStart, opsEnd), - charBank: cs.substring(opsEnd + 1), - }; -}; +exports.unpack = (cs) => Changeset.unpack(cs); /** * Creates an encoded changeset. @@ -1143,14 +1196,8 @@ exports.unpack = (cs) => { * @param {string} bank - Characters for insert operations. * @returns {string} The encoded changeset. */ -exports.pack = (oldLen, newLen, opsStr, bank) => { - const lenDiff = newLen - oldLen; - const lenDiffStr = (lenDiff >= 0 ? `>${exports.numToString(lenDiff)}` - : `<${exports.numToString(-lenDiff)}`); - const a = []; - a.push('Z:', exports.numToString(oldLen), lenDiffStr, opsStr, '$', bank); - return a.join(''); -}; +exports.pack = + (oldLen, newLen, opsStr, bank) => new Changeset(oldLen, newLen, opsStr, bank).toString(); /** * Applies a Changeset to a string. @@ -1160,7 +1207,7 @@ exports.pack = (oldLen, newLen, opsStr, bank) => { * @returns {string} */ exports.applyToText = (cs, str) => { - const unpacked = exports.unpack(cs); + const unpacked = Changeset.unpack(cs); assert(str.length === unpacked.oldLen, `mismatched apply: ${str.length} / ${unpacked.oldLen}`); const bankIter = new StringIterator(unpacked.charBank); const strIter = new StringIterator(str); @@ -1204,7 +1251,7 @@ exports.applyToText = (cs, str) => { * @param {string[]} lines - The lines to which the changeset needs to be applied */ exports.mutateTextLines = (cs, lines) => { - const unpacked = exports.unpack(cs); + const unpacked = Changeset.unpack(cs); const bankIter = new StringIterator(unpacked.charBank); const mut = new TextLinesMutator(lines); for (const op of exports.deserializeOps(unpacked.ops)) { @@ -1356,12 +1403,12 @@ const slicerZipperFunc = (attOp, csOp, pool) => { * @returns {string} */ exports.applyToAttribution = (cs, astr, pool) => { - const unpacked = exports.unpack(cs); + const unpacked = Changeset.unpack(cs); return applyZip(astr, unpacked.ops, (op1, op2) => slicerZipperFunc(op1, op2, pool)); }; exports.mutateAttributionLines = (cs, lines, pool) => { - const unpacked = exports.unpack(cs); + const unpacked = Changeset.unpack(cs); const csOps = exports.deserializeOps(unpacked.ops); let csOpsNext = csOps.next(); const csBank = unpacked.charBank; @@ -1503,8 +1550,8 @@ exports.splitTextLines = (text) => text.match(/[^\n]*(?:\n|[^\n]$)/g); * @returns {string} */ exports.compose = (cs1, cs2, pool) => { - const unpacked1 = exports.unpack(cs1); - const unpacked2 = exports.unpack(cs2); + const unpacked1 = Changeset.unpack(cs1); + const unpacked2 = Changeset.unpack(cs2); const len1 = unpacked1.oldLen; const len2 = unpacked1.newLen; assert(len2 === unpacked2.oldLen, 'mismatched composition of two changesets'); @@ -1526,7 +1573,7 @@ exports.compose = (cs1, cs2, pool) => { return opOut; }); - return exports.pack(len1, len3, newOps, bankAssem); + return new Changeset(len1, len3, newOps, bankAssem).toString(); }; /** @@ -1552,7 +1599,7 @@ exports.attributeTester = (attribPair, pool) => { * @param {number} N - length of the identity changeset * @returns {string} */ -exports.identity = (N) => exports.pack(N, N, '', ''); +exports.identity = (N) => new Changeset(N, N, '', '').toString(); /** * Creates a Changeset which works on oldFullText and removes text from spliceStart to @@ -1585,7 +1632,7 @@ exports.makeSplice = (oldFullText, spliceStart, numRemoved, newText, optNewTextA yield* opsFromText('+', newText, optNewTextAPairs, pool); })(); const serializedOps = exports.serializeOps(exports.canonicalizeOps(ops, true)); - return exports.pack(oldLen, newLen, serializedOps, newText); + return new Changeset(oldLen, newLen, serializedOps, newText).toString(); }; /** @@ -1596,7 +1643,7 @@ exports.makeSplice = (oldFullText, spliceStart, numRemoved, newText, optNewTextA * @returns {[number, number, string][]} */ const toSplices = (cs) => { - const unpacked = exports.unpack(cs); + const unpacked = Changeset.unpack(cs); /** @type {[number, number, string][]} */ const splices = []; @@ -1896,7 +1943,7 @@ exports.prepareForWire = (cs, pool) => { * @returns {boolean} */ exports.isIdentity = (cs) => { - const unpacked = exports.unpack(cs); + const unpacked = Changeset.unpack(cs); return unpacked.ops === '' && unpacked.oldLen === unpacked.newLen; }; @@ -2012,17 +2059,12 @@ class Builder { const serializedOps = exports.serializeOps((function* () { lengthChange = yield* exports.canonicalizeOps(this._ops, true); }).call(this)); - return { - oldLen: this._oldLen, - newLen: this._oldLen + lengthChange, - ops: serializedOps, - charBank: this._charBank, - }; + const newLen = this._oldLen + lengthChange; + return new Changeset(this._oldLen, newLen, serializedOps, this._charBank); } toString() { - const {oldLen, newLen, ops, charBank} = this.build(); - return exports.pack(oldLen, newLen, ops, charBank); + return this.build().toString(); } } exports.Builder = Builder; @@ -2130,7 +2172,7 @@ exports.inverse = (cs, lines, alines, pool) => { let curLineOpsLine; let curLineNextOp = new Op('+'); - const unpacked = exports.unpack(cs); + const unpacked = Changeset.unpack(cs); const builder = new Builder(unpacked.newLen); const consumeAttribRuns = (numChars, func /* (len, attribs, endsLine)*/) => { @@ -2254,13 +2296,13 @@ exports.inverse = (cs, lines, alines, pool) => { } } - return exports.checkRep(builder.toString()); + return builder.build().validate().toString(); }; // %CLIENT FILE ENDS HERE% exports.follow = (cs1, cs2, reverseInsertOrder, pool) => { - const unpacked1 = exports.unpack(cs1); - const unpacked2 = exports.unpack(cs2); + const unpacked1 = Changeset.unpack(cs1); + const unpacked2 = Changeset.unpack(cs2); const len1 = unpacked1.oldLen; const len2 = unpacked2.oldLen; assert(len1 === len2, 'mismatched follow - cannot transform cs1 on top of cs2'); @@ -2396,7 +2438,7 @@ exports.follow = (cs1, cs2, reverseInsertOrder, pool) => { }); newLen += oldLen - oldPos; - return exports.pack(oldLen, newLen, newOps, unpacked2.charBank); + return new Changeset(oldLen, newLen, newOps, unpacked2.charBank).toString(); }; const followAttributes = (att1, att2, pool) => { diff --git a/src/static/js/ace2_inner.js b/src/static/js/ace2_inner.js index b2c8dfa90aa..58061a61480 100644 --- a/src/static/js/ace2_inner.js +++ b/src/static/js/ace2_inner.js @@ -537,8 +537,8 @@ function Ace2Inner(editorInfo, cssManagers) { lengthChange = yield* Changeset.canonicalizeOps(ops, false); })()); const newLen = oldLen + lengthChange; - const changeset = - Changeset.checkRep(Changeset.pack(oldLen, newLen, serializedOps, atext.text.slice(0, -1))); + const changeset = Changeset.pack(oldLen, newLen, serializedOps, atext.text.slice(0, -1)); + Changeset.unpack(changeset).validate(); performDocumentApplyChangeset(changeset); performSelectionChange( @@ -1446,7 +1446,7 @@ function Ace2Inner(editorInfo, cssManagers) { }; const doRepApplyChangeset = (changes, insertsAfterSelection) => { - Changeset.checkRep(changes); + Changeset.unpack(changes).validate(); if (Changeset.oldLen(changes) !== rep.alltext.length) { const errMsg = `${Changeset.oldLen(changes)}/${rep.alltext.length}`; diff --git a/src/static/js/changesettracker.js b/src/static/js/changesettracker.js index fa025af1ad3..cb63a56d092 100644 --- a/src/static/js/changesettracker.js +++ b/src/static/js/changesettracker.js @@ -174,7 +174,7 @@ const makeChangesetTracker = (scheduler, apool, aceCallbacksProvider) => { })(); const serializedOps = Changeset.serializeOps(Changeset.squashOps(ops, true)); userChangeset = Changeset.pack(cs.oldLen, cs.newLen, serializedOps, cs.charBank); - Changeset.checkRep(userChangeset); + Changeset.unpack(userChangeset).validate(); } if (Changeset.isIdentity(userChangeset)) toSubmit = null; else toSubmit = userChangeset; diff --git a/src/tests/frontend/specs/easysync.js b/src/tests/frontend/specs/easysync.js index 0f807743b1f..e159589223e 100644 --- a/src/tests/frontend/specs/easysync.js +++ b/src/tests/frontend/specs/easysync.js @@ -187,7 +187,8 @@ describe('easysync', function () { const runApplyToAttributionTest = (testId, attribs, cs, inAttr, outCorrect) => { it(`applyToAttribution#${testId}`, async function () { const p = poolOrArray(attribs); - const result = Changeset.applyToAttribution(Changeset.checkRep(cs), inAttr, p); + Changeset.unpack(cs).validate(); + const result = Changeset.applyToAttribution(cs, inAttr, p); expect(result).to.equal(outCorrect); }); }; @@ -244,7 +245,8 @@ describe('easysync', function () { it(`runMutateAttributionTest#${testId}`, async function () { const p = poolOrArray(attribs); const alines2 = Array.prototype.slice.call(alines); - Changeset.mutateAttributionLines(Changeset.checkRep(cs), alines2, p); + Changeset.unpack(cs).validate(); + Changeset.mutateAttributionLines(cs, alines2, p); expect(alines2).to.eql(outCorrect); const removeQuestionMarks = (a) => a.replace(/\?/g, ''); @@ -531,7 +533,7 @@ describe('easysync', function () { const outText = `${outTextAssem}\n`; const serializedOps = Changeset.serializeOps(Changeset.canonicalizeOps(ops, true)); const cs = Changeset.pack(oldLen, outText.length, serializedOps, charBank); - Changeset.checkRep(cs); + Changeset.unpack(cs).validate(); return [cs, outText]; }; @@ -553,10 +555,14 @@ describe('easysync', function () { const change3 = x3[0]; const text3 = x3[1]; - const change12 = Changeset.checkRep(Changeset.compose(change1, change2, p)); - const change23 = Changeset.checkRep(Changeset.compose(change2, change3, p)); - const change123 = Changeset.checkRep(Changeset.compose(change12, change3, p)); - const change123a = Changeset.checkRep(Changeset.compose(change1, change23, p)); + const change12 = Changeset.compose(change1, change2, p); + Changeset.unpack(change12).validate(); + const change23 = Changeset.compose(change2, change3, p); + Changeset.unpack(change23).validate(); + const change123 = Changeset.compose(change12, change3, p); + Changeset.unpack(change123).validate(); + const change123a = Changeset.compose(change1, change23, p); + Changeset.unpack(change123a).validate(); expect(change123a).to.equal(change123); expect(Changeset.applyToText(change12, startText)).to.equal(text2); @@ -571,9 +577,12 @@ describe('easysync', function () { const p = new AttributePool(); p.putAttrib(['bold', '']); p.putAttrib(['bold', 'true']); - const cs1 = Changeset.checkRep('Z:2>1*1+1*1=1$x'); - const cs2 = Changeset.checkRep('Z:3>0*0|1=3$'); - const cs12 = Changeset.checkRep(Changeset.compose(cs1, cs2, p)); + const cs1 = 'Z:2>1*1+1*1=1$x'; + Changeset.unpack(cs1).validate(); + const cs2 = 'Z:3>0*0|1=3$'; + Changeset.unpack(cs2).validate(); + const cs12 = Changeset.compose(cs1, cs2, p); + Changeset.unpack(cs12).validate(); expect(cs12).to.equal('Z:2>1+1*0|1=2$x'); }); @@ -615,11 +624,15 @@ describe('easysync', function () { const cs1 = randomTestChangeset(startText)[0]; const cs2 = randomTestChangeset(startText)[0]; - const afb = Changeset.checkRep(Changeset.follow(cs1, cs2, false, p)); - const bfa = Changeset.checkRep(Changeset.follow(cs2, cs1, true, p)); + const afb = Changeset.follow(cs1, cs2, false, p); + Changeset.unpack(afb).validate(); + const bfa = Changeset.follow(cs2, cs1, true, p); + Changeset.unpack(bfa).validate(); - const merge1 = Changeset.checkRep(Changeset.compose(cs1, afb)); - const merge2 = Changeset.checkRep(Changeset.compose(cs2, bfa)); + const merge1 = Changeset.compose(cs1, afb); + Changeset.unpack(merge1).validate(); + const merge2 = Changeset.compose(cs2, bfa); + Changeset.unpack(merge2).validate(); expect(merge2).to.equal(merge1); }); @@ -676,7 +689,8 @@ describe('easysync', function () { }); it('testToSplices', async function () { - const cs = Changeset.checkRep('Z:z>9*0=1=4-3+9=1|1-4-4+1*0+a$123456789abcdefghijk'); + const cs = 'Z:z>9*0=1=4-3+9=1|1-4-4+1*0+a$123456789abcdefghijk'; + Changeset.unpack(cs).validate(); const correctSplices = [ [5, 8, '123456789'], [9, 17, 'abcdefghijk'], @@ -686,7 +700,7 @@ describe('easysync', function () { const testCharacterRangeFollow = (testId, cs, oldRange, insertionsAfter, correctNewRange) => { it(`testCharacterRangeFollow#${testId}`, async function () { - cs = Changeset.checkRep(cs); + Changeset.unpack(cs).validate(); expect(Changeset.characterRangeFollow(cs, oldRange[0], oldRange[1], insertionsAfter)) .to.eql(correctNewRange); }); @@ -851,7 +865,8 @@ describe('easysync', function () { const testInverse = (testId, cs, lines, alines, pool, correctOutput) => { it(`testInverse#${testId}`, async function () { pool = poolOrArray(pool); - const str = Changeset.inverse(Changeset.checkRep(cs), lines, alines, pool); + Changeset.unpack(cs).validate(); + const str = Changeset.inverse(cs, lines, alines, pool); expect(str).to.equal(correctOutput); }); };