Merge pull request #46 from conversation/16461-fix-remote-op-apply-du…

…ring-open-phase

Fix how remote ops are applied during open phase

src/client/doc.coffee

@@ -126,6 +126,28 @@ class Doc
         @meta = msg.meta if msg.meta
         @version = msg.v if msg.v?
 
+        # Apply any server ops that were received in between requesting the document
+        # to open and the open being acknowledged/successful
+        serverOpVs = Object.keys(@serverOps).map (v) -> parseInt(v)
+        serverOpMaxV = Math.max(serverOpVs...)
+
+        versionRange = [(@version)..Math.max(serverOpMaxV, (@version))]
+
+        if serverOpMaxV >= @version
+          for v in versionRange
+            docOp = @serverOps[v]
+
+            # Transform any inflight and pending ops by the queued server ops before
+            # sending them to the server
+            if @inflightOp != null
+              [@inflightOp, docOp] = @_xf @inflightOp, docOp
+            if @pendingOp != null
+              [@pendingOp, docOp] = @_xf @pendingOp, docOp
+
+            @version++
+            # Apply the op to @snapshot and trigger any event listeners
+            @_otApply docOp, true
+
         # Resend any previously queued operation.
         if @inflightOp
           response =
@@ -215,22 +237,26 @@ class Doc
         return if msg.v < @version
 
         return @emit 'error', "Expected docName '#{@name}' but got #{msg.doc}" unless msg.doc == @name
-        return @emit 'error', "Expected version #{@version} but got #{msg.v}" unless msg.v == @version
-
-    #    p "if: #{i @inflightOp} pending: #{i @pendingOp} doc '#{@snapshot}' op: #{i msg.op}"
 
         op = msg.op
-        @serverOps[@version] = op
-
-        docOp = op
-        if @inflightOp != null
-          [@inflightOp, docOp] = @_xf @inflightOp, docOp
-        if @pendingOp != null
-          [@pendingOp, docOp] = @_xf @pendingOp, docOp
-
-        @version++
-        # Finally, apply the op to @snapshot and trigger any event listeners
-        @_otApply docOp, true
+        @serverOps[msg.v] = op
+
+        # Only process ops if we've received the open message back from the server.
+        #
+        # If the document isn't open yet, processing the server ops gets delayed
+        # until the open message is received (see `when msg.open == true` branch above)
+        if @state == 'open'
+          return @emit 'error', "Expected version #{@version} but got #{msg.v}" unless msg.v == @version
+
+          docOp = op
+          if @inflightOp != null
+            [@inflightOp, docOp] = @_xf @inflightOp, docOp
+          if @pendingOp != null
+            [@pendingOp, docOp] = @_xf @pendingOp, docOp
+
+          @version++
+          # Finally, apply the op to @snapshot and trigger any event listeners
+          @_otApply docOp, true
 
       when msg.meta
         {path, value} = msg.meta

test/doc.coffee

@@ -0,0 +1,72 @@
+testCase = require('nodeunit').testCase
+
+assert = require 'assert'
+
+server = require '../src/server'
+types = require '../src/types'
+
+nativeclient = require '../src/client'
+webclient = require './helpers/webclient'
+
+genTests = (client) -> testCase
+  setUp: (callback) ->
+    # The connection we use in this test is just a dummy object, messages are
+    # faked without any real client/server interaction
+    @connection = { send: -> {} }
+    @doc = new client.Doc(@connection, "test", { type: "json" })
+
+    # Open the document and apply some ops to get it into a working state
+    @doc.open()
+    @doc._onMessage({ doc: "test", open: true, snapshot: null, type: "json", create: false, v: 1 })
+    @doc._onMessage({ doc: "test", op: [
+      { "p": [], "oi": {} },
+      { "p": ["text"], "oi": "" },
+      { "p": ["text", 0], "si": "foo" }
+    ], v: 1, meta: { source: "remote" }})
+
+    # Disconnect from the document after setting it up
+    @doc.close()
+    callback()
+
+  tearDown: (callback) ->
+    delete @doc
+    callback()
+
+  'receiving a remote op in between open intent and open success message with pending local op': (test) -> 
+    console.log(@doc.snapshot)
+
+    # To start with just assert that the document snapshot and type is in the 
+    # correct shape as the setup is a bit complex
+    test.strictEqual @doc.type.name, "json"
+    test.strictEqual @doc.version, 2
+    test.deepEqual @doc.snapshot, { text: "foo" }
+
+    # This should set the doc state to "opening" until we hear back from the server
+    @doc.open()
+    test.strictEqual @doc.state, "opening"
+
+    # "submit" an op (while opening, will be stored as pending op)
+    @doc.submitOp { "p": ["text", 3], "si": " bar" }
+    test.deepEqual @doc.pendingOp, [{ "p": ["text", 3], "si": " bar" }]
+
+    # Simulate receiving a remote op at version 2 from the server
+    @doc._onMessage({ doc: "test", op: [{ "p": ["text", 3], "si": " baz"}], v: 2, meta: { source: "remote" }})
+
+    # It should be stored in @serverOps
+    test.deepEqual @doc.serverOps[2], [{ "p": ["text", 3], "si": " baz"}]
+
+    # But does not increment the document version yet
+    test.strictEqual @doc.version, 2
+
+    # Simulate the server acknowledging that we opened the document at version 2
+    @doc._onMessage({ doc: "test", open: true, v: 2 })
+
+    # Our pending op is transformed by the queued remote server op and applied to the snapshot
+    test.deepEqual @doc.snapshot, { text: "foo bar baz" }
+
+    # And our document version should be at 3 now
+    test.strictEqual @doc.version, 3
+    test.done()
+
+exports.native = genTests nativeclient
+exports.webclient = genTests webclient

tests.coffee

@@ -17,6 +17,7 @@ modules = [
   'types/json-api'
 
   'db'
+  'doc'
   'model'
   'useragent'
   'events'