From d8ba01cd429498a1fd420540745ddf8fe225502e Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Fri, 17 May 2024 15:50:15 +0100 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Remove=20usage=20of=20`db.?= =?UTF-8?q?getCommittedOpVersion()`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `DB.getCommitedOpVersion()` function is only used [in one place][1]: in a corner case that differentiates between: - a create op that conflicts with another create op - a create op that has been re-submitted because the connection was closed before the ack was received The first of these cases should result in an error, and the second is non-fatal and the error can be swallowed. At the moment, this differentiation is made using the special `DB.getCommittedOpVersion()` function, which - given an op with a `src` and `seq` combination - will return `null` if the op hasn't been committed, or its version number if it has. If the op has a committed version number, we know that the submit is a duplicate, and we can safely ignore it. This approach is problematic, because: - it [needs a whole op index][2] just to handle this niche corner case - the default behaviour [fetches **all** ops][3] unless the driver overrides this behaviour This change proposes that we actually don't need this function **at all**, and implements an alternative approach to differentiate between the above cases. Instead of having to fetch all ops, we just fetch a *single* op, by version number, which [is already indexed][4] anyway. We then check to see if this retrieved op is our creation op. You can't do this in the general case, because an op's version number may be changed between submit and commit because of conflicts with remote ops, which cause it to be transformed up, and its version number incremented. **However**, the create op is a **special case**, which [**cannot** be transformed against **any** other op][5], which means that the version number it's submitted with **must** be the version number it's committed with. We can therefore use this special property of the create op to safely retrieve itself from the data store by version if it has been committed. [1]: https://github.com/share/sharedb/blob/7b20313ded4c302b9416cc6c7821694a7fa491b8/lib/submit-request.js#L112 [2]: https://github.com/share/sharedb-mongo/issues/94 [3]: https://github.com/share/sharedb/blob/7b20313ded4c302b9416cc6c7821694a7fa491b8/lib/db/index.js#L69 [4]: https://github.com/share/sharedb-mongo/blob/7e88b0a23e968e3672ee151bf6abaf3bfdf62484/index.js#L401 [5]: https://github.com/share/sharedb/blob/7b20313ded4c302b9416cc6c7821694a7fa491b8/test/ot.js#L152-L162 --- lib/submit-request.js | 24 ++++++++++++++++-------- test/client/submit.js | 23 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/lib/submit-request.js b/lib/submit-request.js index b1e1b066d..eab49cfdf 100644 --- a/lib/submit-request.js +++ b/lib/submit-request.js @@ -106,16 +106,24 @@ SubmitRequest.prototype.submit = function(callback) { // 'Document already exists' error in response and fail to submit this // op. However, this could also happen in the case that the op was // already committed and the create op was simply resent. In that - // case, we should return a non-fatal 'Op already submitted' error. We - // must get the past ops and check their src and seq values to - // differentiate. - backend.db.getCommittedOpVersion(collection, id, snapshot, op, null, function(err, version) { + // case, we should return a non-fatal 'Op already submitted' error. + // + // To check this, let's see if our create op has already been committed. + // It can be uniquely identified by src+seq, so we can compare that with the + // database. + // + // We fetch the op by version number, which is okay with creates because: + // - version can only be changed by transforming the op by remote ops + // - a create op cannot be transformed by any other op + // - therefore, a create op's version **never** changes between submit and + // commit, so we can fetch by version number + backend.db.getOps(collection, id, op.v, op.v + 1, null, function(err, ops) { if (err) return callback(err); - if (version == null) { - callback(request.alreadyCreatedError()); - } else { - op.v = version; + var committedOp = ops[0]; + if (op.src === committedOp.src && op.seq === committedOp.seq) { callback(request.alreadySubmittedError()); + } else { + callback(request.alreadyCreatedError()); } }); return; diff --git a/test/client/submit.js b/test/client/submit.js index f02d7aaf2..8b7a8dab2 100644 --- a/test/client/submit.js +++ b/test/client/submit.js @@ -246,6 +246,29 @@ module.exports = function() { }); }); + it('does not fail when resubmitting a create op', function(done) { + var backend = this.backend; + var connection = backend.connect(); + var submitted = false; + backend.use('submit', function(request, next) { + if (!submitted) { + submitted = true; + connection.close(); + backend.connect(connection); + } + next(); + }); + var count = 0; + backend.use('reply', function(message, next) { + if (!message.request.create) return next(); + count++; + next(); + if (count === 2) done(); + }); + var doc = connection.get('dogs', 'fido'); + doc.create({age: 3}, errorHandler(done)); + }); + it('server fetches and transforms by already committed op', function(done) { var doc = this.backend.connect().get('dogs', 'fido'); var doc2 = this.backend.connect().get('dogs', 'fido');