Commit 9419d10e authored by Douwe Maan's avatar Douwe Maan

Merge branch 'bvl-rollback-renamed-system-namespace' into 'master'

Don't rename system when migrating from 9.x -> 9.4

Closes #35525 and #36148

See merge request !13228
parents 6b428fb6 2ea8442f
......@@ -4,7 +4,7 @@ class PersonalFileUploader < FileUploader
end
def self.base_dir
File.join(root_dir, 'system')
File.join(root_dir, '-', 'system')
end
private
......
---
title: Don't rename namespace called system when upgrading from 9.1.x to 9.5
merge_request: 13228
author:
......@@ -5,12 +5,12 @@ scope path: :uploads do
constraints: { model: /note|user|group|project/, mounted_as: /avatar|attachment/, filename: /[^\/]+/ }
# show uploads for models, snippets (notes) available for now
get 'system/:model/:id/:secret/:filename',
get '-/system/:model/:id/:secret/:filename',
to: 'uploads#show',
constraints: { model: /personal_snippet/, id: /\d+/, filename: /[^\/]+/ }
# show temporary uploads
get 'system/temp/:secret/:filename',
get '-/system/temp/:secret/:filename',
to: 'uploads#show',
constraints: { filename: /[^\/]+/ }
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class RenameSystemNamespaces < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
include Gitlab::ShellAdapter
disable_ddl_transaction!
class User < ActiveRecord::Base
self.table_name = 'users'
end
class Namespace < ActiveRecord::Base
self.table_name = 'namespaces'
belongs_to :parent, class_name: 'RenameSystemNamespaces::Namespace'
has_one :route, as: :source
has_many :children, class_name: 'RenameSystemNamespaces::Namespace', foreign_key: :parent_id
belongs_to :owner, class_name: 'RenameSystemNamespaces::User'
# Overridden to have the correct `source_type` for the `route` relation
def self.name
'Namespace'
end
def full_path
if route && route.path.present?
@full_path ||= route.path
else
update_route if persisted?
build_full_path
end
end
def build_full_path
if parent && path
parent.full_path + '/' + path
else
path
end
end
def update_route
prepare_route
route.save
end
def prepare_route
route || build_route(source: self)
route.path = build_full_path
route.name = build_full_name
@full_path = nil
@full_name = nil
end
def build_full_name
if parent && name
parent.human_name + ' / ' + name
else
name
end
end
def human_name
owner&.name
end
end
class Route < ActiveRecord::Base
self.table_name = 'routes'
belongs_to :source, polymorphic: true
end
class Project < ActiveRecord::Base
self.table_name = 'projects'
def repository_storage_path
Gitlab.config.repositories.storages[repository_storage]['path']
end
end
DOWNTIME = false
def up
return unless system_namespace
old_path = system_namespace.path
old_full_path = system_namespace.full_path
# Only remove the last occurrence of the path name to get the parent namespace path
namespace_path = remove_last_occurrence(old_full_path, old_path)
new_path = rename_path(namespace_path, old_path)
new_full_path = join_namespace_path(namespace_path, new_path)
Namespace.where(id: system_namespace).update_all(path: new_path) # skips callbacks & validations
replace_statement = replace_sql(Route.arel_table[:path], old_full_path, new_full_path)
route_matches = [old_full_path, "#{old_full_path}/%"]
update_column_in_batches(:routes, :path, replace_statement) do |table, query|
query.where(Route.arel_table[:path].matches_any(route_matches))
end
clear_cache_for_namespace(system_namespace)
# tasks here are based on `Namespace#move_dir`
move_repositories(system_namespace, old_full_path, new_full_path)
move_namespace_folders(uploads_dir, old_full_path, new_full_path) if file_storage?
move_namespace_folders(pages_dir, old_full_path, new_full_path)
end
def down
# nothing to do
end
def remove_last_occurrence(string, pattern)
string.reverse.sub(pattern.reverse, "").reverse
end
def move_namespace_folders(directory, old_relative_path, new_relative_path)
old_path = File.join(directory, old_relative_path)
return unless File.directory?(old_path)
new_path = File.join(directory, new_relative_path)
FileUtils.mv(old_path, new_path)
end
def move_repositories(namespace, old_full_path, new_full_path)
repo_paths_for_namespace(namespace).each do |repository_storage_path|
# Ensure old directory exists before moving it
gitlab_shell.add_namespace(repository_storage_path, old_full_path)
unless gitlab_shell.mv_namespace(repository_storage_path, old_full_path, new_full_path)
say "Exception moving path #{repository_storage_path} from #{old_full_path} to #{new_full_path}"
end
end
end
def rename_path(namespace_path, path_was)
counter = 0
path = "#{path_was}#{counter}"
while route_exists?(join_namespace_path(namespace_path, path))
counter += 1
path = "#{path_was}#{counter}"
end
path
end
def route_exists?(full_path)
Route.where(Route.arel_table[:path].matches(full_path)).any?
end
def join_namespace_path(namespace_path, path)
if namespace_path.present?
File.join(namespace_path, path)
else
path
end
end
def system_namespace
@system_namespace ||= Namespace.where(parent_id: nil)
.where(arel_table[:path].matches(system_namespace_path))
.first
end
def system_namespace_path
"system"
end
def clear_cache_for_namespace(namespace)
project_ids = projects_for_namespace(namespace).pluck(:id)
update_column_in_batches(:projects, :description_html, nil) do |table, query|
query.where(table[:id].in(project_ids))
end
update_column_in_batches(:issues, :description_html, nil) do |table, query|
query.where(table[:project_id].in(project_ids))
end
update_column_in_batches(:merge_requests, :description_html, nil) do |table, query|
query.where(table[:target_project_id].in(project_ids))
end
update_column_in_batches(:notes, :note_html, nil) do |table, query|
query.where(table[:project_id].in(project_ids))
end
update_column_in_batches(:milestones, :description_html, nil) do |table, query|
query.where(table[:project_id].in(project_ids))
end
end
def projects_for_namespace(namespace)
namespace_ids = child_ids_for_parent(namespace, ids: [namespace.id])
namespace_or_children = Project.arel_table[:namespace_id].in(namespace_ids)
Project.unscoped.where(namespace_or_children)
end
# This won't scale to huge trees, but it should do for a handful of namespaces
# called `system`.
def child_ids_for_parent(namespace, ids: [])
namespace.children.each do |child|
ids << child.id
child_ids_for_parent(child, ids: ids) if child.children.any?
end
ids
end
def repo_paths_for_namespace(namespace)
projects_for_namespace(namespace).distinct
.select(:repository_storage).map(&:repository_storage_path)
end
def uploads_dir
File.join(Rails.root, "public", "uploads")
end
def pages_dir
Settings.pages.path
end
def file_storage?
CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File
end
def arel_table
Namespace.arel_table
end
end
......@@ -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
......@@ -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)
......
......@@ -48,7 +48,7 @@ class UpdateUploadPathsToSystem < ActiveRecord::Migration
end
def new_upload_dir
File.join(base_directory, "system")
File.join(base_directory, "-", "system")
end
def arel_table
......
......@@ -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
......@@ -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
......@@ -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
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class MovePersonalSnippetFilesIntoCorrectFolder < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
DOWNTIME = false
NEW_DIRECTORY = File.join('/uploads', '-', 'system', 'personal_snippet')
OLD_DIRECTORY = File.join('/uploads', 'system', 'personal_snippet')
def up
return unless file_storage?
BackgroundMigrationWorker.perform_async('MovePersonalSnippetFiles',
[OLD_DIRECTORY, NEW_DIRECTORY])
end
def down
return unless file_storage?
BackgroundMigrationWorker.perform_async('MovePersonalSnippetFiles',
[NEW_DIRECTORY, OLD_DIRECTORY])
end
def file_storage?
CarrierWave::Uploader::Base.storage == CarrierWave::Storage::File
end
end
module Gitlab
module BackgroundMigration
class MovePersonalSnippetFiles
delegate :select_all, :execute, :quote_string, to: :connection
def perform(relative_source, relative_destination)
@source_relative_location = relative_source
@destination_relative_location = relative_destination
move_personal_snippet_files
end
def move_personal_snippet_files
query = "SELECT uploads.path, uploads.model_id FROM uploads "\
"INNER JOIN snippets ON snippets.id = uploads.model_id WHERE uploader = 'PersonalFileUploader'"
select_all(query).each do |upload|
secret = upload['path'].split('/')[0]
file_name = upload['path'].split('/')[1]
move_file(upload['model_id'], secret, file_name)
update_markdown(upload['model_id'], secret, file_name)
end
end
def move_file(snippet_id, secret, file_name)
source_dir = File.join(base_directory, @source_relative_location, snippet_id.to_s, secret)
destination_dir = File.join(base_directory, @destination_relative_location, snippet_id.to_s, secret)
source_file_path = File.join(source_dir, file_name)
destination_file_path = File.join(destination_dir, file_name)
unless File.exist?(source_file_path)
say "Source file `#{source_file_path}` doesn't exist. Skipping."
return
end
say "Moving file #{source_file_path} -> #{destination_file_path}"
FileUtils.mkdir_p(destination_dir)
FileUtils.move(source_file_path, destination_file_path)
end
def update_markdown(snippet_id, secret, file_name)
source_markdown_path = File.join(@source_relative_location, snippet_id.to_s, secret, file_name)
destination_markdown_path = File.join(@destination_relative_location, snippet_id.to_s, secret, file_name)
source_markdown = "](#{source_markdown_path})"
destination_markdown = "](#{destination_markdown_path})"
quoted_source = quote_string(source_markdown)
quoted_destination = quote_string(destination_markdown)
execute("UPDATE snippets "\
"SET description = replace(snippets.description, '#{quoted_source}', '#{quoted_destination}'), description_html = NULL "\
"WHERE id = #{snippet_id}")
query = "SELECT id, note FROM notes WHERE noteable_id = #{snippet_id} "\
"AND noteable_type = 'Snippet' AND note IS NOT NULL"
select_all(query).each do |note|
text = note['note'].gsub(source_markdown, destination_markdown)
quoted_text = quote_string(text)
execute("UPDATE notes SET note = '#{quoted_text}', note_html = NULL WHERE id = #{note['id']}")
end
end
def base_directory
File.join(Rails.root, 'public')
end
def connection
ActiveRecord::Base.connection
end
def say(message)
Rails.logger.debug(message)
end
end
end
end
......@@ -186,8 +186,8 @@ describe SnippetsController do
end
context 'when the snippet description contains a file' do
let(:picture_file) { '/system/temp/secret56/picture.jpg' }
let(:text_file) { '/system/temp/secret78/text.txt' }
let(:picture_file) { '/-/system/temp/secret56/picture.jpg' }
let(:text_file) { '/-/system/temp/secret78/text.txt' }
let(:description) do
"Description with picture: ![picture](/uploads#{picture_file}) and "\
"text: [text.txt](/uploads#{text_file})"
......@@ -208,8 +208,8 @@ describe SnippetsController do
snippet = subject
expected_description = "Description with picture: "\
"![picture](/uploads/system/personal_snippet/#{snippet.id}/secret56/picture.jpg) and "\
"text: [text.txt](/uploads/system/personal_snippet/#{snippet.id}/secret78/text.txt)"
"![picture](/uploads/-/system/personal_snippet/#{snippet.id}/secret56/picture.jpg) and "\
"text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/secret78/text.txt)"
expect(snippet.description).to eq(expected_description)
end
......
......@@ -102,7 +102,7 @@ describe UploadsController do
subject
expect(response.body).to match '\"alt\":\"rails_sample\"'
expect(response.body).to match "\"url\":\"/uploads/system/temp"
expect(response.body).to match "\"url\":\"/uploads/-/system/temp"
end
it 'does not create an Upload record' do
......@@ -119,7 +119,7 @@ describe UploadsController do
subject
expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
expect(response.body).to match "\"url\":\"/uploads/system/temp"
expect(response.body).to match "\"url\":\"/uploads/-/system/temp"
end
it 'does not create an Upload record' do
......
......@@ -41,7 +41,7 @@ feature 'User creates snippet', :js do
expect(page).to have_content('My Snippet')
link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
expect(link).to match(%r{/uploads/system/temp/\h{32}/banana_sample\.gif\z})
expect(link).to match(%r{/uploads/-/system/temp/\h{32}/banana_sample\.gif\z})
visit(link)
expect(page.status_code).to eq(200)
......@@ -59,7 +59,7 @@ feature 'User creates snippet', :js do
wait_for_requests
link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
expect(link).to match(%r{/uploads/system/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
expect(link).to match(%r{/uploads/-/system/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
visit(link)
expect(page.status_code).to eq(200)
......@@ -84,7 +84,7 @@ feature 'User creates snippet', :js do
end
expect(page).to have_content('Hello World!')
link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
expect(link).to match(%r{/uploads/system/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
expect(link).to match(%r{/uploads/-/system/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
visit(link)
expect(page.status_code).to eq(200)
......
......@@ -33,7 +33,7 @@ feature 'User edits snippet', :js do
wait_for_requests
link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
expect(link).to match(%r{/uploads/system/personal_snippet/#{snippet.id}/\h{32}/banana_sample\.gif\z})
expect(link).to match(%r{/uploads/-/system/personal_snippet/#{snippet.id}/\h{32}/banana_sample\.gif\z})
end
it 'updates the snippet to make it internal' do
......
require 'spec_helper'
describe Gitlab::BackgroundMigration::MovePersonalSnippetFiles do
let(:test_dir) { File.join(Rails.root, 'tmp', 'tests', 'move_snippet_files_test') }
let(:old_uploads_dir) { File.join('uploads', 'system', 'personal_snippet') }
let(:new_uploads_dir) { File.join('uploads', '-', 'system', 'personal_snippet') }
let(:snippet) do
snippet = create(:personal_snippet)
create_upload_for_snippet(snippet)
snippet.update_attributes!(description: markdown_linking_file(snippet))
snippet
end
let(:migration) { described_class.new }
before do
allow(migration).to receive(:base_directory) { test_dir }
end
describe '#perform' do
it 'moves the file on the disk' do
expected_path = File.join(test_dir, new_uploads_dir, snippet.id.to_s, "secret#{snippet.id}", 'upload.txt')
migration.perform(old_uploads_dir, new_uploads_dir)
expect(File.exist?(expected_path)).to be_truthy
end
it 'updates the markdown of the snippet' do
expected_path = File.join(new_uploads_dir, snippet.id.to_s, "secret#{snippet.id}", 'upload.txt')
expected_markdown = "[an upload](#{expected_path})"
migration.perform(old_uploads_dir, new_uploads_dir)
expect(snippet.reload.description).to eq(expected_markdown)
end
it 'updates the markdown of notes' do
expected_path = File.join(new_uploads_dir, snippet.id.to_s, "secret#{snippet.id}", 'upload.txt')
expected_markdown = "with [an upload](#{expected_path})"
note = create(:note_on_personal_snippet, noteable: snippet, note: "with #{markdown_linking_file(snippet)}")
migration.perform(old_uploads_dir, new_uploads_dir)
expect(note.reload.note).to eq(expected_markdown)
end
end
def create_upload_for_snippet(snippet)
snippet_path = path_for_file_in_snippet(snippet)
path = File.join(old_uploads_dir, snippet.id.to_s, snippet_path)
absolute_path = File.join(test_dir, path)
FileUtils.mkdir_p(File.dirname(absolute_path))
FileUtils.touch(absolute_path)
create(:upload, model: snippet, path: snippet_path, uploader: PersonalFileUploader)
end
def path_for_file_in_snippet(snippet)
secret = "secret#{snippet.id}"
filename = 'upload.txt'
File.join(secret, filename)
end
def markdown_linking_file(snippet)
path = File.join(old_uploads_dir, snippet.id.to_s, path_for_file_in_snippet(snippet))
"[an upload](#{path})"
end
end
......@@ -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') }
......
......@@ -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)
......@@ -42,7 +42,7 @@ describe MovePersonalSnippetsFiles do
describe 'updating the markdown' do
it 'includes the new path when the file exists' do
secret = "secret#{snippet.id}"
file_location = "/uploads/system/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
file_location = "/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
migration.up
......@@ -60,7 +60,7 @@ describe MovePersonalSnippetsFiles do
it 'updates the note markdown' do
secret = "secret#{snippet.id}"
file_location = "/uploads/system/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
file_location = "/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/picture.jpg"
markdown = markdown_linking_file('picture.jpg', snippet)
note = create(:note_on_personal_snippet, noteable: snippet, note: "with #{markdown}")
......@@ -108,7 +108,7 @@ describe MovePersonalSnippetsFiles do
it 'keeps the markdown as is when the file is missing' do
secret = "secret#{snippet_with_missing_file.id}"
file_location = "/uploads/system/personal_snippet/#{snippet_with_missing_file.id}/#{secret}/picture.jpg"
file_location = "/uploads/-/system/personal_snippet/#{snippet_with_missing_file.id}/#{secret}/picture.jpg"
migration.down
......@@ -167,7 +167,7 @@ describe MovePersonalSnippetsFiles do
def markdown_linking_file(filename, snippet, in_new_path: false)
markdown = "![#{filename.split('.')[0]}]"
markdown += '(/uploads'
markdown += '/system' if in_new_path
markdown += '/-/system' if in_new_path
markdown += "/#{model_file_path(filename, snippet)})"
markdown
end
......
......@@ -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
......@@ -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") }