diff options
Diffstat (limited to 'spec/services/pages')
4 files changed, 226 insertions, 38 deletions
diff --git a/spec/services/pages/delete_services_spec.rb b/spec/services/pages/delete_services_spec.rb index 440549020a2..f1edf93b0c1 100644 --- a/spec/services/pages/delete_services_spec.rb +++ b/spec/services/pages/delete_services_spec.rb @@ -3,35 +3,74 @@ require 'spec_helper' RSpec.describe Pages::DeleteService do - shared_examples 'remove pages' do - let_it_be(:project) { create(:project, path: "my.project")} - let_it_be(:admin) { create(:admin) } - let_it_be(:domain) { create(:pages_domain, project: project) } - let_it_be(:service) { described_class.new(project, admin)} + let_it_be(:admin) { create(:admin) } - it 'deletes published pages' do - expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true - expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything) + let(:project) { create(:project, path: "my.project")} + let!(:domain) { create(:pages_domain, project: project) } + let(:service) { described_class.new(project, admin)} - Sidekiq::Testing.inline! { service.execute } + before do + project.mark_pages_as_deployed + end + + it 'deletes published pages', :sidekiq_inline do + expect(project.pages_deployed?).to be(true) + + expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true + expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything) + + service.execute + + expect(project.pages_deployed?).to be(false) + end + + it "doesn't remove anything from the legacy storage if updates on it are disabled", :sidekiq_inline do + stub_feature_flags(pages_update_legacy_storage: false) + + expect(project.pages_deployed?).to be(true) + + expect(PagesWorker).not_to receive(:perform_in) + + service.execute - expect(project.reload.pages_metadatum.deployed?).to be(false) - end + expect(project.pages_deployed?).to be(false) + end + + it 'deletes all domains', :sidekiq_inline do + expect(project.pages_domains.count).to eq(1) + + service.execute + + expect(project.reload.pages_domains.count).to eq(0) + end - it 'deletes all domains' do - expect(project.pages_domains.count).to be 1 + it 'schedules a destruction of pages deployments' do + expect(DestroyPagesDeploymentsWorker).to( + receive(:perform_async).with(project.id) + ) - Sidekiq::Testing.inline! { service.execute } + service.execute + end + + it 'removes pages deployments', :sidekiq_inline do + create(:pages_deployment, project: project) - expect(project.reload.pages_domains.count).to be 0 - end + expect do + service.execute + end.to change { PagesDeployment.count }.by(-1) end - context 'with feature flag enabled' do - before do - expect(PagesRemoveWorker).to receive(:perform_async).and_call_original - end + it 'marks pages as not deployed, deletes domains and schedules worker to remove pages from disk' do + expect(project.pages_deployed?).to eq(true) + expect(project.pages_domains.count).to eq(1) + + service.execute + + expect(project.pages_deployed?).to eq(false) + expect(project.pages_domains.count).to eq(0) + + expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true - it_behaves_like 'remove pages' + Sidekiq::Worker.drain_all end end diff --git a/spec/services/pages/migrate_from_legacy_storage_service_spec.rb b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb new file mode 100644 index 00000000000..4ec57044912 --- /dev/null +++ b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Pages::MigrateFromLegacyStorageService do + let(:service) { described_class.new(Rails.logger, migration_threads: 3, batch_size: 10, ignore_invalid_entries: false) } + + it 'does not try to migrate pages if pages are not deployed' do + expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new) + + expect(service.execute).to eq(migrated: 0, errored: 0) + end + + it 'uses multiple threads' do + projects = create_list(:project, 20) + projects.each do |project| + project.mark_pages_as_deployed + + FileUtils.mkdir_p File.join(project.pages_path, "public") + File.open(File.join(project.pages_path, "public/index.html"), "w") do |f| + f.write("Hello!") + end + end + + service = described_class.new(Rails.logger, migration_threads: 3, batch_size: 2, ignore_invalid_entries: false) + + threads = Concurrent::Set.new + + expect(service).to receive(:migrate_project).exactly(20).times.and_wrap_original do |m, *args| + threads.add(Thread.current) + + # sleep to be 100% certain that once thread can't consume all the queue + # it works without it, but I want to avoid making this test flaky + sleep(0.01) + + m.call(*args) + end + + expect(service.execute).to eq(migrated: 20, errored: 0) + expect(threads.length).to eq(3) + end + + context 'when pages are marked as deployed' do + let(:project) { create(:project) } + + before do + project.mark_pages_as_deployed + end + + context 'when pages directory does not exist' do + it 'tries to migrate the project, but does not crash' do + expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service| + expect(service).to receive(:execute).and_call_original + end + + expect(service.execute).to eq(migrated: 0, errored: 1) + end + end + + context 'when pages directory exists on disk' do + before do + FileUtils.mkdir_p File.join(project.pages_path, "public") + File.open(File.join(project.pages_path, "public/index.html"), "w") do |f| + f.write("Hello!") + end + end + + it 'migrates pages projects without deployments' do + expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service| + expect(service).to receive(:execute).and_call_original + end + + expect do + expect(service.execute).to eq(migrated: 1, errored: 0) + end.to change { project.pages_metadatum.reload.pages_deployment }.from(nil) + end + + context 'when deployed already exists for the project' do + before do + deployment = create(:pages_deployment, project: project) + project.set_first_pages_deployment!(deployment) + end + + it 'does not try to migrate project' do + expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new) + + expect(service.execute).to eq(migrated: 0, errored: 0) + end + end + end + end +end diff --git a/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb b/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb index 29023621413..d95303c3e85 100644 --- a/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb +++ b/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb @@ -6,6 +6,14 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do let(:project) { create(:project, :repository) } let(:service) { described_class.new(project) } + it 'calls ::Pages::ZipDirectoryService' do + expect_next_instance_of(::Pages::ZipDirectoryService, project.pages_path, ignore_invalid_entries: true) do |zip_service| + expect(zip_service).to receive(:execute).and_call_original + end + + expect(described_class.new(project, ignore_invalid_entries: true).execute[:status]).to eq(:error) + end + it 'marks pages as not deployed if public directory is absent' do project.mark_pages_as_deployed diff --git a/spec/services/pages/zip_directory_service_spec.rb b/spec/services/pages/zip_directory_service_spec.rb index dcab6b2dada..9de68dd62bb 100644 --- a/spec/services/pages/zip_directory_service_spec.rb +++ b/spec/services/pages/zip_directory_service_spec.rb @@ -10,8 +10,14 @@ RSpec.describe Pages::ZipDirectoryService do end end + let(:ignore_invalid_entries) { false } + + let(:service) do + described_class.new(@work_dir, ignore_invalid_entries: ignore_invalid_entries) + end + let(:result) do - described_class.new(@work_dir).execute + service.execute end let(:status) { result[:status] } @@ -20,6 +26,8 @@ RSpec.describe Pages::ZipDirectoryService do let(:entries_count) { result[:entries_count] } it 'returns error if project pages dir does not exist' do + expect(Gitlab::ErrorTracking).not_to receive(:track_exception) + expect( described_class.new("/tmp/not/existing/dir").execute ).to eq(status: :error, message: "Can not find valid public dir in /tmp/not/existing/dir") @@ -132,32 +140,69 @@ RSpec.describe Pages::ZipDirectoryService do end end - it 'ignores the symlink pointing outside of public directory' do - create_file("target.html", "hello") - create_link("public/link.html", "../target.html") + shared_examples "raises or ignores file" do |raised_exception, file| + it 'raises error' do + expect do + result + end.to raise_error(raised_exception) + end - with_zip_file do |zip_file| - expect { zip_file.get_entry("public/link.html") }.to raise_error(Errno::ENOENT) + context 'when errors are ignored' do + let(:ignore_invalid_entries) { true } + + it 'does not create entry' do + with_zip_file do |zip_file| + expect { zip_file.get_entry(file) }.to raise_error(Errno::ENOENT) + end + end end end - it 'ignores the symlink if target is absent' do - create_link("public/link.html", "./target.html") + context 'when symlink points outside of public directory' do + before do + create_file("target.html", "hello") + create_link("public/link.html", "../target.html") + end - with_zip_file do |zip_file| - expect { zip_file.get_entry("public/link.html") }.to raise_error(Errno::ENOENT) + include_examples "raises or ignores file", described_class::InvalidEntryError, "public/link.html" + end + + context 'when target of the symlink is absent' do + before do + create_link("public/link.html", "./target.html") end + + include_examples "raises or ignores file", Errno::ENOENT, "public/link.html" end - it 'ignores symlink if is absolute and points to outside of directory' do - target = File.join(@work_dir, "target") - FileUtils.touch(target) + context 'when targets itself' do + before do + create_link("public/link.html", "./link.html") + end - create_link("public/link.html", target) + include_examples "raises or ignores file", Errno::ELOOP, "public/link.html" + end - with_zip_file do |zip_file| - expect { zip_file.get_entry("public/link.html") }.to raise_error(Errno::ENOENT) + context 'when symlink is absolute and points to outside of directory' do + before do + target = File.join(@work_dir, "target") + FileUtils.touch(target) + + create_link("public/link.html", target) end + + include_examples "raises or ignores file", described_class::InvalidEntryError, "public/link.html" + end + + context 'when entry has unknown ftype' do + before do + file = create_file("public/index.html", "hello") + + allow(File).to receive(:lstat).and_call_original + expect(File).to receive(:lstat).with(file) { double("lstat", ftype: "unknown") } + end + + include_examples "raises or ignores file", described_class::InvalidEntryError, "public/index.html" end it "includes raw symlink if it's target is a valid directory" do @@ -204,9 +249,13 @@ RSpec.describe Pages::ZipDirectoryService do end def create_file(name, content) - File.open(File.join(@work_dir, name), "w") do |f| + file_path = File.join(@work_dir, name) + + File.open(file_path, "w") do |f| f.write(content) end + + file_path end def create_dir(dir) |