From a29ae17d3a57c6cd92716b53bbab5b9a229f77e7 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Tue, 12 May 2020 16:55:24 +1000 Subject: [PATCH] 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 | 22 +++++++++---------- app/models/draft_sequence.rb | 2 +- ...2064023_change_draft_sequence_to_bigint.rb | 8 +++++++ spec/models/draft_spec.rb | 21 ++++++++++++------ spec/requests/draft_controller_spec.rb | 8 +++---- 5 files changed, 38 insertions(+), 23 deletions(-) create mode 100644 db/migrate/20200512064023_change_draft_sequence_to_bigint.rb diff --git a/app/models/draft.rb b/app/models/draft.rb index 9d4c08e56f1..a2fba62fe50 100644 --- a/app/models/draft.rb +++ b/app/models/draft.rb @@ -43,17 +43,17 @@ class Draft < ActiveRecord::Base raise Draft::OutOfSequence end - if owner && current_owner && current_owner != owner - sequence += 1 + sequence += 1 - DraftSequence.upsert({ - sequence: sequence, - draft_key: key, - user_id: user.id, - }, - unique_by: [:user_id, :draft_key] - ) - end + # 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 @@ end # 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 # diff --git a/app/models/draft_sequence.rb b/app/models/draft_sequence.rb index ecdfff4c396..411d77492f4 100644 --- a/app/models/draft_sequence.rb +++ b/app/models/draft_sequence.rb @@ -48,7 +48,7 @@ end # 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 # diff --git a/db/migrate/20200512064023_change_draft_sequence_to_bigint.rb b/db/migrate/20200512064023_change_draft_sequence_to_bigint.rb new file mode 100644 index 00000000000..97d511ee159 --- /dev/null +++ b/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 diff --git a/spec/models/draft_spec.rb b/spec/models/draft_spec.rb index 823ab5fab90..b2066b048de 100644 --- a/spec/models/draft_spec.rb +++ b/spec/models/draft_spec.rb @@ -21,13 +21,13 @@ describe Draft do 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 @@ describe Draft do # 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 @@ describe Draft do 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 @@ describe Draft do 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 diff --git a/spec/requests/draft_controller_spec.rb b/spec/requests/draft_controller_spec.rb index f0596ce20e1..f828e6d89de 100644 --- a/spec/requests/draft_controller_spec.rb +++ b/spec/requests/draft_controller_spec.rb @@ -22,7 +22,7 @@ describe DraftController do 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 @@ describe DraftController do 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