diff --git a/db/migrate/20170713104829_add_foreign_key_to_merge_requests.rb b/db/migrate/20170713104829_add_foreign_key_to_merge_requests.rb new file mode 100644 index 00000000000..c25d4fd5986 --- /dev/null +++ b/db/migrate/20170713104829_add_foreign_key_to_merge_requests.rb @@ -0,0 +1,45 @@ +class AddForeignKeyToMergeRequests < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + class MergeRequest < ActiveRecord::Base + self.table_name = 'merge_requests' + include ::EachBatch + end + + def up + scope = <<-SQL.strip_heredoc + head_pipeline_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM ci_pipelines + WHERE ci_pipelines.id = merge_requests.head_pipeline_id + ) + SQL + + MergeRequest.where(scope).each_batch(of: 1000) do |merge_requests| + merge_requests.update_all(head_pipeline_id: nil) + end + + unless foreign_key_exists?(:merge_requests, :head_pipeline_id) + add_concurrent_foreign_key(:merge_requests, :ci_pipelines, + column: :head_pipeline_id, on_delete: :nullify) + end + end + + def down + if foreign_key_exists?(:merge_requests, :head_pipeline_id) + remove_foreign_key(:merge_requests, column: :head_pipeline_id) + end + end + + private + + def foreign_key_exists?(table, column) + foreign_keys(table).any? do |key| + key.options[:column] == column.to_s + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 5264fc99557..9c8c64fe2d0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170707184244) do +ActiveRecord::Schema.define(version: 20170713104829) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1615,6 +1615,7 @@ ActiveRecord::Schema.define(version: 20170707184244) do add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade + add_foreign_key "merge_requests", "ci_pipelines", column: "head_pipeline_id", name: "fk_fd82eae0b9", on_delete: :nullify add_foreign_key "merge_requests", "projects", column: "target_project_id", name: "fk_a6963e8447", on_delete: :cascade add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade add_foreign_key "merge_requests_closing_issues", "merge_requests", on_delete: :cascade diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 0643c56db9b..69ca9aa596b 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -140,6 +140,8 @@ module Gitlab return add_foreign_key(source, target, column: column, on_delete: on_delete) + else + on_delete = 'SET NULL' if on_delete == :nullify end disable_statement_timeout @@ -155,7 +157,7 @@ module Gitlab ADD CONSTRAINT #{key_name} FOREIGN KEY (#{column}) REFERENCES #{target} (id) - #{on_delete ? "ON DELETE #{on_delete}" : ''} + #{on_delete ? "ON DELETE #{on_delete.upcase}" : ''} NOT VALID; EOF diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 4259be3f522..a2acd15c8fb 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -174,13 +174,23 @@ describe Gitlab::Database::MigrationHelpers, lib: true do allow(Gitlab::Database).to receive(:mysql?).and_return(false) end - it 'creates a concurrent foreign key' do + it 'creates a concurrent foreign key and validates it' do expect(model).to receive(:disable_statement_timeout) expect(model).to receive(:execute).ordered.with(/NOT VALID/) expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) model.add_concurrent_foreign_key(:projects, :users, column: :user_id) end + + it 'appends a valid ON DELETE statement' do + expect(model).to receive(:disable_statement_timeout) + expect(model).to receive(:execute).with(/ON DELETE SET NULL/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id, + on_delete: :nullify) + end end end end diff --git a/spec/migrations/add_foreign_key_to_merge_requests_spec.rb b/spec/migrations/add_foreign_key_to_merge_requests_spec.rb new file mode 100644 index 00000000000..d9ad9a585f0 --- /dev/null +++ b/spec/migrations/add_foreign_key_to_merge_requests_spec.rb @@ -0,0 +1,39 @@ +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20170713104829_add_foreign_key_to_merge_requests.rb') + +describe AddForeignKeyToMergeRequests, :migration do + let(:projects) { table(:projects) } + let(:merge_requests) { table(:merge_requests) } + let(:pipelines) { table(:ci_pipelines) } + + before do + projects.create!(name: 'gitlab', path: 'gitlab-org/gitlab-ce') + pipelines.create!(project_id: projects.first.id, + ref: 'some-branch', + sha: 'abc12345') + + # merge request without a pipeline + create_merge_request(head_pipeline_id: nil) + + # merge request with non-existent pipeline + create_merge_request(head_pipeline_id: 1234) + + # merge reqeust with existing pipeline assigned + create_merge_request(head_pipeline_id: pipelines.first.id) + end + + it 'correctly adds a foreign key to head_pipeline_id' do + migrate! + + expect(merge_requests.first.head_pipeline_id).to be_nil + expect(merge_requests.second.head_pipeline_id).to be_nil + expect(merge_requests.third.head_pipeline_id).to eq pipelines.first.id + end + + def create_merge_request(**opts) + merge_requests.create!(source_project_id: projects.first.id, + target_project_id: projects.first.id, + source_branch: 'some-branch', + target_branch: 'master', **opts) + end +end