From 9e531e49955801bd41a4a8a48ca8d764ebfe342a Mon Sep 17 00:00:00 2001 From: f Date: Tue, 12 Jul 2022 15:38:16 -0300 Subject: [PATCH 01/12] no cambiar el valor si no se pudo cargar el archivo esto hace que sutilmente desaparezcan archivos --- app/models/metadata_file.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/app/models/metadata_file.rb b/app/models/metadata_file.rb index 71d3f049..2b628a57 100644 --- a/app/models/metadata_file.rb +++ b/app/models/metadata_file.rb @@ -41,14 +41,10 @@ class MetadataFile < MetadataTemplate end # Asociar la imagen subida al sitio y obtener la ruta - # - # XXX: Si evitamos guardar cambios con changed? no tenemos forma de - # saber que un archivo subido manualmente se convirtió en - # un Attachment y cada vez que lo editemos vamos a subir una imagen - # repetida. + # @return [Boolean] def save value['description'] = sanitize value['description'] - value['path'] = static_file ? relative_destination_path_with_filename.to_s : nil + value['path'] = relative_destination_path_with_filename.to_s if static_file true end From 0b06bf35417b1f23176f798611c70ffc49822985 Mon Sep 17 00:00:00 2001 From: f Date: Tue, 12 Jul 2022 15:39:08 -0300 Subject: [PATCH 02/12] notificar cuando no se pudo cargar el archivo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no perder información silenciosamente --- app/models/metadata_file.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/metadata_file.rb b/app/models/metadata_file.rb index 2b628a57..942b448d 100644 --- a/app/models/metadata_file.rb +++ b/app/models/metadata_file.rb @@ -58,9 +58,6 @@ class MetadataFile < MetadataTemplate # * El archivo es una ruta que apunta a un archivo asociado al sitio # * El archivo es una ruta a un archivo dentro del repositorio # - # XXX: La última opción provoca archivos duplicados, pero es lo mejor - # que tenemos hasta que resolvamos https://0xacab.org/sutty/sutty/-/issues/213 - # # @todo encontrar una forma de obtener el attachment sin tener que # recurrir al último subido. # @@ -77,8 +74,12 @@ class MetadataFile < MetadataTemplate site.static_files.last.tap do |s| s.blob.update(key: key_from_path) end + else + raise ArgumentError, 'No se pudo subir el archivo' end end + rescue ArgumentError => e + ExceptionNotifier.notify_exception(e, data: { site: site.name, path: value['path'] }) end # Obtiene la ruta absoluta al archivo From def4402313e0d142b72321192c1b09783fdfe520 Mon Sep 17 00:00:00 2001 From: f Date: Tue, 12 Jul 2022 15:39:42 -0300 Subject: [PATCH 03/12] permitir cargar archivos con la misma key podemos tener sitios distintos con la misma llave identificatoria. ahora permitimos solo una llave por cada sitio distinto. si subimos un archivo duplicado en un mismo sitio el error va a seguir siendo el mismo. --- app/models/metadata_file.rb | 2 +- ...nge_blob_key_uniqueness_to_include_service_name.rb | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20220712135053_change_blob_key_uniqueness_to_include_service_name.rb diff --git a/app/models/metadata_file.rb b/app/models/metadata_file.rb index 942b448d..fa570ac0 100644 --- a/app/models/metadata_file.rb +++ b/app/models/metadata_file.rb @@ -68,7 +68,7 @@ class MetadataFile < MetadataTemplate when ActionDispatch::Http::UploadedFile site.static_files.last if site.static_files.attach(value['path']) when String - if (blob_id = ActiveStorage::Blob.where(key: key_from_path).pluck(:id).first) + if (blob_id = ActiveStorage::Blob.where(key: key_from_path, service_name: site.name).pluck(:id).first) site.static_files.find_by(blob_id: blob_id) elsif path? && pathname.exist? && site.static_files.attach(io: pathname.open, filename: pathname.basename) site.static_files.last.tap do |s| diff --git a/db/migrate/20220712135053_change_blob_key_uniqueness_to_include_service_name.rb b/db/migrate/20220712135053_change_blob_key_uniqueness_to_include_service_name.rb new file mode 100644 index 00000000..00bae7ea --- /dev/null +++ b/db/migrate/20220712135053_change_blob_key_uniqueness_to_include_service_name.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +# Cambia el índice único para incluir el nombre del servicio, de forma +# que podamos tener varias copias del mismo sitio (por ejemplo para +# test) sin que falle la creación de archivos. +class ChangeBlobKeyUniquenessToIncludeServiceName < ActiveRecord::Migration[6.1] + def change + remove_index :active_storage_blobs, %i[key], unique: true + add_index :active_storage_blobs, %i[key service_name], unique: true + end +end From cd523babade074fd69471e9c54cf1c391a2730da Mon Sep 17 00:00:00 2001 From: f Date: Tue, 12 Jul 2022 15:58:31 -0300 Subject: [PATCH 04/12] siempre devolver Pathname closes #6982 closes #6979 closes #6974 closes #6973 closes #6972 closes #6970 closes #6968 closes #6967 --- app/models/metadata_file.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/metadata_file.rb b/app/models/metadata_file.rb index fa570ac0..4ea026b4 100644 --- a/app/models/metadata_file.rb +++ b/app/models/metadata_file.rb @@ -126,7 +126,7 @@ class MetadataFile < MetadataTemplate rescue Errno::ENOENT => e ExceptionNotifier.notify_exception(e) - value['path'] + Pathname.new(value['path']) end def relative_destination_path_with_filename From aab8ed9fed5c1411da527814a78e5ad7db132bd6 Mon Sep 17 00:00:00 2001 From: f Date: Wed, 13 Jul 2022 13:34:45 -0300 Subject: [PATCH 05/12] =?UTF-8?q?informar=20cuando=20el=20nombre=20de=20ar?= =?UTF-8?q?chivo=20est=C3=A1=20vac=C3=ADo?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/lib/active_storage/service/jekyll_service.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/lib/active_storage/service/jekyll_service.rb b/app/lib/active_storage/service/jekyll_service.rb index 92b26e0e..3edd2653 100644 --- a/app/lib/active_storage/service/jekyll_service.rb +++ b/app/lib/active_storage/service/jekyll_service.rb @@ -67,7 +67,9 @@ module ActiveStorage # @param :key [String] # @return [String] def filename_for(key) - ActiveStorage::Blob.where(key: key).limit(1).pluck(:filename).first + ActiveStorage::Blob.where(key: key).limit(1).pluck(:filename).first.tap do |filename| + raise ArgumentError, "Filename for key #{key} is blank" if filename.blank? + end end # Crea una ruta para la llave con un nombre conocido. From 5397ae66a2709aeb30da2dbb2b9064cf3e42e8c7 Mon Sep 17 00:00:00 2001 From: f Date: Wed, 13 Jul 2022 14:18:39 -0300 Subject: [PATCH 06/12] informar con mas detalle cuando el archivo no existe closes #6991 closes #6990 closes #6985 closes #6969 closes #6966 --- app/models/metadata_file.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/metadata_file.rb b/app/models/metadata_file.rb index 4ea026b4..00ac7af0 100644 --- a/app/models/metadata_file.rb +++ b/app/models/metadata_file.rb @@ -124,13 +124,15 @@ class MetadataFile < MetadataTemplate # devolvemos la ruta original, que puede ser el archivo que no existe # o vacía si se está subiendo uno. rescue Errno::ENOENT => e - ExceptionNotifier.notify_exception(e) + ExceptionNotifier.notify_exception(e, data: { site: site.name, path: value['path'] }) Pathname.new(value['path']) end def relative_destination_path_with_filename destination_path_with_filename.relative_path_from(Pathname.new(site.path).realpath) + rescue ArgumentError => e + ExceptionNotifier.notify_exception(e, data: { site: site.name, path: value['path'] }) end def static_file_path From 83384b686af0ddb9be18ee3fbbb10d1a9d3ea899 Mon Sep 17 00:00:00 2001 From: f Date: Fri, 15 Jul 2022 18:22:06 -0300 Subject: [PATCH 07/12] solo copiar el archivo si no existe --- app/lib/active_storage/service/jekyll_service.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/lib/active_storage/service/jekyll_service.rb b/app/lib/active_storage/service/jekyll_service.rb index 3edd2653..88ffa83c 100644 --- a/app/lib/active_storage/service/jekyll_service.rb +++ b/app/lib/active_storage/service/jekyll_service.rb @@ -20,6 +20,18 @@ module ActiveStorage end end + # Solo copiamos el archivo si no existe + # + # @param :key [String] + # @param :io [IO] + # @param :checksum [String] + def upload(key, io, checksum: nil, **) + instrument :upload, key: key, checksum: checksum do + IO.copy_stream(io, make_path_for(key)) unless exist?(key) + ensure_integrity_of(key, checksum) if checksum + end + end + # Lo mismo que en DiskService agregando el nombre de archivo en la # firma. Esto permite que luego podamos guardar el archivo donde # corresponde. From 6c62c20572a1a763ceb23b62eea924fed4b86e25 Mon Sep 17 00:00:00 2001 From: f Date: Fri, 15 Jul 2022 19:32:06 -0300 Subject: [PATCH 08/12] subir directamente los archivos a la base de datos MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cuando subimos un archivo usando attach en realidad estábamos generando una copia y dejándola huérfana en el repositorio del sitio. usando blob podemos imitar un servicio de subida de archivos sin generar io innecesario. --- app/models/metadata_file.rb | 42 ++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/app/models/metadata_file.rb b/app/models/metadata_file.rb index 00ac7af0..c7296de4 100644 --- a/app/models/metadata_file.rb +++ b/app/models/metadata_file.rb @@ -68,18 +68,8 @@ class MetadataFile < MetadataTemplate when ActionDispatch::Http::UploadedFile site.static_files.last if site.static_files.attach(value['path']) when String - if (blob_id = ActiveStorage::Blob.where(key: key_from_path, service_name: site.name).pluck(:id).first) - site.static_files.find_by(blob_id: blob_id) - elsif path? && pathname.exist? && site.static_files.attach(io: pathname.open, filename: pathname.basename) - site.static_files.last.tap do |s| - s.blob.update(key: key_from_path) - end - else - raise ArgumentError, 'No se pudo subir el archivo' - end + site.static_files.find_by(blob_id: blob_id) || migrate_static_file! end - rescue ArgumentError => e - ExceptionNotifier.notify_exception(e, data: { site: site.name, path: value['path'] }) end # Obtiene la ruta absoluta al archivo @@ -95,7 +85,7 @@ class MetadataFile < MetadataTemplate # # @return [String] def key_from_path - pathname.dirname.basename.to_s + @key_from_path ||= pathname.dirname.basename.to_s end def path? @@ -148,4 +138,32 @@ class MetadataFile < MetadataTemplate def no_file_for_description? !path? && description? end + + # Obtiene el id del blob asociado + # + # @return [Integer,nil] + def blob_id + @blob_id ||= ActiveStorage::Blob.where(key: key_from_path, service_name: site.name).pluck(:id).first + end + + # Genera el blob para un archivo que ya se encuentra en el + # repositorio y lo agrega a la base de datos. + # + # @return [ActiveStorage::Attachment] + def migrate_static_file! + raise ArgumentError, 'El archivo no existe' unless path? && pathname.exist? + + Site.transaction do + blob = + ActiveStorage::Blob.create_after_unfurling!(key: key_from_path, + io: pathname.open, + filename: pathname.basename, + service_name: site.name) + + ActiveStorage::Attachment.create!(name: 'static_files', record: site, blob: blob) + end + rescue ArgumentError => e + ExceptionNotifier.notify_exception(e, data: { site: site.name, path: value['path'] }) + nil + end end From 4fc2ece2d5bd7f913559a9a501bd6a724ef01fa4 Mon Sep 17 00:00:00 2001 From: f Date: Mon, 28 Nov 2022 19:30:20 -0300 Subject: [PATCH 09/12] =?UTF-8?q?fix:=20permitir=20borrar=20la=20imagen=20?= =?UTF-8?q?sin=20vaciar=20la=20descripci=C3=B3n=20#8347?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sin embargo no estamos pudiendo distinguir entre imagen borrada e imagen sin subir, con lo que perdimos una verificación --- app/models/metadata_file.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/app/models/metadata_file.rb b/app/models/metadata_file.rb index c7296de4..e67761b2 100644 --- a/app/models/metadata_file.rb +++ b/app/models/metadata_file.rb @@ -23,7 +23,6 @@ class MetadataFile < MetadataTemplate errors << I18n.t("metadata.#{type}.site_invalid") if site.invalid? errors << I18n.t("metadata.#{type}.path_required") if path_missing? - errors << I18n.t("metadata.#{type}.no_file_for_description") if no_file_for_description? errors << I18n.t("metadata.#{type}.attachment_missing") if path? && !static_file errors.compact! @@ -134,11 +133,6 @@ class MetadataFile < MetadataTemplate end end - # No hay archivo pero se lo describió - def no_file_for_description? - !path? && description? - end - # Obtiene el id del blob asociado # # @return [Integer,nil] From 581e8e10c2af693a9d0b43fc38e398bc12c8b000 Mon Sep 17 00:00:00 2001 From: f Date: Mon, 28 Nov 2022 19:31:09 -0300 Subject: [PATCH 10/12] fix: vaciar la imagen al borrarla --- app/models/metadata_file.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/models/metadata_file.rb b/app/models/metadata_file.rb index e67761b2..a155a414 100644 --- a/app/models/metadata_file.rb +++ b/app/models/metadata_file.rb @@ -42,8 +42,12 @@ class MetadataFile < MetadataTemplate # Asociar la imagen subida al sitio y obtener la ruta # @return [Boolean] def save - value['description'] = sanitize value['description'] - value['path'] = relative_destination_path_with_filename.to_s if static_file + if value['path'].blank? + self[:value] = default_value + else + value['description'] = sanitize value['description'] + value['path'] = relative_destination_path_with_filename.to_s if static_file + end true end From 53b11cba155e4cef51dd5e0801220c5112eae318 Mon Sep 17 00:00:00 2001 From: f Date: Wed, 11 Jan 2023 16:54:45 -0300 Subject: [PATCH 11/12] fix: no devolver 'true' si no existe el archivo closes #9380 closes #9379 closes #9375 closes #9374 closes #9366 closes #8888 closes #8765 closes #8764 closes #8674 closes #8099 closes #8098 closes #7845 closes #7844 closes #7612 closes #7611 closes #9378 closes #9377 closes #9364 closes #9362 --- app/models/metadata_file.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/models/metadata_file.rb b/app/models/metadata_file.rb index a155a414..a55549cb 100644 --- a/app/models/metadata_file.rb +++ b/app/models/metadata_file.rb @@ -122,10 +122,17 @@ class MetadataFile < MetadataTemplate Pathname.new(value['path']) end + # Obtener la ruta relativa al sitio. + # + # Si algo falla, devolver la ruta original para no romper el archivo. + # + # @return [String, nil] def relative_destination_path_with_filename destination_path_with_filename.relative_path_from(Pathname.new(site.path).realpath) rescue ArgumentError => e ExceptionNotifier.notify_exception(e, data: { site: site.name, path: value['path'] }) + + value['path'] end def static_file_path From 5117ffbce3e41ee3315803e25cf3d2f05c8259a9 Mon Sep 17 00:00:00 2001 From: f Date: Wed, 11 Jan 2023 17:04:22 -0300 Subject: [PATCH 12/12] fix: siempre devolver la ruta completa closes #6984 closes #7053 closes #7807 closes #7998 closes #8262 closes #8690 closes #8887 closes #9363 closes #9376 --- app/models/metadata_file.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/metadata_file.rb b/app/models/metadata_file.rb index a55549cb..3ac89c9b 100644 --- a/app/models/metadata_file.rb +++ b/app/models/metadata_file.rb @@ -119,7 +119,7 @@ class MetadataFile < MetadataTemplate rescue Errno::ENOENT => e ExceptionNotifier.notify_exception(e, data: { site: site.name, path: value['path'] }) - Pathname.new(value['path']) + Pathname.new(File.join(site.path, value['path'])) end # Obtener la ruta relativa al sitio.