From ae5362dc1948637b88b7a8167a0ab540fcdcfa9c Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Wed, 19 Nov 2025 09:49:23 +0100 Subject: [PATCH] Unify storage_file methods implemented in different places This was highly duplicated code with incomplete and inconsistent implementations across the board. Every implementation had own attributes to add, that another implementation didn't care filling out and some aspects, such as the escaping of the location were not consistent at all. --- .../commands/create_folder_command.rb | 43 +------ .../nextcloud/commands/upload_file_command.rb | 46 +------- .../nextcloud/queries/files_query.rb | 109 ++---------------- .../nextcloud/storage_file_transformer.rb | 108 +++++++++++++++++ .../nextcloud/queries/files_query_spec.rb | 20 ++-- .../create_folder_command_examples.rb | 1 + 6 files changed, 134 insertions(+), 193 deletions(-) create mode 100644 modules/storages/app/common/storages/adapters/providers/nextcloud/storage_file_transformer.rb diff --git a/modules/storages/app/common/storages/adapters/providers/nextcloud/commands/create_folder_command.rb b/modules/storages/app/common/storages/adapters/providers/nextcloud/commands/create_folder_command.rb index 9cc5b277e80..8113b15bf27 100644 --- a/modules/storages/app/common/storages/adapters/providers/nextcloud/commands/create_folder_command.rb +++ b/modules/storages/app/common/storages/adapters/providers/nextcloud/commands/create_folder_command.rb @@ -55,9 +55,9 @@ module Storages def create_folder_request(auth_strategy, request_url, path_prefix) Authentication[auth_strategy].call(storage: @storage) do |http| handle_response(http.mkcol(request_url)).bind do - handle_response(http.propfind(request_url, requested_properties)).bind do |response| + handle_response(http.propfind(request_url, storage_file_transformer.requested_properties)).bind do |response| info "Folder successfully created" - storage_file(path_prefix, response) + storage_file_transformer.transform_document(response.xml, path_prefix) end end end @@ -80,43 +80,8 @@ module Storages end end - def requested_properties - Nokogiri::XML::Builder.new do |xml| - xml["d"].propfind("xmlns:d" => "DAV:", "xmlns:oc" => "http://owncloud.org/ns") do - xml["d"].prop do - xml["oc"].fileid - xml["oc"].size - xml["d"].getlastmodified - xml["oc"].permissions - xml["oc"].send(:"owner-display-name") - end - end - end.to_xml - end - - # FIXME: Move this to a transformer? - # rubocop:disable Metrics/AbcSize - def storage_file(path_prefix, response) - xml = response.xml - path = xml.xpath("//d:response/d:href/text()").to_s - timestamp = xml.xpath("//d:response/d:propstat/d:prop/d:getlastmodified/text()").to_s - creator = xml.xpath("//d:response/d:propstat/d:prop/oc:owner-display-name/text()").to_s - location = CGI.unescapeURIComponent( - UrlBuilder.path(CGI.unescapeURIComponent(path)).gsub(path_prefix, "") - ).delete_suffix("/") - - Results::StorageFile.build( - id: xml.xpath("//d:response/d:propstat/d:prop/oc:fileid/text()").to_s, - name: location.split("/").last, - size: xml.xpath("//d:response/d:propstat/d:prop/oc:size/text()").to_s, - mime_type: "application/x-op-directory", - created_at: Time.zone.parse(timestamp), - last_modified_at: Time.zone.parse(timestamp), - created_by_name: creator, - last_modified_by_name: creator, - location: - ) - # rubocop:enable Metrics/AbcSize + def storage_file_transformer + @storage_file_transformer ||= StorageFileTransformer.new end end end diff --git a/modules/storages/app/common/storages/adapters/providers/nextcloud/commands/upload_file_command.rb b/modules/storages/app/common/storages/adapters/providers/nextcloud/commands/upload_file_command.rb index 9b07be40121..69b57b21c42 100644 --- a/modules/storages/app/common/storages/adapters/providers/nextcloud/commands/upload_file_command.rb +++ b/modules/storages/app/common/storages/adapters/providers/nextcloud/commands/upload_file_command.rb @@ -54,9 +54,9 @@ module Storages Authentication[auth_strategy].call(storage: @storage) do |http| handle_response(http.put(request_url, body: io)).bind do info "File successfully uploaded, fetching its file info back..." - handle_response(http.propfind(request_url, requested_properties)).bind do |response| + handle_response(http.propfind(request_url, storage_file_transformer.requested_properties)).bind do |response| info "Info of uploaded file fetched" - storage_file(path_prefix, response) + storage_file_transformer.transform_document(response.xml, path_prefix) end end end @@ -79,47 +79,9 @@ module Storages end end - # TODO: deduplicate - def requested_properties - Nokogiri::XML::Builder.new do |xml| - xml["d"].propfind("xmlns:d" => "DAV:", "xmlns:oc" => "http://owncloud.org/ns") do - xml["d"].prop do - xml["oc"].fileid - xml["oc"].size - xml["d"].getcontenttype - xml["d"].getlastmodified - xml["oc"].permissions - xml["oc"].send(:"owner-display-name") - end - end - end.to_xml + def storage_file_transformer + @storage_file_transformer ||= StorageFileTransformer.new end - - # TODO: deduplicate - # FIXME: Move this to a transformer? - # rubocop:disable Metrics/AbcSize - def storage_file(path_prefix, response) - xml = response.xml - path = xml.xpath("//d:response/d:href/text()").to_s - timestamp = xml.xpath("//d:response/d:propstat/d:prop/d:getlastmodified/text()").to_s - creator = xml.xpath("//d:response/d:propstat/d:prop/oc:owner-display-name/text()").to_s - location = CGI.unescapeURIComponent( - UrlBuilder.path(CGI.unescapeURIComponent(path)).gsub(path_prefix, "") - ).delete_suffix("/") - - Results::StorageFile.build( - id: xml.xpath("//d:response/d:propstat/d:prop/oc:fileid/text()").to_s, - name: location.split("/").last, - size: xml.xpath("//d:response/d:propstat/d:prop/oc:size/text()").to_s, - mime_type: xml.xpath("//d:response/d:propstat/d:prop/d:getcontenttype/text()").to_s, - created_at: Time.zone.parse(timestamp), - last_modified_at: Time.zone.parse(timestamp), - created_by_name: creator, - last_modified_by_name: creator, - location: - ) - end - # rubocop:enable Metrics/AbcSize end end end diff --git a/modules/storages/app/common/storages/adapters/providers/nextcloud/queries/files_query.rb b/modules/storages/app/common/storages/adapters/providers/nextcloud/queries/files_query.rb index 16d863805ee..f9ec1dba8ce 100644 --- a/modules/storages/app/common/storages/adapters/providers/nextcloud/queries/files_query.rb +++ b/modules/storages/app/common/storages/adapters/providers/nextcloud/queries/files_query.rb @@ -36,7 +36,7 @@ module Storages class FilesQuery < Base def call(auth_strategy:, input_data:) origin_user_id(auth_strategy:).bind do |origin_user| - @location_prefix = CGI.unescape(UrlBuilder.path(@storage.uri.path, "remote.php/dav/files", origin_user)) + @location_prefix = UrlBuilder.path(@storage.uri.path, "remote.php/dav/files", origin_user) make_request(auth_strategy:, folder: input_data.folder, origin_user:).bind do |xml| storage_files(xml) end @@ -52,7 +52,7 @@ module Storages "remote.php/dav/files", origin_user, folder.path), - xml: requested_properties) + xml: storage_file_transformer.requested_properties) handle_response(response) end end @@ -72,25 +72,10 @@ module Storages end end - # rubocop:disable Metrics/AbcSize - def requested_properties - Nokogiri::XML::Builder.new do |xml| - xml["d"].propfind("xmlns:d" => "DAV:", "xmlns:oc" => "http://owncloud.org/ns") do - xml["d"].prop do - xml["oc"].fileid - xml["oc"].size - xml["d"].getcontenttype - xml["d"].getlastmodified - xml["oc"].permissions - xml["oc"].send(:"owner-display-name") - end - end - end.to_xml - end - # rubocop:enable Metrics/AbcSize - def storage_files(xml) - parent, *files = xml.xpath("//d:response").to_a.map { |file_element| storage_file(file_element) } + parent, *files = xml.xpath("//d:response").to_a.map do |file_element| + storage_file_transformer.transform_element(file_element, @location_prefix).value! + end Results::StorageFileCollection.build(files:, parent:, ancestors: ancestors(parent.location)) end @@ -117,88 +102,8 @@ module Storages location == "/" ? "Root" : CGI.unescape(location.split("/").last) end - def storage_file(file_element) - location = location(file_element) - - Results::StorageFile.new( - id: id(file_element), - name: name(location), - size: size(file_element), - mime_type: mime_type(file_element), - last_modified_at: last_modified_at(file_element), - created_by_name: created_by(file_element), - location:, - permissions: permissions(file_element) - ) - end - - def id(element) - element - .xpath(".//oc:fileid") - .map(&:inner_text) - .reject(&:empty?) - .first - end - - def location(element) - texts = element - .xpath("d:href") - .map(&:inner_text) - - return nil if texts.empty? - - element_name = texts.first.delete_prefix(@location_prefix) - - return element_name if element_name == "/" - - element_name.delete_suffix("/") - end - - def size(element) - element - .xpath(".//oc:size") - .map(&:inner_text) - .map { |e| Integer(e) } - .first - end - - def mime_type(element) - element - .xpath(".//d:getcontenttype") - .map(&:inner_text) - .reject(&:empty?) - .first || "application/x-op-directory" - end - - def last_modified_at(element) - element - .xpath(".//d:getlastmodified") - .map { |e| DateTime.parse(e) } - .first - end - - def created_by(element) - element - .xpath(".//oc:owner-display-name") - .map(&:inner_text) - .reject(&:empty?) - .first - end - - def permissions(element) - permissions_string = - element - .xpath(".//oc:permissions") - .map(&:inner_text) - .reject(&:empty?) - .first - - # Nextcloud Dav permissions: - # https://github.com/nextcloud/server/blob/66648011c6bc278ace57230db44fd6d63d67b864/lib/public/Files/DavUtil.php - result = [] - result << :readable if permissions_string&.include?("G") - result << :writeable if permissions_string&.match?(/W|CK/) - result + def storage_file_transformer + @storage_file_transformer ||= StorageFileTransformer.new end end end diff --git a/modules/storages/app/common/storages/adapters/providers/nextcloud/storage_file_transformer.rb b/modules/storages/app/common/storages/adapters/providers/nextcloud/storage_file_transformer.rb new file mode 100644 index 00000000000..d0619f054a5 --- /dev/null +++ b/modules/storages/app/common/storages/adapters/providers/nextcloud/storage_file_transformer.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Storages + module Adapters + module Providers + module Nextcloud + class StorageFileTransformer + def transform_document(xml, path_prefix) + transform_element(xml.xpath("//d:response"), path_prefix) + end + + def transform_element(xml, path_prefix) + location = extract_location(xml, path_prefix) + + Results::StorageFile.build( + id: prop_text(xml, "oc:fileid"), + name: location == "/" ? "Root" : location.split("/").last, + size: extract_size(xml), + mime_type: prop_text(xml, "d:getcontenttype").presence || "application/x-op-directory", + last_modified_at: Time.zone.parse(prop_text(xml, "d:getlastmodified")), + created_by_name: prop_text(xml, "oc:owner-display-name").presence || "Unknown", + location:, + permissions: parse_permissions(prop_text(xml, "oc:permissions")) + ) + end + + # Returns the XML definition that needs to be sent to Nextcloud, so that it will respond with the required properties + # for a successful call to #transform. + # rubocop:disable Metrics/AbcSize + def requested_properties + Nokogiri::XML::Builder.new do |xml| + xml["d"].propfind("xmlns:d" => "DAV:", "xmlns:oc" => "http://owncloud.org/ns") do + xml["d"].prop do + xml["oc"].fileid + xml["oc"].size + xml["d"].getcontenttype + xml["d"].getlastmodified + xml["oc"].permissions + xml["oc"].send(:"owner-display-name") + end + end + end.to_xml + end + # rubocop:enable Metrics/AbcSize + + private + + def prop_text(xml, prop_key) + xml.xpath("./d:propstat/d:prop/#{prop_key}/text()").to_s + end + + def extract_location(xml, path_prefix) + path = xml.xpath("./d:href/text()").to_s + + location = CGI.unescapeURIComponent(UrlBuilder.path(CGI.unescapeURIComponent(path)).delete_prefix(path_prefix)) + return "/" if location == "" + + location + end + + def extract_size(xml) + string = prop_text(xml, "oc:size") + return nil if string.blank? + + Integer(string) + end + + def parse_permissions(permissions_string) + # Nextcloud Dav permissions: + # https://github.com/nextcloud/server/blob/66648011c6bc278ace57230db44fd6d63d67b864/lib/public/Files/DavUtil.php + result = [] + result << :readable if permissions_string&.include?("G") + result << :writeable if permissions_string&.match?(/W|CK/) + result + end + end + end + end + end +end diff --git a/modules/storages/spec/common/storages/adapters/providers/nextcloud/queries/files_query_spec.rb b/modules/storages/spec/common/storages/adapters/providers/nextcloud/queries/files_query_spec.rb index fb839415e91..9342435b36e 100644 --- a/modules/storages/spec/common/storages/adapters/providers/nextcloud/queries/files_query_spec.rb +++ b/modules/storages/spec/common/storages/adapters/providers/nextcloud/queries/files_query_spec.rb @@ -74,7 +74,7 @@ module Storages last_modified_at: Time.zone.parse("2024-08-09T11:52:09Z"), created_by_name: "Mara Jade", last_modified_by_name: nil, - location: "/Folder%20with%20spaces", + location: "/Folder with spaces", permissions: %i[readable writeable]), Results::StorageFile.new(id: "562", name: "Ümlæûts", @@ -84,7 +84,7 @@ module Storages last_modified_at: Time.zone.parse("2024-08-09T11:51:48Z"), created_by_name: "Mara Jade", last_modified_by_name: nil, - location: "/%c3%9cml%c3%a6%c3%bbts", + location: "/Ümlæûts", permissions: %i[readable writeable]) ], parent: Results::StorageFile.new(id: "385", @@ -117,7 +117,7 @@ module Storages last_modified_at: Time.zone.parse("2024-08-09T11:53:24Z"), created_by_name: "Mara Jade", last_modified_by_name: nil, - location: "/Folder/Nested%20Folder/giphy.gif", + location: "/Folder/Nested Folder/giphy.gif", permissions: %i[readable writeable]), Results::StorageFile.new(id: "604", name: "release_meme.jpg", @@ -127,7 +127,7 @@ module Storages last_modified_at: Time.zone.parse("2024-08-09T11:53:30Z"), created_by_name: "Mara Jade", last_modified_by_name: nil, - location: "/Folder/Nested%20Folder/release_meme.jpg", + location: "/Folder/Nested Folder/release_meme.jpg", permissions: %i[readable writeable]), Results::StorageFile.new(id: "602", name: "todo.txt", @@ -137,7 +137,7 @@ module Storages last_modified_at: Time.zone.parse("2024-08-09T11:53:35Z"), created_by_name: "Mara Jade", last_modified_by_name: nil, - location: "/Folder/Nested%20Folder/todo.txt", + location: "/Folder/Nested Folder/todo.txt", permissions: %i[readable writeable]) ], parent: Results::StorageFile.new(id: "601", @@ -148,7 +148,7 @@ module Storages last_modified_at: Time.zone.parse("2024-08-09T11:53:42Z"), created_by_name: "Mara Jade", last_modified_by_name: nil, - location: "/Folder/Nested%20Folder", + location: "/Folder/Nested Folder", permissions: %i[readable writeable]), ancestors: [ Results::StorageFileAncestor.new(name: "Root", location: "/"), @@ -173,7 +173,7 @@ module Storages last_modified_at: Time.zone.parse("2024-08-09T11:52:04Z"), created_by_name: "Mara Jade", last_modified_by_name: nil, - location: "/Folder%20with%20spaces/very%20empty%20folder", + location: "/Folder with spaces/very empty folder", permissions: %i[readable writeable]), ancestors: [ Results::StorageFileAncestor.new(name: "Root", location: "/"), @@ -198,7 +198,7 @@ module Storages last_modified_at: Time.zone.parse("2024-08-09T11:51:40Z"), created_by_name: "Mara Jade", last_modified_by_name: nil, - location: "/%c3%9cml%c3%a6%c3%bbts/Anr%c3%bcchiges%20deutsches%20Dokument.docx", + location: "/Ümlæûts/Anrüchiges deutsches Dokument.docx", permissions: %i[readable writeable]), Results::StorageFile.new(id: "563", name: "data", @@ -208,7 +208,7 @@ module Storages last_modified_at: Time.zone.parse("2024-08-09T11:51:30Z"), created_by_name: "Mara Jade", last_modified_by_name: nil, - location: "/%c3%9cml%c3%a6%c3%bbts/data", + location: "/Ümlæûts/data", permissions: %i[readable writeable]) ], parent: Results::StorageFile.new(id: "562", @@ -219,7 +219,7 @@ module Storages last_modified_at: Time.zone.parse("2024-08-09T11:51:48Z"), created_by_name: "Mara Jade", last_modified_by_name: nil, - location: "/%c3%9cml%c3%a6%c3%bbts", + location: "/Ümlæûts", permissions: %i[readable writeable]), ancestors: [ Results::StorageFileAncestor.new(name: "Root", location: "/") diff --git a/modules/storages/spec/support/shared_examples_for_storage_providers/create_folder_command_examples.rb b/modules/storages/spec/support/shared_examples_for_storage_providers/create_folder_command_examples.rb index 5132d2f88d4..b05bc61c88e 100644 --- a/modules/storages/spec/support/shared_examples_for_storage_providers/create_folder_command_examples.rb +++ b/modules/storages/spec/support/shared_examples_for_storage_providers/create_folder_command_examples.rb @@ -51,6 +51,7 @@ RSpec.shared_examples_for "adapter create_folder_command: successful folder crea expect(response).to be_a(Storages::Adapters::Results::StorageFile) expect(response.name).to eq(folder_name) expect(response.location).to eq(path) + expect(response.mime_type).to eq("application/x-op-directory") ensure delete_created_folder(response) end