Merge pull request #15893 from opf/bug/no-method-for-symbol

Handle errors when Copying Project Folders when the Storage is broken

modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/copy_template_folder_command.rb

@@ -33,28 +33,27 @@ module Nextcloud
         class CopyTemplateFolderCommand
           using ServiceResultRefinements
 
-          def self.call(storage:, source_path:, destination_path:)
-            new(storage).call(source_path:, destination_path:)
+          def self.call(auth_strategy:, storage:, source_path:, destination_path:)
+            new(storage).call(auth_strategy:, source_path:, destination_path:)
           end
 
           def initialize(storage)
             @storage = storage
             @data = ResultData::CopyTemplateFolder.new(id: nil, polling_url: nil, requires_polling: false)
           end
 
-          def call(source_path:, destination_path:)
+          def call(auth_strategy:, source_path:, destination_path:)
             valid_input_result = validate_inputs(source_path, destination_path).on_failure { |failure| return failure }
 
             remote_urls = build_origin_urls(**valid_input_result.result)
 
-            ensure_remote_folder_does_not_exist(remote_urls[:destination_url]).on_failure { |failure| return failure }
+            ensure_remote_folder_does_not_exist(auth_strategy, remote_urls[:destination_url]).on_failure do |failure|
+              return failure
+            end
 
-            copy_folder(**remote_urls).on_failure { |failure| return failure }
+            copy_folder(auth_strategy, **remote_urls).on_failure { |failure| return failure }
 
-            get_folder_id(valid_input_result.result[:destination_path]).on_success do |command_result|
-              return ServiceResult
-                .success(result: @data.with(id: command_result.result[destination_path]["fileid"]))
-            end
+            get_folder_id(valid_input_result.result[:destination_path])
           end
 
           private
@@ -87,45 +86,67 @@ def build_origin_urls(source_path:, destination_path:)
             { source_url:, destination_url: }
           end
 
-          def ensure_remote_folder_does_not_exist(destination_url)
-            response = OpenProject.httpx.basic_auth(@storage.username, @storage.password).head(destination_url)
+          def ensure_remote_folder_does_not_exist(auth_strategy, destination_url)
+            response = Authentication[auth_strategy].call(storage: @storage) { |http| http.head(destination_url) }
 
             case response
             in { status: 200..299 }
-              Util.error(:conflict, "Destination folder already exists.")
+              ServiceResult.failure(result: :conflict,
+                                    errors: Util.storage_error(
+                                      response:, code: :conflict, source: self.class,
+                                      log_message: "The copy would overwrite an already existing folder"
+                                    ))
             in { status: 401 }
-              Util.error(:unauthorized, "unauthorized (validate_destination)")
+              ServiceResult.failure(result: :unauthorized,
+                                    errors: Util.storage_error(response:, code: :unauthorized, source: self.class))
             in { status: 404 }
               ServiceResult.success
             else
-              Util.error(:unknown, "Unexpected response (validate_destination): #{response.code}", response)
+              ServiceResult.failure(result: :error,
+                                    errors: Util.storage_error(response:, code: :error, source: self.class))
+            end
+          end
+
+          def copy_folder(auth_strategy, source_url:, destination_url:)
+            response = Authentication[auth_strategy].call(storage: @storage) do |http|
+              http.request("COPY", source_url, headers: { "Destination" => destination_url, "Depth" => "infinity" })
             end
+
+            handle_response(response)
           end
 
-          def copy_folder(source_url:, destination_url:)
-            response = OpenProject
-              .httpx
-              .basic_auth(@storage.username, @storage.password)
-              .request("COPY", source_url, headers: { "Destination" => destination_url, "Depth" => "infinity" })
+          def handle_response(response)
+            source = self.class
 
             case response
             in { status: 200..299 }
               ServiceResult.success(message: "Folder was successfully copied")
             in { status: 401 }
-              Util.error(:unauthorized, "Unauthorized (copy_folder)")
+              ServiceResult.failure(result: :unauthorized,
+                                    errors: Util.storage_error(response:, code: :unauthorized, source:))
+            in { status: 403 }
+              ServiceResult.failure(result: :forbidden,
+                                    errors: Util.storage_error(response:, code: :forbidden, source:))
             in { status: 404 }
-              Util.error(:not_found, "Project folder not found (copy_folder)")
+              ServiceResult.failure(result: :not_found,
+                                    errors: Util.storage_error(response:, code: :not_found, source:,
+                                                               log_message: "Template folder not found"))
             in { status: 409 }
-              Util.error(:conflict, Util.error_text_from_response(response))
+              ServiceResult.failure(result: :conflict,
+                                    errors: Util.storage_error(
+                                      response:, code: :conflict, source:,
+                                      log_message: Util.error_text_from_response(response)
+                                    ))
             else
-              Util.error(:unknown, "Unexpected response (copy_folder): #{response.status}", response)
+              ServiceResult.failure(result: :error,
+                                    errors: Util.storage_error(response:, code: :error, source:))
             end
           end
 
           def get_folder_id(destination_path)
             Registry
               .resolve("#{@storage.short_provider_type}.queries.file_ids")
-              .call(storage: @storage, path: destination_path)
+              .call(storage: @storage, path: destination_path).map { |result| @data.with(id: result[destination_path]["fileid"]) }
           end
         end
       end

modules/storages/app/common/storages/peripherals/storage_interaction/nextcloud/util.rb

@@ -54,6 +54,13 @@ def webdav_request_with_depth(number)
               { headers: { "Depth" => number } }
             end
 
+            def storage_error(response:, code:, source:, log_message: nil)
+              # Some errors, like timeouts, aren't json responses so we need to adapt
+              data = StorageErrorData.new(source:, payload: response.to_s)
+
+              StorageError.new(code:, data:, log_message:)
+            end
+
             def error(code, log_message = nil, data = nil)
               ServiceResult.failure(
                 result: code, # This is needed to work with the ConnectionManager token refresh mechanism.

modules/storages/app/common/storages/peripherals/storage_interaction/one_drive/copy_template_folder_command.rb

@@ -33,7 +33,7 @@ module Peripherals
     module StorageInteraction
       module OneDrive
         class CopyTemplateFolderCommand
-          def self.call(storage:, source_path:, destination_path:)
+          def self.call(auth_strategy:, storage:, source_path:, destination_path:)
             if source_path.blank? || destination_path.blank?
               return ServiceResult.failure(
                 result: :error,
@@ -42,16 +42,16 @@ def self.call(storage:, source_path:, destination_path:)
               )
             end
 
-            new(storage).call(source_location: source_path, destination_name: destination_path)
+            new(storage).call(auth_strategy:, source_location: source_path, destination_name: destination_path)
           end
 
           def initialize(storage)
             @storage = storage
             @data = ResultData::CopyTemplateFolder.new(id: nil, polling_url: nil, requires_polling: true)
           end
 
-          def call(source_location:, destination_name:)
-            Util.using_admin_token(@storage) do |httpx|
+          def call(auth_strategy:, source_location:, destination_name:)
+            Authentication[auth_strategy].call(storage: @storage) do |httpx|
               handle_response(
                 httpx.post(copy_path_for(source_location), json: { name: destination_name })
               )
@@ -61,24 +61,35 @@ def call(source_location:, destination_name:)
           private
 
           def handle_response(response)
+            source = self.class
+
             case response
             in { status: 202 }
               ServiceResult.success(result: @data.with(polling_url: response.headers[:location]))
             in { status: 401 }
-              ServiceResult.failure(result: :unauthorized)
+              ServiceResult.failure(result: :unauthorized,
+                                    errors: Util.storage_error(response:, code: :unauthorized, source:))
             in { status: 403 }
-              ServiceResult.failure(result: :forbidden)
+              ServiceResult.failure(result: :forbidden,
+                                    errors: Util.storage_error(response:, code: :forbidden, source:))
             in { status: 404 }
-              ServiceResult.failure(result: :not_found, message: "Template folder not found")
+              ServiceResult.failure(result: :not_found,
+                                    errors: Util.storage_error(response:, code: :not_found, source:,
+                                                               log_message: "Template folder not found"))
             in { status: 409 }
-              ServiceResult.failure(result: :conflict, message: "The copy would overwrite an already existing folder")
+              ServiceResult.failure(result: :conflict,
+                                    errors: Util.storage_error(
+                                      response:, code: :conflict, source:,
+                                      log_message: "The copy would overwrite an already existing folder"
+                                    ))
             else
-              ServiceResult.failure(result: :error)
+              ServiceResult.failure(result: :error,
+                                    errors: Util.storage_error(response:, code: :error, source:))
             end
           end
 
           def copy_path_for(source_location)
-            "/v1.0/drives/#{@storage.drive_id}/items/#{source_location}/[email protected]=fail"
+            "#{@storage.uri}v1.0/drives/#{@storage.drive_id}/items/#{source_location}/[email protected]=fail"
           end
         end
       end

modules/storages/app/services/storages/project_storages/copy_project_folders_service.rb

@@ -44,9 +44,12 @@ def call(source, target)
         return ServiceResult.success(result: @data) if source.project_folder_inactive?
         return ServiceResult.success(result: @data.with(id: source.project_folder_id)) if source.project_folder_manual?
 
+        auth_strategy = Peripherals::Registry.resolve("#{source.storage.short_provider_type}.authentication.userless").call
+
         Peripherals::Registry
           .resolve("#{source.storage.short_provider_type}.commands.copy_template_folder")
-          .call(storage: source.storage,
+          .call(auth_strategy:,
+                storage: source.storage,
                 source_path: source.project_folder_location,
                 destination_path: target.managed_project_folder_path)
       end

modules/storages/app/workers/storages/copy_project_folders_job.rb

@@ -42,6 +42,7 @@ def perform(source:, target:, work_packages_map:)
       user = batch.properties[:user]
 
       project_folder_result = results_from_polling || initiate_copy(target)
+      project_folder_result.on_failure { |failed| return log_failure(failed) }
 
       ProjectStorages::UpdateService.new(user:, model: target)
                                     .call(project_folder_id: project_folder_result.result.id,
@@ -95,7 +96,16 @@ def results_from_polling
     def log_failure(failed)
       return if failed.success?
 
-      OpenProject.logger.warn failed.errors.inspect.to_s
+      if failed.errors.is_a? StorageError
+        errors = failed.errors.to_active_model_errors.full_messages
+        batch.properties[:errors].push(*errors)
+      else
+        batch.properties[:errors].push(*failed.errors.to_s)
+      end
+
+      batch.save
+
+      OpenProject.logger.warn failed.errors.to_s
     end
 
     def polling_info

modules/storages/spec/common/storages/peripherals/storage_interaction/nextcloud/copy_template_folder_command_spec.rb

@@ -43,6 +43,7 @@
   let(:destination_path) { "boring-destination" }
   let(:source_url) { "#{url}/remote.php/dav/files/#{CGI.escape(origin_user_id)}/#{source_path}" }
   let(:destination_url) { "#{url}/remote.php/dav/files/#{CGI.escape(origin_user_id)}/#{destination_path}" }
+  let(:auth_strategy) { Storages::Peripherals::StorageInteraction::AuthenticationStrategies::NextcloudStrategies::UserLess.call }
 
   subject { described_class.new(storage) }
 
@@ -51,14 +52,14 @@
 
     describe "parameter validation" do
       it "source_path cannot be blank" do
-        result = subject.call(source_path: "", destination_path: "destination")
+        result = subject.call(auth_strategy:, source_path: "", destination_path: "destination")
 
         expect(result).to be_failure
         expect(result.errors.log_message).to eq("Source and destination paths must be present.")
       end
 
       it "destination_path cannot blank" do
-        result = subject.call(source_path: "source", destination_path: "")
+        result = subject.call(auth_strategy:, source_path: "source", destination_path: "")
 
         expect(result).to be_failure
         expect(result.errors.log_message).to eq("Source and destination paths must be present.")
@@ -68,10 +69,10 @@
     describe "remote server overwrite protection" do
       it "destination_path must not exist on the remote server" do
         stub_request(:head, destination_url).to_return(status: 200)
-        result = subject.call(source_path:, destination_path:)
+        result = subject.call(auth_strategy:, source_path:, destination_path:)
 
         expect(result).to be_failure
-        expect(result.errors.log_message).to eq("Destination folder already exists.")
+        expect(result.errors.log_message).to eq("The copy would overwrite an already existing folder")
       end
     end
 
@@ -112,7 +113,7 @@
       end
 
       it "must be successful" do
-        result = subject.call(source_path:, destination_path:)
+        result = subject.call(auth_strategy:, source_path:, destination_path:)
 
         expect(result).to be_success
         expect(result.result.id).to eq("349")
@@ -134,7 +135,7 @@
       end
 
       it "returns a :conflict failure if the copy fails" do
-        result = subject.call(source_path:, destination_path:)
+        result = subject.call(auth_strategy:, source_path:, destination_path:)
 
         expect(result).to be_failure
         expect(result.errors.code).to eq(:conflict)

modules/storages/spec/common/storages/peripherals/storage_interaction/one_drive/copy_template_folder_command_spec.rb

@@ -50,8 +50,7 @@
 
   shared_let(:source_path) { base_template_folder.id }
 
-  it "is registered under commands.one_drive.copy_template_folder",
-     skip: "Skipped while we decide on what to do with the copy project folder" do
+  it "is registered under commands.one_drive.copy_template_folder" do
     expect(Storages::Peripherals::Registry.resolve("one_drive.commands.copy_template_folder")).to eq(described_class)
   end
 
@@ -62,13 +61,14 @@
   it ".call takes 3 required parameters: storage, source_path, destination_path" do
     method = described_class.method(:call)
 
-    expect(method.parameters).to contain_exactly(%i[keyreq storage], %i[keyreq source_path], %i[keyreq destination_path])
+    expect(method.parameters)
+      .to contain_exactly(%i[keyreq auth_strategy], %i[keyreq storage], %i[keyreq source_path], %i[keyreq destination_path])
   end
 
   it "destination_path and source_path can't be empty" do
-    missing_source = described_class.call(storage:, source_path: "", destination_path: "Path")
-    missing_path = described_class.call(storage:, source_path: "Path", destination_path: nil)
-    missing_both = described_class.call(storage:, source_path: nil, destination_path: "")
+    missing_source = described_class.call(auth_strategy:, storage:, source_path: "", destination_path: "Path")
+    missing_path = described_class.call(auth_strategy:, storage:, source_path: "Path", destination_path: nil)
+    missing_both = described_class.call(auth_strategy:, storage:, source_path: nil, destination_path: "")
 
     expect([missing_both, missing_path, missing_source]).to all(be_failure)
   end
@@ -92,7 +92,7 @@
 
     it "copies origin folder and all underlying files and folders to the destination_path",
        vcr: "one_drive/copy_template_folder_copy_successful" do
-      command_result = described_class.call(storage:, source_path:, destination_path: "My New Folder")
+      command_result = described_class.call(auth_strategy:, storage:, source_path:, destination_path: "My New Folder")
 
       expect(command_result).to be_success
       expect(command_result.result).to be_requires_polling
@@ -104,36 +104,32 @@
     describe "error handling" do
       context "when the source_path does not exist" do
         it "fails", vcr: "one_drive/copy_template_source_not_found" do
-          result = described_class.call(storage:, source_path: "TheCakeIsALie", destination_path: "Not Happening")
+          result = described_class.call(auth_strategy:, storage:, source_path: "TheCakeIsALie", destination_path: "Not Happening")
 
           expect(result).to be_failure
         end
 
         it "explains the nature of the error", vcr: "one_drive/copy_template_source_not_found" do
-          result = described_class.call(storage:, source_path: "TheCakeIsALie", destination_path: "Not Happening")
+          result = described_class.call(auth_strategy:, storage:, source_path: "TheCakeIsALie", destination_path: "Not Happening")
 
-          expect(result.message).to eq("Template folder not found")
+          expect(result.errors.to_s).to match(/not_found \| Template folder not found/)
         end
-
-        it "logs the occurrence"
       end
 
       context "when it would overwrite an already existing folder" do
         it "fails", vcr: "one_drive/copy_template_folder_no_overwrite" do
           existing_folder = original_folders.first[:name]
-          result = described_class.call(storage:, source_path:, destination_path: existing_folder)
+          result = described_class.call(auth_strategy:, storage:, source_path:, destination_path: existing_folder)
 
           expect(result).to be_failure
         end
 
         it "explains the nature of the error", vcr: "one_drive/copy_template_folder_no_overwrite" do
           existing_folder = original_folders.first[:name]
-          result = described_class.call(storage:, source_path:, destination_path: existing_folder)
+          result = described_class.call(auth_strategy:, storage:, source_path:, destination_path: existing_folder)
 
-          expect(result.message).to eq("The copy would overwrite an already existing folder")
+          expect(result.errors.to_s).to match(/conflict \| The copy would overwrite an already existing folder/)
         end
-
-        it "logs the occurrence"
       end
     end
   end