From fa1dac78b279211326551d790debcd72554fe008 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 18 Oct 2021 19:02:35 -0400 Subject: [PATCH] Changeset: Move changeset logic to a new `Changeset` class --- src/node/handler/PadMessageHandler.js | 5 +- src/node/utils/padDiff.js | 4 +- src/static/js/Changeset.js | 225 +++++++++++++++----------- src/static/js/ace2_inner.js | 6 +- src/static/js/changesettracker.js | 2 +- src/tests/frontend/specs/easysync.js | 49 ++++-- 6 files changed, 174 insertions(+), 117 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index fb999bd0964..3c04e151a6b 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -574,8 +574,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. @@ -586,7 +585,7 @@ const handleUserChanges = async (socket, message) => { }); // Validate all added 'author' attribs to be the same value as the current user - for (const op of new Changeset.OpIter(Changeset.unpack(changeset).ops)) { + for (const op of new Changeset.OpIter(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 a3525a80903..f832ab015cb 100644 --- a/src/node/utils/padDiff.js +++ b/src/node/utils/padDiff.js @@ -456,7 +456,9 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) { } } - return Changeset.checkRep(builder.toString()); + const packed = builder.toString(); + Changeset.unpack(packed).validate(); + return packed; }; // export the constructor diff --git a/src/static/js/Changeset.js b/src/static/js/Changeset.js index 88134574cee..79b9f343853 100644 --- a/src/static/js/Changeset.js +++ b/src/static/js/Changeset.js @@ -134,13 +134,114 @@ exports.Op = class { /** * Describes changes to apply to a document. Does not include the attribute pool or the original * document. - * - * @typedef {object} Changeset - * @property {number} oldLen - - * @property {number} newLen - - * @property {string} ops - - * @property {string} charBank - */ +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 - The length of the document before applying the changeset. + * @param {number} newLen - The length of the document after applying the changeset. + * @param {string} ops - Encoded operations to apply to the document. + * @param {string} charBank - Characters for insert operations. + */ + constructor(oldLen, newLen, ops, charBank) { + /** + * @type {number} + * @public + */ + this.oldLen = oldLen; + + /** + * @type {number} + * @public + */ + this.newLen = newLen; + + /** + * @type {string} + * @public + */ + this.ops = ops; + + /** + * @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 ops = (function* () { + for (const o of new exports.OpIter(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 ', this); + break; + case '+': + calcNewLen += o.chars; + numInserted += o.chars; + assert(calcNewLen <= this.newLen, calcNewLen, ' > ', this.newLen, ' in ', this); + 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 === this.toString(), 'Invalid changeset'); + return this; + } +} /** * Returns the required length of the text before changeset can be applied. @@ -148,7 +249,7 @@ exports.Op = class { * @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. @@ -156,7 +257,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; /** * Iterator over a changeset's operations. @@ -587,49 +688,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 new exports.OpIter(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; }; @@ -1089,26 +1153,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. @@ -1119,14 +1164,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. @@ -1136,7 +1175,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); @@ -1180,7 +1219,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 new exports.OpIter(unpacked.ops)) { @@ -1323,12 +1362,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 csIter = new exports.OpIter(unpacked.ops); const csBank = unpacked.charBank; let csBankIndex = 0; @@ -1457,8 +1496,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'); @@ -1480,7 +1519,7 @@ exports.compose = (cs1, cs2, pool) => { return opOut; }); - return exports.pack(len1, len3, newOps, bankAssem); + return new Changeset(len1, len3, newOps, bankAssem).toString(); }; /** @@ -1506,7 +1545,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 @@ -1539,7 +1578,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(); }; /** @@ -1550,7 +1589,7 @@ exports.makeSplice = (oldFullText, spliceStart, numRemoved, newText, optNewTextA * @returns {Array<[number,number,string]>} */ const toSplices = (cs) => { - const unpacked = exports.unpack(cs); + const unpacked = Changeset.unpack(cs); const splices = []; let oldPos = 0; @@ -1852,7 +1891,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; }; @@ -1937,7 +1976,7 @@ exports.Builder = class { lengthChange = yield* exports.canonicalizeOps(this._ops, true); }).call(this)); const newLen = this._oldLen + lengthChange; - return exports.pack(this._oldLen, newLen, serializedOps, this._charBank); + return new Changeset(this._oldLen, newLen, serializedOps, this._charBank).toString(); } }; @@ -2039,7 +2078,7 @@ exports.inverse = (cs, lines, alines, pool) => { let curLineOpIterLine; let curLineNextOp = new exports.Op('+'); - const unpacked = exports.unpack(cs); + const unpacked = Changeset.unpack(cs); const builder = new exports.Builder(unpacked.newLen); const consumeAttribRuns = (numChars, func /* (len, attribs, endsLine)*/) => { @@ -2156,13 +2195,15 @@ exports.inverse = (cs, lines, alines, pool) => { } } - return exports.checkRep(builder.toString()); + const packed = builder.toString(); + Changeset.unpack(packed).validate(); + return packed; }; // %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'); @@ -2298,7 +2339,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 259ee9cccac..129ec7822c5 100644 --- a/src/static/js/ace2_inner.js +++ b/src/static/js/ace2_inner.js @@ -546,8 +546,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( @@ -1474,7 +1474,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 44371402131..c01d407b842 100644 --- a/src/static/js/changesettracker.js +++ b/src/static/js/changesettracker.js @@ -183,7 +183,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); + 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 c7edeb12b73..c88701fbe9c 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); }); @@ -675,7 +688,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'], @@ -685,7 +699,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); }); @@ -850,7 +864,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); }); };