diff --git a/db/migrate/20170316163845_move_uploads_to_system_dir.rb b/db/migrate/20170316163845_move_uploads_to_system_dir.rb index 564ee10b5ab..cfcb909ddaf 100644 --- a/db/migrate/20170316163845_move_uploads_to_system_dir.rb +++ b/db/migrate/20170316163845_move_uploads_to_system_dir.rb @@ -54,6 +54,6 @@ class MoveUploadsToSystemDir < ActiveRecord::Migration end def new_upload_dir - File.join(base_directory, "public", "uploads", "system") + File.join(base_directory, "public", "uploads", "-", "system") end end diff --git a/db/migrate/20170717074009_move_system_upload_folder.rb b/db/migrate/20170717074009_move_system_upload_folder.rb index cce31794115..d3caa53a7a4 100644 --- a/db/migrate/20170717074009_move_system_upload_folder.rb +++ b/db/migrate/20170717074009_move_system_upload_folder.rb @@ -15,6 +15,11 @@ class MoveSystemUploadFolder < ActiveRecord::Migration return end + if File.directory?(new_directory) + say "#{new_directory} already exists. No need to redo the move." + return + end + FileUtils.mkdir_p(File.join(base_directory, '-')) say "Moving #{old_directory} -> #{new_directory}" @@ -33,6 +38,11 @@ class MoveSystemUploadFolder < ActiveRecord::Migration return end + if !File.symlink?(old_directory) && File.directory?(old_directory) + say "#{old_directory} already exists and is not a symlink, no need to revert." + return + end + if File.symlink?(old_directory) say "Removing #{old_directory} -> #{new_directory} symlink" FileUtils.rm(old_directory) diff --git a/db/post_migrate/20170317162059_update_upload_paths_to_system.rb b/db/post_migrate/20170317162059_update_upload_paths_to_system.rb new file mode 100644 index 00000000000..92e33848bf0 --- /dev/null +++ b/db/post_migrate/20170317162059_update_upload_paths_to_system.rb @@ -0,0 +1,57 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class UpdateUploadPathsToSystem < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + AFFECTED_MODELS = %w(User Project Note Namespace Appearance) + + disable_ddl_transaction! + + def up + update_column_in_batches(:uploads, :path, replace_sql(arel_table[:path], base_directory, new_upload_dir)) do |_table, query| + query.where(uploads_to_switch_to_new_path) + end + end + + def down + update_column_in_batches(:uploads, :path, replace_sql(arel_table[:path], new_upload_dir, base_directory)) do |_table, query| + query.where(uploads_to_switch_to_old_path) + end + end + + # "SELECT \"uploads\".* FROM \"uploads\" WHERE \"uploads\".\"model_type\" IN ('User', 'Project', 'Note', 'Namespace', 'Appearance') AND (\"uploads\".\"path\" ILIKE 'uploads/%' AND NOT (\"uploads\".\"path\" ILIKE 'uploads/system/%'))" + def uploads_to_switch_to_new_path + affected_uploads.and(starting_with_base_directory).and(starting_with_new_upload_directory.not) + end + + # "SELECT \"uploads\".* FROM \"uploads\" WHERE \"uploads\".\"model_type\" IN ('User', 'Project', 'Note', 'Namespace', 'Appearance') AND (\"uploads\".\"path\" ILIKE 'uploads/%' AND \"uploads\".\"path\" ILIKE 'uploads/system/%')" + def uploads_to_switch_to_old_path + affected_uploads.and(starting_with_new_upload_directory) + end + + def starting_with_base_directory + arel_table[:path].matches("#{base_directory}/%") + end + + def starting_with_new_upload_directory + arel_table[:path].matches("#{new_upload_dir}/%") + end + + def affected_uploads + arel_table[:model_type].in(AFFECTED_MODELS) + end + + def base_directory + "uploads" + end + + def new_upload_dir + File.join(base_directory, "-", "system") + end + + def arel_table + Arel::Table.new(:uploads) + end +end diff --git a/db/post_migrate/20170406111121_clean_upload_symlinks.rb b/db/post_migrate/20170406111121_clean_upload_symlinks.rb index fc3a4acc0bb..f2ce25d4524 100644 --- a/db/post_migrate/20170406111121_clean_upload_symlinks.rb +++ b/db/post_migrate/20170406111121_clean_upload_symlinks.rb @@ -47,6 +47,6 @@ class CleanUploadSymlinks < ActiveRecord::Migration end def new_upload_dir - File.join(base_directory, "public", "uploads", "system") + File.join(base_directory, "public", "uploads", "-", "system") end end diff --git a/db/post_migrate/20170606202615_move_appearance_to_system_dir.rb b/db/post_migrate/20170606202615_move_appearance_to_system_dir.rb index 561de59ec69..07935ab8a52 100644 --- a/db/post_migrate/20170606202615_move_appearance_to_system_dir.rb +++ b/db/post_migrate/20170606202615_move_appearance_to_system_dir.rb @@ -52,6 +52,6 @@ class MoveAppearanceToSystemDir < ActiveRecord::Migration end def new_upload_dir - File.join(base_directory, "public", "uploads", "system") + File.join(base_directory, "public", "uploads", "-", "system") end end diff --git a/db/post_migrate/20170612071012_move_personal_snippets_files.rb b/db/post_migrate/20170612071012_move_personal_snippets_files.rb index 33043364bde..2b79a87ccd8 100644 --- a/db/post_migrate/20170612071012_move_personal_snippets_files.rb +++ b/db/post_migrate/20170612071012_move_personal_snippets_files.rb @@ -10,7 +10,7 @@ class MovePersonalSnippetsFiles < ActiveRecord::Migration return unless file_storage? @source_relative_location = File.join('/uploads', 'personal_snippet') - @destination_relative_location = File.join('/uploads', 'system', 'personal_snippet') + @destination_relative_location = File.join('/uploads', '-', 'system', 'personal_snippet') move_personal_snippet_files end @@ -18,7 +18,7 @@ class MovePersonalSnippetsFiles < ActiveRecord::Migration def down return unless file_storage? - @source_relative_location = File.join('/uploads', 'system', 'personal_snippet') + @source_relative_location = File.join('/uploads', '-', 'system', 'personal_snippet') @destination_relative_location = File.join('/uploads', 'personal_snippet') move_personal_snippet_files diff --git a/spec/migrations/clean_upload_symlinks_spec.rb b/spec/migrations/clean_upload_symlinks_spec.rb index cecb3ddac53..26653b9c008 100644 --- a/spec/migrations/clean_upload_symlinks_spec.rb +++ b/spec/migrations/clean_upload_symlinks_spec.rb @@ -5,7 +5,7 @@ describe CleanUploadSymlinks do let(:migration) { described_class.new } let(:test_dir) { File.join(Rails.root, "tmp", "tests", "move_uploads_test") } let(:uploads_dir) { File.join(test_dir, "public", "uploads") } - let(:new_uploads_dir) { File.join(uploads_dir, "system") } + let(:new_uploads_dir) { File.join(uploads_dir, "-", "system") } let(:original_path) { File.join(new_uploads_dir, 'user') } let(:symlink_path) { File.join(uploads_dir, 'user') } diff --git a/spec/migrations/move_personal_snippets_files_spec.rb b/spec/migrations/move_personal_snippets_files_spec.rb index 8505c7bf3e3..c17e453fe68 100644 --- a/spec/migrations/move_personal_snippets_files_spec.rb +++ b/spec/migrations/move_personal_snippets_files_spec.rb @@ -5,7 +5,7 @@ describe MovePersonalSnippetsFiles do let(:migration) { described_class.new } let(:test_dir) { File.join(Rails.root, "tmp", "tests", "move_snippet_files_test") } let(:uploads_dir) { File.join(test_dir, 'uploads') } - let(:new_uploads_dir) { File.join(uploads_dir, 'system') } + let(:new_uploads_dir) { File.join(uploads_dir, '-', 'system') } before do allow(CarrierWave).to receive(:root).and_return(test_dir) diff --git a/spec/migrations/move_system_upload_folder_spec.rb b/spec/migrations/move_system_upload_folder_spec.rb index b622b4e9536..d3180477db3 100644 --- a/spec/migrations/move_system_upload_folder_spec.rb +++ b/spec/migrations/move_system_upload_folder_spec.rb @@ -33,6 +33,15 @@ describe MoveSystemUploadFolder do expect(File.symlink?(File.join(test_base, 'system'))).to be_truthy expect(File.exist?(File.join(test_base, 'system', 'file'))).to be_truthy end + + it 'does not move if the target directory already exists' do + FileUtils.mkdir_p(File.join(test_base, '-', 'system')) + + expect(FileUtils).not_to receive(:mv) + expect(migration).to receive(:say).with(/already exists. No need to redo the move/) + + migration.up + end end describe '#down' do @@ -58,5 +67,14 @@ describe MoveSystemUploadFolder do expect(File.directory?(File.join(test_base, 'system'))).to be_truthy expect(File.symlink?(File.join(test_base, 'system'))).to be_falsey end + + it 'does not move if the old directory already exists' do + FileUtils.mkdir_p(File.join(test_base, 'system')) + + expect(FileUtils).not_to receive(:mv) + expect(migration).to receive(:say).with(/already exists and is not a symlink, no need to revert/) + + migration.down + end end end diff --git a/spec/migrations/move_uploads_to_system_dir_spec.rb b/spec/migrations/move_uploads_to_system_dir_spec.rb index 37d66452447..ca11a2004c5 100644 --- a/spec/migrations/move_uploads_to_system_dir_spec.rb +++ b/spec/migrations/move_uploads_to_system_dir_spec.rb @@ -5,7 +5,7 @@ describe MoveUploadsToSystemDir do let(:migration) { described_class.new } let(:test_dir) { File.join(Rails.root, "tmp", "move_uploads_test") } let(:uploads_dir) { File.join(test_dir, "public", "uploads") } - let(:new_uploads_dir) { File.join(uploads_dir, "system") } + let(:new_uploads_dir) { File.join(uploads_dir, "-", "system") } before do FileUtils.remove_dir(test_dir) if File.directory?(test_dir) diff --git a/spec/migrations/update_upload_paths_to_system_spec.rb b/spec/migrations/update_upload_paths_to_system_spec.rb new file mode 100644 index 00000000000..0a45c5ea32d --- /dev/null +++ b/spec/migrations/update_upload_paths_to_system_spec.rb @@ -0,0 +1,53 @@ +require "spec_helper" +require Rails.root.join("db", "post_migrate", "20170317162059_update_upload_paths_to_system.rb") + +describe UpdateUploadPathsToSystem do + let(:migration) { described_class.new } + + before do + allow(migration).to receive(:say) + end + + describe "#uploads_to_switch_to_new_path" do + it "contains only uploads with the old path for the correct models" do + _upload_for_other_type = create(:upload, model: create(:ci_pipeline), path: "uploads/ci_pipeline/avatar.jpg") + _upload_with_system_path = create(:upload, model: create(:project), path: "uploads/-/system/project/avatar.jpg") + _upload_with_other_path = create(:upload, model: create(:project), path: "thelongsecretforafileupload/avatar.jpg") + old_upload = create(:upload, model: create(:project), path: "uploads/project/avatar.jpg") + group_upload = create(:upload, model: create(:group), path: "uploads/group/avatar.jpg") + + expect(Upload.where(migration.uploads_to_switch_to_new_path)).to contain_exactly(old_upload, group_upload) + end + end + + describe "#uploads_to_switch_to_old_path" do + it "contains only uploads with the new path for the correct models" do + _upload_for_other_type = create(:upload, model: create(:ci_pipeline), path: "uploads/ci_pipeline/avatar.jpg") + upload_with_system_path = create(:upload, model: create(:project), path: "uploads/-/system/project/avatar.jpg") + _upload_with_other_path = create(:upload, model: create(:project), path: "thelongsecretforafileupload/avatar.jpg") + _old_upload = create(:upload, model: create(:project), path: "uploads/project/avatar.jpg") + + expect(Upload.where(migration.uploads_to_switch_to_old_path)).to contain_exactly(upload_with_system_path) + end + end + + describe "#up", truncate: true do + it "updates old upload records to the new path" do + old_upload = create(:upload, model: create(:project), path: "uploads/project/avatar.jpg") + + migration.up + + expect(old_upload.reload.path).to eq("uploads/-/system/project/avatar.jpg") + end + end + + describe "#down", truncate: true do + it "updates the new system patsh to the old paths" do + new_upload = create(:upload, model: create(:project), path: "uploads/-/system/project/avatar.jpg") + + migration.down + + expect(new_upload.reload.path).to eq("uploads/project/avatar.jpg") + end + end +end