Update migrations to move directly into the `-/system` folder

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

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)

db/post_migrate/20170317162059_update_upload_paths_to_system.rb

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

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

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

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

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') }
 

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)

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

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)

spec/migrations/update_upload_paths_to_system_spec.rb

+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