diff --git a/modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/files_info_query.rb b/modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/files_info_query.rb index 3eee5b468003..25faa2fa58c1 100644 --- a/modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/files_info_query.rb +++ b/modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/files_info_query.rb @@ -33,6 +33,7 @@ module Peripherals module StorageInteraction module Nextcloud class FilesInfoQuery + include TaggedLogging using ServiceResultRefinements FILES_INFO_PATH = "ocs/v1.php/apps/integration_openproject/filesinfo" @@ -46,17 +47,21 @@ def initialize(storage) end def call(auth_strategy:, file_ids:) - if file_ids.nil? - return Util.error(:error, "File IDs can not be nil", file_ids) - end + with_tagged_logger do + if file_ids.nil? + return Util.error(:error, "File IDs can not be nil", file_ids) + end - if file_ids.empty? - return ServiceResult.success(result: []) - end + if file_ids.empty? + return ServiceResult.success(result: []) + end - http_options = Util.ocs_api_request.deep_merge(Util.accept_json) - Authentication[auth_strategy].call(storage: @storage, http_options:) do |http| - files_info(http, file_ids).map(&parse_json) >> handle_failure >> create_storage_file_infos + info "Retrieving file information for #{file_ids.join(', ')}" + http_options = Util.ocs_api_request.deep_merge(Util.accept_json) + Authentication[auth_strategy].call(storage: @storage, http_options:) do |http| + parsed_response = files_info(http, file_ids).on_failure { return _1 }.result + create_storage_file_infos(parsed_response) + end end end @@ -68,7 +73,12 @@ def files_info(http, file_ids) case response in { status: 200..299 } - ServiceResult.success(result: response.body) + json_response = response.json(symbolize_keys: true) + if json_response.dig(:ocs, :meta, :status) == "ok" + ServiceResult.success(result: json_response) + else + Util.error(:error, "Outbound request failed!", error_data) + end in { status: 404 } Util.error(:not_found, "Outbound request destination not found!", error_data) in { status: 401 } @@ -78,57 +88,36 @@ def files_info(http, file_ids) end end - def parse_json - ->(response_body) do - # rubocop:disable Style/OpenStructUse - JSON.parse(response_body, object_class: OpenStruct) - # rubocop:enable Style/OpenStructUse - end - end - - def handle_failure - ->(response_object) do - if response_object.ocs.meta.status == "ok" - ServiceResult.success(result: response_object) - else - error_data = StorageErrorData.new(source: self.class, payload: response_object) - Util.error(:error, "Outbound request failed!", error_data) - end - end - end - # rubocop:disable Metrics/AbcSize - def create_storage_file_infos - ->(response_object) do - ServiceResult.success( - result: response_object.ocs.data.each_pair.map do |key, value| - if value.statuscode == 200 - StorageFileInfo.new( - status: value.status, - status_code: value.statuscode, - id: value.id, - name: value.name, - last_modified_at: Time.zone.at(value.mtime), - created_at: Time.zone.at(value.ctime), - mime_type: value.mimetype, - size: value.size, - owner_name: value.owner_name, - owner_id: value.owner_id, - last_modified_by_name: value.modifier_name, - last_modified_by_id: value.modifier_id, - permissions: value.dav_permissions, - location: location(value.path, value.mimetype) - ) - else - StorageFileInfo.new( - status: value.status, - status_code: value.statuscode, - id: key.to_s.to_i - ) - end + def create_storage_file_infos(parsed_json) + ServiceResult.success( + result: parsed_json.dig(:ocs, :data)&.map do |(key, value)| + if value[:statuscode] == 200 + StorageFileInfo.new( + status: value[:status], + status_code: value[:statuscode], + id: value[:id], + name: value[:name], + last_modified_at: Time.zone.at(value[:mtime]), + created_at: Time.zone.at(value[:ctime]), + mime_type: value[:mimetype], + size: value[:size], + owner_name: value[:owner_name], + owner_id: value[:owner_id], + last_modified_by_name: value[:modifier_name], + last_modified_by_id: value[:modifier_id], + permissions: value[:dav_permissions], + location: location(value[:path], value[:mimetype]) + ) + else + StorageFileInfo.new( + status: value[:status], + status_code: value[:statuscode], + id: key.to_s.to_i + ) end - ) - end + end + ) end # rubocop:enable Metrics/AbcSize diff --git a/modules/storages/app/common/storages/peripherals/storage_interaction/one_drive/files_info_query.rb b/modules/storages/app/common/storages/peripherals/storage_interaction/one_drive/files_info_query.rb index cd8e40723a2a..d1d7a9982143 100644 --- a/modules/storages/app/common/storages/peripherals/storage_interaction/one_drive/files_info_query.rb +++ b/modules/storages/app/common/storages/peripherals/storage_interaction/one_drive/files_info_query.rb @@ -33,6 +33,7 @@ module Peripherals module StorageInteraction module OneDrive class FilesInfoQuery + include TaggedLogging using ServiceResultRefinements def self.call(storage:, auth_strategy:, file_ids: []) @@ -51,17 +52,20 @@ def call(auth_strategy:, file_ids:) ) end - result = Array(file_ids).map do |file_id| - file_info_result = FileInfoQuery.call(storage: @storage, auth_strategy:, file_id:) + with_tagged_logger do + info "Retrieving file information for #{file_ids.join(', ')}" + result = Array(file_ids).map do |file_id| + file_info_result = FileInfoQuery.call(storage: @storage, auth_strategy:, file_id:) - file_info_result.on_failure do |failed_result| - return failed_result if failed_result.error_source.module_parent == AuthenticationStrategies + file_info_result.on_failure do |failed_result| + return failed_result if failed_result.error_source.module_parent == AuthenticationStrategies + end + + wrap_storage_file_error(file_id, file_info_result) end - wrap_storage_file_error(file_id, file_info_result) + ServiceResult.success(result:) end - - ServiceResult.success(result:) end private diff --git a/modules/storages/app/services/storages/file_links/copy_file_links_service.rb b/modules/storages/app/services/storages/file_links/copy_file_links_service.rb index 259a048e103c..718b620c0ea3 100644 --- a/modules/storages/app/services/storages/file_links/copy_file_links_service.rb +++ b/modules/storages/app/services/storages/file_links/copy_file_links_service.rb @@ -31,6 +31,7 @@ module Storages module FileLinks class CopyFileLinksService + include TaggedLogging include OpenProject::LocaleHelper def self.call(source:, target:, user:, work_packages_map:) @@ -45,42 +46,51 @@ def initialize(source:, target:, user:, work_packages_map:) end def call - source_file_links = FileLink - .includes(:creator) - .where(storage: @source.storage, - container_id: @work_packages_map.keys, - container_type: "WorkPackage") - - with_locale_for(@user) do - if @source.project_folder_automatic? - create_managed_file_links(source_file_links) - else - create_unmanaged_file_links(source_file_links) + with_tagged_logger([self.class, @source.id, @target.id]) do + source_file_links = FileLink.includes(:creator) + .where(storage: @source.storage, + container_id: @work_packages_map.keys, + container_type: "WorkPackage") + + info "Found #{source_file_links.count} source file links" + with_locale_for(@user) do + info "Creating file links..." + if @source.project_folder_automatic? + create_managed_file_links(source_file_links) + else + create_unmanaged_file_links(source_file_links) + end end end + info "File link creation finished" end private # rubocop:disable Metrics/AbcSize def create_managed_file_links(source_file_links) + info "Getting information about the source file links" source_info = source_files_info(source_file_links).on_failure do |failed| - log_errors(failed) + log_storage_error(failed.errors) return failed end + info "Getting information about the copied target files" target_map = target_files_map.on_failure do |failed| - log_errors(failed) + log_storage_error(failed.errors) return failed end + info "Building equivalency map..." location_map = build_location_map(source_info.result, target_map.result) + info "Creating file links based on the location map #{location_map}" source_file_links.find_each do |source_link| - next unless location_map.has_key?(source_link.origin_id) + next if location_map[source_link.origin_id].blank? attributes = source_link.dup.attributes attributes.merge!( + "storage_id" => @target.storage_id, "creator_id" => @user.id, "container_id" => @work_packages_map[source_link.container_id], "origin_id" => location_map[source_link.origin_id] @@ -92,15 +102,11 @@ def create_managed_file_links(source_file_links) end # rubocop:enable Metrics/AbcSize - # rubocop:disable Metrics/AbcSize def build_location_map(source_files, target_location_map) # We need this due to inconsistencies of how we represent the File Path target_location_map.transform_keys! { |key| key.starts_with?("/") ? key : "/#{key}" } - # Since right now we can't make the relevant call as a remote admin we need to filter out 403 responses - source_location_map = source_files.filter { |info| info.status_code.to_i == 200 }.to_h do |info| - [info.id.to_s, info.clean_location] - end + source_location_map = source_files.to_h { |info| [info.id.to_s, info.clean_location] } source_location_map.each_with_object({}) do |(id, location), output| target = location.gsub(@source.managed_project_folder_path, @target.managed_project_folder_path) @@ -108,7 +114,6 @@ def build_location_map(source_files, target_location_map) output[id] = target_location_map[target]&.id || id end end - # rubocop:enable Metrics/AbcSize def auth_strategy Peripherals::Registry.resolve("#{@source.storage.short_provider_type}.authentication.userless").call @@ -122,13 +127,14 @@ def source_files_info(source_file_links) def target_files_map Peripherals::Registry - .resolve("#{@source.storage.short_provider_type}.queries.file_path_to_id_map") - .call(storage: @source.storage, auth_strategy:, folder: Peripherals::ParentFolder.new(@target.project_folder_location)) + .resolve("#{@target.storage.short_provider_type}.queries.file_path_to_id_map") + .call(storage: @target.storage, auth_strategy:, folder: Peripherals::ParentFolder.new(@target.project_folder_location)) end def create_unmanaged_file_links(source_file_links) source_file_links.find_each do |source_file_link| attributes = source_file_link.dup.attributes + attributes["storage_id"] = @target.storage_id attributes["creator_id"] = @user.id attributes["container_id"] = @work_packages_map[source_file_link.container_id] @@ -138,8 +144,8 @@ def create_unmanaged_file_links(source_file_links) end def log_errors(failure) - OpenProject.logger.error failure.inspect - OpenProject.logger.error failure.errors.inspect + error failure.inspect + error failure.errors.inspect end end end diff --git a/modules/storages/spec/services/storages/file_links/copy_file_links_service_spec.rb b/modules/storages/spec/services/storages/file_links/copy_file_links_service_spec.rb new file mode 100644 index 000000000000..afe18b5932a4 --- /dev/null +++ b/modules/storages/spec/services/storages/file_links/copy_file_links_service_spec.rb @@ -0,0 +1,88 @@ +#-- copyright +#++ + +require "spec_helper" +require_module_spec_helper + +RSpec.describe Storages::FileLinks::CopyFileLinksService, :webmock do + let(:source) { create(:nextcloud_storage_with_complete_configuration) } + let(:target) { create(:nextcloud_storage_with_complete_configuration) } + + let(:source_storage) { create(:project_storage, storage: source, project_folder_mode: "automatic") } + let(:target_storage) { create(:project_storage, storage: target, project_folder_mode: "automatic") } + + let(:source_wp) { create_list(:work_package, 5, project: source_storage.project) } + let(:target_wp) { create_list(:work_package, 5, project: target_storage.project) } + + let(:source_links) { source_wp.map { create(:file_link, container: _1, storage: source) } } + + let(:wp_map) { source_wp.map(&:id).zip(target_wp.map(&:id)).to_h } + + let(:user) { create(:user) } + + subject(:service) { described_class.new(source: source_storage, target: target_storage, user:, work_packages_map: wp_map) } + + before { source_links } + + context "when unmanaged storage" do + let(:source_storage) { create(:project_storage, storage: source, project_folder_mode: "manual") } + let(:target_storage) { create(:project_storage, storage: target, project_folder_mode: "manual") } + + it "creates file links pointing to the same files" do + expect { service.call }.to change(Storages::FileLink, :count).by(5) + + Storages::FileLink.last(5).each_with_index do |link, index| + expect(link.origin_id).to eq(source_links[index].origin_id) + end + end + end + + context "when AMPF is enabled" do + let(:files_info) { class_double(Storages::Peripherals::StorageInteraction::Nextcloud::FilesInfoQuery) } + let(:file_path_to_id) { class_double(Storages::Peripherals::StorageInteraction::Nextcloud::FilePathToIdMapQuery) } + let(:auth_strategy) do + Storages::Peripherals::StorageInteraction::AuthenticationStrategies::Strategy.new(key: :basic_auth) + end + + let(:target_folder) { Storages::Peripherals::ParentFolder.new(target_storage.managed_project_folder_path) } + + let(:remote_source_info) do + source_links.map do |link| + Storages::StorageFileInfo.new(status: "ok", status_code: 200, id: link.origin_id, name: link.origin_name, + location: File.join(source_storage.managed_project_folder_path, link.origin_name)) + end + end + + let(:path_to_ids) do + source_links.each_with_object({}) do |link, hash| + key = File.join(target_storage.managed_project_folder_path, link.origin_name) + id = Storages::StorageFileId.new(id: "#{link.origin_id}_target") + hash[key] = id + end + end + + before do + Storages::Peripherals::Registry.stub("nextcloud.queries.files_info", files_info) + Storages::Peripherals::Registry.stub("nextcloud.authentication.userless", -> { auth_strategy }) + Storages::Peripherals::Registry.stub("nextcloud.queries.file_path_to_id_map", file_path_to_id) + + allow(Storages::Peripherals::ParentFolder).to receive(:new).with(target_storage.project_folder_location) + .and_return(target_folder) + + allow(files_info).to receive(:call).with(file_ids: source_links.map(&:origin_id), storage: source, auth_strategy:) + .and_return(ServiceResult.success(result: remote_source_info)) + + allow(file_path_to_id).to receive(:call).with(storage: target, auth_strategy:, folder: target_folder) + .and_return(ServiceResult.success(result: path_to_ids)) + end + + it "create links to the newly copied files" do + expect { service.call }.to change(Storages::FileLink, :count).by(5) + + Storages::FileLink.last(5).each do |link| + expect(link.origin_id).to match /_target$/ + expect(link.storage_id).to eq(target.id) + end + end + end +end