From 8f9a7ca854ffda26c5ce9aed2aec10bf155d0463 Mon Sep 17 00:00:00 2001
From: Valery Sizov <valery@gitlab.com>
Date: Mon, 1 Aug 2016 18:34:17 +0300
Subject: [PATCH] Revert the revert of Optimistic Locking
---
CHANGELOG | 2 +
app/controllers/projects/issues_controller.rb | 6 +-
.../projects/merge_requests_controller.rb | 5 +-
app/models/concerns/issuable.rb | 6 ++
app/views/shared/issuable/_form.html.haml | 9 +++
config/initializers/ar_monkey_patch.rb | 57 +++++++++++++++++++
.../20160707104333_add_lock_to_issuables.rb | 18 ++++++
db/schema.rb | 32 ++++++-----
features/project/merge_requests.feature | 2 +-
spec/features/issues_spec.rb | 11 ++++
spec/features/merge_requests/edit_mr_spec.rb | 11 ++++
11 files changed, 141 insertions(+), 18 deletions(-)
create mode 100644 config/initializers/ar_monkey_patch.rb
create mode 100644 db/migrate/20160707104333_add_lock_to_issuables.rb
diff --git a/CHANGELOG b/CHANGELOG
index c6a4fe5f5b829..0ea728cf89ba6 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,4 +1,6 @@
Please view this file on the master branch, on stable branches it's out of date.
+v 8.12.0 (unreleased)
+ - Optimistic locking for Issues and Merge Requests (title and description overriding prevention)
v 8.11.0 (unreleased)
- Use test coverage value from the latest successful pipeline in badge. !5862
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 639cf4c0ef285..7b0189150f801 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -125,6 +125,10 @@ def update
render json: @issue.to_json(include: { milestone: {}, assignee: { methods: :avatar_url }, labels: { methods: :text_color } })
end
end
+
+ rescue ActiveRecord::StaleObjectError
+ @conflict = true
+ render :edit
end
def referenced_merge_requests
@@ -230,7 +234,7 @@ def redirect_old
def issue_params
params.require(:issue).permit(
:title, :assignee_id, :position, :description, :confidential,
- :milestone_id, :due_date, :state_event, :task_num, label_ids: []
+ :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: []
)
end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index d3fe441c4d20a..6a8c7166b395b 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -258,6 +258,9 @@ def update
else
render "edit"
end
+ rescue ActiveRecord::StaleObjectError
+ @conflict = true
+ render :edit
end
def remove_wip
@@ -493,7 +496,7 @@ def merge_request_params
:title, :assignee_id, :source_project_id, :source_branch,
:target_project_id, :target_branch, :milestone_id,
:state_event, :description, :task_num, :force_remove_source_branch,
- label_ids: []
+ :lock_version, label_ids: []
)
end
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index afb5ce37c06f6..8e11d4f57cf41 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -87,6 +87,12 @@ def update_assignee_cache_counts
User.find(assignee_id_was).update_cache_counts if assignee_id_was
assignee.update_cache_counts if assignee
end
+
+ # We want to use optimistic lock for cases when only title or description are involved
+ # http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html
+ def locking_enabled?
+ title_changed? || description_changed?
+ end
end
module ClassMethods
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index 544ed6203aa6b..d8cfa1fca7273 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -1,5 +1,12 @@
= form_errors(issuable)
+- if @conflict
+ .alert.alert-danger
+ Someone edited the #{issuable.class.model_name.human.downcase} the same time you did.
+ Please check out
+ = link_to "the #{issuable.class.model_name.human.downcase}", polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), target: "_blank"
+ and make sure your changes will not unintentionally remove theirs
+
.form-group
= f.label :title, class: 'control-label'
@@ -172,3 +179,5 @@
= link_to 'Delete', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), data: { confirm: "#{issuable.class.name.titleize} will be removed! Are you sure?" },
method: :delete, class: 'btn btn-danger btn-grouped'
= link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), class: 'btn btn-grouped btn-cancel'
+
+= f.hidden_field :lock_version
diff --git a/config/initializers/ar_monkey_patch.rb b/config/initializers/ar_monkey_patch.rb
new file mode 100644
index 0000000000000..0da584626eeae
--- /dev/null
+++ b/config/initializers/ar_monkey_patch.rb
@@ -0,0 +1,57 @@
+# rubocop:disable Lint/RescueException
+
+# This patch fixes https://github.com/rails/rails/issues/26024
+# TODO: Remove it when it's no longer necessary
+
+module ActiveRecord
+ module Locking
+ module Optimistic
+ # We overwrite this method because we don't want to have default value
+ # for newly created records
+ def _create_record(attribute_names = self.attribute_names, *) # :nodoc:
+ super
+ end
+
+ def _update_record(attribute_names = self.attribute_names) #:nodoc:
+ return super unless locking_enabled?
+ return 0 if attribute_names.empty?
+
+ lock_col = self.class.locking_column
+
+ previous_lock_value = send(lock_col).to_i
+
+ # This line is added as a patch
+ previous_lock_value = nil if previous_lock_value == '0' || previous_lock_value == 0
+
+ increment_lock
+
+ attribute_names += [lock_col]
+ attribute_names.uniq!
+
+ begin
+ relation = self.class.unscoped
+
+ affected_rows = relation.where(
+ self.class.primary_key => id,
+ lock_col => previous_lock_value,
+ ).update_all(
+ attributes_for_update(attribute_names).map do |name|
+ [name, _read_attribute(name)]
+ end.to_h
+ )
+
+ unless affected_rows == 1
+ raise ActiveRecord::StaleObjectError.new(self, "update")
+ end
+
+ affected_rows
+
+ # If something went wrong, revert the version.
+ rescue Exception
+ send(lock_col + '=', previous_lock_value)
+ raise
+ end
+ end
+ end
+ end
+end
diff --git a/db/migrate/20160707104333_add_lock_to_issuables.rb b/db/migrate/20160707104333_add_lock_to_issuables.rb
new file mode 100644
index 0000000000000..54866d02cbc46
--- /dev/null
+++ b/db/migrate/20160707104333_add_lock_to_issuables.rb
@@ -0,0 +1,18 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddLockToIssuables < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ def up
+ add_column :issues, :lock_version, :integer
+ add_column :merge_requests, :lock_version, :integer
+ end
+
+ def down
+ remove_column :issues, :lock_version
+ remove_column :merge_requests, :lock_version
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 6cbc766831b63..25cfb5de2fa2b 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -84,8 +84,8 @@
t.string "health_check_access_token"
t.boolean "send_user_confirmation_email", default: false
t.integer "container_registry_token_expire_delay", default: 5
- t.boolean "user_default_external", default: false, null: false
t.text "after_sign_up_text"
+ t.boolean "user_default_external", default: false, null: false
t.string "repository_storage", default: "default"
t.string "enabled_git_access_protocol"
t.boolean "domain_blacklist_enabled", default: false
@@ -175,8 +175,8 @@
t.text "artifacts_metadata"
t.integer "erased_by_id"
t.datetime "erased_at"
- t.string "environment"
t.datetime "artifacts_expire_at"
+ t.string "environment"
t.integer "artifacts_size"
t.string "when"
t.text "yaml_variables"
@@ -471,6 +471,7 @@
t.datetime "deleted_at"
t.date "due_date"
t.integer "moved_to_id"
+ t.integer "lock_version"
end
add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -613,12 +614,13 @@
t.datetime "locked_at"
t.integer "updated_by_id"
t.string "merge_error"
+ t.text "merge_params"
t.boolean "merge_when_build_succeeds", default: false, null: false
t.integer "merge_user_id"
t.string "merge_commit_sha"
t.datetime "deleted_at"
t.string "in_progress_merge_commit_sha"
- t.text "merge_params"
+ t.integer "lock_version"
end
add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
@@ -772,10 +774,10 @@
t.integer "user_id", null: false
t.string "token", null: false
t.string "name", null: false
- t.datetime "created_at", null: false
- t.datetime "updated_at", null: false
t.boolean "revoked", default: false
t.datetime "expires_at"
+ t.datetime "created_at", null: false
+ t.datetime "updated_at", null: false
end
add_index "personal_access_tokens", ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree
@@ -838,8 +840,8 @@
t.boolean "only_allow_merge_if_build_succeeds", default: false, null: false
t.boolean "has_external_issue_tracker"
t.string "repository_storage", default: "default", null: false
- t.boolean "has_external_wiki"
t.boolean "request_access_enabled", default: true, null: false
+ t.boolean "has_external_wiki"
end
add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree
@@ -960,8 +962,8 @@
t.string "noteable_type"
t.string "title"
t.text "description"
- t.datetime "created_at", null: false
- t.datetime "updated_at", null: false
+ t.datetime "created_at", null: false
+ t.datetime "updated_at", null: false
t.boolean "submitted_as_ham", default: false, null: false
end
@@ -1032,13 +1034,13 @@
add_index "u2f_registrations", ["user_id"], name: "index_u2f_registrations_on_user_id", using: :btree
create_table "user_agent_details", force: :cascade do |t|
- t.string "user_agent", null: false
- t.string "ip_address", null: false
- t.integer "subject_id", null: false
- t.string "subject_type", null: false
+ t.string "user_agent", null: false
+ t.string "ip_address", null: false
+ t.integer "subject_id", null: false
+ t.string "subject_type", null: false
t.boolean "submitted", default: false, null: false
- t.datetime "created_at", null: false
- t.datetime "updated_at", null: false
+ t.datetime "created_at", null: false
+ t.datetime "updated_at", null: false
end
create_table "users", force: :cascade do |t|
@@ -1154,4 +1156,4 @@
add_foreign_key "protected_branch_merge_access_levels", "protected_branches"
add_foreign_key "protected_branch_push_access_levels", "protected_branches"
add_foreign_key "u2f_registrations", "users"
-end
\ No newline at end of file
+end
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index 6bac601146764..967f2edb243f8 100644
--- a/features/project/merge_requests.feature
+++ b/features/project/merge_requests.feature
@@ -89,7 +89,7 @@ Feature: Project Merge Requests
Then The list should be sorted by "Oldest updated"
@javascript
- Scenario: Visiting Merge Requests from a differente Project after sorting
+ Scenario: Visiting Merge Requests from a different Project after sorting
Given I visit project "Shop" merge requests page
And I sort the list by "Oldest updated"
And I visit dashboard merge requests page
diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb
index 2e595959f04fb..d744d0eb6aff5 100644
--- a/spec/features/issues_spec.rb
+++ b/spec/features/issues_spec.rb
@@ -122,6 +122,17 @@
expect(page).to have_content date.to_s(:medium)
end
end
+
+ it 'warns about version conflict' do
+ issue.update(title: "New title")
+
+ fill_in 'issue_title', with: 'bug 345'
+ fill_in 'issue_description', with: 'bug description'
+
+ click_button 'Save changes'
+
+ expect(page).to have_content 'Someone edited the issue the same time you did'
+ end
end
end
diff --git a/spec/features/merge_requests/edit_mr_spec.rb b/spec/features/merge_requests/edit_mr_spec.rb
index 4109e78f56031..c77e719c5df66 100644
--- a/spec/features/merge_requests/edit_mr_spec.rb
+++ b/spec/features/merge_requests/edit_mr_spec.rb
@@ -17,5 +17,16 @@
it 'has class js-quick-submit in form' do
expect(page).to have_selector('.js-quick-submit')
end
+
+ it 'warns about version conflict' do
+ merge_request.update(title: "New title")
+
+ fill_in 'merge_request_title', with: 'bug 345'
+ fill_in 'merge_request_description', with: 'bug description'
+
+ click_button 'Save changes'
+
+ expect(page).to have_content 'Someone edited the merge request the same time you did'
+ end
end
end
--
GitLab