Commit 8f9a7ca8 authored by Valery Sizov's avatar Valery Sizov
Browse files

Revert the revert of Optimistic Locking

parent 2a7349169e35
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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
= 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
# 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
# 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
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment