From d6435b68c4fc4e0325ec6a3deb807d0e3dd4dec4 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Mon, 6 Nov 2017 13:44:30 -0800 Subject: Add TrackUntrackedUploads post-deploy migration To create the table, and schedule the background migration that begins the work. --- spec/migrations/track_untracked_uploads_spec.rb | 113 ++++++++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 spec/migrations/track_untracked_uploads_spec.rb (limited to 'spec') diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb new file mode 100644 index 00000000000..5c1113a5e47 --- /dev/null +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -0,0 +1,113 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads') + +describe TrackUntrackedUploads, :migration, :sidekiq do + matcher :be_scheduled_migration do + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['args'] == [migration] + end + end + + failure_message do |migration| + "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!" + end + end + + it 'correctly schedules the follow-up background migration' do + Sidekiq::Testing.fake! do + migrate! + + expect(described_class::MIGRATION).to be_scheduled_migration + expect(BackgroundMigrationWorker.jobs.size).to eq(1) + end + end + + it 'ensures the unhashed_upload_files table exists' do + expect do + migrate! + end.to change { table_exists?(:unhashed_upload_files) }.from(false).to(true) + end + + it 'has a path field long enough for really long paths' do + class UnhashedUploadFile < ActiveRecord::Base + self.table_name = 'unhashed_upload_files' + end + + migrate! + + max_length_namespace_path = max_length_project_path = max_length_filename = 'a' * 255 + long_path = "./uploads#{("/#{max_length_namespace_path}") * Namespace::NUMBER_OF_ANCESTORS_ALLOWED}/#{max_length_project_path}/#{max_length_filename}" + unhashed_upload_file = UnhashedUploadFile.new(path: long_path) + unhashed_upload_file.save! + expect(UnhashedUploadFile.first.path.size).to eq(5641) + end + + context 'with tracked and untracked uploads' do + let(:user1) { create(:user) } + let(:user2) { create(:user) } + let(:project1) { create(:project) } + let(:project2) { create(:project) } + let(:appearance) { create(:appearance) } + + before do + fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + + # Tracked, by doing normal file upload + uploaded_file = fixture_file_upload(fixture) + user1.update(avatar: uploaded_file) + project1.update(avatar: uploaded_file) + UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload + appearance.update(logo: uploaded_file) + + # Untracked, by doing normal file upload then deleting records from DB + uploaded_file = fixture_file_upload(fixture) + user2.update(avatar: uploaded_file) + user2.uploads.delete_all + project2.update(avatar: uploaded_file) + UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload + project2.uploads.delete_all + appearance.update(header_logo: uploaded_file) + appearance.uploads.last.destroy + end + + it 'schedules background migrations' do + Sidekiq::Testing.inline! do + migrate! + + # Tracked uploads still exist + expect(user1.uploads.first.attributes).to include({ + "path" => "uploads/-/system/user/avatar/1/rails_sample.jpg", + "uploader" => "AvatarUploader" + }) + expect(project1.uploads.first.attributes).to include({ + "path" => "uploads/-/system/project/avatar/1/rails_sample.jpg", + "uploader" => "AvatarUploader" + }) + expect(appearance.uploads.first.attributes).to include({ + "path" => "uploads/-/system/appearance/logo/1/rails_sample.jpg", + "uploader" => "AttachmentUploader" + }) + expect(project1.uploads.last.path).to match(/\w+\/rails_sample\.jpg/) + expect(project1.uploads.last.uploader).to eq('FileUploader') + + # Untracked uploads are now tracked + expect(user2.uploads.first.attributes).to include({ + "path" => "uploads/-/system/user/avatar/2/rails_sample.jpg", + "uploader" => "AvatarUploader" + }) + expect(project2.uploads.first.attributes).to include({ + "path" => "uploads/-/system/project/avatar/2/rails_sample.jpg", + "uploader" => "AvatarUploader" + }) + expect(appearance.uploads.count).to eq(2) + expect(appearance.uploads.last.attributes).to include({ + "path" => "uploads/-/system/appearance/header_logo/1/rails_sample.jpg", + "uploader" => "AttachmentUploader" + }) + expect(project2.uploads.last.path).to match(/\w+\/rails_sample\.jpg/) + expect(project2.uploads.last.uploader).to eq('FileUploader') + end + end + end +end -- cgit v1.2.1 From ab814e4dd3cf06559a95ca5dd19722431314f6fa Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 8 Nov 2017 12:53:10 -0800 Subject: Backport `which` from EE --- spec/lib/gitlab/utils_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'spec') diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index 3137a72fdc4..e872a5290c5 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/spec/lib/gitlab/utils_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::Utils do - delegate :to_boolean, :boolean_to_yes_no, :slugify, :random_string, to: :described_class + delegate :to_boolean, :boolean_to_yes_no, :slugify, :random_string, :which, to: :described_class describe '.slugify' do { @@ -59,4 +59,12 @@ describe Gitlab::Utils do expect(random_string).to be_kind_of(String) end end + + describe '.which' do + it 'finds the full path to an executable binary' do + expect(File).to receive(:executable?).with('/bin/sh').and_return(true) + + expect(which('sh', 'PATH' => '/bin')).to eq('/bin/sh') + end + end end -- cgit v1.2.1 From b6ea41d13073ce8b4d16b2edb602c82aae10ea0b Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Mon, 6 Nov 2017 17:07:35 -0800 Subject: Find and store unhashed upload file paths --- .../prepare_unhashed_uploads_spec.rb | 80 ++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb new file mode 100644 index 00000000000..2f641a5deed --- /dev/null +++ b/spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' + +describe Gitlab::BackgroundMigration::PrepareUnhashedUploads, :migration, schema: 20171103140253 do + let!(:unhashed_upload_files) { table(:unhashed_upload_files) } + + let(:user1) { create(:user) } + let(:user2) { create(:user) } + let(:project1) { create(:project) } + let(:project2) { create(:project) } + let(:appearance) { create(:appearance) } + + context 'when files were uploaded before and after hashed storage was enabled' do + before do + fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + uploaded_file = fixture_file_upload(fixture) + + user1.update(avatar: uploaded_file) + project1.update(avatar: uploaded_file) + appearance.update(logo: uploaded_file, header_logo: uploaded_file) + uploaded_file = fixture_file_upload(fixture) + UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload + + stub_application_setting(hashed_storage_enabled: true) + + # Hashed files + uploaded_file = fixture_file_upload(fixture) + UploadService.new(project2, uploaded_file, FileUploader).execute + end + + it 'adds unhashed files to the unhashed_upload_files table' do + expect do + described_class.new.perform + end.to change { unhashed_upload_files.count }.from(0).to(5) + end + + it 'does not add hashed files to the unhashed_upload_files table' do + described_class.new.perform + + hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path + expect(unhashed_upload_files.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey + end + + # E.g. from a previous failed run of this background migration + context 'when there is existing data in unhashed_upload_files' do + before do + unhashed_upload_files.create(path: '/foo/bar.jpg') + end + + it 'clears existing data before adding new data' do + expect do + described_class.new.perform + end.to change { unhashed_upload_files.count }.from(1).to(5) + end + end + + # E.g. The installation is in use at the time of migration, and someone has + # just uploaded a file + context 'when there are files in /uploads/tmp' do + before do + FileUtils.touch(Rails.root.join(described_class::UPLOAD_DIR, 'tmp', 'some_file.jpg')) + end + + it 'does not add files from /uploads/tmp' do + expect do + described_class.new.perform + end.to change { unhashed_upload_files.count }.from(0).to(5) + end + end + end + + # Very new or lightly-used installations that are running this migration + # may not have an upload directory because they have no uploads. + context 'when no files were ever uploaded' do + it 'does not add to the unhashed_upload_files table (and does not raise error)' do + expect do + described_class.new.perform + end.not_to change { unhashed_upload_files.count }.from(0) + end + end +end -- cgit v1.2.1 From 8315c66a569bbc1b4806762e4da49c22813fc523 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 7 Nov 2017 12:53:24 -0800 Subject: Kick off follow up background migration jobs To process the unhashed_upload_files table. --- .../prepare_unhashed_uploads_spec.rb | 63 ++++++++++++++++------ 1 file changed, 47 insertions(+), 16 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb index 2f641a5deed..76d126e8f00 100644 --- a/spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::BackgroundMigration::PrepareUnhashedUploads, :migration, schema: 20171103140253 do +describe Gitlab::BackgroundMigration::PrepareUnhashedUploads, :migration, :sidekiq, schema: 20171103140253 do let!(:unhashed_upload_files) { table(:unhashed_upload_files) } let(:user1) { create(:user) } @@ -9,6 +9,18 @@ describe Gitlab::BackgroundMigration::PrepareUnhashedUploads, :migration, schema let(:project2) { create(:project) } let(:appearance) { create(:appearance) } + matcher :be_scheduled_migration do |*expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['args'] == [migration, expected] + end + end + + failure_message do |migration| + "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!" + end + end + context 'when files were uploaded before and after hashed storage was enabled' do before do fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') @@ -28,16 +40,29 @@ describe Gitlab::BackgroundMigration::PrepareUnhashedUploads, :migration, schema end it 'adds unhashed files to the unhashed_upload_files table' do - expect do - described_class.new.perform - end.to change { unhashed_upload_files.count }.from(0).to(5) + Sidekiq::Testing.fake! do + expect do + described_class.new.perform + end.to change { unhashed_upload_files.count }.from(0).to(5) + end end it 'does not add hashed files to the unhashed_upload_files table' do - described_class.new.perform + Sidekiq::Testing.fake! do + described_class.new.perform - hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path - expect(unhashed_upload_files.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey + hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path + expect(unhashed_upload_files.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey + end + end + + it 'correctly schedules the follow-up background migration jobs' do + Sidekiq::Testing.fake! do + described_class.new.perform + + expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5) + expect(BackgroundMigrationWorker.jobs.size).to eq(1) + end end # E.g. from a previous failed run of this background migration @@ -47,9 +72,11 @@ describe Gitlab::BackgroundMigration::PrepareUnhashedUploads, :migration, schema end it 'clears existing data before adding new data' do - expect do - described_class.new.perform - end.to change { unhashed_upload_files.count }.from(1).to(5) + Sidekiq::Testing.fake! do + expect do + described_class.new.perform + end.to change { unhashed_upload_files.count }.from(1).to(5) + end end end @@ -61,9 +88,11 @@ describe Gitlab::BackgroundMigration::PrepareUnhashedUploads, :migration, schema end it 'does not add files from /uploads/tmp' do - expect do - described_class.new.perform - end.to change { unhashed_upload_files.count }.from(0).to(5) + Sidekiq::Testing.fake! do + expect do + described_class.new.perform + end.to change { unhashed_upload_files.count }.from(0).to(5) + end end end end @@ -72,9 +101,11 @@ describe Gitlab::BackgroundMigration::PrepareUnhashedUploads, :migration, schema # may not have an upload directory because they have no uploads. context 'when no files were ever uploaded' do it 'does not add to the unhashed_upload_files table (and does not raise error)' do - expect do - described_class.new.perform - end.not_to change { unhashed_upload_files.count }.from(0) + Sidekiq::Testing.fake! do + expect do + described_class.new.perform + end.not_to change { unhashed_upload_files.count }.from(0) + end end end end -- cgit v1.2.1 From 3a0ad99d59506592e8d5c6abf0de0fc2104f0bf2 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 7 Nov 2017 19:08:02 -0800 Subject: Add untracked files to uploads --- .../populate_untracked_uploads_spec.rb | 581 +++++++++++++++++++++ spec/migrations/track_untracked_uploads_spec.rb | 61 ++- 2 files changed, 619 insertions(+), 23 deletions(-) create mode 100644 spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb new file mode 100644 index 00000000000..ae6a712f2ee --- /dev/null +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -0,0 +1,581 @@ +require 'spec_helper' + +describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sidekiq, schema: 20171103140253 do + let!(:unhashed_upload_files) { table(:unhashed_upload_files) } + let!(:uploads) { table(:uploads) } + + let(:user1) { create(:user) } + let(:user2) { create(:user) } + let(:project1) { create(:project) } + let(:project2) { create(:project) } + let(:appearance) { create(:appearance) } + + context 'with untracked files and tracked files in unhashed_upload_files' do + before do + fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + + # Tracked, by doing normal file upload + uploaded_file = fixture_file_upload(fixture) + user1.update!(avatar: uploaded_file) + project1.update!(avatar: uploaded_file) + UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload + appearance.update!(logo: uploaded_file) + + # Untracked, by doing normal file upload then later deleting records from DB + uploaded_file = fixture_file_upload(fixture) + user2.update!(avatar: uploaded_file) + project2.update!(avatar: uploaded_file) + UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload + appearance.update!(header_logo: uploaded_file) + + # Unhashed upload files created by PrepareUnhashedUploads + unhashed_upload_files.create!(path: appearance.logo.file.file) + unhashed_upload_files.create!(path: appearance.header_logo.file.file) + unhashed_upload_files.create!(path: user1.avatar.file.file) + unhashed_upload_files.create!(path: user2.avatar.file.file) + unhashed_upload_files.create!(path: project1.avatar.file.file) + unhashed_upload_files.create!(path: project2.avatar.file.file) + unhashed_upload_files.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project1.full_path}/#{project1.uploads.last.path}") + unhashed_upload_files.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project2.full_path}/#{project2.uploads.last.path}") + + user2.uploads.delete_all + project2.uploads.delete_all + appearance.uploads.last.destroy + end + + it 'adds untracked files to the uploads table' do + expect do + described_class.new.perform(1, 1000) + end.to change { uploads.count }.from(4).to(8) + + expect(user2.uploads.count).to eq(1) + expect(project2.uploads.count).to eq(2) + expect(appearance.uploads.count).to eq(2) + end + + it 'does not create duplicate uploads of already tracked files' do + described_class.new.perform(1, 1000) + + expect(user1.uploads.count).to eq(1) + expect(project1.uploads.count).to eq(2) + expect(appearance.uploads.count).to eq(2) + end + end + + context 'with no untracked files' do + it 'does not add to the uploads table (and does not raise error)' do + expect do + described_class.new.perform(1, 1000) + end.not_to change { uploads.count }.from(0) + end + end +end + +describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFile do + let(:upload_class) { Gitlab::BackgroundMigration::PopulateUntrackedUploads::Upload } + + describe '#ensure_tracked!' do + let(:user1) { create(:user) } + + context 'when the file is already in the uploads table' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/user/avatar/#{user1.id}/avatar.jpg") } + + before do + upload_class.create!(path: "uploads/-/system/user/avatar/#{user1.id}/avatar.jpg", uploader: 'AvatarUploader', model_type: 'User', model_id: user1.id, size: 1234) + end + + it 'does not add an upload' do + expect do + unhashed_upload_file.ensure_tracked! + end.to_not change { upload_class.count }.from(1) + end + end + end + + describe '#add_to_uploads' do + let(:fixture) { Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') } + let(:uploaded_file) { fixture_file_upload(fixture) } + + context 'for an appearance logo file path' do + let(:appearance) { create(:appearance) } + let(:unhashed_upload_file) { described_class.create!(path: appearance.logo.file.file) } + + before do + appearance.update!(logo: uploaded_file) + appearance.uploads.delete_all + end + + it 'creates an Upload record' do + expect do + unhashed_upload_file.add_to_uploads + end.to change { upload_class.count }.from(0).to(1) + + expect(upload_class.first.attributes).to include({ + "size" => 35255, + "path" => "uploads/-/system/appearance/logo/#{appearance.id}/rails_sample.jpg", + "checksum" => nil, + "model_id" => appearance.id, + "model_type" => "Appearance", + "uploader" => "AttachmentUploader" + }) + end + end + + context 'for an appearance header_logo file path' do + let(:appearance) { create(:appearance) } + let(:unhashed_upload_file) { described_class.create!(path: appearance.header_logo.file.file) } + + before do + appearance.update!(header_logo: uploaded_file) + appearance.uploads.delete_all + end + + it 'creates an Upload record' do + expect do + unhashed_upload_file.add_to_uploads + end.to change { upload_class.count }.from(0).to(1) + + expect(upload_class.first.attributes).to include({ + "size" => 35255, + "path" => "uploads/-/system/appearance/header_logo/#{appearance.id}/rails_sample.jpg", + "checksum" => nil, + "model_id" => appearance.id, + "model_type" => "Appearance", + "uploader" => "AttachmentUploader" + }) + end + end + + context 'for a pre-Markdown Note attachment file path' do + let(:note) { create(:note) } + let(:unhashed_upload_file) { described_class.create!(path: note.attachment.file.file) } + + before do + note.update!(attachment: uploaded_file) + upload_class.delete_all + end + + it 'creates an Upload record' do + expect do + unhashed_upload_file.add_to_uploads + end.to change { upload_class.count }.from(0).to(1) + + expect(upload_class.first.attributes).to include({ + "size" => 35255, + "path" => "uploads/-/system/note/attachment/#{note.id}/rails_sample.jpg", + "checksum" => nil, + "model_id" => note.id, + "model_type" => "Note", + "uploader" => "AttachmentUploader" + }) + end + end + + context 'for a user avatar file path' do + let(:user) { create(:user) } + let(:unhashed_upload_file) { described_class.create!(path: user.avatar.file.file) } + + before do + user.update!(avatar: uploaded_file) + upload_class.delete_all + end + + it 'creates an Upload record' do + expect do + unhashed_upload_file.add_to_uploads + end.to change { upload_class.count }.from(0).to(1) + + expect(upload_class.first.attributes).to include({ + "size" => 35255, + "path" => "uploads/-/system/user/avatar/#{user.id}/rails_sample.jpg", + "checksum" => nil, + "model_id" => user.id, + "model_type" => "User", + "uploader" => "AvatarUploader" + }) + end + end + + context 'for a group avatar file path' do + let(:group) { create(:group) } + let(:unhashed_upload_file) { described_class.create!(path: group.avatar.file.file) } + + before do + group.update!(avatar: uploaded_file) + upload_class.delete_all + end + + it 'creates an Upload record' do + expect do + unhashed_upload_file.add_to_uploads + end.to change { upload_class.count }.from(0).to(1) + + expect(upload_class.first.attributes).to include({ + "size" => 35255, + "path" => "uploads/-/system/group/avatar/#{group.id}/rails_sample.jpg", + "checksum" => nil, + "model_id" => group.id, + "model_type" => "Group", + "uploader" => "AvatarUploader" + }) + end + end + + context 'for a project avatar file path' do + let(:project) { create(:project) } + let(:unhashed_upload_file) { described_class.create!(path: project.avatar.file.file) } + + before do + project.update!(avatar: uploaded_file) + upload_class.delete_all + end + + it 'creates an Upload record' do + expect do + unhashed_upload_file.add_to_uploads + end.to change { upload_class.count }.from(0).to(1) + + expect(upload_class.first.attributes).to include({ + "size" => 35255, + "path" => "uploads/-/system/project/avatar/#{project.id}/rails_sample.jpg", + "checksum" => nil, + "model_id" => project.id, + "model_type" => "Project", + "uploader" => "AvatarUploader" + }) + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + let(:project) { create(:project) } + let(:unhashed_upload_file) { described_class.new(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{project.uploads.first.path}") } + + before do + UploadService.new(project, uploaded_file, FileUploader).execute # Markdown upload + unhashed_upload_file.save! + upload_class.delete_all + end + + it 'creates an Upload record' do + expect do + unhashed_upload_file.add_to_uploads + end.to change { upload_class.count }.from(0).to(1) + + random_hex = unhashed_upload_file.path.match(/\/(\h+)\/rails_sample.jpg/)[1] + expect(upload_class.first.attributes).to include({ + "size" => 35255, + "path" => "#{random_hex}/rails_sample.jpg", + "checksum" => nil, + "model_id" => project.id, + "model_type" => "Project", + "uploader" => "FileUploader" + }) + end + end + end + + describe '#mark_as_tracked' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + + it 'saves the record with tracked set to true' do + expect do + unhashed_upload_file.mark_as_tracked + end.to change { unhashed_upload_file.tracked }.from(false).to(true) + + expect(unhashed_upload_file.persisted?).to be_truthy + end + end + + describe '#upload_path' do + context 'for an appearance logo file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + + it 'returns the file path relative to the CarrierWave root' do + expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/appearance/logo/1/some_logo.jpg') + end + end + + context 'for an appearance header_logo file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + + it 'returns the file path relative to the CarrierWave root' do + expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/appearance/header_logo/1/some_logo.jpg') + end + end + + context 'for a pre-Markdown Note attachment file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + + it 'returns the file path relative to the CarrierWave root' do + expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/note/attachment/1234/some_attachment.pdf') + end + end + + context 'for a user avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + + it 'returns the file path relative to the CarrierWave root' do + expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/user/avatar/1234/avatar.jpg') + end + end + + context 'for a group avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + + it 'returns the file path relative to the CarrierWave root' do + expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/group/avatar/1234/avatar.jpg') + end + end + + context 'for a project avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + + it 'returns the file path relative to the CarrierWave root' do + expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/project/avatar/1234/avatar.jpg') + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + let(:project) { create(:project) } + let(:random_hex) { SecureRandom.hex } + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{random_hex}/Some file.jpg") } + + it 'returns the file path relative to the project directory in uploads' do + expect(unhashed_upload_file.upload_path).to eq("#{random_hex}/Some file.jpg") + end + end + end + + describe '#uploader' do + context 'for an appearance logo file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + + it 'returns AttachmentUploader as a string' do + expect(unhashed_upload_file.uploader).to eq('AttachmentUploader') + end + end + + context 'for an appearance header_logo file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + + it 'returns AttachmentUploader as a string' do + expect(unhashed_upload_file.uploader).to eq('AttachmentUploader') + end + end + + context 'for a pre-Markdown Note attachment file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + + it 'returns AttachmentUploader as a string' do + expect(unhashed_upload_file.uploader).to eq('AttachmentUploader') + end + end + + context 'for a user avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + + it 'returns AvatarUploader as a string' do + expect(unhashed_upload_file.uploader).to eq('AvatarUploader') + end + end + + context 'for a group avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + + it 'returns AvatarUploader as a string' do + expect(unhashed_upload_file.uploader).to eq('AvatarUploader') + end + end + + context 'for a project avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + + it 'returns AvatarUploader as a string' do + expect(unhashed_upload_file.uploader).to eq('AvatarUploader') + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + let(:project) { create(:project) } + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } + + it 'returns FileUploader as a string' do + expect(unhashed_upload_file.uploader).to eq('FileUploader') + end + end + end + + describe '#model_type' do + context 'for an appearance logo file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + + it 'returns Appearance as a string' do + expect(unhashed_upload_file.model_type).to eq('Appearance') + end + end + + context 'for an appearance header_logo file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + + it 'returns Appearance as a string' do + expect(unhashed_upload_file.model_type).to eq('Appearance') + end + end + + context 'for a pre-Markdown Note attachment file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + + it 'returns Note as a string' do + expect(unhashed_upload_file.model_type).to eq('Note') + end + end + + context 'for a user avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + + it 'returns User as a string' do + expect(unhashed_upload_file.model_type).to eq('User') + end + end + + context 'for a group avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + + it 'returns Group as a string' do + expect(unhashed_upload_file.model_type).to eq('Group') + end + end + + context 'for a project avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + + it 'returns Project as a string' do + expect(unhashed_upload_file.model_type).to eq('Project') + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + let(:project) { create(:project) } + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } + + it 'returns Project as a string' do + expect(unhashed_upload_file.model_type).to eq('Project') + end + end + end + + describe '#model_id' do + context 'for an appearance logo file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + + it 'returns the ID as a string' do + expect(unhashed_upload_file.model_id).to eq('1') + end + end + + context 'for an appearance header_logo file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + + it 'returns the ID as a string' do + expect(unhashed_upload_file.model_id).to eq('1') + end + end + + context 'for a pre-Markdown Note attachment file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + + it 'returns the ID as a string' do + expect(unhashed_upload_file.model_id).to eq('1234') + end + end + + context 'for a user avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + + it 'returns the ID as a string' do + expect(unhashed_upload_file.model_id).to eq('1234') + end + end + + context 'for a group avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + + it 'returns the ID as a string' do + expect(unhashed_upload_file.model_id).to eq('1234') + end + end + + context 'for a project avatar file path' do + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + + it 'returns the ID as a string' do + expect(unhashed_upload_file.model_id).to eq('1234') + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + let(:project) { create(:project) } + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } + + it 'returns the ID as a string' do + expect(unhashed_upload_file.model_id).to eq(project.id.to_s) + end + end + end + + describe '#file_size' do + let(:fixture) { Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') } + let(:uploaded_file) { fixture_file_upload(fixture) } + + context 'for an appearance logo file path' do + let(:appearance) { create(:appearance) } + let(:unhashed_upload_file) { described_class.create!(path: appearance.logo.file.file) } + + before do + appearance.update!(logo: uploaded_file) + end + + it 'returns the file size' do + expect(unhashed_upload_file.file_size).to eq(35255) + end + + it 'returns the same thing that CarrierWave would return' do + expect(unhashed_upload_file.file_size).to eq(appearance.logo.size) + end + end + + context 'for a project avatar file path' do + let(:project) { create(:project) } + let(:unhashed_upload_file) { described_class.create!(path: project.avatar.file.file) } + + before do + project.update!(avatar: uploaded_file) + end + + it 'returns the file size' do + expect(unhashed_upload_file.file_size).to eq(35255) + end + + it 'returns the same thing that CarrierWave would return' do + expect(unhashed_upload_file.file_size).to eq(project.avatar.size) + end + end + + context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do + let(:project) { create(:project) } + let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{project.uploads.first.path}") } + + before do + UploadService.new(project, uploaded_file, FileUploader).execute + end + + it 'returns the file size' do + expect(unhashed_upload_file.file_size).to eq(35255) + end + + it 'returns the same thing that CarrierWave would return' do + expect(unhashed_upload_file.file_size).to eq(project.uploads.first.size) + end + end + end +end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 5c1113a5e47..8db41786397 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -2,6 +2,10 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads') describe TrackUntrackedUploads, :migration, :sidekiq do + class UnhashedUploadFile < ActiveRecord::Base + self.table_name = 'unhashed_upload_files' + end + matcher :be_scheduled_migration do match do |migration| BackgroundMigrationWorker.jobs.any? do |job| @@ -30,10 +34,6 @@ describe TrackUntrackedUploads, :migration, :sidekiq do end it 'has a path field long enough for really long paths' do - class UnhashedUploadFile < ActiveRecord::Base - self.table_name = 'unhashed_upload_files' - end - migrate! max_length_namespace_path = max_length_project_path = max_length_filename = 'a' * 255 @@ -57,7 +57,8 @@ describe TrackUntrackedUploads, :migration, :sidekiq do uploaded_file = fixture_file_upload(fixture) user1.update(avatar: uploaded_file) project1.update(avatar: uploaded_file) - UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload + upload_result = UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload + @project1_markdown_upload_path = upload_result[:url].sub(/\A\/uploads\//, '') appearance.update(logo: uploaded_file) # Untracked, by doing normal file upload then deleting records from DB @@ -65,48 +66,62 @@ describe TrackUntrackedUploads, :migration, :sidekiq do user2.update(avatar: uploaded_file) user2.uploads.delete_all project2.update(avatar: uploaded_file) - UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload + upload_result = UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload + @project2_markdown_upload_path = upload_result[:url].sub(/\A\/uploads\//, '') project2.uploads.delete_all appearance.update(header_logo: uploaded_file) appearance.uploads.last.destroy end - it 'schedules background migrations' do + it 'tracks untracked migrations' do Sidekiq::Testing.inline! do migrate! # Tracked uploads still exist - expect(user1.uploads.first.attributes).to include({ - "path" => "uploads/-/system/user/avatar/1/rails_sample.jpg", + expect(user1.reload.uploads.first.attributes).to include({ + "path" => "uploads/-/system/user/avatar/#{user1.id}/rails_sample.jpg", "uploader" => "AvatarUploader" }) - expect(project1.uploads.first.attributes).to include({ - "path" => "uploads/-/system/project/avatar/1/rails_sample.jpg", + expect(project1.reload.uploads.first.attributes).to include({ + "path" => "uploads/-/system/project/avatar/#{project1.id}/rails_sample.jpg", "uploader" => "AvatarUploader" }) - expect(appearance.uploads.first.attributes).to include({ - "path" => "uploads/-/system/appearance/logo/1/rails_sample.jpg", + expect(appearance.reload.uploads.first.attributes).to include({ + "path" => "uploads/-/system/appearance/logo/#{appearance.id}/rails_sample.jpg", "uploader" => "AttachmentUploader" }) - expect(project1.uploads.last.path).to match(/\w+\/rails_sample\.jpg/) - expect(project1.uploads.last.uploader).to eq('FileUploader') + expect(project1.uploads.last.attributes).to include({ + "path" => @project1_markdown_upload_path, + "uploader" => "FileUploader" + }) # Untracked uploads are now tracked - expect(user2.uploads.first.attributes).to include({ - "path" => "uploads/-/system/user/avatar/2/rails_sample.jpg", + expect(user2.reload.uploads.first.attributes).to include({ + "path" => "uploads/-/system/user/avatar/#{user2.id}/rails_sample.jpg", "uploader" => "AvatarUploader" }) - expect(project2.uploads.first.attributes).to include({ - "path" => "uploads/-/system/project/avatar/2/rails_sample.jpg", + expect(project2.reload.uploads.first.attributes).to include({ + "path" => "uploads/-/system/project/avatar/#{project2.id}/rails_sample.jpg", "uploader" => "AvatarUploader" }) - expect(appearance.uploads.count).to eq(2) + expect(appearance.reload.uploads.count).to eq(2) expect(appearance.uploads.last.attributes).to include({ - "path" => "uploads/-/system/appearance/header_logo/1/rails_sample.jpg", + "path" => "uploads/-/system/appearance/header_logo/#{appearance.id}/rails_sample.jpg", "uploader" => "AttachmentUploader" }) - expect(project2.uploads.last.path).to match(/\w+\/rails_sample\.jpg/) - expect(project2.uploads.last.uploader).to eq('FileUploader') + expect(project2.uploads.last.attributes).to include({ + "path" => @project2_markdown_upload_path, + "uploader" => "FileUploader" + }) + end + end + + it 'all UnhashedUploadFile records are marked as tracked' do + Sidekiq::Testing.inline! do + migrate! + + expect(UnhashedUploadFile.count).to eq(8) + expect(UnhashedUploadFile.count).to eq(UnhashedUploadFile.where(tracked: true).count) end end end -- cgit v1.2.1 From 1bae010b63f0bcff79f32ce99190f2d5b6d9fbcd Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 7 Nov 2017 19:54:28 -0800 Subject: Calculate checksums by copy-pasting in the whole `Upload` class. Also, fix `Namespace` `model_type` (it should not be `Group`). --- .../populate_untracked_uploads_spec.rb | 149 +++++++++------------ spec/migrations/track_untracked_uploads_spec.rb | 64 +++++---- 2 files changed, 105 insertions(+), 108 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index ae6a712f2ee..c61a207d012 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -97,61 +97,53 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi let(:uploaded_file) { fixture_file_upload(fixture) } context 'for an appearance logo file path' do - let(:appearance) { create(:appearance) } - let(:unhashed_upload_file) { described_class.create!(path: appearance.logo.file.file) } + let(:model) { create(:appearance) } + let(:unhashed_upload_file) { described_class.create!(path: model.logo.file.file) } before do - appearance.update!(logo: uploaded_file) - appearance.uploads.delete_all + model.update!(logo: uploaded_file) + model.uploads.delete_all end it 'creates an Upload record' do expect do unhashed_upload_file.add_to_uploads - end.to change { upload_class.count }.from(0).to(1) + end.to change { model.reload.uploads.count }.from(0).to(1) - expect(upload_class.first.attributes).to include({ - "size" => 35255, - "path" => "uploads/-/system/appearance/logo/#{appearance.id}/rails_sample.jpg", - "checksum" => nil, - "model_id" => appearance.id, - "model_type" => "Appearance", + expect(model.uploads.first.attributes).to include({ + "path" => "uploads/-/system/appearance/logo/#{model.id}/rails_sample.jpg", "uploader" => "AttachmentUploader" - }) + }.merge(rails_sample_jpg_attrs)) end end context 'for an appearance header_logo file path' do - let(:appearance) { create(:appearance) } - let(:unhashed_upload_file) { described_class.create!(path: appearance.header_logo.file.file) } + let(:model) { create(:appearance) } + let(:unhashed_upload_file) { described_class.create!(path: model.header_logo.file.file) } before do - appearance.update!(header_logo: uploaded_file) - appearance.uploads.delete_all + model.update!(header_logo: uploaded_file) + model.uploads.delete_all end it 'creates an Upload record' do expect do unhashed_upload_file.add_to_uploads - end.to change { upload_class.count }.from(0).to(1) + end.to change { model.reload.uploads.count }.from(0).to(1) - expect(upload_class.first.attributes).to include({ - "size" => 35255, - "path" => "uploads/-/system/appearance/header_logo/#{appearance.id}/rails_sample.jpg", - "checksum" => nil, - "model_id" => appearance.id, - "model_type" => "Appearance", + expect(model.uploads.first.attributes).to include({ + "path" => "uploads/-/system/appearance/header_logo/#{model.id}/rails_sample.jpg", "uploader" => "AttachmentUploader" - }) + }.merge(rails_sample_jpg_attrs)) end end context 'for a pre-Markdown Note attachment file path' do - let(:note) { create(:note) } - let(:unhashed_upload_file) { described_class.create!(path: note.attachment.file.file) } + let(:model) { create(:note) } + let(:unhashed_upload_file) { described_class.create!(path: model.attachment.file.file) } before do - note.update!(attachment: uploaded_file) + model.update!(attachment: uploaded_file) upload_class.delete_all end @@ -161,115 +153,99 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi end.to change { upload_class.count }.from(0).to(1) expect(upload_class.first.attributes).to include({ - "size" => 35255, - "path" => "uploads/-/system/note/attachment/#{note.id}/rails_sample.jpg", - "checksum" => nil, - "model_id" => note.id, + "path" => "uploads/-/system/note/attachment/#{model.id}/rails_sample.jpg", + "model_id" => model.id, "model_type" => "Note", "uploader" => "AttachmentUploader" - }) + }.merge(rails_sample_jpg_attrs)) end end context 'for a user avatar file path' do - let(:user) { create(:user) } - let(:unhashed_upload_file) { described_class.create!(path: user.avatar.file.file) } + let(:model) { create(:user) } + let(:unhashed_upload_file) { described_class.create!(path: model.avatar.file.file) } before do - user.update!(avatar: uploaded_file) - upload_class.delete_all + model.update!(avatar: uploaded_file) + model.uploads.delete_all end it 'creates an Upload record' do expect do unhashed_upload_file.add_to_uploads - end.to change { upload_class.count }.from(0).to(1) + end.to change { model.reload.uploads.count }.from(0).to(1) - expect(upload_class.first.attributes).to include({ - "size" => 35255, - "path" => "uploads/-/system/user/avatar/#{user.id}/rails_sample.jpg", - "checksum" => nil, - "model_id" => user.id, - "model_type" => "User", + expect(model.uploads.first.attributes).to include({ + "path" => "uploads/-/system/user/avatar/#{model.id}/rails_sample.jpg", "uploader" => "AvatarUploader" - }) + }.merge(rails_sample_jpg_attrs)) end end context 'for a group avatar file path' do - let(:group) { create(:group) } - let(:unhashed_upload_file) { described_class.create!(path: group.avatar.file.file) } + let(:model) { create(:group) } + let(:unhashed_upload_file) { described_class.create!(path: model.avatar.file.file) } before do - group.update!(avatar: uploaded_file) - upload_class.delete_all + model.update!(avatar: uploaded_file) + model.uploads.delete_all end it 'creates an Upload record' do expect do unhashed_upload_file.add_to_uploads - end.to change { upload_class.count }.from(0).to(1) + end.to change { model.reload.uploads.count }.from(0).to(1) - expect(upload_class.first.attributes).to include({ - "size" => 35255, - "path" => "uploads/-/system/group/avatar/#{group.id}/rails_sample.jpg", - "checksum" => nil, - "model_id" => group.id, - "model_type" => "Group", + expect(model.uploads.first.attributes).to include({ + "path" => "uploads/-/system/group/avatar/#{model.id}/rails_sample.jpg", + "model_id" => model.id, + "model_type" => "Namespace", # Explicitly calling this out because it was unexpected to me (I assumed it should be "Group") "uploader" => "AvatarUploader" - }) + }.merge(rails_sample_jpg_attrs)) end end context 'for a project avatar file path' do - let(:project) { create(:project) } - let(:unhashed_upload_file) { described_class.create!(path: project.avatar.file.file) } + let(:model) { create(:project) } + let(:unhashed_upload_file) { described_class.create!(path: model.avatar.file.file) } before do - project.update!(avatar: uploaded_file) - upload_class.delete_all + model.update!(avatar: uploaded_file) + model.uploads.delete_all end it 'creates an Upload record' do expect do unhashed_upload_file.add_to_uploads - end.to change { upload_class.count }.from(0).to(1) + end.to change { model.reload.uploads.count }.from(0).to(1) - expect(upload_class.first.attributes).to include({ - "size" => 35255, - "path" => "uploads/-/system/project/avatar/#{project.id}/rails_sample.jpg", - "checksum" => nil, - "model_id" => project.id, - "model_type" => "Project", + expect(model.uploads.first.attributes).to include({ + "path" => "uploads/-/system/project/avatar/#{model.id}/rails_sample.jpg", "uploader" => "AvatarUploader" - }) + }.merge(rails_sample_jpg_attrs)) end end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - let(:project) { create(:project) } - let(:unhashed_upload_file) { described_class.new(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{project.uploads.first.path}") } + let(:model) { create(:project) } + let(:unhashed_upload_file) { described_class.new(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{model.full_path}/#{model.uploads.first.path}") } before do - UploadService.new(project, uploaded_file, FileUploader).execute # Markdown upload + UploadService.new(model, uploaded_file, FileUploader).execute # Markdown upload unhashed_upload_file.save! - upload_class.delete_all + model.reload.uploads.delete_all end it 'creates an Upload record' do expect do unhashed_upload_file.add_to_uploads - end.to change { upload_class.count }.from(0).to(1) + end.to change { model.reload.uploads.count }.from(0).to(1) - random_hex = unhashed_upload_file.path.match(/\/(\h+)\/rails_sample.jpg/)[1] - expect(upload_class.first.attributes).to include({ - "size" => 35255, - "path" => "#{random_hex}/rails_sample.jpg", - "checksum" => nil, - "model_id" => project.id, - "model_type" => "Project", + hex_secret = unhashed_upload_file.path.match(/\/(\h+)\/rails_sample.jpg/)[1] + expect(model.uploads.first.attributes).to include({ + "path" => "#{hex_secret}/rails_sample.jpg", "uploader" => "FileUploader" - }) + }.merge(rails_sample_jpg_attrs)) end end end @@ -441,8 +417,8 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi context 'for a group avatar file path' do let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } - it 'returns Group as a string' do - expect(unhashed_upload_file.model_type).to eq('Group') + it 'returns Namespace as a string' do + expect(unhashed_upload_file.model_type).to eq('Namespace') end end @@ -578,4 +554,11 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi end end end + + def rails_sample_jpg_attrs + { + "size" => 35255, + "checksum" => 'f2d1fd9d8d8a3368d468fa067888605d74a66f41c16f55979ceaf2af77375844' + } + end end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 8db41786397..9539993be31 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -49,6 +49,7 @@ describe TrackUntrackedUploads, :migration, :sidekiq do let(:project1) { create(:project) } let(:project2) { create(:project) } let(:appearance) { create(:appearance) } + let(:uploads) { table(:uploads) } before do fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') @@ -73,46 +74,52 @@ describe TrackUntrackedUploads, :migration, :sidekiq do appearance.uploads.last.destroy end - it 'tracks untracked migrations' do + it 'tracks untracked uploads' do Sidekiq::Testing.inline! do - migrate! + expect do + migrate! + end.to change { uploads.count }.from(4).to(8) - # Tracked uploads still exist - expect(user1.reload.uploads.first.attributes).to include({ - "path" => "uploads/-/system/user/avatar/#{user1.id}/rails_sample.jpg", - "uploader" => "AvatarUploader" - }) - expect(project1.reload.uploads.first.attributes).to include({ - "path" => "uploads/-/system/project/avatar/#{project1.id}/rails_sample.jpg", - "uploader" => "AvatarUploader" - }) - expect(appearance.reload.uploads.first.attributes).to include({ - "path" => "uploads/-/system/appearance/logo/#{appearance.id}/rails_sample.jpg", - "uploader" => "AttachmentUploader" - }) - expect(project1.uploads.last.attributes).to include({ - "path" => @project1_markdown_upload_path, - "uploader" => "FileUploader" - }) - - # Untracked uploads are now tracked expect(user2.reload.uploads.first.attributes).to include({ "path" => "uploads/-/system/user/avatar/#{user2.id}/rails_sample.jpg", "uploader" => "AvatarUploader" - }) + }.merge(rails_sample_jpg_attrs)) expect(project2.reload.uploads.first.attributes).to include({ "path" => "uploads/-/system/project/avatar/#{project2.id}/rails_sample.jpg", "uploader" => "AvatarUploader" - }) + }.merge(rails_sample_jpg_attrs)) expect(appearance.reload.uploads.count).to eq(2) expect(appearance.uploads.last.attributes).to include({ "path" => "uploads/-/system/appearance/header_logo/#{appearance.id}/rails_sample.jpg", "uploader" => "AttachmentUploader" - }) + }.merge(rails_sample_jpg_attrs)) expect(project2.uploads.last.attributes).to include({ "path" => @project2_markdown_upload_path, "uploader" => "FileUploader" - }) + }.merge(rails_sample_jpg_attrs)) + end + end + + it 'ignores already-tracked uploads' do + Sidekiq::Testing.inline! do + migrate! + + expect(user1.reload.uploads.first.attributes).to include({ + "path" => "uploads/-/system/user/avatar/#{user1.id}/rails_sample.jpg", + "uploader" => "AvatarUploader", + }.merge(rails_sample_jpg_attrs)) + expect(project1.reload.uploads.first.attributes).to include({ + "path" => "uploads/-/system/project/avatar/#{project1.id}/rails_sample.jpg", + "uploader" => "AvatarUploader" + }.merge(rails_sample_jpg_attrs)) + expect(appearance.reload.uploads.first.attributes).to include({ + "path" => "uploads/-/system/appearance/logo/#{appearance.id}/rails_sample.jpg", + "uploader" => "AttachmentUploader" + }.merge(rails_sample_jpg_attrs)) + expect(project1.uploads.last.attributes).to include({ + "path" => @project1_markdown_upload_path, + "uploader" => "FileUploader" + }.merge(rails_sample_jpg_attrs)) end end @@ -125,4 +132,11 @@ describe TrackUntrackedUploads, :migration, :sidekiq do end end end + + def rails_sample_jpg_attrs + { + "size" => 35255, + "checksum" => 'f2d1fd9d8d8a3368d468fa067888605d74a66f41c16f55979ceaf2af77375844' + } + end end -- cgit v1.2.1 From 13e0ee373573c41d4c1d095fbeec2a133ae626bb Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 7 Nov 2017 20:38:17 -0800 Subject: Test batch processing --- .../populate_untracked_uploads_spec.rb | 42 ++++++++++++++++++++++ 1 file changed, 42 insertions(+) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index c61a207d012..5446edae06f 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -53,6 +53,12 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid expect(appearance.uploads.count).to eq(2) end + it 'sets all added or confirmed tracked files to tracked' do + expect do + described_class.new.perform(1, 1000) + end.to change { unhashed_upload_files.where(tracked: true).count }.from(0).to(8) + end + it 'does not create duplicate uploads of already tracked files' do described_class.new.perform(1, 1000) @@ -60,6 +66,42 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid expect(project1.uploads.count).to eq(2) expect(appearance.uploads.count).to eq(2) end + + it 'uses the start and end batch ids [only 1st half]' do + start_id = unhashed_upload_files.all.to_a[0].id + end_id = unhashed_upload_files.all.to_a[3].id + + expect do + described_class.new.perform(start_id, end_id) + end.to change { uploads.count }.from(4).to(6) + + expect(user1.uploads.count).to eq(1) + expect(user2.uploads.count).to eq(1) + expect(appearance.uploads.count).to eq(2) + expect(project1.uploads.count).to eq(2) + expect(project2.uploads.count).to eq(0) + + # Only 4 have been either confirmed or added to uploads + expect(unhashed_upload_files.where(tracked: true).count).to eq(4) + end + + it 'uses the start and end batch ids [only 2nd half]' do + start_id = unhashed_upload_files.all.to_a[4].id + end_id = unhashed_upload_files.all.to_a[7].id + + expect do + described_class.new.perform(start_id, end_id) + end.to change { uploads.count }.from(4).to(6) + + expect(user1.uploads.count).to eq(1) + expect(user2.uploads.count).to eq(0) + expect(appearance.uploads.count).to eq(1) + expect(project1.uploads.count).to eq(2) + expect(project2.uploads.count).to eq(2) + + # Only 4 have been either confirmed or added to uploads + expect(unhashed_upload_files.where(tracked: true).count).to eq(4) + end end context 'with no untracked files' do -- cgit v1.2.1 From ffbaf19fe8a83f651f45380749ccc12cd38ec29f Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 7 Nov 2017 20:54:54 -0800 Subject: Fix Rubocop offenses --- .../gitlab/background_migration/populate_untracked_uploads_spec.rb | 2 +- spec/migrations/track_untracked_uploads_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 5446edae06f..82d5a588a56 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -129,7 +129,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi it 'does not add an upload' do expect do unhashed_upload_file.ensure_tracked! - end.to_not change { upload_class.count }.from(1) + end.not_to change { upload_class.count }.from(1) end end end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 9539993be31..d896a3e7d54 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -37,7 +37,7 @@ describe TrackUntrackedUploads, :migration, :sidekiq do migrate! max_length_namespace_path = max_length_project_path = max_length_filename = 'a' * 255 - long_path = "./uploads#{("/#{max_length_namespace_path}") * Namespace::NUMBER_OF_ANCESTORS_ALLOWED}/#{max_length_project_path}/#{max_length_filename}" + long_path = "./uploads#{"/#{max_length_namespace_path}" * Namespace::NUMBER_OF_ANCESTORS_ALLOWED}/#{max_length_project_path}/#{max_length_filename}" unhashed_upload_file = UnhashedUploadFile.new(path: long_path) unhashed_upload_file.save! expect(UnhashedUploadFile.first.path.size).to eq(5641) @@ -106,7 +106,7 @@ describe TrackUntrackedUploads, :migration, :sidekiq do expect(user1.reload.uploads.first.attributes).to include({ "path" => "uploads/-/system/user/avatar/#{user1.id}/rails_sample.jpg", - "uploader" => "AvatarUploader", + "uploader" => "AvatarUploader" }.merge(rails_sample_jpg_attrs)) expect(project1.reload.uploads.first.attributes).to include({ "path" => "uploads/-/system/project/avatar/#{project1.id}/rails_sample.jpg", -- cgit v1.2.1 From 7c43692f68dd62773b0a7b094453f582a4242ef7 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 8 Nov 2017 12:44:49 -0800 Subject: Make regexes more readable --- spec/migrations/track_untracked_uploads_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'spec') diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index d896a3e7d54..308d8924f19 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -59,7 +59,7 @@ describe TrackUntrackedUploads, :migration, :sidekiq do user1.update(avatar: uploaded_file) project1.update(avatar: uploaded_file) upload_result = UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload - @project1_markdown_upload_path = upload_result[:url].sub(/\A\/uploads\//, '') + @project1_markdown_upload_path = upload_result[:url].sub(%r{\A/uploads/}, '') appearance.update(logo: uploaded_file) # Untracked, by doing normal file upload then deleting records from DB @@ -68,7 +68,7 @@ describe TrackUntrackedUploads, :migration, :sidekiq do user2.uploads.delete_all project2.update(avatar: uploaded_file) upload_result = UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload - @project2_markdown_upload_path = upload_result[:url].sub(/\A\/uploads\//, '') + @project2_markdown_upload_path = upload_result[:url].sub(%r{\A/uploads/}, '') project2.uploads.delete_all appearance.update(header_logo: uploaded_file) appearance.uploads.last.destroy -- cgit v1.2.1 From 2ab3031bd35213802e508fef6eebceaaf40cee9b Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 8 Nov 2017 15:05:08 -0800 Subject: Refactor, no change in behavior --- .../populate_untracked_uploads_spec.rb | 9 ++------ spec/migrations/track_untracked_uploads_spec.rb | 26 ++++++++++++---------- spec/support/track_untracked_uploads_helpers.rb | 12 ++++++++++ 3 files changed, 28 insertions(+), 19 deletions(-) create mode 100644 spec/support/track_untracked_uploads_helpers.rb (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 82d5a588a56..28a9ee73470 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -114,6 +114,8 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFile do + include TrackUntrackedUploadsHelpers + let(:upload_class) { Gitlab::BackgroundMigration::PopulateUntrackedUploads::Upload } describe '#ensure_tracked!' do @@ -596,11 +598,4 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi end end end - - def rails_sample_jpg_attrs - { - "size" => 35255, - "checksum" => 'f2d1fd9d8d8a3368d468fa067888605d74a66f41c16f55979ceaf2af77375844' - } - end end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 308d8924f19..49758ede1a4 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -2,6 +2,8 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads') describe TrackUntrackedUploads, :migration, :sidekiq do + include TrackUntrackedUploadsHelpers + class UnhashedUploadFile < ActiveRecord::Base self.table_name = 'unhashed_upload_files' end @@ -36,11 +38,18 @@ describe TrackUntrackedUploads, :migration, :sidekiq do it 'has a path field long enough for really long paths' do migrate! - max_length_namespace_path = max_length_project_path = max_length_filename = 'a' * 255 - long_path = "./uploads#{"/#{max_length_namespace_path}" * Namespace::NUMBER_OF_ANCESTORS_ALLOWED}/#{max_length_project_path}/#{max_length_filename}" - unhashed_upload_file = UnhashedUploadFile.new(path: long_path) - unhashed_upload_file.save! - expect(UnhashedUploadFile.first.path.size).to eq(5641) + component = 'a'*255 + + long_path = [ + CarrierWave.root, + 'uploads', + [component] * Namespace::NUMBER_OF_ANCESTORS_ALLOWED, # namespaces + component, # project + component # filename + ].flatten.join('/') + + record = UnhashedUploadFile.create!(path: long_path) + expect(record.reload.path.size).to eq(5711) end context 'with tracked and untracked uploads' do @@ -132,11 +141,4 @@ describe TrackUntrackedUploads, :migration, :sidekiq do end end end - - def rails_sample_jpg_attrs - { - "size" => 35255, - "checksum" => 'f2d1fd9d8d8a3368d468fa067888605d74a66f41c16f55979ceaf2af77375844' - } - end end diff --git a/spec/support/track_untracked_uploads_helpers.rb b/spec/support/track_untracked_uploads_helpers.rb new file mode 100644 index 00000000000..749c5775bb0 --- /dev/null +++ b/spec/support/track_untracked_uploads_helpers.rb @@ -0,0 +1,12 @@ +module TrackUntrackedUploadsHelpers + def rails_sample_jpg_attrs + @rails_sample_jpg_attrs ||= { + "size" => File.size(rails_sample_file_path), + "checksum" => Digest::SHA256.file(rails_sample_file_path).hexdigest + } + end + + def rails_sample_file_path + Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + end +end -- cgit v1.2.1 From a210cb6b827d9d918788578fc4ae956471de3b12 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 9 Nov 2017 17:17:56 -0800 Subject: Rename table to untracked_files_for_uploads --- .../populate_untracked_uploads_spec.rb | 212 ++++++++++----------- .../prepare_unhashed_uploads_spec.rb | 111 ----------- .../prepare_untracked_uploads_spec.rb | 111 +++++++++++ spec/migrations/track_untracked_uploads_spec.rb | 16 +- 4 files changed, 225 insertions(+), 225 deletions(-) delete mode 100644 spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb create mode 100644 spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 28a9ee73470..b3122e90c83 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sidekiq, schema: 20171103140253 do - let!(:unhashed_upload_files) { table(:unhashed_upload_files) } + let!(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } let!(:uploads) { table(:uploads) } let(:user1) { create(:user) } @@ -10,7 +10,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid let(:project2) { create(:project) } let(:appearance) { create(:appearance) } - context 'with untracked files and tracked files in unhashed_upload_files' do + context 'with untracked files and tracked files in untracked_files_for_uploads' do before do fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') @@ -28,15 +28,15 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload appearance.update!(header_logo: uploaded_file) - # Unhashed upload files created by PrepareUnhashedUploads - unhashed_upload_files.create!(path: appearance.logo.file.file) - unhashed_upload_files.create!(path: appearance.header_logo.file.file) - unhashed_upload_files.create!(path: user1.avatar.file.file) - unhashed_upload_files.create!(path: user2.avatar.file.file) - unhashed_upload_files.create!(path: project1.avatar.file.file) - unhashed_upload_files.create!(path: project2.avatar.file.file) - unhashed_upload_files.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project1.full_path}/#{project1.uploads.last.path}") - unhashed_upload_files.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project2.full_path}/#{project2.uploads.last.path}") + # File records created by PrepareUntrackedUploads + untracked_files_for_uploads.create!(path: appearance.logo.file.file) + untracked_files_for_uploads.create!(path: appearance.header_logo.file.file) + untracked_files_for_uploads.create!(path: user1.avatar.file.file) + untracked_files_for_uploads.create!(path: user2.avatar.file.file) + untracked_files_for_uploads.create!(path: project1.avatar.file.file) + untracked_files_for_uploads.create!(path: project2.avatar.file.file) + untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project1.full_path}/#{project1.uploads.last.path}") + untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project2.full_path}/#{project2.uploads.last.path}") user2.uploads.delete_all project2.uploads.delete_all @@ -56,7 +56,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid it 'sets all added or confirmed tracked files to tracked' do expect do described_class.new.perform(1, 1000) - end.to change { unhashed_upload_files.where(tracked: true).count }.from(0).to(8) + end.to change { untracked_files_for_uploads.where(tracked: true).count }.from(0).to(8) end it 'does not create duplicate uploads of already tracked files' do @@ -68,8 +68,8 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end it 'uses the start and end batch ids [only 1st half]' do - start_id = unhashed_upload_files.all.to_a[0].id - end_id = unhashed_upload_files.all.to_a[3].id + start_id = untracked_files_for_uploads.all.to_a[0].id + end_id = untracked_files_for_uploads.all.to_a[3].id expect do described_class.new.perform(start_id, end_id) @@ -82,12 +82,12 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid expect(project2.uploads.count).to eq(0) # Only 4 have been either confirmed or added to uploads - expect(unhashed_upload_files.where(tracked: true).count).to eq(4) + expect(untracked_files_for_uploads.where(tracked: true).count).to eq(4) end it 'uses the start and end batch ids [only 2nd half]' do - start_id = unhashed_upload_files.all.to_a[4].id - end_id = unhashed_upload_files.all.to_a[7].id + start_id = untracked_files_for_uploads.all.to_a[4].id + end_id = untracked_files_for_uploads.all.to_a[7].id expect do described_class.new.perform(start_id, end_id) @@ -100,7 +100,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid expect(project2.uploads.count).to eq(2) # Only 4 have been either confirmed or added to uploads - expect(unhashed_upload_files.where(tracked: true).count).to eq(4) + expect(untracked_files_for_uploads.where(tracked: true).count).to eq(4) end end @@ -113,7 +113,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end end -describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFile do +describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do include TrackUntrackedUploadsHelpers let(:upload_class) { Gitlab::BackgroundMigration::PopulateUntrackedUploads::Upload } @@ -122,7 +122,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi let(:user1) { create(:user) } context 'when the file is already in the uploads table' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/user/avatar/#{user1.id}/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/user/avatar/#{user1.id}/avatar.jpg") } before do upload_class.create!(path: "uploads/-/system/user/avatar/#{user1.id}/avatar.jpg", uploader: 'AvatarUploader', model_type: 'User', model_id: user1.id, size: 1234) @@ -130,7 +130,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi it 'does not add an upload' do expect do - unhashed_upload_file.ensure_tracked! + untracked_file.ensure_tracked! end.not_to change { upload_class.count }.from(1) end end @@ -142,7 +142,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi context 'for an appearance logo file path' do let(:model) { create(:appearance) } - let(:unhashed_upload_file) { described_class.create!(path: model.logo.file.file) } + let(:untracked_file) { described_class.create!(path: model.logo.file.file) } before do model.update!(logo: uploaded_file) @@ -151,7 +151,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi it 'creates an Upload record' do expect do - unhashed_upload_file.add_to_uploads + untracked_file.add_to_uploads end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include({ @@ -163,7 +163,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi context 'for an appearance header_logo file path' do let(:model) { create(:appearance) } - let(:unhashed_upload_file) { described_class.create!(path: model.header_logo.file.file) } + let(:untracked_file) { described_class.create!(path: model.header_logo.file.file) } before do model.update!(header_logo: uploaded_file) @@ -172,7 +172,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi it 'creates an Upload record' do expect do - unhashed_upload_file.add_to_uploads + untracked_file.add_to_uploads end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include({ @@ -184,7 +184,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi context 'for a pre-Markdown Note attachment file path' do let(:model) { create(:note) } - let(:unhashed_upload_file) { described_class.create!(path: model.attachment.file.file) } + let(:untracked_file) { described_class.create!(path: model.attachment.file.file) } before do model.update!(attachment: uploaded_file) @@ -193,7 +193,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi it 'creates an Upload record' do expect do - unhashed_upload_file.add_to_uploads + untracked_file.add_to_uploads end.to change { upload_class.count }.from(0).to(1) expect(upload_class.first.attributes).to include({ @@ -207,7 +207,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi context 'for a user avatar file path' do let(:model) { create(:user) } - let(:unhashed_upload_file) { described_class.create!(path: model.avatar.file.file) } + let(:untracked_file) { described_class.create!(path: model.avatar.file.file) } before do model.update!(avatar: uploaded_file) @@ -216,7 +216,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi it 'creates an Upload record' do expect do - unhashed_upload_file.add_to_uploads + untracked_file.add_to_uploads end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include({ @@ -228,7 +228,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi context 'for a group avatar file path' do let(:model) { create(:group) } - let(:unhashed_upload_file) { described_class.create!(path: model.avatar.file.file) } + let(:untracked_file) { described_class.create!(path: model.avatar.file.file) } before do model.update!(avatar: uploaded_file) @@ -237,7 +237,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi it 'creates an Upload record' do expect do - unhashed_upload_file.add_to_uploads + untracked_file.add_to_uploads end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include({ @@ -251,7 +251,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi context 'for a project avatar file path' do let(:model) { create(:project) } - let(:unhashed_upload_file) { described_class.create!(path: model.avatar.file.file) } + let(:untracked_file) { described_class.create!(path: model.avatar.file.file) } before do model.update!(avatar: uploaded_file) @@ -260,7 +260,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi it 'creates an Upload record' do expect do - unhashed_upload_file.add_to_uploads + untracked_file.add_to_uploads end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include({ @@ -272,20 +272,20 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:model) { create(:project) } - let(:unhashed_upload_file) { described_class.new(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{model.full_path}/#{model.uploads.first.path}") } + let(:untracked_file) { described_class.new(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{model.full_path}/#{model.uploads.first.path}") } before do UploadService.new(model, uploaded_file, FileUploader).execute # Markdown upload - unhashed_upload_file.save! + untracked_file.save! model.reload.uploads.delete_all end it 'creates an Upload record' do expect do - unhashed_upload_file.add_to_uploads + untracked_file.add_to_uploads end.to change { model.reload.uploads.count }.from(0).to(1) - hex_secret = unhashed_upload_file.path.match(/\/(\h+)\/rails_sample.jpg/)[1] + hex_secret = untracked_file.path.match(/\/(\h+)\/rails_sample.jpg/)[1] expect(model.uploads.first.attributes).to include({ "path" => "#{hex_secret}/rails_sample.jpg", "uploader" => "FileUploader" @@ -295,250 +295,250 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi end describe '#mark_as_tracked' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } it 'saves the record with tracked set to true' do expect do - unhashed_upload_file.mark_as_tracked - end.to change { unhashed_upload_file.tracked }.from(false).to(true) + untracked_file.mark_as_tracked + end.to change { untracked_file.tracked }.from(false).to(true) - expect(unhashed_upload_file.persisted?).to be_truthy + expect(untracked_file.persisted?).to be_truthy end end describe '#upload_path' do context 'for an appearance logo file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } it 'returns the file path relative to the CarrierWave root' do - expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/appearance/logo/1/some_logo.jpg') + expect(untracked_file.upload_path).to eq('uploads/-/system/appearance/logo/1/some_logo.jpg') end end context 'for an appearance header_logo file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } it 'returns the file path relative to the CarrierWave root' do - expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/appearance/header_logo/1/some_logo.jpg') + expect(untracked_file.upload_path).to eq('uploads/-/system/appearance/header_logo/1/some_logo.jpg') end end context 'for a pre-Markdown Note attachment file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } it 'returns the file path relative to the CarrierWave root' do - expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/note/attachment/1234/some_attachment.pdf') + expect(untracked_file.upload_path).to eq('uploads/-/system/note/attachment/1234/some_attachment.pdf') end end context 'for a user avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } it 'returns the file path relative to the CarrierWave root' do - expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/user/avatar/1234/avatar.jpg') + expect(untracked_file.upload_path).to eq('uploads/-/system/user/avatar/1234/avatar.jpg') end end context 'for a group avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } it 'returns the file path relative to the CarrierWave root' do - expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/group/avatar/1234/avatar.jpg') + expect(untracked_file.upload_path).to eq('uploads/-/system/group/avatar/1234/avatar.jpg') end end context 'for a project avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } it 'returns the file path relative to the CarrierWave root' do - expect(unhashed_upload_file.upload_path).to eq('uploads/-/system/project/avatar/1234/avatar.jpg') + expect(untracked_file.upload_path).to eq('uploads/-/system/project/avatar/1234/avatar.jpg') end end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } let(:random_hex) { SecureRandom.hex } - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{random_hex}/Some file.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project.full_path}/#{random_hex}/Some file.jpg") } it 'returns the file path relative to the project directory in uploads' do - expect(unhashed_upload_file.upload_path).to eq("#{random_hex}/Some file.jpg") + expect(untracked_file.upload_path).to eq("#{random_hex}/Some file.jpg") end end end describe '#uploader' do context 'for an appearance logo file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } it 'returns AttachmentUploader as a string' do - expect(unhashed_upload_file.uploader).to eq('AttachmentUploader') + expect(untracked_file.uploader).to eq('AttachmentUploader') end end context 'for an appearance header_logo file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } it 'returns AttachmentUploader as a string' do - expect(unhashed_upload_file.uploader).to eq('AttachmentUploader') + expect(untracked_file.uploader).to eq('AttachmentUploader') end end context 'for a pre-Markdown Note attachment file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } it 'returns AttachmentUploader as a string' do - expect(unhashed_upload_file.uploader).to eq('AttachmentUploader') + expect(untracked_file.uploader).to eq('AttachmentUploader') end end context 'for a user avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } it 'returns AvatarUploader as a string' do - expect(unhashed_upload_file.uploader).to eq('AvatarUploader') + expect(untracked_file.uploader).to eq('AvatarUploader') end end context 'for a group avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } it 'returns AvatarUploader as a string' do - expect(unhashed_upload_file.uploader).to eq('AvatarUploader') + expect(untracked_file.uploader).to eq('AvatarUploader') end end context 'for a project avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } it 'returns AvatarUploader as a string' do - expect(unhashed_upload_file.uploader).to eq('AvatarUploader') + expect(untracked_file.uploader).to eq('AvatarUploader') end end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } it 'returns FileUploader as a string' do - expect(unhashed_upload_file.uploader).to eq('FileUploader') + expect(untracked_file.uploader).to eq('FileUploader') end end end describe '#model_type' do context 'for an appearance logo file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } it 'returns Appearance as a string' do - expect(unhashed_upload_file.model_type).to eq('Appearance') + expect(untracked_file.model_type).to eq('Appearance') end end context 'for an appearance header_logo file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } it 'returns Appearance as a string' do - expect(unhashed_upload_file.model_type).to eq('Appearance') + expect(untracked_file.model_type).to eq('Appearance') end end context 'for a pre-Markdown Note attachment file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } it 'returns Note as a string' do - expect(unhashed_upload_file.model_type).to eq('Note') + expect(untracked_file.model_type).to eq('Note') end end context 'for a user avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } it 'returns User as a string' do - expect(unhashed_upload_file.model_type).to eq('User') + expect(untracked_file.model_type).to eq('User') end end context 'for a group avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } it 'returns Namespace as a string' do - expect(unhashed_upload_file.model_type).to eq('Namespace') + expect(untracked_file.model_type).to eq('Namespace') end end context 'for a project avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } it 'returns Project as a string' do - expect(unhashed_upload_file.model_type).to eq('Project') + expect(untracked_file.model_type).to eq('Project') end end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } it 'returns Project as a string' do - expect(unhashed_upload_file.model_type).to eq('Project') + expect(untracked_file.model_type).to eq('Project') end end end describe '#model_id' do context 'for an appearance logo file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } it 'returns the ID as a string' do - expect(unhashed_upload_file.model_id).to eq('1') + expect(untracked_file.model_id).to eq('1') end end context 'for an appearance header_logo file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } it 'returns the ID as a string' do - expect(unhashed_upload_file.model_id).to eq('1') + expect(untracked_file.model_id).to eq('1') end end context 'for a pre-Markdown Note attachment file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } it 'returns the ID as a string' do - expect(unhashed_upload_file.model_id).to eq('1234') + expect(untracked_file.model_id).to eq('1234') end end context 'for a user avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } it 'returns the ID as a string' do - expect(unhashed_upload_file.model_id).to eq('1234') + expect(untracked_file.model_id).to eq('1234') end end context 'for a group avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } it 'returns the ID as a string' do - expect(unhashed_upload_file.model_id).to eq('1234') + expect(untracked_file.model_id).to eq('1234') end end context 'for a project avatar file path' do - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } it 'returns the ID as a string' do - expect(unhashed_upload_file.model_id).to eq('1234') + expect(untracked_file.model_id).to eq('1234') end end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } it 'returns the ID as a string' do - expect(unhashed_upload_file.model_id).to eq(project.id.to_s) + expect(untracked_file.model_id).to eq(project.id.to_s) end end end @@ -549,52 +549,52 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi context 'for an appearance logo file path' do let(:appearance) { create(:appearance) } - let(:unhashed_upload_file) { described_class.create!(path: appearance.logo.file.file) } + let(:untracked_file) { described_class.create!(path: appearance.logo.file.file) } before do appearance.update!(logo: uploaded_file) end it 'returns the file size' do - expect(unhashed_upload_file.file_size).to eq(35255) + expect(untracked_file.file_size).to eq(35255) end it 'returns the same thing that CarrierWave would return' do - expect(unhashed_upload_file.file_size).to eq(appearance.logo.size) + expect(untracked_file.file_size).to eq(appearance.logo.size) end end context 'for a project avatar file path' do let(:project) { create(:project) } - let(:unhashed_upload_file) { described_class.create!(path: project.avatar.file.file) } + let(:untracked_file) { described_class.create!(path: project.avatar.file.file) } before do project.update!(avatar: uploaded_file) end it 'returns the file size' do - expect(unhashed_upload_file.file_size).to eq(35255) + expect(untracked_file.file_size).to eq(35255) end it 'returns the same thing that CarrierWave would return' do - expect(unhashed_upload_file.file_size).to eq(project.avatar.size) + expect(untracked_file.file_size).to eq(project.avatar.size) end end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } - let(:unhashed_upload_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/#{project.full_path}/#{project.uploads.first.path}") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project.full_path}/#{project.uploads.first.path}") } before do UploadService.new(project, uploaded_file, FileUploader).execute end it 'returns the file size' do - expect(unhashed_upload_file.file_size).to eq(35255) + expect(untracked_file.file_size).to eq(35255) end it 'returns the same thing that CarrierWave would return' do - expect(unhashed_upload_file.file_size).to eq(project.uploads.first.size) + expect(untracked_file.file_size).to eq(project.uploads.first.size) end end end diff --git a/spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb deleted file mode 100644 index 76d126e8f00..00000000000 --- a/spec/lib/gitlab/background_migration/prepare_unhashed_uploads_spec.rb +++ /dev/null @@ -1,111 +0,0 @@ -require 'spec_helper' - -describe Gitlab::BackgroundMigration::PrepareUnhashedUploads, :migration, :sidekiq, schema: 20171103140253 do - let!(:unhashed_upload_files) { table(:unhashed_upload_files) } - - let(:user1) { create(:user) } - let(:user2) { create(:user) } - let(:project1) { create(:project) } - let(:project2) { create(:project) } - let(:appearance) { create(:appearance) } - - matcher :be_scheduled_migration do |*expected| - match do |migration| - BackgroundMigrationWorker.jobs.any? do |job| - job['args'] == [migration, expected] - end - end - - failure_message do |migration| - "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!" - end - end - - context 'when files were uploaded before and after hashed storage was enabled' do - before do - fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') - uploaded_file = fixture_file_upload(fixture) - - user1.update(avatar: uploaded_file) - project1.update(avatar: uploaded_file) - appearance.update(logo: uploaded_file, header_logo: uploaded_file) - uploaded_file = fixture_file_upload(fixture) - UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload - - stub_application_setting(hashed_storage_enabled: true) - - # Hashed files - uploaded_file = fixture_file_upload(fixture) - UploadService.new(project2, uploaded_file, FileUploader).execute - end - - it 'adds unhashed files to the unhashed_upload_files table' do - Sidekiq::Testing.fake! do - expect do - described_class.new.perform - end.to change { unhashed_upload_files.count }.from(0).to(5) - end - end - - it 'does not add hashed files to the unhashed_upload_files table' do - Sidekiq::Testing.fake! do - described_class.new.perform - - hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path - expect(unhashed_upload_files.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey - end - end - - it 'correctly schedules the follow-up background migration jobs' do - Sidekiq::Testing.fake! do - described_class.new.perform - - expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5) - expect(BackgroundMigrationWorker.jobs.size).to eq(1) - end - end - - # E.g. from a previous failed run of this background migration - context 'when there is existing data in unhashed_upload_files' do - before do - unhashed_upload_files.create(path: '/foo/bar.jpg') - end - - it 'clears existing data before adding new data' do - Sidekiq::Testing.fake! do - expect do - described_class.new.perform - end.to change { unhashed_upload_files.count }.from(1).to(5) - end - end - end - - # E.g. The installation is in use at the time of migration, and someone has - # just uploaded a file - context 'when there are files in /uploads/tmp' do - before do - FileUtils.touch(Rails.root.join(described_class::UPLOAD_DIR, 'tmp', 'some_file.jpg')) - end - - it 'does not add files from /uploads/tmp' do - Sidekiq::Testing.fake! do - expect do - described_class.new.perform - end.to change { unhashed_upload_files.count }.from(0).to(5) - end - end - end - end - - # Very new or lightly-used installations that are running this migration - # may not have an upload directory because they have no uploads. - context 'when no files were ever uploaded' do - it 'does not add to the unhashed_upload_files table (and does not raise error)' do - Sidekiq::Testing.fake! do - expect do - described_class.new.perform - end.not_to change { unhashed_upload_files.count }.from(0) - end - end - end -end diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb new file mode 100644 index 00000000000..087fa35dd15 --- /dev/null +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -0,0 +1,111 @@ +require 'spec_helper' + +describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :sidekiq, schema: 20171103140253 do + let!(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } + + let(:user1) { create(:user) } + let(:user2) { create(:user) } + let(:project1) { create(:project) } + let(:project2) { create(:project) } + let(:appearance) { create(:appearance) } + + matcher :be_scheduled_migration do |*expected| + match do |migration| + BackgroundMigrationWorker.jobs.any? do |job| + job['args'] == [migration, expected] + end + end + + failure_message do |migration| + "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!" + end + end + + context 'when files were uploaded before and after hashed storage was enabled' do + before do + fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + uploaded_file = fixture_file_upload(fixture) + + user1.update(avatar: uploaded_file) + project1.update(avatar: uploaded_file) + appearance.update(logo: uploaded_file, header_logo: uploaded_file) + uploaded_file = fixture_file_upload(fixture) + UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload + + stub_application_setting(hashed_storage_enabled: true) + + # Hashed files + uploaded_file = fixture_file_upload(fixture) + UploadService.new(project2, uploaded_file, FileUploader).execute + end + + it 'adds unhashed files to the untracked_files_for_uploads table' do + Sidekiq::Testing.fake! do + expect do + described_class.new.perform + end.to change { untracked_files_for_uploads.count }.from(0).to(5) + end + end + + it 'does not add hashed files to the untracked_files_for_uploads table' do + Sidekiq::Testing.fake! do + described_class.new.perform + + hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path + expect(untracked_files_for_uploads.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey + end + end + + it 'correctly schedules the follow-up background migration jobs' do + Sidekiq::Testing.fake! do + described_class.new.perform + + expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5) + expect(BackgroundMigrationWorker.jobs.size).to eq(1) + end + end + + # E.g. from a previous failed run of this background migration + context 'when there is existing data in untracked_files_for_uploads' do + before do + untracked_files_for_uploads.create(path: '/foo/bar.jpg') + end + + it 'clears existing data before adding new data' do + Sidekiq::Testing.fake! do + expect do + described_class.new.perform + end.to change { untracked_files_for_uploads.count }.from(1).to(5) + end + end + end + + # E.g. The installation is in use at the time of migration, and someone has + # just uploaded a file + context 'when there are files in /uploads/tmp' do + before do + FileUtils.touch(Rails.root.join(described_class::UPLOAD_DIR, 'tmp', 'some_file.jpg')) + end + + it 'does not add files from /uploads/tmp' do + Sidekiq::Testing.fake! do + expect do + described_class.new.perform + end.to change { untracked_files_for_uploads.count }.from(0).to(5) + end + end + end + end + + # Very new or lightly-used installations that are running this migration + # may not have an upload directory because they have no uploads. + context 'when no files were ever uploaded' do + it 'does not add to the untracked_files_for_uploads table (and does not raise error)' do + Sidekiq::Testing.fake! do + expect do + described_class.new.perform + end.not_to change { untracked_files_for_uploads.count }.from(0) + end + end + end +end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 49758ede1a4..95496696bc6 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -4,8 +4,8 @@ require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_up describe TrackUntrackedUploads, :migration, :sidekiq do include TrackUntrackedUploadsHelpers - class UnhashedUploadFile < ActiveRecord::Base - self.table_name = 'unhashed_upload_files' + class UntrackedFile < ActiveRecord::Base + self.table_name = 'untracked_files_for_uploads' end matcher :be_scheduled_migration do @@ -29,10 +29,10 @@ describe TrackUntrackedUploads, :migration, :sidekiq do end end - it 'ensures the unhashed_upload_files table exists' do + it 'ensures the untracked_files_for_uploads table exists' do expect do migrate! - end.to change { table_exists?(:unhashed_upload_files) }.from(false).to(true) + end.to change { table_exists?(:untracked_files_for_uploads) }.from(false).to(true) end it 'has a path field long enough for really long paths' do @@ -48,7 +48,7 @@ describe TrackUntrackedUploads, :migration, :sidekiq do component # filename ].flatten.join('/') - record = UnhashedUploadFile.create!(path: long_path) + record = UntrackedFile.create!(path: long_path) expect(record.reload.path.size).to eq(5711) end @@ -132,12 +132,12 @@ describe TrackUntrackedUploads, :migration, :sidekiq do end end - it 'all UnhashedUploadFile records are marked as tracked' do + it 'all UntrackedFile records are marked as tracked' do Sidekiq::Testing.inline! do migrate! - expect(UnhashedUploadFile.count).to eq(8) - expect(UnhashedUploadFile.count).to eq(UnhashedUploadFile.where(tracked: true).count) + expect(UntrackedFile.count).to eq(8) + expect(UntrackedFile.count).to eq(UntrackedFile.where(tracked: true).count) end end end -- cgit v1.2.1 From 1807bc16466a37684c5e1de6b498230dfbd3be06 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 9 Nov 2017 17:20:00 -0800 Subject: Reword test --- spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index 087fa35dd15..3d2504f84a1 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -71,7 +71,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side untracked_files_for_uploads.create(path: '/foo/bar.jpg') end - it 'clears existing data before adding new data' do + it 'does not error or produce duplicates of existing data' do Sidekiq::Testing.fake! do expect do described_class.new.perform -- cgit v1.2.1 From c77a353dca25c2e08c409f1f3fc5a61a3eea69dc Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 9 Nov 2017 17:21:32 -0800 Subject: Remove unnecessary clearing Since duplicate inserts are now ignored. --- .../lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index 3d2504f84a1..cf1cad15c9a 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -68,14 +68,14 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side # E.g. from a previous failed run of this background migration context 'when there is existing data in untracked_files_for_uploads' do before do - untracked_files_for_uploads.create(path: '/foo/bar.jpg') + described_class.new.perform end it 'does not error or produce duplicates of existing data' do Sidekiq::Testing.fake! do expect do described_class.new.perform - end.to change { untracked_files_for_uploads.count }.from(1).to(5) + end.not_to change { untracked_files_for_uploads.count }.from(5) end end end -- cgit v1.2.1 From 1ba4d417fe7bec333c410cff861d43f0bd1e1c03 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 9 Nov 2017 17:26:15 -0800 Subject: Clean up after test --- .../gitlab/background_migration/prepare_untracked_uploads_spec.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index cf1cad15c9a..913622cdb95 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -83,8 +83,14 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side # E.g. The installation is in use at the time of migration, and someone has # just uploaded a file context 'when there are files in /uploads/tmp' do + let(:tmp_file) { Rails.root.join(described_class::UPLOAD_DIR, 'tmp', 'some_file.jpg') } + before do - FileUtils.touch(Rails.root.join(described_class::UPLOAD_DIR, 'tmp', 'some_file.jpg')) + FileUtils.touch(tmp_file) + end + + after do + FileUtils.rm(tmp_file) end it 'does not add files from /uploads/tmp' do -- cgit v1.2.1 From 3dc0b118eca3716c0cda841232e0789739627957 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 14 Nov 2017 16:11:53 -0800 Subject: Store paths relative to CarrierWave.root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So the path on source installs cannot be too long for our column. And fix the column length test since Route.path is limited to 255 chars, it doesn’t matter how many nested groups there are. --- .../populate_untracked_uploads_spec.rb | 102 +++++++++++---------- .../prepare_untracked_uploads_spec.rb | 11 ++- spec/migrations/track_untracked_uploads_spec.rb | 6 +- 3 files changed, 66 insertions(+), 53 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index b3122e90c83..953793a353c 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -29,14 +29,14 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid appearance.update!(header_logo: uploaded_file) # File records created by PrepareUntrackedUploads - untracked_files_for_uploads.create!(path: appearance.logo.file.file) - untracked_files_for_uploads.create!(path: appearance.header_logo.file.file) - untracked_files_for_uploads.create!(path: user1.avatar.file.file) - untracked_files_for_uploads.create!(path: user2.avatar.file.file) - untracked_files_for_uploads.create!(path: project1.avatar.file.file) - untracked_files_for_uploads.create!(path: project2.avatar.file.file) - untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project1.full_path}/#{project1.uploads.last.path}") - untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project2.full_path}/#{project2.uploads.last.path}") + untracked_files_for_uploads.create!(path: found_path(appearance.logo.file.file)) + untracked_files_for_uploads.create!(path: found_path(appearance.header_logo.file.file)) + untracked_files_for_uploads.create!(path: found_path(user1.avatar.file.file)) + untracked_files_for_uploads.create!(path: found_path(user2.avatar.file.file)) + untracked_files_for_uploads.create!(path: found_path(project1.avatar.file.file)) + untracked_files_for_uploads.create!(path: found_path(project2.avatar.file.file)) + untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project1.full_path}/#{project1.uploads.last.path}") + untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project2.full_path}/#{project2.uploads.last.path}") user2.uploads.delete_all project2.uploads.delete_all @@ -122,7 +122,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do let(:user1) { create(:user) } context 'when the file is already in the uploads table' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/user/avatar/#{user1.id}/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "uploads/-/system/user/avatar/#{user1.id}/avatar.jpg") } before do upload_class.create!(path: "uploads/-/system/user/avatar/#{user1.id}/avatar.jpg", uploader: 'AvatarUploader', model_type: 'User', model_id: user1.id, size: 1234) @@ -142,7 +142,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for an appearance logo file path' do let(:model) { create(:appearance) } - let(:untracked_file) { described_class.create!(path: model.logo.file.file) } + let(:untracked_file) { described_class.create!(path: found_path(model.logo.file.file)) } before do model.update!(logo: uploaded_file) @@ -163,7 +163,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for an appearance header_logo file path' do let(:model) { create(:appearance) } - let(:untracked_file) { described_class.create!(path: model.header_logo.file.file) } + let(:untracked_file) { described_class.create!(path: found_path(model.header_logo.file.file)) } before do model.update!(header_logo: uploaded_file) @@ -184,7 +184,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a pre-Markdown Note attachment file path' do let(:model) { create(:note) } - let(:untracked_file) { described_class.create!(path: model.attachment.file.file) } + let(:untracked_file) { described_class.create!(path: found_path(model.attachment.file.file)) } before do model.update!(attachment: uploaded_file) @@ -207,7 +207,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a user avatar file path' do let(:model) { create(:user) } - let(:untracked_file) { described_class.create!(path: model.avatar.file.file) } + let(:untracked_file) { described_class.create!(path: found_path(model.avatar.file.file)) } before do model.update!(avatar: uploaded_file) @@ -228,7 +228,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a group avatar file path' do let(:model) { create(:group) } - let(:untracked_file) { described_class.create!(path: model.avatar.file.file) } + let(:untracked_file) { described_class.create!(path: found_path(model.avatar.file.file)) } before do model.update!(avatar: uploaded_file) @@ -251,7 +251,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a project avatar file path' do let(:model) { create(:project) } - let(:untracked_file) { described_class.create!(path: model.avatar.file.file) } + let(:untracked_file) { described_class.create!(path: found_path(model.avatar.file.file)) } before do model.update!(avatar: uploaded_file) @@ -272,7 +272,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:model) { create(:project) } - let(:untracked_file) { described_class.new(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{model.full_path}/#{model.uploads.first.path}") } + let(:untracked_file) { described_class.new(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{model.full_path}/#{model.uploads.first.path}") } before do UploadService.new(model, uploaded_file, FileUploader).execute # Markdown upload @@ -295,7 +295,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end describe '#mark_as_tracked' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } it 'saves the record with tracked set to true' do expect do @@ -308,7 +308,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do describe '#upload_path' do context 'for an appearance logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } it 'returns the file path relative to the CarrierWave root' do expect(untracked_file.upload_path).to eq('uploads/-/system/appearance/logo/1/some_logo.jpg') @@ -316,7 +316,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for an appearance header_logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } it 'returns the file path relative to the CarrierWave root' do expect(untracked_file.upload_path).to eq('uploads/-/system/appearance/header_logo/1/some_logo.jpg') @@ -324,7 +324,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a pre-Markdown Note attachment file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } it 'returns the file path relative to the CarrierWave root' do expect(untracked_file.upload_path).to eq('uploads/-/system/note/attachment/1234/some_attachment.pdf') @@ -332,7 +332,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a user avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } it 'returns the file path relative to the CarrierWave root' do expect(untracked_file.upload_path).to eq('uploads/-/system/user/avatar/1234/avatar.jpg') @@ -340,7 +340,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a group avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } it 'returns the file path relative to the CarrierWave root' do expect(untracked_file.upload_path).to eq('uploads/-/system/group/avatar/1234/avatar.jpg') @@ -348,7 +348,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a project avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } it 'returns the file path relative to the CarrierWave root' do expect(untracked_file.upload_path).to eq('uploads/-/system/project/avatar/1234/avatar.jpg') @@ -358,7 +358,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } let(:random_hex) { SecureRandom.hex } - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project.full_path}/#{random_hex}/Some file.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project.full_path}/#{random_hex}/Some file.jpg") } it 'returns the file path relative to the project directory in uploads' do expect(untracked_file.upload_path).to eq("#{random_hex}/Some file.jpg") @@ -368,7 +368,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do describe '#uploader' do context 'for an appearance logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } it 'returns AttachmentUploader as a string' do expect(untracked_file.uploader).to eq('AttachmentUploader') @@ -376,7 +376,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for an appearance header_logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } it 'returns AttachmentUploader as a string' do expect(untracked_file.uploader).to eq('AttachmentUploader') @@ -384,7 +384,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a pre-Markdown Note attachment file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } it 'returns AttachmentUploader as a string' do expect(untracked_file.uploader).to eq('AttachmentUploader') @@ -392,7 +392,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a user avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } it 'returns AvatarUploader as a string' do expect(untracked_file.uploader).to eq('AvatarUploader') @@ -400,7 +400,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a group avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } it 'returns AvatarUploader as a string' do expect(untracked_file.uploader).to eq('AvatarUploader') @@ -408,7 +408,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a project avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } it 'returns AvatarUploader as a string' do expect(untracked_file.uploader).to eq('AvatarUploader') @@ -417,7 +417,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } it 'returns FileUploader as a string' do expect(untracked_file.uploader).to eq('FileUploader') @@ -427,7 +427,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do describe '#model_type' do context 'for an appearance logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } it 'returns Appearance as a string' do expect(untracked_file.model_type).to eq('Appearance') @@ -435,7 +435,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for an appearance header_logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } it 'returns Appearance as a string' do expect(untracked_file.model_type).to eq('Appearance') @@ -443,7 +443,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a pre-Markdown Note attachment file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } it 'returns Note as a string' do expect(untracked_file.model_type).to eq('Note') @@ -451,7 +451,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a user avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } it 'returns User as a string' do expect(untracked_file.model_type).to eq('User') @@ -459,7 +459,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a group avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } it 'returns Namespace as a string' do expect(untracked_file.model_type).to eq('Namespace') @@ -467,7 +467,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a project avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } it 'returns Project as a string' do expect(untracked_file.model_type).to eq('Project') @@ -476,7 +476,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } it 'returns Project as a string' do expect(untracked_file.model_type).to eq('Project') @@ -486,7 +486,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do describe '#model_id' do context 'for an appearance logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } it 'returns the ID as a string' do expect(untracked_file.model_id).to eq('1') @@ -494,7 +494,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for an appearance header_logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } it 'returns the ID as a string' do expect(untracked_file.model_id).to eq('1') @@ -502,7 +502,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a pre-Markdown Note attachment file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } it 'returns the ID as a string' do expect(untracked_file.model_id).to eq('1234') @@ -510,7 +510,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a user avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } it 'returns the ID as a string' do expect(untracked_file.model_id).to eq('1234') @@ -518,7 +518,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a group avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } it 'returns the ID as a string' do expect(untracked_file.model_id).to eq('1234') @@ -526,7 +526,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a project avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } it 'returns the ID as a string' do expect(untracked_file.model_id).to eq('1234') @@ -535,7 +535,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } it 'returns the ID as a string' do expect(untracked_file.model_id).to eq(project.id.to_s) @@ -549,7 +549,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for an appearance logo file path' do let(:appearance) { create(:appearance) } - let(:untracked_file) { described_class.create!(path: appearance.logo.file.file) } + let(:untracked_file) { described_class.create!(path: found_path(appearance.logo.file.file)) } before do appearance.update!(logo: uploaded_file) @@ -566,7 +566,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a project avatar file path' do let(:project) { create(:project) } - let(:untracked_file) { described_class.create!(path: project.avatar.file.file) } + let(:untracked_file) { described_class.create!(path: found_path(project.avatar.file.file)) } before do project.update!(avatar: uploaded_file) @@ -583,7 +583,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::UPLOAD_DIR}/#{project.full_path}/#{project.uploads.first.path}") } + let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project.full_path}/#{project.uploads.first.path}") } before do UploadService.new(project, uploaded_file, FileUploader).execute @@ -599,3 +599,9 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end end end + +# The path returned by the find command in PrepareUntrackedUploads +# AKA the path relative to CarrierWave.root, without a leading slash. +def found_path(absolute_path) + absolute_path.sub(%r{\A#{CarrierWave.root}/}, '') +end diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index 913622cdb95..d0dd6b7c157 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -47,6 +47,15 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end end + it 'adds files with paths relative to CarrierWave.root' do + Sidekiq::Testing.fake! do + described_class.new.perform + untracked_files_for_uploads.all.each do |file| + expect(file.path.start_with?('uploads/')).to be_truthy + end + end + end + it 'does not add hashed files to the untracked_files_for_uploads table' do Sidekiq::Testing.fake! do described_class.new.perform @@ -83,7 +92,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side # E.g. The installation is in use at the time of migration, and someone has # just uploaded a file context 'when there are files in /uploads/tmp' do - let(:tmp_file) { Rails.root.join(described_class::UPLOAD_DIR, 'tmp', 'some_file.jpg') } + let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') } before do FileUtils.touch(tmp_file) diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 95496696bc6..83eb6bbd537 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -41,15 +41,13 @@ describe TrackUntrackedUploads, :migration, :sidekiq do component = 'a'*255 long_path = [ - CarrierWave.root, 'uploads', - [component] * Namespace::NUMBER_OF_ANCESTORS_ALLOWED, # namespaces - component, # project + component, # project.full_path component # filename ].flatten.join('/') record = UntrackedFile.create!(path: long_path) - expect(record.reload.path.size).to eq(5711) + expect(record.reload.path.size).to eq(519) end context 'with tracked and untracked uploads' do -- cgit v1.2.1 From d530085685105e2d7cd6d87ba866756683f0488d Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 14 Nov 2017 23:15:30 -0800 Subject: Refactor specs --- .../populate_untracked_uploads_spec.rb | 384 ++++++++------------- .../prepare_untracked_uploads_spec.rb | 26 +- spec/migrations/track_untracked_uploads_spec.rb | 98 ++---- spec/support/track_untracked_uploads_helpers.rb | 12 +- 4 files changed, 183 insertions(+), 337 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 953793a353c..c794a2f152b 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -1,46 +1,36 @@ require 'spec_helper' describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sidekiq, schema: 20171103140253 do + include TrackUntrackedUploadsHelpers + let!(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } let!(:uploads) { table(:uploads) } - let(:user1) { create(:user) } - let(:user2) { create(:user) } - let(:project1) { create(:project) } - let(:project2) { create(:project) } - let(:appearance) { create(:appearance) } - context 'with untracked files and tracked files in untracked_files_for_uploads' do - before do - fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } + let!(:user1) { create(:user, :with_avatar) } + let!(:user2) { create(:user, :with_avatar) } + let!(:project1) { create(:project, :with_avatar) } + let!(:project2) { create(:project, :with_avatar) } - # Tracked, by doing normal file upload - uploaded_file = fixture_file_upload(fixture) - user1.update!(avatar: uploaded_file) - project1.update!(avatar: uploaded_file) + before do UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload - appearance.update!(logo: uploaded_file) - - # Untracked, by doing normal file upload then later deleting records from DB - uploaded_file = fixture_file_upload(fixture) - user2.update!(avatar: uploaded_file) - project2.update!(avatar: uploaded_file) UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload - appearance.update!(header_logo: uploaded_file) # File records created by PrepareUntrackedUploads - untracked_files_for_uploads.create!(path: found_path(appearance.logo.file.file)) - untracked_files_for_uploads.create!(path: found_path(appearance.header_logo.file.file)) - untracked_files_for_uploads.create!(path: found_path(user1.avatar.file.file)) - untracked_files_for_uploads.create!(path: found_path(user2.avatar.file.file)) - untracked_files_for_uploads.create!(path: found_path(project1.avatar.file.file)) - untracked_files_for_uploads.create!(path: found_path(project2.avatar.file.file)) + untracked_files_for_uploads.create!(path: appearance.uploads.first.path) + untracked_files_for_uploads.create!(path: appearance.uploads.last.path) + untracked_files_for_uploads.create!(path: user1.uploads.first.path) + untracked_files_for_uploads.create!(path: user2.uploads.first.path) + untracked_files_for_uploads.create!(path: project1.uploads.first.path) + untracked_files_for_uploads.create!(path: project2.uploads.first.path) untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project1.full_path}/#{project1.uploads.last.path}") untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project2.full_path}/#{project2.uploads.last.path}") + # Untrack 4 files user2.uploads.delete_all - project2.uploads.delete_all - appearance.uploads.last.destroy + project2.uploads.delete_all # 2 files: avatar and a Markdown upload + appearance.uploads.where("path like '%header_logo%'").delete_all end it 'adds untracked files to the uploads table' do @@ -119,33 +109,36 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do let(:upload_class) { Gitlab::BackgroundMigration::PopulateUntrackedUploads::Upload } describe '#ensure_tracked!' do - let(:user1) { create(:user) } + let!(:user1) { create(:user, :with_avatar) } + let!(:untracked_file) { described_class.create!(path: user1.uploads.first.path) } context 'when the file is already in the uploads table' do - let(:untracked_file) { described_class.create!(path: "uploads/-/system/user/avatar/#{user1.id}/avatar.jpg") } + it 'does not add an upload' do + expect do + untracked_file.ensure_tracked! + end.not_to change { upload_class.count }.from(1) + end + end + context 'when the file is not already in the uploads table' do before do - upload_class.create!(path: "uploads/-/system/user/avatar/#{user1.id}/avatar.jpg", uploader: 'AvatarUploader', model_type: 'User', model_id: user1.id, size: 1234) + user1.uploads.delete_all end - it 'does not add an upload' do + it 'adds an upload' do expect do untracked_file.ensure_tracked! - end.not_to change { upload_class.count }.from(1) + end.to change { upload_class.count }.from(0).to(1) end end end describe '#add_to_uploads' do - let(:fixture) { Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') } - let(:uploaded_file) { fixture_file_upload(fixture) } - - context 'for an appearance logo file path' do - let(:model) { create(:appearance) } - let(:untracked_file) { described_class.create!(path: found_path(model.logo.file.file)) } + shared_examples_for 'add_to_uploads_non_markdown_files' do + let!(:expected_upload_attrs) { model.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') } + let!(:untracked_file) { described_class.create!(path: expected_upload_attrs['path']) } before do - model.update!(logo: uploaded_file) model.uploads.delete_all end @@ -154,129 +147,68 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do untracked_file.add_to_uploads end.to change { model.reload.uploads.count }.from(0).to(1) - expect(model.uploads.first.attributes).to include({ - "path" => "uploads/-/system/appearance/logo/#{model.id}/rails_sample.jpg", - "uploader" => "AttachmentUploader" - }.merge(rails_sample_jpg_attrs)) + expect(model.uploads.first.attributes).to include(expected_upload_attrs) end end - context 'for an appearance header_logo file path' do - let(:model) { create(:appearance) } - let(:untracked_file) { described_class.create!(path: found_path(model.header_logo.file.file)) } + context 'for an appearance logo file path' do + let(:model) { create(:appearance, logo: uploaded_file) } - before do - model.update!(header_logo: uploaded_file) - model.uploads.delete_all - end + it_behaves_like 'add_to_uploads_non_markdown_files' + end - it 'creates an Upload record' do - expect do - untracked_file.add_to_uploads - end.to change { model.reload.uploads.count }.from(0).to(1) + context 'for an appearance header_logo file path' do + let(:model) { create(:appearance, header_logo: uploaded_file) } - expect(model.uploads.first.attributes).to include({ - "path" => "uploads/-/system/appearance/header_logo/#{model.id}/rails_sample.jpg", - "uploader" => "AttachmentUploader" - }.merge(rails_sample_jpg_attrs)) - end + it_behaves_like 'add_to_uploads_non_markdown_files' end context 'for a pre-Markdown Note attachment file path' do - let(:model) { create(:note) } - let(:untracked_file) { described_class.create!(path: found_path(model.attachment.file.file)) } - - before do - model.update!(attachment: uploaded_file) - upload_class.delete_all + class Note < ActiveRecord::Base + has_many :uploads, as: :model, dependent: :destroy end - it 'creates an Upload record' do - expect do - untracked_file.add_to_uploads - end.to change { upload_class.count }.from(0).to(1) + let(:model) { create(:note, :with_attachment) } - expect(upload_class.first.attributes).to include({ - "path" => "uploads/-/system/note/attachment/#{model.id}/rails_sample.jpg", - "model_id" => model.id, - "model_type" => "Note", - "uploader" => "AttachmentUploader" - }.merge(rails_sample_jpg_attrs)) - end + it_behaves_like 'add_to_uploads_non_markdown_files' end context 'for a user avatar file path' do - let(:model) { create(:user) } - let(:untracked_file) { described_class.create!(path: found_path(model.avatar.file.file)) } + let(:model) { create(:user, :with_avatar) } - before do - model.update!(avatar: uploaded_file) - model.uploads.delete_all - end - - it 'creates an Upload record' do - expect do - untracked_file.add_to_uploads - end.to change { model.reload.uploads.count }.from(0).to(1) - - expect(model.uploads.first.attributes).to include({ - "path" => "uploads/-/system/user/avatar/#{model.id}/rails_sample.jpg", - "uploader" => "AvatarUploader" - }.merge(rails_sample_jpg_attrs)) - end + it_behaves_like 'add_to_uploads_non_markdown_files' end context 'for a group avatar file path' do - let(:model) { create(:group) } - let(:untracked_file) { described_class.create!(path: found_path(model.avatar.file.file)) } - - before do - model.update!(avatar: uploaded_file) - model.uploads.delete_all - end + let(:model) { create(:group, :with_avatar) } - it 'creates an Upload record' do - expect do - untracked_file.add_to_uploads - end.to change { model.reload.uploads.count }.from(0).to(1) - - expect(model.uploads.first.attributes).to include({ - "path" => "uploads/-/system/group/avatar/#{model.id}/rails_sample.jpg", - "model_id" => model.id, - "model_type" => "Namespace", # Explicitly calling this out because it was unexpected to me (I assumed it should be "Group") - "uploader" => "AvatarUploader" - }.merge(rails_sample_jpg_attrs)) - end + it_behaves_like 'add_to_uploads_non_markdown_files' end context 'for a project avatar file path' do - let(:model) { create(:project) } - let(:untracked_file) { described_class.create!(path: found_path(model.avatar.file.file)) } - - before do - model.update!(avatar: uploaded_file) - model.uploads.delete_all - end - - it 'creates an Upload record' do - expect do - untracked_file.add_to_uploads - end.to change { model.reload.uploads.count }.from(0).to(1) + let(:model) { create(:project, :with_avatar) } - expect(model.uploads.first.attributes).to include({ - "path" => "uploads/-/system/project/avatar/#{model.id}/rails_sample.jpg", - "uploader" => "AvatarUploader" - }.merge(rails_sample_jpg_attrs)) - end + it_behaves_like 'add_to_uploads_non_markdown_files' end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:model) { create(:project) } - let(:untracked_file) { described_class.new(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{model.full_path}/#{model.uploads.first.path}") } + let(:expected_upload_attrs) { {} } + + # UntrackedFile.path is different than Upload.path + let(:untracked_file) { create_untracked_file("/#{model.full_path}/#{model.uploads.first.path}") } before do - UploadService.new(model, uploaded_file, FileUploader).execute # Markdown upload - untracked_file.save! + # Upload the file + UploadService.new(model, uploaded_file, FileUploader).execute + + # Create the untracked_files_for_uploads record + untracked_file + + # Save the expected upload attributes + expected_upload_attrs = model.reload.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') + + # Untrack the file model.reload.uploads.delete_all end @@ -286,18 +218,15 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end.to change { model.reload.uploads.count }.from(0).to(1) hex_secret = untracked_file.path.match(/\/(\h+)\/rails_sample.jpg/)[1] - expect(model.uploads.first.attributes).to include({ - "path" => "#{hex_secret}/rails_sample.jpg", - "uploader" => "FileUploader" - }.merge(rails_sample_jpg_attrs)) + expect(model.uploads.first.attributes).to include(expected_upload_attrs) end end end describe '#mark_as_tracked' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } - it 'saves the record with tracked set to true' do + untracked_file = create_untracked_file("/-/system/appearance/logo/1/some_logo.jpg") + expect do untracked_file.mark_as_tracked end.to change { untracked_file.tracked }.from(false).to(true) @@ -307,253 +236,218 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end describe '#upload_path' do - context 'for an appearance logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + def assert_upload_path(file_path, expected_upload_path) + untracked_file = create_untracked_file(file_path) + expect(untracked_file.upload_path).to eq(expected_upload_path) + end + + context 'for an appearance logo file path' do it 'returns the file path relative to the CarrierWave root' do - expect(untracked_file.upload_path).to eq('uploads/-/system/appearance/logo/1/some_logo.jpg') + assert_upload_path('/-/system/appearance/logo/1/some_logo.jpg', 'uploads/-/system/appearance/logo/1/some_logo.jpg') end end context 'for an appearance header_logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } - it 'returns the file path relative to the CarrierWave root' do - expect(untracked_file.upload_path).to eq('uploads/-/system/appearance/header_logo/1/some_logo.jpg') + assert_upload_path('/-/system/appearance/header_logo/1/some_logo.jpg', 'uploads/-/system/appearance/header_logo/1/some_logo.jpg') end end context 'for a pre-Markdown Note attachment file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } - it 'returns the file path relative to the CarrierWave root' do - expect(untracked_file.upload_path).to eq('uploads/-/system/note/attachment/1234/some_attachment.pdf') + assert_upload_path('/-/system/note/attachment/1234/some_attachment.pdf', 'uploads/-/system/note/attachment/1234/some_attachment.pdf') end end context 'for a user avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } - it 'returns the file path relative to the CarrierWave root' do - expect(untracked_file.upload_path).to eq('uploads/-/system/user/avatar/1234/avatar.jpg') + assert_upload_path('/-/system/user/avatar/1234/avatar.jpg', 'uploads/-/system/user/avatar/1234/avatar.jpg') end end context 'for a group avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } - it 'returns the file path relative to the CarrierWave root' do - expect(untracked_file.upload_path).to eq('uploads/-/system/group/avatar/1234/avatar.jpg') + assert_upload_path('/-/system/group/avatar/1234/avatar.jpg', 'uploads/-/system/group/avatar/1234/avatar.jpg') end end context 'for a project avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } - it 'returns the file path relative to the CarrierWave root' do - expect(untracked_file.upload_path).to eq('uploads/-/system/project/avatar/1234/avatar.jpg') + assert_upload_path('/-/system/project/avatar/1234/avatar.jpg', 'uploads/-/system/project/avatar/1234/avatar.jpg') end end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - let(:project) { create(:project) } - let(:random_hex) { SecureRandom.hex } - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project.full_path}/#{random_hex}/Some file.jpg") } - it 'returns the file path relative to the project directory in uploads' do - expect(untracked_file.upload_path).to eq("#{random_hex}/Some file.jpg") + project = create(:project) + random_hex = SecureRandom.hex + + assert_upload_path("/#{project.full_path}/#{random_hex}/Some file.jpg", "#{random_hex}/Some file.jpg") end end end describe '#uploader' do - context 'for an appearance logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + def assert_uploader(file_path, expected_uploader) + untracked_file = create_untracked_file(file_path) + + expect(untracked_file.uploader).to eq(expected_uploader) + end + context 'for an appearance logo file path' do it 'returns AttachmentUploader as a string' do - expect(untracked_file.uploader).to eq('AttachmentUploader') + assert_uploader('/-/system/appearance/logo/1/some_logo.jpg', 'AttachmentUploader') end end context 'for an appearance header_logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } - it 'returns AttachmentUploader as a string' do - expect(untracked_file.uploader).to eq('AttachmentUploader') + assert_uploader('/-/system/appearance/header_logo/1/some_logo.jpg', 'AttachmentUploader') end end context 'for a pre-Markdown Note attachment file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } - it 'returns AttachmentUploader as a string' do - expect(untracked_file.uploader).to eq('AttachmentUploader') + assert_uploader('/-/system/note/attachment/1234/some_attachment.pdf', 'AttachmentUploader') end end context 'for a user avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } - it 'returns AvatarUploader as a string' do - expect(untracked_file.uploader).to eq('AvatarUploader') + assert_uploader('/-/system/user/avatar/1234/avatar.jpg', 'AvatarUploader') end end context 'for a group avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } - it 'returns AvatarUploader as a string' do - expect(untracked_file.uploader).to eq('AvatarUploader') + assert_uploader('/-/system/group/avatar/1234/avatar.jpg', 'AvatarUploader') end end context 'for a project avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } - it 'returns AvatarUploader as a string' do - expect(untracked_file.uploader).to eq('AvatarUploader') + assert_uploader('/-/system/project/avatar/1234/avatar.jpg', 'AvatarUploader') end end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - let(:project) { create(:project) } - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } - it 'returns FileUploader as a string' do - expect(untracked_file.uploader).to eq('FileUploader') + project = create(:project) + + assert_uploader("/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg", 'FileUploader') end end end describe '#model_type' do - context 'for an appearance logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + def assert_model_type(file_path, expected_model_type) + untracked_file = create_untracked_file(file_path) + expect(untracked_file.model_type).to eq(expected_model_type) + end + + context 'for an appearance logo file path' do it 'returns Appearance as a string' do - expect(untracked_file.model_type).to eq('Appearance') + assert_model_type('/-/system/appearance/logo/1/some_logo.jpg', 'Appearance') end end context 'for an appearance header_logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } - it 'returns Appearance as a string' do - expect(untracked_file.model_type).to eq('Appearance') + assert_model_type('/-/system/appearance/header_logo/1/some_logo.jpg', 'Appearance') end end context 'for a pre-Markdown Note attachment file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } - it 'returns Note as a string' do - expect(untracked_file.model_type).to eq('Note') + assert_model_type('/-/system/note/attachment/1234/some_attachment.pdf', 'Note') end end context 'for a user avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } - it 'returns User as a string' do - expect(untracked_file.model_type).to eq('User') + assert_model_type('/-/system/user/avatar/1234/avatar.jpg', 'User') end end context 'for a group avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } - it 'returns Namespace as a string' do - expect(untracked_file.model_type).to eq('Namespace') + assert_model_type('/-/system/group/avatar/1234/avatar.jpg', 'Namespace') end end context 'for a project avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } - it 'returns Project as a string' do - expect(untracked_file.model_type).to eq('Project') + assert_model_type('/-/system/project/avatar/1234/avatar.jpg', 'Project') end end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - let(:project) { create(:project) } - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } - it 'returns Project as a string' do - expect(untracked_file.model_type).to eq('Project') + project = create(:project) + + assert_model_type("/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg", 'Project') end end end describe '#model_id' do - context 'for an appearance logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/logo/1/some_logo.jpg") } + def assert_model_id(file_path, expected_model_id) + untracked_file = create_untracked_file(file_path) + + expect(untracked_file.model_id).to eq(expected_model_id) + end + context 'for an appearance logo file path' do it 'returns the ID as a string' do - expect(untracked_file.model_id).to eq('1') + assert_model_id('/-/system/appearance/logo/1/some_logo.jpg', '1') end end context 'for an appearance header_logo file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/appearance/header_logo/1/some_logo.jpg") } - it 'returns the ID as a string' do - expect(untracked_file.model_id).to eq('1') + assert_model_id('/-/system/appearance/header_logo/1/some_logo.jpg', '1') end end context 'for a pre-Markdown Note attachment file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/note/attachment/1234/some_attachment.pdf") } - it 'returns the ID as a string' do - expect(untracked_file.model_id).to eq('1234') + assert_model_id('/-/system/note/attachment/1234/some_attachment.pdf', '1234') end end context 'for a user avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/user/avatar/1234/avatar.jpg") } - it 'returns the ID as a string' do - expect(untracked_file.model_id).to eq('1234') + assert_model_id('/-/system/user/avatar/1234/avatar.jpg', '1234') end end context 'for a group avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/group/avatar/1234/avatar.jpg") } - it 'returns the ID as a string' do - expect(untracked_file.model_id).to eq('1234') + assert_model_id('/-/system/group/avatar/1234/avatar.jpg', '1234') end end context 'for a project avatar file path' do - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/-/system/project/avatar/1234/avatar.jpg") } - it 'returns the ID as a string' do - expect(untracked_file.model_id).to eq('1234') + assert_model_id('/-/system/project/avatar/1234/avatar.jpg', '1234') end end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do - let(:project) { create(:project) } - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg") } - it 'returns the ID as a string' do - expect(untracked_file.model_id).to eq(project.id.to_s) + project = create(:project) + + assert_model_id("/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg", project.id.to_s) end end end describe '#file_size' do - let(:fixture) { Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') } - let(:uploaded_file) { fixture_file_upload(fixture) } - context 'for an appearance logo file path' do - let(:appearance) { create(:appearance) } - let(:untracked_file) { described_class.create!(path: found_path(appearance.logo.file.file)) } - - before do - appearance.update!(logo: uploaded_file) - end + let(:appearance) { create(:appearance, logo: uploaded_file) } + let(:untracked_file) { described_class.create!(path: appearance.uploads.first.path) } it 'returns the file size' do expect(untracked_file.file_size).to eq(35255) @@ -565,12 +459,8 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end context 'for a project avatar file path' do - let(:project) { create(:project) } - let(:untracked_file) { described_class.create!(path: found_path(project.avatar.file.file)) } - - before do - project.update!(avatar: uploaded_file) - end + let(:project) { create(:project, avatar: uploaded_file) } + let(:untracked_file) { described_class.create!(path: project.uploads.first.path) } it 'returns the file size' do expect(untracked_file.file_size).to eq(35255) @@ -583,7 +473,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:project) { create(:project) } - let(:untracked_file) { described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project.full_path}/#{project.uploads.first.path}") } + let(:untracked_file) { create_untracked_file("/#{project.full_path}/#{project.uploads.first.path}") } before do UploadService.new(project, uploaded_file, FileUploader).execute @@ -598,10 +488,8 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end end end -end -# The path returned by the find command in PrepareUntrackedUploads -# AKA the path relative to CarrierWave.root, without a leading slash. -def found_path(absolute_path) - absolute_path.sub(%r{\A#{CarrierWave.root}/}, '') + def create_untracked_file(path_relative_to_upload_dir) + described_class.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}#{path_relative_to_upload_dir}") + end end diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index d0dd6b7c157..d61135912dd 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -1,13 +1,9 @@ require 'spec_helper' describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :sidekiq, schema: 20171103140253 do - let!(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } + include TrackUntrackedUploadsHelpers - let(:user1) { create(:user) } - let(:user2) { create(:user) } - let(:project1) { create(:project) } - let(:project2) { create(:project) } - let(:appearance) { create(:appearance) } + let!(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } matcher :be_scheduled_migration do |*expected| match do |migration| @@ -22,20 +18,18 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end context 'when files were uploaded before and after hashed storage was enabled' do - before do - fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') - uploaded_file = fixture_file_upload(fixture) + let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } + let!(:user) { create(:user, :with_avatar) } + let!(:project1) { create(:project, :with_avatar) } + let(:project2) { create(:project) } # instantiate after enabling hashed_storage - user1.update(avatar: uploaded_file) - project1.update(avatar: uploaded_file) - appearance.update(logo: uploaded_file, header_logo: uploaded_file) - uploaded_file = fixture_file_upload(fixture) - UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload + before do + # Markdown upload before enabling hashed_storage + UploadService.new(project1, uploaded_file, FileUploader).execute stub_application_setting(hashed_storage_enabled: true) - # Hashed files - uploaded_file = fixture_file_upload(fixture) + # Markdown upload after enabling hashed_storage UploadService.new(project2, uploaded_file, FileUploader).execute end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 83eb6bbd537..a6e880279b6 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -4,9 +4,7 @@ require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_up describe TrackUntrackedUploads, :migration, :sidekiq do include TrackUntrackedUploadsHelpers - class UntrackedFile < ActiveRecord::Base - self.table_name = 'untracked_files_for_uploads' - end + let(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } matcher :be_scheduled_migration do match do |migration| @@ -46,39 +44,36 @@ describe TrackUntrackedUploads, :migration, :sidekiq do component # filename ].flatten.join('/') - record = UntrackedFile.create!(path: long_path) + record = untracked_files_for_uploads.create!(path: long_path) expect(record.reload.path.size).to eq(519) end context 'with tracked and untracked uploads' do - let(:user1) { create(:user) } - let(:user2) { create(:user) } - let(:project1) { create(:project) } - let(:project2) { create(:project) } - let(:appearance) { create(:appearance) } + let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } + let!(:user1) { create(:user, :with_avatar) } + let!(:user2) { create(:user, :with_avatar) } + let!(:project1) { create(:project, :with_avatar) } + let!(:project2) { create(:project, :with_avatar) } let(:uploads) { table(:uploads) } before do - fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') - - # Tracked, by doing normal file upload - uploaded_file = fixture_file_upload(fixture) - user1.update(avatar: uploaded_file) - project1.update(avatar: uploaded_file) - upload_result = UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload - @project1_markdown_upload_path = upload_result[:url].sub(%r{\A/uploads/}, '') - appearance.update(logo: uploaded_file) - - # Untracked, by doing normal file upload then deleting records from DB - uploaded_file = fixture_file_upload(fixture) - user2.update(avatar: uploaded_file) + UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload + UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload + + # Save expected Upload attributes + @appearance_logo_attributes = appearance.uploads.where("path like '%/logo/%'").first.attributes.slice('path', 'uploader', 'size', 'checksum') + @appearance_header_logo_attributes = appearance.uploads.where("path like '%/header_logo/%'").first.attributes.slice('path', 'uploader', 'size', 'checksum') + @user1_avatar_attributes = user1.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') + @user2_avatar_attributes = user2.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') + @project1_avatar_attributes = project1.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') + @project2_avatar_attributes = project2.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') + @project1_markdown_attributes = project1.uploads.last.attributes.slice('path', 'uploader', 'size', 'checksum') + @project2_markdown_attributes = project2.uploads.last.attributes.slice('path', 'uploader', 'size', 'checksum') + + # Untrack 4 files user2.uploads.delete_all - project2.update(avatar: uploaded_file) - upload_result = UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload - @project2_markdown_upload_path = upload_result[:url].sub(%r{\A/uploads/}, '') - project2.uploads.delete_all - appearance.update(header_logo: uploaded_file) - appearance.uploads.last.destroy + project2.uploads.delete_all # 2 files: avatar and a Markdown upload + appearance.uploads.where("path like '%header_logo%'").delete_all end it 'tracks untracked uploads' do @@ -87,23 +82,10 @@ describe TrackUntrackedUploads, :migration, :sidekiq do migrate! end.to change { uploads.count }.from(4).to(8) - expect(user2.reload.uploads.first.attributes).to include({ - "path" => "uploads/-/system/user/avatar/#{user2.id}/rails_sample.jpg", - "uploader" => "AvatarUploader" - }.merge(rails_sample_jpg_attrs)) - expect(project2.reload.uploads.first.attributes).to include({ - "path" => "uploads/-/system/project/avatar/#{project2.id}/rails_sample.jpg", - "uploader" => "AvatarUploader" - }.merge(rails_sample_jpg_attrs)) - expect(appearance.reload.uploads.count).to eq(2) - expect(appearance.uploads.last.attributes).to include({ - "path" => "uploads/-/system/appearance/header_logo/#{appearance.id}/rails_sample.jpg", - "uploader" => "AttachmentUploader" - }.merge(rails_sample_jpg_attrs)) - expect(project2.uploads.last.attributes).to include({ - "path" => @project2_markdown_upload_path, - "uploader" => "FileUploader" - }.merge(rails_sample_jpg_attrs)) + expect(appearance.reload.uploads.where("path like '%/header_logo/%'").first.attributes).to include(@appearance_header_logo_attributes) + expect(user2.reload.uploads.first.attributes).to include(@user2_avatar_attributes) + expect(project2.reload.uploads.first.attributes).to include(@project2_avatar_attributes) + expect(project2.uploads.last.attributes).to include(@project2_markdown_attributes) end end @@ -111,31 +93,19 @@ describe TrackUntrackedUploads, :migration, :sidekiq do Sidekiq::Testing.inline! do migrate! - expect(user1.reload.uploads.first.attributes).to include({ - "path" => "uploads/-/system/user/avatar/#{user1.id}/rails_sample.jpg", - "uploader" => "AvatarUploader" - }.merge(rails_sample_jpg_attrs)) - expect(project1.reload.uploads.first.attributes).to include({ - "path" => "uploads/-/system/project/avatar/#{project1.id}/rails_sample.jpg", - "uploader" => "AvatarUploader" - }.merge(rails_sample_jpg_attrs)) - expect(appearance.reload.uploads.first.attributes).to include({ - "path" => "uploads/-/system/appearance/logo/#{appearance.id}/rails_sample.jpg", - "uploader" => "AttachmentUploader" - }.merge(rails_sample_jpg_attrs)) - expect(project1.uploads.last.attributes).to include({ - "path" => @project1_markdown_upload_path, - "uploader" => "FileUploader" - }.merge(rails_sample_jpg_attrs)) + expect(appearance.reload.uploads.where("path like '%/logo/%'").first.attributes).to include(@appearance_logo_attributes) + expect(user1.reload.uploads.first.attributes).to include(@user1_avatar_attributes) + expect(project1.reload.uploads.first.attributes).to include(@project1_avatar_attributes) + expect(project1.uploads.last.attributes).to include(@project1_markdown_attributes) end end - it 'all UntrackedFile records are marked as tracked' do + it 'all untracked_files_for_uploads records are marked as tracked' do Sidekiq::Testing.inline! do migrate! - expect(UntrackedFile.count).to eq(8) - expect(UntrackedFile.count).to eq(UntrackedFile.where(tracked: true).count) + expect(untracked_files_for_uploads.count).to eq(8) + expect(untracked_files_for_uploads.count).to eq(untracked_files_for_uploads.where(tracked: true).count) end end end diff --git a/spec/support/track_untracked_uploads_helpers.rb b/spec/support/track_untracked_uploads_helpers.rb index 749c5775bb0..5b832929602 100644 --- a/spec/support/track_untracked_uploads_helpers.rb +++ b/spec/support/track_untracked_uploads_helpers.rb @@ -1,12 +1,6 @@ module TrackUntrackedUploadsHelpers - def rails_sample_jpg_attrs - @rails_sample_jpg_attrs ||= { - "size" => File.size(rails_sample_file_path), - "checksum" => Digest::SHA256.file(rails_sample_file_path).hexdigest - } - end - - def rails_sample_file_path - Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + def uploaded_file + fixture_path = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + fixture_file_upload(fixture_path) end end -- cgit v1.2.1 From dd8680a7ae4be279ae1d90f0889317a1e6ee0d95 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 15 Nov 2017 02:36:25 -0800 Subject: Drop temporary tracking table when finished --- .../populate_untracked_uploads_spec.rb | 36 ++++++++-- .../prepare_untracked_uploads_spec.rb | 63 ++++++++-------- spec/migrations/track_untracked_uploads_spec.rb | 84 +++++++++++----------- spec/support/track_untracked_uploads_helpers.rb | 14 ++++ 4 files changed, 113 insertions(+), 84 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index c794a2f152b..52f57408bfa 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -1,11 +1,19 @@ require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads') -describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sidekiq, schema: 20171103140253 do +describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sidekiq, :temp_table_may_drop, schema: 20171103140253 do include TrackUntrackedUploadsHelpers + subject { described_class.new } + let!(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } let!(:uploads) { table(:uploads) } + before do + # Prevent the TrackUntrackedUploads migration from running PrepareUntrackedUploads job + allow(BackgroundMigrationWorker).to receive(:perform_async).and_return(true) + end + context 'with untracked files and tracked files in untracked_files_for_uploads' do let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } let!(:user1) { create(:user, :with_avatar) } @@ -35,7 +43,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid it 'adds untracked files to the uploads table' do expect do - described_class.new.perform(1, 1000) + subject.perform(1, 1000) end.to change { uploads.count }.from(4).to(8) expect(user2.uploads.count).to eq(1) @@ -44,13 +52,15 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end it 'sets all added or confirmed tracked files to tracked' do + expect(subject).to receive(:drop_temp_table_if_finished) # Don't drop the table so we can look at it + expect do - described_class.new.perform(1, 1000) + subject.perform(1, 1000) end.to change { untracked_files_for_uploads.where(tracked: true).count }.from(0).to(8) end it 'does not create duplicate uploads of already tracked files' do - described_class.new.perform(1, 1000) + subject.perform(1, 1000) expect(user1.uploads.count).to eq(1) expect(project1.uploads.count).to eq(2) @@ -62,7 +72,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end_id = untracked_files_for_uploads.all.to_a[3].id expect do - described_class.new.perform(start_id, end_id) + subject.perform(start_id, end_id) end.to change { uploads.count }.from(4).to(6) expect(user1.uploads.count).to eq(1) @@ -80,7 +90,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end_id = untracked_files_for_uploads.all.to_a[7].id expect do - described_class.new.perform(start_id, end_id) + subject.perform(start_id, end_id) end.to change { uploads.count }.from(4).to(6) expect(user1.uploads.count).to eq(1) @@ -92,12 +102,24 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid # Only 4 have been either confirmed or added to uploads expect(untracked_files_for_uploads.where(tracked: true).count).to eq(4) end + + it 'does not drop the temporary tracking table after processing the batch, if there are still untracked rows' do + subject.perform(1, untracked_files_for_uploads.last.id - 1) + + expect(table_exists?(:untracked_files_for_uploads)).to be_truthy + end + + it 'drops the temporary tracking table after processing the batch, if there are no untracked rows left' do + subject.perform(1, untracked_files_for_uploads.last.id) + + expect(table_exists?(:untracked_files_for_uploads)).to be_falsey + end end context 'with no untracked files' do it 'does not add to the uploads table (and does not raise error)' do expect do - described_class.new.perform(1, 1000) + subject.perform(1, 1000) end.not_to change { uploads.count }.from(0) end end diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index d61135912dd..8fd20fd0bb3 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -17,6 +17,13 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end end + around do |example| + # Especially important so the follow-up migration does not get run + Sidekiq::Testing.fake! do + example.run + end + end + context 'when files were uploaded before and after hashed storage was enabled' do let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } let!(:user) { create(:user, :with_avatar) } @@ -34,38 +41,30 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end it 'adds unhashed files to the untracked_files_for_uploads table' do - Sidekiq::Testing.fake! do - expect do - described_class.new.perform - end.to change { untracked_files_for_uploads.count }.from(0).to(5) - end + expect do + described_class.new.perform + end.to change { untracked_files_for_uploads.count }.from(0).to(5) end it 'adds files with paths relative to CarrierWave.root' do - Sidekiq::Testing.fake! do - described_class.new.perform - untracked_files_for_uploads.all.each do |file| - expect(file.path.start_with?('uploads/')).to be_truthy - end + described_class.new.perform + untracked_files_for_uploads.all.each do |file| + expect(file.path.start_with?('uploads/')).to be_truthy end end it 'does not add hashed files to the untracked_files_for_uploads table' do - Sidekiq::Testing.fake! do - described_class.new.perform + described_class.new.perform - hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path - expect(untracked_files_for_uploads.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey - end + hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path + expect(untracked_files_for_uploads.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey end it 'correctly schedules the follow-up background migration jobs' do - Sidekiq::Testing.fake! do - described_class.new.perform + described_class.new.perform - expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5) - expect(BackgroundMigrationWorker.jobs.size).to eq(1) - end + expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5) + expect(BackgroundMigrationWorker.jobs.size).to eq(1) end # E.g. from a previous failed run of this background migration @@ -75,11 +74,9 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end it 'does not error or produce duplicates of existing data' do - Sidekiq::Testing.fake! do - expect do - described_class.new.perform - end.not_to change { untracked_files_for_uploads.count }.from(5) - end + expect do + described_class.new.perform + end.not_to change { untracked_files_for_uploads.count }.from(5) end end @@ -97,11 +94,9 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end it 'does not add files from /uploads/tmp' do - Sidekiq::Testing.fake! do - expect do - described_class.new.perform - end.to change { untracked_files_for_uploads.count }.from(0).to(5) - end + expect do + described_class.new.perform + end.to change { untracked_files_for_uploads.count }.from(0).to(5) end end end @@ -110,11 +105,9 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side # may not have an upload directory because they have no uploads. context 'when no files were ever uploaded' do it 'does not add to the untracked_files_for_uploads table (and does not raise error)' do - Sidekiq::Testing.fake! do - expect do - described_class.new.perform - end.not_to change { untracked_files_for_uploads.count }.from(0) - end + expect do + described_class.new.perform + end.not_to change { untracked_files_for_uploads.count }.from(0) end end end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index a6e880279b6..a17251ba052 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads') -describe TrackUntrackedUploads, :migration, :sidekiq do +describe TrackUntrackedUploads, :migration, :sidekiq, :temp_table_may_drop do include TrackUntrackedUploadsHelpers let(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } @@ -18,34 +18,41 @@ describe TrackUntrackedUploads, :migration, :sidekiq do end end - it 'correctly schedules the follow-up background migration' do - Sidekiq::Testing.fake! do + context 'before running the background migration' do + around do |example| + # Especially important so the follow-up migration does not get run + Sidekiq::Testing.fake! do + example.run + end + end + + it 'correctly schedules the follow-up background migration' do migrate! expect(described_class::MIGRATION).to be_scheduled_migration expect(BackgroundMigrationWorker.jobs.size).to eq(1) end - end - it 'ensures the untracked_files_for_uploads table exists' do - expect do - migrate! - end.to change { table_exists?(:untracked_files_for_uploads) }.from(false).to(true) - end + it 'ensures the untracked_files_for_uploads table exists' do + expect do + migrate! + end.to change { table_exists?(:untracked_files_for_uploads) }.from(false).to(true) + end - it 'has a path field long enough for really long paths' do - migrate! + it 'has a path field long enough for really long paths' do + migrate! - component = 'a'*255 + component = 'a'*255 - long_path = [ - 'uploads', - component, # project.full_path - component # filename - ].flatten.join('/') + long_path = [ + 'uploads', + component, # project.full_path + component # filename + ].flatten.join('/') - record = untracked_files_for_uploads.create!(path: long_path) - expect(record.reload.path.size).to eq(519) + record = untracked_files_for_uploads.create!(path: long_path) + expect(record.reload.path.size).to eq(519) + end end context 'with tracked and untracked uploads' do @@ -77,36 +84,29 @@ describe TrackUntrackedUploads, :migration, :sidekiq do end it 'tracks untracked uploads' do - Sidekiq::Testing.inline! do - expect do - migrate! - end.to change { uploads.count }.from(4).to(8) - - expect(appearance.reload.uploads.where("path like '%/header_logo/%'").first.attributes).to include(@appearance_header_logo_attributes) - expect(user2.reload.uploads.first.attributes).to include(@user2_avatar_attributes) - expect(project2.reload.uploads.first.attributes).to include(@project2_avatar_attributes) - expect(project2.uploads.last.attributes).to include(@project2_markdown_attributes) - end + expect do + migrate! + end.to change { uploads.count }.from(4).to(8) + + expect(appearance.reload.uploads.where("path like '%/header_logo/%'").first.attributes).to include(@appearance_header_logo_attributes) + expect(user2.reload.uploads.first.attributes).to include(@user2_avatar_attributes) + expect(project2.reload.uploads.first.attributes).to include(@project2_avatar_attributes) + expect(project2.uploads.last.attributes).to include(@project2_markdown_attributes) end it 'ignores already-tracked uploads' do - Sidekiq::Testing.inline! do - migrate! + migrate! - expect(appearance.reload.uploads.where("path like '%/logo/%'").first.attributes).to include(@appearance_logo_attributes) - expect(user1.reload.uploads.first.attributes).to include(@user1_avatar_attributes) - expect(project1.reload.uploads.first.attributes).to include(@project1_avatar_attributes) - expect(project1.uploads.last.attributes).to include(@project1_markdown_attributes) - end + expect(appearance.reload.uploads.where("path like '%/logo/%'").first.attributes).to include(@appearance_logo_attributes) + expect(user1.reload.uploads.first.attributes).to include(@user1_avatar_attributes) + expect(project1.reload.uploads.first.attributes).to include(@project1_avatar_attributes) + expect(project1.uploads.last.attributes).to include(@project1_markdown_attributes) end - it 'all untracked_files_for_uploads records are marked as tracked' do - Sidekiq::Testing.inline! do - migrate! + it 'the temporary table untracked_files_for_uploads no longer exists' do + migrate! - expect(untracked_files_for_uploads.count).to eq(8) - expect(untracked_files_for_uploads.count).to eq(untracked_files_for_uploads.where(tracked: true).count) - end + expect(table_exists?(:untracked_files_for_uploads)).to be_falsey end end end diff --git a/spec/support/track_untracked_uploads_helpers.rb b/spec/support/track_untracked_uploads_helpers.rb index 5b832929602..bb700bc53f1 100644 --- a/spec/support/track_untracked_uploads_helpers.rb +++ b/spec/support/track_untracked_uploads_helpers.rb @@ -3,4 +3,18 @@ module TrackUntrackedUploadsHelpers fixture_path = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') fixture_file_upload(fixture_path) end + + def recreate_temp_table_if_dropped + TrackUntrackedUploads.new.ensure_temporary_tracking_table_exists + end + + RSpec.configure do |config| + config.after(:each, :temp_table_may_drop) do + recreate_temp_table_if_dropped + end + + config.after(:context, :temp_table_may_drop) do + recreate_temp_table_if_dropped + end + end end -- cgit v1.2.1 From 7fd26434196df01091b18747f91f54c0701bb292 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 15 Nov 2017 03:01:35 -0800 Subject: Fix Rubocop offenses --- .../gitlab/background_migration/populate_untracked_uploads_spec.rb | 6 ++---- .../gitlab/background_migration/prepare_untracked_uploads_spec.rb | 2 +- spec/migrations/track_untracked_uploads_spec.rb | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 52f57408bfa..7f5c7b99742 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -215,7 +215,6 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:model) { create(:project) } - let(:expected_upload_attrs) { {} } # UntrackedFile.path is different than Upload.path let(:untracked_file) { create_untracked_file("/#{model.full_path}/#{model.uploads.first.path}") } @@ -228,7 +227,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do untracked_file # Save the expected upload attributes - expected_upload_attrs = model.reload.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') + @expected_upload_attrs = model.reload.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') # Untrack the file model.reload.uploads.delete_all @@ -239,8 +238,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do untracked_file.add_to_uploads end.to change { model.reload.uploads.count }.from(0).to(1) - hex_secret = untracked_file.path.match(/\/(\h+)\/rails_sample.jpg/)[1] - expect(model.uploads.first.attributes).to include(expected_upload_attrs) + expect(model.uploads.first.attributes).to include(@expected_upload_attrs) end end end diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index 8fd20fd0bb3..81af3f78307 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -36,7 +36,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side stub_application_setting(hashed_storage_enabled: true) - # Markdown upload after enabling hashed_storage + # Markdown upload after enabling hashed_storage UploadService.new(project2, uploaded_file, FileUploader).execute end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index a17251ba052..5632c81f231 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -42,7 +42,7 @@ describe TrackUntrackedUploads, :migration, :sidekiq, :temp_table_may_drop do it 'has a path field long enough for really long paths' do migrate! - component = 'a'*255 + component = 'a' * 255 long_path = [ 'uploads', -- cgit v1.2.1 From 87529ce5823036d4b9dd9ca412643befc8e490c3 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 15 Nov 2017 05:19:07 -0800 Subject: Move temp table creation into the prepare job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Hopefully fixes spec failures in which the table doesn’t exist * Decouples the background migration from the post-deploy migration, e.g. we could easily run it again even though the table is dropped when finished. --- .../populate_untracked_uploads_spec.rb | 17 ++++++-- .../prepare_untracked_uploads_spec.rb | 47 +++++++++++++++++----- spec/migrations/track_untracked_uploads_spec.rb | 34 ++-------------- spec/support/track_untracked_uploads_helpers.rb | 14 ++----- 4 files changed, 59 insertions(+), 53 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 7f5c7b99742..317890bd854 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads') -describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sidekiq, :temp_table_may_drop, schema: 20171103140253 do +describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sidekiq, schema: 20171103140253 do include TrackUntrackedUploadsHelpers subject { described_class.new } @@ -10,8 +10,11 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid let!(:uploads) { table(:uploads) } before do - # Prevent the TrackUntrackedUploads migration from running PrepareUntrackedUploads job - allow(BackgroundMigrationWorker).to receive(:perform_async).and_return(true) + ensure_temporary_tracking_table_exists + end + + after(:all) do + drop_temp_table_if_exists end context 'with untracked files and tracked files in untracked_files_for_uploads' do @@ -130,6 +133,14 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do let(:upload_class) { Gitlab::BackgroundMigration::PopulateUntrackedUploads::Upload } + before(:all) do + ensure_temporary_tracking_table_exists + end + + after(:all) do + drop_temp_table_if_exists + end + describe '#ensure_tracked!' do let!(:user1) { create(:user, :with_avatar) } let!(:untracked_file) { described_class.create!(path: user1.uploads.first.path) } diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index 81af3f78307..042fdead281 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -17,6 +17,14 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end end + before do + drop_temp_table_if_exists + end + + after(:all) do + drop_temp_table_if_exists + end + around do |example| # Especially important so the follow-up migration does not get run Sidekiq::Testing.fake! do @@ -24,6 +32,27 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end end + it 'ensures the untracked_files_for_uploads table exists' do + expect do + described_class.new.perform + end.to change { table_exists?(:untracked_files_for_uploads) }.from(false).to(true) + end + + it 'has a path field long enough for really long paths' do + described_class.new.perform + + component = 'a' * 255 + + long_path = [ + 'uploads', + component, # project.full_path + component # filename + ].flatten.join('/') + + record = untracked_files_for_uploads.create!(path: long_path) + expect(record.reload.path.size).to eq(519) + end + context 'when files were uploaded before and after hashed storage was enabled' do let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } let!(:user) { create(:user, :with_avatar) } @@ -41,9 +70,9 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end it 'adds unhashed files to the untracked_files_for_uploads table' do - expect do - described_class.new.perform - end.to change { untracked_files_for_uploads.count }.from(0).to(5) + described_class.new.perform + + expect(untracked_files_for_uploads.count).to eq(5) end it 'adds files with paths relative to CarrierWave.root' do @@ -94,9 +123,9 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end it 'does not add files from /uploads/tmp' do - expect do - described_class.new.perform - end.to change { untracked_files_for_uploads.count }.from(0).to(5) + described_class.new.perform + + expect(untracked_files_for_uploads.count).to eq(5) end end end @@ -105,9 +134,9 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side # may not have an upload directory because they have no uploads. context 'when no files were ever uploaded' do it 'does not add to the untracked_files_for_uploads table (and does not raise error)' do - expect do - described_class.new.perform - end.not_to change { untracked_files_for_uploads.count }.from(0) + described_class.new.perform + + expect(untracked_files_for_uploads.count).to eq(0) end end end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 5632c81f231..11824bebb91 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads') -describe TrackUntrackedUploads, :migration, :sidekiq, :temp_table_may_drop do +describe TrackUntrackedUploads, :migration, :sidekiq do include TrackUntrackedUploadsHelpers let(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } @@ -18,41 +18,13 @@ describe TrackUntrackedUploads, :migration, :sidekiq, :temp_table_may_drop do end end - context 'before running the background migration' do - around do |example| - # Especially important so the follow-up migration does not get run - Sidekiq::Testing.fake! do - example.run - end - end - - it 'correctly schedules the follow-up background migration' do + it 'correctly schedules the follow-up background migration' do + Sidekiq::Testing.fake! do migrate! expect(described_class::MIGRATION).to be_scheduled_migration expect(BackgroundMigrationWorker.jobs.size).to eq(1) end - - it 'ensures the untracked_files_for_uploads table exists' do - expect do - migrate! - end.to change { table_exists?(:untracked_files_for_uploads) }.from(false).to(true) - end - - it 'has a path field long enough for really long paths' do - migrate! - - component = 'a' * 255 - - long_path = [ - 'uploads', - component, # project.full_path - component # filename - ].flatten.join('/') - - record = untracked_files_for_uploads.create!(path: long_path) - expect(record.reload.path.size).to eq(519) - end end context 'with tracked and untracked uploads' do diff --git a/spec/support/track_untracked_uploads_helpers.rb b/spec/support/track_untracked_uploads_helpers.rb index bb700bc53f1..4d4745fd7f4 100644 --- a/spec/support/track_untracked_uploads_helpers.rb +++ b/spec/support/track_untracked_uploads_helpers.rb @@ -4,17 +4,11 @@ module TrackUntrackedUploadsHelpers fixture_file_upload(fixture_path) end - def recreate_temp_table_if_dropped - TrackUntrackedUploads.new.ensure_temporary_tracking_table_exists + def ensure_temporary_tracking_table_exists + Gitlab::BackgroundMigration::PrepareUntrackedUploads.new.send(:ensure_temporary_tracking_table_exists) end - RSpec.configure do |config| - config.after(:each, :temp_table_may_drop) do - recreate_temp_table_if_dropped - end - - config.after(:context, :temp_table_may_drop) do - recreate_temp_table_if_dropped - end + def drop_temp_table_if_exists + ActiveRecord::Base.connection.drop_table(:untracked_files_for_uploads) if ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads) end end -- cgit v1.2.1 From f5fa39872402c7f27307ffe9bd769c4f120a0f2e Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 16 Nov 2017 16:26:40 -0800 Subject: Attempt to fix spec in CI --- spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index 042fdead281..b6b046ec3aa 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -21,7 +21,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side drop_temp_table_if_exists end - after(:all) do + after do drop_temp_table_if_exists end -- cgit v1.2.1 From edb5cac46c1cba1029fb3e67d4853027590584f6 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Mon, 20 Nov 2017 16:27:24 -0800 Subject: Use bulk inserts --- .../prepare_untracked_uploads_spec.rb | 192 ++++++++++++++++----- spec/migrations/track_untracked_uploads_spec.rb | 8 +- 2 files changed, 149 insertions(+), 51 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index b6b046ec3aa..f1eb7173717 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -53,80 +53,178 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side expect(record.reload.path.size).to eq(519) end - context 'when files were uploaded before and after hashed storage was enabled' do - let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } - let!(:user) { create(:user, :with_avatar) } - let!(:project1) { create(:project, :with_avatar) } - let(:project2) { create(:project) } # instantiate after enabling hashed_storage + context "test bulk insert with ON CONFLICT DO NOTHING or IGNORE" do + around do |example| + # If this is CI, we use Postgres 9.2 so this whole context should be + # skipped since we're unable to use ON CONFLICT DO NOTHING or IGNORE. + if described_class.new.send(:can_bulk_insert_and_ignore_duplicates?) + example.run + end + end - before do - # Markdown upload before enabling hashed_storage - UploadService.new(project1, uploaded_file, FileUploader).execute + context 'when files were uploaded before and after hashed storage was enabled' do + let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } + let!(:user) { create(:user, :with_avatar) } + let!(:project1) { create(:project, :with_avatar) } + let(:project2) { create(:project) } # instantiate after enabling hashed_storage - stub_application_setting(hashed_storage_enabled: true) + before do + # Markdown upload before enabling hashed_storage + UploadService.new(project1, uploaded_file, FileUploader).execute - # Markdown upload after enabling hashed_storage - UploadService.new(project2, uploaded_file, FileUploader).execute - end + stub_application_setting(hashed_storage_enabled: true) - it 'adds unhashed files to the untracked_files_for_uploads table' do - described_class.new.perform + # Markdown upload after enabling hashed_storage + UploadService.new(project2, uploaded_file, FileUploader).execute + end - expect(untracked_files_for_uploads.count).to eq(5) - end + it 'adds unhashed files to the untracked_files_for_uploads table' do + described_class.new.perform - it 'adds files with paths relative to CarrierWave.root' do - described_class.new.perform - untracked_files_for_uploads.all.each do |file| - expect(file.path.start_with?('uploads/')).to be_truthy + expect(untracked_files_for_uploads.count).to eq(5) end - end - it 'does not add hashed files to the untracked_files_for_uploads table' do - described_class.new.perform - - hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path - expect(untracked_files_for_uploads.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey - end + it 'adds files with paths relative to CarrierWave.root' do + described_class.new.perform + untracked_files_for_uploads.all.each do |file| + expect(file.path.start_with?('uploads/')).to be_truthy + end + end - it 'correctly schedules the follow-up background migration jobs' do - described_class.new.perform + it 'does not add hashed files to the untracked_files_for_uploads table' do + described_class.new.perform - expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5) - expect(BackgroundMigrationWorker.jobs.size).to eq(1) - end + hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path + expect(untracked_files_for_uploads.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey + end - # E.g. from a previous failed run of this background migration - context 'when there is existing data in untracked_files_for_uploads' do - before do + it 'correctly schedules the follow-up background migration jobs' do described_class.new.perform + + expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5) + expect(BackgroundMigrationWorker.jobs.size).to eq(1) end - it 'does not error or produce duplicates of existing data' do - expect do + # E.g. from a previous failed run of this background migration + context 'when there is existing data in untracked_files_for_uploads' do + before do described_class.new.perform - end.not_to change { untracked_files_for_uploads.count }.from(5) + end + + it 'does not error or produce duplicates of existing data' do + expect do + described_class.new.perform + end.not_to change { untracked_files_for_uploads.count }.from(5) + end end + + # E.g. The installation is in use at the time of migration, and someone has + # just uploaded a file + context 'when there are files in /uploads/tmp' do + let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') } + + before do + FileUtils.touch(tmp_file) + end + + after do + FileUtils.rm(tmp_file) + end + + it 'does not add files from /uploads/tmp' do + described_class.new.perform + + expect(untracked_files_for_uploads.count).to eq(5) + end + end + end + end + + context 'test bulk insert without ON CONFLICT DO NOTHING or IGNORE' do + before do + # If this is CI, we use Postgres 9.2 so this stub has no effect. + # + # If this is being run on Postgres 9.5+ or MySQL, then this stub allows us + # to test the bulk insert functionality without ON CONFLICT DO NOTHING or + # IGNORE. + allow_any_instance_of(described_class).to receive(:postgresql_pre_9_5?).and_return(true) end - # E.g. The installation is in use at the time of migration, and someone has - # just uploaded a file - context 'when there are files in /uploads/tmp' do - let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') } + context 'when files were uploaded before and after hashed storage was enabled' do + let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } + let!(:user) { create(:user, :with_avatar) } + let!(:project1) { create(:project, :with_avatar) } + let(:project2) { create(:project) } # instantiate after enabling hashed_storage before do - FileUtils.touch(tmp_file) - end + # Markdown upload before enabling hashed_storage + UploadService.new(project1, uploaded_file, FileUploader).execute - after do - FileUtils.rm(tmp_file) + stub_application_setting(hashed_storage_enabled: true) + + # Markdown upload after enabling hashed_storage + UploadService.new(project2, uploaded_file, FileUploader).execute end - it 'does not add files from /uploads/tmp' do + it 'adds unhashed files to the untracked_files_for_uploads table' do described_class.new.perform expect(untracked_files_for_uploads.count).to eq(5) end + + it 'adds files with paths relative to CarrierWave.root' do + described_class.new.perform + untracked_files_for_uploads.all.each do |file| + expect(file.path.start_with?('uploads/')).to be_truthy + end + end + + it 'does not add hashed files to the untracked_files_for_uploads table' do + described_class.new.perform + + hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path + expect(untracked_files_for_uploads.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey + end + + it 'correctly schedules the follow-up background migration jobs' do + described_class.new.perform + + expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5) + expect(BackgroundMigrationWorker.jobs.size).to eq(1) + end + + # E.g. from a previous failed run of this background migration + context 'when there is existing data in untracked_files_for_uploads' do + before do + described_class.new.perform + end + + it 'does not error or produce duplicates of existing data' do + expect do + described_class.new.perform + end.not_to change { untracked_files_for_uploads.count }.from(5) + end + end + + # E.g. The installation is in use at the time of migration, and someone has + # just uploaded a file + context 'when there are files in /uploads/tmp' do + let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') } + + before do + FileUtils.touch(tmp_file) + end + + after do + FileUtils.rm(tmp_file) + end + + it 'does not add files from /uploads/tmp' do + described_class.new.perform + + expect(untracked_files_for_uploads.count).to eq(5) + end + end end end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 11824bebb91..01bfe26744f 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -62,8 +62,8 @@ describe TrackUntrackedUploads, :migration, :sidekiq do expect(appearance.reload.uploads.where("path like '%/header_logo/%'").first.attributes).to include(@appearance_header_logo_attributes) expect(user2.reload.uploads.first.attributes).to include(@user2_avatar_attributes) - expect(project2.reload.uploads.first.attributes).to include(@project2_avatar_attributes) - expect(project2.uploads.last.attributes).to include(@project2_markdown_attributes) + expect(project2.reload.uploads.where(uploader: 'AvatarUploader').first.attributes).to include(@project2_avatar_attributes) + expect(project2.uploads.where(uploader: 'FileUploader').first.attributes).to include(@project2_markdown_attributes) end it 'ignores already-tracked uploads' do @@ -71,8 +71,8 @@ describe TrackUntrackedUploads, :migration, :sidekiq do expect(appearance.reload.uploads.where("path like '%/logo/%'").first.attributes).to include(@appearance_logo_attributes) expect(user1.reload.uploads.first.attributes).to include(@user1_avatar_attributes) - expect(project1.reload.uploads.first.attributes).to include(@project1_avatar_attributes) - expect(project1.uploads.last.attributes).to include(@project1_markdown_attributes) + expect(project1.reload.uploads.where(uploader: 'AvatarUploader').first.attributes).to include(@project1_avatar_attributes) + expect(project1.uploads.where(uploader: 'FileUploader').first.attributes).to include(@project1_markdown_attributes) end it 'the temporary table untracked_files_for_uploads no longer exists' do -- cgit v1.2.1 From 67b58ffdc357bb94ec62d696b7b9a4aecf751c75 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 22 Nov 2017 10:44:33 -0800 Subject: Get rid of tracked field It makes a debugging slightly easier, but is not necessary, and is a waste of resources. --- .../populate_untracked_uploads_spec.rb | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 317890bd854..04719d50f5c 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -54,12 +54,12 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid expect(appearance.uploads.count).to eq(2) end - it 'sets all added or confirmed tracked files to tracked' do + it 'deletes rows after processing them' do expect(subject).to receive(:drop_temp_table_if_finished) # Don't drop the table so we can look at it expect do subject.perform(1, 1000) - end.to change { untracked_files_for_uploads.where(tracked: true).count }.from(0).to(8) + end.to change { untracked_files_for_uploads.count }.from(8).to(0) end it 'does not create duplicate uploads of already tracked files' do @@ -85,7 +85,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid expect(project2.uploads.count).to eq(0) # Only 4 have been either confirmed or added to uploads - expect(untracked_files_for_uploads.where(tracked: true).count).to eq(4) + expect(untracked_files_for_uploads.count).to eq(4) end it 'uses the start and end batch ids [only 2nd half]' do @@ -103,7 +103,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid expect(project2.uploads.count).to eq(2) # Only 4 have been either confirmed or added to uploads - expect(untracked_files_for_uploads.where(tracked: true).count).to eq(4) + expect(untracked_files_for_uploads.count).to eq(4) end it 'does not drop the temporary tracking table after processing the batch, if there are still untracked rows' do @@ -254,18 +254,6 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end end - describe '#mark_as_tracked' do - it 'saves the record with tracked set to true' do - untracked_file = create_untracked_file("/-/system/appearance/logo/1/some_logo.jpg") - - expect do - untracked_file.mark_as_tracked - end.to change { untracked_file.tracked }.from(false).to(true) - - expect(untracked_file.persisted?).to be_truthy - end - end - describe '#upload_path' do def assert_upload_path(file_path, expected_upload_path) untracked_file = create_untracked_file(file_path) -- cgit v1.2.1 From 7549d17f721b3be84f83c1dfa491d6a2ebf4ec28 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 22 Nov 2017 12:19:25 -0800 Subject: Refactor --- .../gitlab/background_migration/populate_untracked_uploads_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 04719d50f5c..72243ff98a4 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -166,7 +166,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do end end - describe '#add_to_uploads' do + describe '#add_to_uploads_if_needed' do shared_examples_for 'add_to_uploads_non_markdown_files' do let!(:expected_upload_attrs) { model.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') } let!(:untracked_file) { described_class.create!(path: expected_upload_attrs['path']) } @@ -177,7 +177,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do it 'creates an Upload record' do expect do - untracked_file.add_to_uploads + untracked_file.add_to_uploads_if_needed end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include(expected_upload_attrs) @@ -246,7 +246,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do it 'creates an Upload record' do expect do - untracked_file.add_to_uploads + untracked_file.add_to_uploads_if_needed end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include(@expected_upload_attrs) -- cgit v1.2.1 From 908aacddda571bc82c722798f399fd5a68ebe1da Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 23 Nov 2017 23:12:24 -0800 Subject: Filter existing uploads with one query --- .../populate_untracked_uploads_spec.rb | 80 +++++++--------------- 1 file changed, 26 insertions(+), 54 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 72243ff98a4..623725bffca 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -126,50 +126,11 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end.not_to change { uploads.count }.from(0) end end -end - -describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do - include TrackUntrackedUploadsHelpers - - let(:upload_class) { Gitlab::BackgroundMigration::PopulateUntrackedUploads::Upload } - - before(:all) do - ensure_temporary_tracking_table_exists - end - - after(:all) do - drop_temp_table_if_exists - end - describe '#ensure_tracked!' do - let!(:user1) { create(:user, :with_avatar) } - let!(:untracked_file) { described_class.create!(path: user1.uploads.first.path) } - - context 'when the file is already in the uploads table' do - it 'does not add an upload' do - expect do - untracked_file.ensure_tracked! - end.not_to change { upload_class.count }.from(1) - end - end - - context 'when the file is not already in the uploads table' do - before do - user1.uploads.delete_all - end - - it 'adds an upload' do - expect do - untracked_file.ensure_tracked! - end.to change { upload_class.count }.from(0).to(1) - end - end - end - - describe '#add_to_uploads_if_needed' do - shared_examples_for 'add_to_uploads_non_markdown_files' do + describe 'upload outcomes for each path pattern' do + shared_examples_for 'non_markdown_file' do let!(:expected_upload_attrs) { model.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') } - let!(:untracked_file) { described_class.create!(path: expected_upload_attrs['path']) } + let!(:untracked_file) { untracked_files_for_uploads.create!(path: expected_upload_attrs['path']) } before do model.uploads.delete_all @@ -177,7 +138,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do it 'creates an Upload record' do expect do - untracked_file.add_to_uploads_if_needed + subject.perform(1, 1000) end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include(expected_upload_attrs) @@ -187,13 +148,13 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for an appearance logo file path' do let(:model) { create(:appearance, logo: uploaded_file) } - it_behaves_like 'add_to_uploads_non_markdown_files' + it_behaves_like 'non_markdown_file' end context 'for an appearance header_logo file path' do let(:model) { create(:appearance, header_logo: uploaded_file) } - it_behaves_like 'add_to_uploads_non_markdown_files' + it_behaves_like 'non_markdown_file' end context 'for a pre-Markdown Note attachment file path' do @@ -203,39 +164,36 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do let(:model) { create(:note, :with_attachment) } - it_behaves_like 'add_to_uploads_non_markdown_files' + it_behaves_like 'non_markdown_file' end context 'for a user avatar file path' do let(:model) { create(:user, :with_avatar) } - it_behaves_like 'add_to_uploads_non_markdown_files' + it_behaves_like 'non_markdown_file' end context 'for a group avatar file path' do let(:model) { create(:group, :with_avatar) } - it_behaves_like 'add_to_uploads_non_markdown_files' + it_behaves_like 'non_markdown_file' end context 'for a project avatar file path' do let(:model) { create(:project, :with_avatar) } - it_behaves_like 'add_to_uploads_non_markdown_files' + it_behaves_like 'non_markdown_file' end context 'for a project Markdown attachment (notes, issues, MR descriptions) file path' do let(:model) { create(:project) } - # UntrackedFile.path is different than Upload.path - let(:untracked_file) { create_untracked_file("/#{model.full_path}/#{model.uploads.first.path}") } - before do # Upload the file UploadService.new(model, uploaded_file, FileUploader).execute # Create the untracked_files_for_uploads record - untracked_file + untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{model.full_path}/#{model.uploads.first.path}") # Save the expected upload attributes @expected_upload_attrs = model.reload.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') @@ -246,13 +204,27 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do it 'creates an Upload record' do expect do - untracked_file.add_to_uploads_if_needed + subject.perform(1, 1000) end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include(@expected_upload_attrs) end end end +end + +describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do + include TrackUntrackedUploadsHelpers + + let(:upload_class) { Gitlab::BackgroundMigration::PopulateUntrackedUploads::Upload } + + before(:all) do + ensure_temporary_tracking_table_exists + end + + after(:all) do + drop_temp_table_if_exists + end describe '#upload_path' do def assert_upload_path(file_path, expected_upload_path) -- cgit v1.2.1 From 473ddfb453d820f1a32fb48477e17ba45bdbd2f0 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Fri, 24 Nov 2017 00:49:04 -0800 Subject: =?UTF-8?q?Don=E2=80=99t=20recreate=20deleted=20uploads?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../populate_untracked_uploads_spec.rb | 14 +++++++------- spec/migrations/track_untracked_uploads_spec.rb | 9 +++++++++ 2 files changed, 16 insertions(+), 7 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 623725bffca..85da8ac5b1e 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -392,37 +392,37 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do context 'for an appearance logo file path' do it 'returns the ID as a string' do - assert_model_id('/-/system/appearance/logo/1/some_logo.jpg', '1') + assert_model_id('/-/system/appearance/logo/1/some_logo.jpg', 1) end end context 'for an appearance header_logo file path' do it 'returns the ID as a string' do - assert_model_id('/-/system/appearance/header_logo/1/some_logo.jpg', '1') + assert_model_id('/-/system/appearance/header_logo/1/some_logo.jpg', 1) end end context 'for a pre-Markdown Note attachment file path' do it 'returns the ID as a string' do - assert_model_id('/-/system/note/attachment/1234/some_attachment.pdf', '1234') + assert_model_id('/-/system/note/attachment/1234/some_attachment.pdf', 1234) end end context 'for a user avatar file path' do it 'returns the ID as a string' do - assert_model_id('/-/system/user/avatar/1234/avatar.jpg', '1234') + assert_model_id('/-/system/user/avatar/1234/avatar.jpg', 1234) end end context 'for a group avatar file path' do it 'returns the ID as a string' do - assert_model_id('/-/system/group/avatar/1234/avatar.jpg', '1234') + assert_model_id('/-/system/group/avatar/1234/avatar.jpg', 1234) end end context 'for a project avatar file path' do it 'returns the ID as a string' do - assert_model_id('/-/system/project/avatar/1234/avatar.jpg', '1234') + assert_model_id('/-/system/project/avatar/1234/avatar.jpg', 1234) end end @@ -430,7 +430,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do it 'returns the ID as a string' do project = create(:project) - assert_model_id("/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg", project.id.to_s) + assert_model_id("/#{project.full_path}/#{SecureRandom.hex}/Some file.jpg", project.id) end end end diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 01bfe26744f..9fa586ff177 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -75,6 +75,15 @@ describe TrackUntrackedUploads, :migration, :sidekiq do expect(project1.uploads.where(uploader: 'FileUploader').first.attributes).to include(@project1_markdown_attributes) end + it 'ignores uploads for deleted models' do + user2.destroy + project2.destroy + + expect do + migrate! + end.to change { uploads.count }.from(4).to(5) + end + it 'the temporary table untracked_files_for_uploads no longer exists' do migrate! -- cgit v1.2.1 From dd4b35f864e43b94532c2229e0135eaa47ddc9fe Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Sun, 26 Nov 2017 20:57:51 -0800 Subject: Fix test for MySQL --- .../background_migration/populate_untracked_uploads_spec.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 85da8ac5b1e..7c69755c4cf 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -71,8 +71,9 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end it 'uses the start and end batch ids [only 1st half]' do - start_id = untracked_files_for_uploads.all.to_a[0].id - end_id = untracked_files_for_uploads.all.to_a[3].id + ids = untracked_files_for_uploads.all.order(:id).pluck(:id) + start_id = ids[0] + end_id = ids[3] expect do subject.perform(start_id, end_id) @@ -89,8 +90,9 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end it 'uses the start and end batch ids [only 2nd half]' do - start_id = untracked_files_for_uploads.all.to_a[4].id - end_id = untracked_files_for_uploads.all.to_a[7].id + ids = untracked_files_for_uploads.all.order(:id).pluck(:id) + start_id = ids[4] + end_id = ids[7] expect do subject.perform(start_id, end_id) -- cgit v1.2.1 From 71d27044189bf114904e798017e541697acad8e9 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Sun, 26 Nov 2017 23:10:51 -0800 Subject: Add tests for disable_quote option --- spec/lib/gitlab/database_spec.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'spec') diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index fcddfad3f9f..8872bf7fc87 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -199,6 +199,22 @@ describe Gitlab::Database do described_class.bulk_insert('test', rows) end + it 'does not quote values of a column in the disable_quote option' do + [1, 2, 4, 5].each do |i| + expect(connection).to receive(:quote).with(i) + end + + described_class.bulk_insert('test', rows, disable_quote: :c) + end + + it 'does not quote values of columns in the disable_quote option' do + [2, 5].each do |i| + expect(connection).to receive(:quote).with(i) + end + + described_class.bulk_insert('test', rows, disable_quote: [:a, :c]) + end + it 'handles non-UTF-8 data' do expect { described_class.bulk_insert('test', [{ a: "\255" }]) }.not_to raise_error end -- cgit v1.2.1 From 602f6bc89c9464fabab827b833133439af26a3c4 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Mon, 4 Dec 2017 12:58:02 -0800 Subject: =?UTF-8?q?Make=20sure=20empty=20uploads=20doesn=E2=80=99t=20break?= =?UTF-8?q?=20anything?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- spec/migrations/track_untracked_uploads_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'spec') diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index 9fa586ff177..c9bbc09cc49 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -5,6 +5,7 @@ describe TrackUntrackedUploads, :migration, :sidekiq do include TrackUntrackedUploadsHelpers let(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } + let(:uploads) { table(:uploads) } matcher :be_scheduled_migration do match do |migration| @@ -33,7 +34,6 @@ describe TrackUntrackedUploads, :migration, :sidekiq do let!(:user2) { create(:user, :with_avatar) } let!(:project1) { create(:project, :with_avatar) } let!(:project2) { create(:project, :with_avatar) } - let(:uploads) { table(:uploads) } before do UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload @@ -90,4 +90,12 @@ describe TrackUntrackedUploads, :migration, :sidekiq do expect(table_exists?(:untracked_files_for_uploads)).to be_falsey end end + + context 'without any uploads ever' do + it 'does not add any upload records' do + expect do + migrate! + end.not_to change { uploads.count }.from(0) + end + end end -- cgit v1.2.1 From 77be7efcf58e0c6cf7a16c249161c0c791c545e1 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 5 Dec 2017 10:43:08 -0800 Subject: Guarantee all IDs are included --- .../background_migration/populate_untracked_uploads_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 7c69755c4cf..35ea8059510 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -46,7 +46,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid it 'adds untracked files to the uploads table' do expect do - subject.perform(1, 1000) + subject.perform(1, untracked_files_for_uploads.last.id) end.to change { uploads.count }.from(4).to(8) expect(user2.uploads.count).to eq(1) @@ -58,12 +58,12 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid expect(subject).to receive(:drop_temp_table_if_finished) # Don't drop the table so we can look at it expect do - subject.perform(1, 1000) + subject.perform(1, untracked_files_for_uploads.last.id) end.to change { untracked_files_for_uploads.count }.from(8).to(0) end it 'does not create duplicate uploads of already tracked files' do - subject.perform(1, 1000) + subject.perform(1, untracked_files_for_uploads.last.id) expect(user1.uploads.count).to eq(1) expect(project1.uploads.count).to eq(2) @@ -140,7 +140,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid it 'creates an Upload record' do expect do - subject.perform(1, 1000) + subject.perform(1, untracked_files_for_uploads.last.id) end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include(expected_upload_attrs) @@ -206,7 +206,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid it 'creates an Upload record' do expect do - subject.perform(1, 1000) + subject.perform(1, untracked_files_for_uploads.last.id) end.to change { model.reload.uploads.count }.from(0).to(1) expect(model.uploads.first.attributes).to include(@expected_upload_attrs) -- cgit v1.2.1 From 869d08b581495161352a661ac29b20b3925deaf0 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 5 Dec 2017 12:26:20 -0800 Subject: Process normal paths in batch containing bad paths --- .../populate_untracked_uploads_spec.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 35ea8059510..e1a5a17a60c 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -119,6 +119,26 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid expect(table_exists?(:untracked_files_for_uploads)).to be_falsey end + + it 'does not block a whole batch because of one bad path' do + untracked_files_for_uploads.create!(path: "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project2.full_path}/._7d37bf4c747916390e596744117d5d1a") + expect(untracked_files_for_uploads.count).to eq(9) + expect(uploads.count).to eq(4) + + subject.perform(1, untracked_files_for_uploads.last.id) + + expect(untracked_files_for_uploads.count).to eq(1) + expect(uploads.count).to eq(8) + end + + it 'an unparseable path is shown in error output' do + bad_path = "#{Gitlab::BackgroundMigration::PrepareUntrackedUploads::RELATIVE_UPLOAD_DIR}/#{project2.full_path}/._7d37bf4c747916390e596744117d5d1a" + untracked_files_for_uploads.create!(path: bad_path) + + expect(Rails.logger).to receive(:error).with(/Error parsing path "#{bad_path}":/) + + subject.perform(1, untracked_files_for_uploads.last.id) + end end context 'with no untracked files' do -- cgit v1.2.1 From 03cba8c0f18f18a14453b17c9f4fa300547f0ab5 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 5 Dec 2017 23:08:45 -0800 Subject: Fix specs after rebase Later migrations added fields to the EE DB which were used by factories which were used in these specs. And in CE on MySQL, a single appearance row is enforced. The migration and migration specs should not depend on the codebase staying the same. --- .../populate_untracked_uploads_spec.rb | 19 +++--- .../prepare_untracked_uploads_spec.rb | 10 +-- spec/migrations/track_untracked_uploads_spec.rb | 74 ---------------------- spec/support/track_untracked_uploads_helpers.rb | 6 ++ 4 files changed, 20 insertions(+), 89 deletions(-) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index e1a5a17a60c..9c1261881df 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -1,13 +1,12 @@ require 'spec_helper' -require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads') -describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sidekiq, schema: 20171103140253 do +describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq do include TrackUntrackedUploadsHelpers subject { described_class.new } - let!(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } - let!(:uploads) { table(:uploads) } + let!(:untracked_files_for_uploads) { described_class::UntrackedFile } + let!(:uploads) { described_class::Upload } before do ensure_temporary_tracking_table_exists @@ -18,7 +17,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end context 'with untracked files and tracked files in untracked_files_for_uploads' do - let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } + let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) } let!(:user1) { create(:user, :with_avatar) } let!(:user2) { create(:user, :with_avatar) } let!(:project1) { create(:project, :with_avatar) } @@ -111,13 +110,13 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid it 'does not drop the temporary tracking table after processing the batch, if there are still untracked rows' do subject.perform(1, untracked_files_for_uploads.last.id - 1) - expect(table_exists?(:untracked_files_for_uploads)).to be_truthy + expect(ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads)).to be_truthy end it 'drops the temporary tracking table after processing the batch, if there are no untracked rows left' do subject.perform(1, untracked_files_for_uploads.last.id) - expect(table_exists?(:untracked_files_for_uploads)).to be_falsey + expect(ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads)).to be_falsey end it 'does not block a whole batch because of one bad path' do @@ -168,13 +167,13 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid end context 'for an appearance logo file path' do - let(:model) { create(:appearance, logo: uploaded_file) } + let(:model) { create_or_update_appearance(logo: uploaded_file) } it_behaves_like 'non_markdown_file' end context 'for an appearance header_logo file path' do - let(:model) { create(:appearance, header_logo: uploaded_file) } + let(:model) { create_or_update_appearance(header_logo: uploaded_file) } it_behaves_like 'non_markdown_file' end @@ -459,7 +458,7 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do describe '#file_size' do context 'for an appearance logo file path' do - let(:appearance) { create(:appearance, logo: uploaded_file) } + let(:appearance) { create_or_update_appearance(logo: uploaded_file) } let(:untracked_file) { described_class.create!(path: appearance.uploads.first.path) } it 'returns the file size' do diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index f1eb7173717..dfe37e43751 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -1,9 +1,9 @@ require 'spec_helper' -describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :sidekiq, schema: 20171103140253 do +describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do include TrackUntrackedUploadsHelpers - let!(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } + let!(:untracked_files_for_uploads) { described_class::UntrackedFile } matcher :be_scheduled_migration do |*expected| match do |migration| @@ -35,7 +35,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side it 'ensures the untracked_files_for_uploads table exists' do expect do described_class.new.perform - end.to change { table_exists?(:untracked_files_for_uploads) }.from(false).to(true) + end.to change { ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads) }.from(false).to(true) end it 'has a path field long enough for really long paths' do @@ -63,7 +63,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end context 'when files were uploaded before and after hashed storage was enabled' do - let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } + let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) } let!(:user) { create(:user, :with_avatar) } let!(:project1) { create(:project, :with_avatar) } let(:project2) { create(:project) } # instantiate after enabling hashed_storage @@ -151,7 +151,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :migration, :side end context 'when files were uploaded before and after hashed storage was enabled' do - let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } + let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) } let!(:user) { create(:user, :with_avatar) } let!(:project1) { create(:project, :with_avatar) } let(:project2) { create(:project) } # instantiate after enabling hashed_storage diff --git a/spec/migrations/track_untracked_uploads_spec.rb b/spec/migrations/track_untracked_uploads_spec.rb index c9bbc09cc49..7fe7a140e2f 100644 --- a/spec/migrations/track_untracked_uploads_spec.rb +++ b/spec/migrations/track_untracked_uploads_spec.rb @@ -4,9 +4,6 @@ require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_up describe TrackUntrackedUploads, :migration, :sidekiq do include TrackUntrackedUploadsHelpers - let(:untracked_files_for_uploads) { table(:untracked_files_for_uploads) } - let(:uploads) { table(:uploads) } - matcher :be_scheduled_migration do match do |migration| BackgroundMigrationWorker.jobs.any? do |job| @@ -27,75 +24,4 @@ describe TrackUntrackedUploads, :migration, :sidekiq do expect(BackgroundMigrationWorker.jobs.size).to eq(1) end end - - context 'with tracked and untracked uploads' do - let!(:appearance) { create(:appearance, logo: uploaded_file, header_logo: uploaded_file) } - let!(:user1) { create(:user, :with_avatar) } - let!(:user2) { create(:user, :with_avatar) } - let!(:project1) { create(:project, :with_avatar) } - let!(:project2) { create(:project, :with_avatar) } - - before do - UploadService.new(project1, uploaded_file, FileUploader).execute # Markdown upload - UploadService.new(project2, uploaded_file, FileUploader).execute # Markdown upload - - # Save expected Upload attributes - @appearance_logo_attributes = appearance.uploads.where("path like '%/logo/%'").first.attributes.slice('path', 'uploader', 'size', 'checksum') - @appearance_header_logo_attributes = appearance.uploads.where("path like '%/header_logo/%'").first.attributes.slice('path', 'uploader', 'size', 'checksum') - @user1_avatar_attributes = user1.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') - @user2_avatar_attributes = user2.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') - @project1_avatar_attributes = project1.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') - @project2_avatar_attributes = project2.uploads.first.attributes.slice('path', 'uploader', 'size', 'checksum') - @project1_markdown_attributes = project1.uploads.last.attributes.slice('path', 'uploader', 'size', 'checksum') - @project2_markdown_attributes = project2.uploads.last.attributes.slice('path', 'uploader', 'size', 'checksum') - - # Untrack 4 files - user2.uploads.delete_all - project2.uploads.delete_all # 2 files: avatar and a Markdown upload - appearance.uploads.where("path like '%header_logo%'").delete_all - end - - it 'tracks untracked uploads' do - expect do - migrate! - end.to change { uploads.count }.from(4).to(8) - - expect(appearance.reload.uploads.where("path like '%/header_logo/%'").first.attributes).to include(@appearance_header_logo_attributes) - expect(user2.reload.uploads.first.attributes).to include(@user2_avatar_attributes) - expect(project2.reload.uploads.where(uploader: 'AvatarUploader').first.attributes).to include(@project2_avatar_attributes) - expect(project2.uploads.where(uploader: 'FileUploader').first.attributes).to include(@project2_markdown_attributes) - end - - it 'ignores already-tracked uploads' do - migrate! - - expect(appearance.reload.uploads.where("path like '%/logo/%'").first.attributes).to include(@appearance_logo_attributes) - expect(user1.reload.uploads.first.attributes).to include(@user1_avatar_attributes) - expect(project1.reload.uploads.where(uploader: 'AvatarUploader').first.attributes).to include(@project1_avatar_attributes) - expect(project1.uploads.where(uploader: 'FileUploader').first.attributes).to include(@project1_markdown_attributes) - end - - it 'ignores uploads for deleted models' do - user2.destroy - project2.destroy - - expect do - migrate! - end.to change { uploads.count }.from(4).to(5) - end - - it 'the temporary table untracked_files_for_uploads no longer exists' do - migrate! - - expect(table_exists?(:untracked_files_for_uploads)).to be_falsey - end - end - - context 'without any uploads ever' do - it 'does not add any upload records' do - expect do - migrate! - end.not_to change { uploads.count }.from(0) - end - end end diff --git a/spec/support/track_untracked_uploads_helpers.rb b/spec/support/track_untracked_uploads_helpers.rb index 4d4745fd7f4..d05eda08201 100644 --- a/spec/support/track_untracked_uploads_helpers.rb +++ b/spec/support/track_untracked_uploads_helpers.rb @@ -11,4 +11,10 @@ module TrackUntrackedUploadsHelpers def drop_temp_table_if_exists ActiveRecord::Base.connection.drop_table(:untracked_files_for_uploads) if ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads) end + + def create_or_update_appearance(attrs) + a = Appearance.first_or_initialize(title: 'foo', description: 'bar') + a.update!(attrs) + a + end end -- cgit v1.2.1 From 24c348f0d1270fe27268aa23e034473651b0cdf9 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 6 Dec 2017 12:20:06 -0800 Subject: Fix specs for MySQL --- .../lib/gitlab/background_migration/populate_untracked_uploads_spec.rb | 3 +++ spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb | 2 ++ 2 files changed, 5 insertions(+) (limited to 'spec') diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb index 9c1261881df..b80df6956b0 100644 --- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb @@ -9,7 +9,10 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq do let!(:uploads) { described_class::Upload } before do + DatabaseCleaner.clean + drop_temp_table_if_exists ensure_temporary_tracking_table_exists + uploads.delete_all end after(:all) do diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index dfe37e43751..cd3f1a45270 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -18,6 +18,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do end before do + DatabaseCleaner.clean + drop_temp_table_if_exists end -- cgit v1.2.1