FIX: saving drafts unconditionally increases sequence

Previously we only changed sequence on ownership change, this
cause a race condition between tabs where user could type for a
long time without being warned of an out of date draft.

This change is a radical change and we should watch closely.

Code was already in place to track sequence on the client so no
changes are needed there.

app/models/draft.rb

@@ -43,17 +43,17 @@ def self.set(user, key, sequence, data, owner = nil)
         raise Draft::OutOfSequence
       end
 
-      if owner && current_owner && current_owner != owner
-        sequence += 1
-
-        DraftSequence.upsert({
-            sequence: sequence,
-            draft_key: key,
-            user_id: user.id,
-          },
-          unique_by: [:user_id, :draft_key]
-        )
-      end
+      sequence += 1
+
+      # we need to keep upping our sequence on every save
+      # if we do not do that there are bad race conditions
+      DraftSequence.upsert({
+          sequence: sequence,
+          draft_key: key,
+          user_id: user.id,
+        },
+        unique_by: [:user_id, :draft_key]
+      )
 
       DB.exec(<<~SQL, id: draft_id, sequence: sequence, data: data, owner: owner || current_owner)
         UPDATE drafts
@@ -337,7 +337,7 @@ def self.ensure_draft_topic!(user)
 #  data       :text             not null
 #  created_at :datetime         not null
 #  updated_at :datetime         not null
-#  sequence   :integer          default(0), not null
+#  sequence   :bigint           default(0), not null
 #  revisions  :integer          default(1), not null
 #  owner      :string
 #

app/models/draft_sequence.rb

@@ -48,7 +48,7 @@ def self.invalid_user_id?(user_id)
 #  id        :integer          not null, primary key
 #  user_id   :integer          not null
 #  draft_key :string           not null
-#  sequence  :integer          not null
+#  sequence  :bigint           not null
 #
 # Indexes
 #

db/migrate/20200512064023_change_draft_sequence_to_bigint.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class ChangeDraftSequenceToBigint < ActiveRecord::Migration[6.0]
+  def change
+    change_column :drafts, :sequence, :bigint, default: 0, null: false
+    change_column :draft_sequences, :sequence, :bigint, null: false
+  end
+end

spec/models/draft_spec.rb

@@ -21,13 +21,13 @@
         random_key: "random"
       }
 
-      Draft.set(user, "xyz", 0, draft.to_json)
+      seq = Draft.set(user, "xyz", 0, draft.to_json)
       draft["reply"] = "test" * 100
 
       half_grace = (SiteSetting.editing_grace_period / 2 + 1).seconds
 
       freeze_time half_grace.from_now
-      Draft.set(user, "xyz", 0, draft.to_json)
+      seq = Draft.set(user, "xyz", seq, draft.to_json)
 
       draft_post = BackupDraftPost.find_by(user_id: user.id, key: "xyz").post
 
@@ -37,7 +37,7 @@
 
       # this should trigger a post revision as 10 minutes have passed
       draft["reply"] = "hello"
-      Draft.set(user, "xyz", 0, draft.to_json)
+      Draft.set(user, "xyz", seq, draft.to_json)
 
       draft_topic = BackupDraftTopic.find_by(user_id: user.id)
       expect(draft_topic.topic.posts_count).to eq(2)
@@ -58,9 +58,16 @@
   end
 
   it "should overwrite draft data correctly" do
-    Draft.set(user, "test", 0, "data")
-    Draft.set(user, "test", 0, "new data")
-    expect(Draft.get(user, "test", 0)).to eq "new data"
+    seq = Draft.set(user, "test", 0, "data")
+    seq = Draft.set(user, "test", seq, "new data")
+    expect(Draft.get(user, "test", seq)).to eq "new data"
+  end
+
+  it "should increase the sequence on every save" do
+    seq = Draft.set(user, "test", 0, "data")
+    expect(seq).to eq(0)
+    seq = Draft.set(user, "test", 0, "data")
+    expect(seq).to eq(1)
   end
 
   it "should clear drafts on request" do
@@ -227,7 +234,7 @@
       expect(draft_seq).to eq(1)
 
       draft_seq = Draft.set(user, 'new_topic', 1, 'hello world', _owner = 'HIJKL')
-      expect(draft_seq).to eq(1)
+      expect(draft_seq).to eq(2)
     end
 
     it 'can correctly preload drafts' do

spec/requests/draft_controller_spec.rb

@@ -22,7 +22,7 @@
   end
 
   it "returns 404 when the key is missing" do
-    user = sign_in(Fabricate(:user))
+    _user = sign_in(Fabricate(:user))
     post "/draft.json", params: { data: { my: "data" }.to_json, sequence: 0 }
     expect(response.status).to eq(404)
   end
@@ -135,18 +135,18 @@
 
     expect(response.status).to eq(200)
     json = response.parsed_body
-    expect(json["draft_sequence"]).to eq(1)
+    expect(json["draft_sequence"]).to eq(2)
 
     post "/draft.json", params: {
       draft_key: "abc",
-      sequence: 1,
+      sequence: 2,
       data: { c: "test" }.to_json,
       owner: "abc"
     }
 
     expect(response.status).to eq(200)
     json = response.parsed_body
-    expect(json["draft_sequence"]).to eq(2)
+    expect(json["draft_sequence"]).to eq(3)
   end
 
   it 'raises an error for out-of-sequence draft setting' do