Skip to content

Commit

Permalink
Merge pull request #16537 from opf/impl/improve-logs-over-file-link-s…
Browse files Browse the repository at this point in the history
…ervices

Expands logging to CopyFileLinksService
  • Loading branch information
mereghost authored Sep 2, 2024
2 parents b01d512 + 2a9b43d commit 67e0b13
Show file tree
Hide file tree
Showing 4 changed files with 177 additions and 90 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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

Expand All @@ -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 }
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ module Peripherals
module StorageInteraction
module OneDrive
class FilesInfoQuery
include TaggedLogging
using ServiceResultRefinements

def self.call(storage:, auth_strategy:, file_ids: [])
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
module Storages
module FileLinks
class CopyFileLinksService
include TaggedLogging
include OpenProject::LocaleHelper

def self.call(source:, target:, user:, work_packages_map:)
Expand All @@ -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]
Expand All @@ -92,23 +102,18 @@ 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)

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
Expand All @@ -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]

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 67e0b13

Please sign in to comment.