decidim-assemblies/app/serializers/decidim/assemblies/assembly_importer.rb (2)

199-214: Add timeouts to prevent indefinite blocking.

Same concern as flagged on ParticipatoryProcessImporter#fetch_content_type — the Net::HTTP connection here lacks explicit open_timeout and read_timeout, risking indefinite hangs on unresponsive servers.

🛡️ Proposed fix

         uri = URI.parse(url)
         http_connection = Net::HTTP.new(uri.host, uri.port)
         http_connection.use_ssl = true if uri.scheme == "https"
+        http_connection.open_timeout = 5
+        http_connection.read_timeout = 5
         http_connection.start do |http|

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@decidim-assemblies/app/serializers/decidim/assemblies/assembly_importer.rb`
around lines 199 - 214, The fetch_content_type method currently creates a
Net::HTTP instance without timeouts which can hang; update the Net::HTTP object
(in Assemblies::AssemblyImporter#fetch_content_type) to set explicit
open_timeout and read_timeout (e.g. open_timeout = 5, read_timeout = 10) before
starting the connection, keep the existing use_ssl logic, and preserve the
existing response handling and rescue behavior so unresponsive hosts time out
instead of blocking indefinitely.

---

70-108: Same double-HEAD-request concern as in ParticipatoryProcessImporter.

The remote_file_error(url) call at line 77 and fetch_content_type(url) at line 93 each issue a separate HTTP HEAD request to the same URL. See the recommendation on the parallel ParticipatoryProcessImporter for a merged approach that avoids the redundant round-trip.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@decidim-assemblies/app/serializers/decidim/assemblies/assembly_importer.rb`
around lines 70 - 108, The importer makes two separate HEAD requests for the
same URL (remote_file_error and fetch_content_type) causing redundant network
calls; change import_folders_and_attachments to perform a single HEAD fetch
(e.g. via a new helper fetch_remote_file_head or by updating remote_file_error
to return both error and headers) and reuse its result: call that helper once
per url, check the returned error and push a warning if present, and pass the
retrieved content_type/header info into Attachment.new (content_type) instead of
calling fetch_content_type again; adjust error handling around the single HEAD
call to preserve the existing rescue behavior used when assigning
attached_uploader(:file).remote_url.

decidim-participatory_processes/app/serializers/decidim/participatory_processes/participatory_process_importer.rb (3)

227-241: Substantial code duplication with AssemblyImporter.

fetch_content_type, format_error, remote_file_error, attachment_title, and the bulk of import_folders_and_attachments are near-identical copies between this file and decidim-assemblies/app/serializers/decidim/assemblies/assembly_importer.rb. This makes future maintenance error-prone — a fix in one importer can easily be forgotten in the other.

Consider extracting the shared logic into a common module/concern (e.g., Decidim::Importers::RemoteAttachmentHandling) that both importers include. This could be done as a follow-up.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@decidim-participatory_processes/app/serializers/decidim/participatory_processes/participatory_process_importer.rb`
around lines 227 - 241, The two importer classes
(Decidim::ParticipatoryProcesses::ParticipatoryProcessImporter and
Decidim::Assemblies::AssemblyImporter) duplicate methods like
fetch_content_type, format_error, remote_file_error, attachment_title and large
parts of import_folders_and_attachments; extract these shared behaviors into a
single concern/module (suggested name:
Decidim::Importers::RemoteAttachmentHandling) that implements those methods and
any helper logic, then have both importers include that module and delegate to
its methods (replace local method definitions in ParticipatoryProcessImporter
and AssemblyImporter with includes/calls to the new module) so future fixes are
made in one place.

---

92-130: Double HTTP HEAD request per attachment — consider merging remote_file_error and fetch_content_type.

For each attachment, remote_file_error(url) (line 99) issues an HTTP HEAD request, and then fetch_content_type(url) (line 115) issues a second HEAD request to the same URL. This doubles the network round-trips for every imported attachment.

Consider refactoring remote_file_error to return both the error and the content type from a single HEAD request, e.g., returning a struct/hash like { error: nil, content_type: "application/pdf" }.

♻️ Sketch of a merged approach

-          error = remote_file_error(url)
-          if error.present?
+          file_info = remote_file_info(url)
+          if file_info[:error].present?
             `@warnings` << I18n.t(
               "decidim.participatory_processes.admin.imports.attachment_error",
               title: attachment_title(file),
-              error:
+              error: file_info[:error]
             )
             next
           end
 
           Decidim.traceability.perform_action!("create", Attachment, `@user`) do
             attachment = Attachment.new(
               title: file["title"],
               description: file["description"],
               attached_to: `@imported_process`,
               weight: file["weight"],
-              content_type: fetch_content_type(url)
+              content_type: file_info[:content_type]
             )

Where remote_file_info does a single HEAD and returns both pieces of data.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@decidim-participatory_processes/app/serializers/decidim/participatory_processes/participatory_process_importer.rb`
around lines 92 - 130, The import currently calls remote_file_error(url) and
then fetch_content_type(url), causing two HTTP HEADs per attachment; change to
perform a single HEAD by adding a combined helper (e.g., remote_file_info or
remote_head_for_attachment) that returns both error and content_type (e.g., {
error: ..., content_type: ... }) and update import_folders_and_attachments to
call that helper once, use the returned :error to push warnings (preserving
format_error handling) and the returned :content_type when building the
Attachment (replace fetch_content_type(url) with the returned value); keep
existing rescue logic around assigning remote_url and the attachment creation
flow.

---

227-241: Missing timeouts on the HTTP connection — risk of indefinite blocking.

Net::HTTP.new without explicit open_timeout / read_timeout uses Ruby's default (which is effectively no timeout for open_timeout in older Rubies, and 60s for read_timeout). If the remote server is slow or unresponsive, this can block the import thread for a long time or indefinitely.

The same concern applies to remote_file_error (lines 175-176), but since that's pre-existing code, flagging it here for the new method.

🛡️ Proposed fix

         uri = URI.parse(url)
         http_connection = Net::HTTP.new(uri.host, uri.port)
         http_connection.use_ssl = true if uri.scheme == "https"
+        http_connection.open_timeout = 5
+        http_connection.read_timeout = 5
         http_connection.start do |http|

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@decidim-participatory_processes/app/serializers/decidim/participatory_processes/participatory_process_importer.rb`
around lines 227 - 241, The fetch_content_type method creates a Net::HTTP
connection without timeouts causing potential indefinite blocking; update
fetch_content_type to set explicit open_timeout and read_timeout on the
Net::HTTP instance created in URI.parse (e.g., when calling
Net::HTTP.new(uri.host, uri.port)) and pass reasonable values (for example a few
seconds) before calling http.start and http.head; ensure the same pattern
(explicit open_timeout/read_timeout) is applied to the earlier remote_file_error
HTTP usage to avoid the same blocking issue.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro