Merge branch 'revert_revert_issuable_lock' into 'master'

Revert the revert of Optimistic Locking

## What does this MR do?

It reverts the revert of optimistic lock https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5146 (and revert MR https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5245)

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/19871

Actually the code is already reviewed, except:
* Monkey patch `config/initializers/ar_monkey_patch.rb`
* We removed default values from migration `db/migrate/20160707104333_add_lock_to_issuables.rb`

See merge request !5623

CHANGELOG

 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

app/controllers/projects/issues_controller.rb

@@ -125,6 +125,10 @@ class Projects::IssuesController < Projects::ApplicationController
         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 @@ class Projects::IssuesController < Projects::ApplicationController
   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
 

app/controllers/projects/merge_requests_controller.rb

@@ -258,6 +258,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     else
       render "edit"
     end
+  rescue ActiveRecord::StaleObjectError
+    @conflict = true
+    render :edit
   end
 
   def remove_wip
@@ -493,7 +496,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       :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
 

app/models/concerns/issuable.rb

@@ -87,6 +87,12 @@ module Issuable
       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

app/views/shared/issuable/_form.html.haml

 = 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

config/initializers/ar_monkey_patch.rb

+# 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

db/migrate/20160707104333_add_lock_to_issuables.rb

+# 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

db/schema.rb

@@ -84,8 +84,8 @@ ActiveRecord::Schema.define(version: 20160819221833) do
     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 @@ ActiveRecord::Schema.define(version: 20160819221833) do
     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 @@ ActiveRecord::Schema.define(version: 20160819221833) do
     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 @@ ActiveRecord::Schema.define(version: 20160819221833) do
     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 @@ ActiveRecord::Schema.define(version: 20160819221833) do
     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 @@ ActiveRecord::Schema.define(version: 20160819221833) do
     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 @@ ActiveRecord::Schema.define(version: 20160819221833) do
     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 @@ ActiveRecord::Schema.define(version: 20160819221833) do
   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 @@ ActiveRecord::Schema.define(version: 20160819221833) do
   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

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

spec/features/issues_spec.rb

@@ -122,6 +122,17 @@ describe 'Issues', feature: true do
           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
 

spec/features/merge_requests/edit_mr_spec.rb

@@ -17,5 +17,16 @@ feature 'Edit Merge Request', feature: true do
     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