From 06f8788754e521c043c74c321343779257143ad3 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Mon, 27 Apr 2026 10:16:12 +0200 Subject: [PATCH 1/4] feat(RelatableResource): extract sub query into constant Extracts the shared polymorphic-reference subquery from RelatableResource into a constant so including classes can reuse it if they oeverride the .deletable scope, because you can't call super in scopes. (cherry picked from commit d33e09cb699eeaa624e0500e1bb1f10c03824871) # Conflicts: # app/models/concerns/alchemy/relatable_resource.rb --- .../concerns/alchemy/relatable_resource.rb | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/app/models/concerns/alchemy/relatable_resource.rb b/app/models/concerns/alchemy/relatable_resource.rb index cd487d4dc0..026a2991fe 100644 --- a/app/models/concerns/alchemy/relatable_resource.rb +++ b/app/models/concerns/alchemy/relatable_resource.rb @@ -2,12 +2,22 @@ module Alchemy module RelatableResource extend ActiveSupport::Concern + # SQL subquery selecting +related_object_id+ values for ingredients that + # reference a given polymorphic type. Intended to be composed into a + # +NOT IN (...)+ clause by +deletable+ and any overrides in including + # classes. Takes one named bind :type - the polymorphic type name, + # typically the class name of the including model + # (e.g. +"Alchemy::Attachment"+). + RELATED_INGREDIENTS_SUBQUERY = <<~SQL.squish + SELECT related_object_id + FROM alchemy_ingredients + WHERE related_object_id IS NOT NULL + AND related_object_type = :type + SQL + included do scope :deletable, -> do - where( - "#{table_name}.id NOT IN (SELECT related_object_id FROM alchemy_ingredients WHERE related_object_id IS NOT NULL AND related_object_type = ?)", - name - ) + where("#{table_name}.id NOT IN (#{RELATED_INGREDIENTS_SUBQUERY})", type: name) end has_many :related_ingredients, From 41bb7054cb688c4c3c8e70fbf7bfd5e55bbbba7e Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Mon, 27 Apr 2026 10:16:22 +0200 Subject: [PATCH 2/4] fix(Attachment): Detect references in ingredient values The deletable scope only considered polymorphic related_object references, which missed attachments linked from /attachment/:id/download URLs written into ingredient values by the file tab of the link dialog. On sites where attachments are linked from Richtext, Link, or Html ingredients, every attachment was reported as deletable, risking accidental data loss. The scope now adds a correlated NOT EXISTS against ingredient value columns. The per-row LIKE pattern is built with Arel::Nodes::Concat so it compiles to || on SQLite and PostgreSQL and to CONCAT() on MySQL without adapter-specific SQL. The /download suffix in the pattern delimits the id, so matching attachment 1 does not incorrectly pull in URLs pointing at attachment 10. (cherry picked from commit 3ed90b48cc48772eec9bfd33eb6e81bf8a8d29a4) --- app/models/alchemy/attachment.rb | 40 ++++++++++++++++ spec/models/alchemy/attachment_spec.rb | 66 ++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) diff --git a/app/models/alchemy/attachment.rb b/app/models/alchemy/attachment.rb index 3adf4b2286..1a739c2c77 100644 --- a/app/models/alchemy/attachment.rb +++ b/app/models/alchemy/attachment.rb @@ -40,6 +40,33 @@ class Attachment < BaseRecord scope :recent, -> { where("#{table_name}.created_at > ?", Time.current - 24.hours).order(:created_at) } scope :without_tag, -> { left_outer_joins(:taggings).where(gutentag_taggings: {id: nil}) } + # Override +Alchemy::RelatableResource#deletable+ to also exclude + # attachments referenced from +/attachment/:id/download+ URLs inside + # ingredient values (e.g. Richtext markup, Link ingredients, raw Html). + # Those URLs are written by the file tab of the link dialog and are + # not tracked via the polymorphic +related_object+ association, so the + # base scope cannot see them. + # + # Uses a correlated +NOT EXISTS+ subquery that builds the per-row LIKE + # pattern with +Arel::Nodes::Concat+, which compiles to +||+ on + # SQLite/PostgreSQL and +CONCAT()+ on MySQL. + scope :deletable, -> do + ingredients = Alchemy::Ingredient.arel_table + pattern = Arel::Nodes::Concat.new( + Arel::Nodes::Concat.new( + Arel::Nodes.build_quoted("%/attachment/"), + arel_table[:id] + ), + Arel::Nodes.build_quoted("/download%") + ) + referenced = ingredients + .project(1) + .where(ingredients[:value].matches(pattern)) + + where("#{table_name}.id NOT IN (#{RelatableResource::RELATED_INGREDIENTS_SUBQUERY})", type: name) + .where.not(referenced.exists) + end + # We need to define this method here to have it available in the validations below. class << self # The class used to generate URLs for attachments @@ -112,6 +139,13 @@ def slug CGI.escape(file_name.gsub(/\.#{extension}$/, "").tr(".", " ")) end + # Override +Alchemy::RelatableResource#deletable?+ to also consider + # +/attachment/:id/download+ links inside ingredient values (e.g. + # Richtext markup, Link ingredients, raw Html). + def deletable? + super && !referenced_in_ingredient_value? + end + # Checks if the attachment is restricted, because it is attached on restricted pages only def restricted? pages.any? && pages.not_restricted.blank? @@ -178,6 +212,12 @@ def file_type_allowed end end + def referenced_in_ingredient_value? + Alchemy::Ingredient + .where("value LIKE ?", "%/attachment/#{id}/download%") + .exists? + end + def set_name self.name ||= convert_to_humanized_name(file_name, extension) end diff --git a/spec/models/alchemy/attachment_spec.rb b/spec/models/alchemy/attachment_spec.rb index 56757f7ccf..e7344921ce 100644 --- a/spec/models/alchemy/attachment_spec.rb +++ b/spec/models/alchemy/attachment_spec.rb @@ -21,6 +21,72 @@ module Alchemy resource_name: :attachment, ingredient_type: :file + describe ".deletable" do + let!(:richtext_linked_attachment) { create(:alchemy_attachment) } + let!(:link_linked_attachment) { create(:alchemy_attachment) } + let!(:unlinked_attachment) { create(:alchemy_attachment) } + + before do + create( + :alchemy_ingredient_richtext, + value: %(

See this

) + ) + create( + :alchemy_ingredient_link, + value: "/attachment/#{link_linked_attachment.id}/download" + ) + end + + it "excludes attachments linked from any ingredient download URL" do + expect(described_class.deletable).to include(unlinked_attachment) + expect(described_class.deletable).not_to include(richtext_linked_attachment) + expect(described_class.deletable).not_to include(link_linked_attachment) + end + end + + describe "#deletable?" do + let(:attachment) { create(:alchemy_attachment) } + + subject { attachment.deletable? } + + context "when referenced by a /attachment/:id/download URL in a Richtext ingredient" do + before do + create( + :alchemy_ingredient_richtext, + value: %(x) + ) + end + + it { is_expected.to be(false) } + end + + context "when referenced by a /attachment/:id/download URL in a Link ingredient" do + before do + create( + :alchemy_ingredient_link, + value: "/attachment/#{attachment.id}/download" + ) + end + + it { is_expected.to be(false) } + end + + context "when another ingredient links to a different attachment whose id shares a digit prefix" do + let(:other_id) { attachment.id * 10 } + + before do + create( + :alchemy_ingredient_richtext, + value: %(x) + ) + end + + it "does not falsely report as referenced" do + is_expected.to be(true) + end + end + end + it "has file mime type accessor" do expect(attachment.file_mime_type).to eq("image/png") end From 007102e74e91dd767a487b3e208f338ab524937f Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 24 Apr 2026 22:04:22 +0200 Subject: [PATCH 3/4] perf(Admin::AttachmentsController): Preload deletable ids Rendering the attachment index used to call Attachment#deletable? per row to decide which delete buttons to enable. Each call issued up to two queries (the polymorphic related_ingredients check and the LIKE scan for ingredient-value references), so a page of N attachments cost up to 2N extra queries in addition to the main fetch. The controller now runs the deletable scope once for the ids on the current page and stores the result in a Set. Because the scope relation is chained as the subquery source of an IN clause, Rails applies the same Ransack filter, sort, and pagination on the database side without materializing the ids Ruby-side, keeping the check tight to the rows the view actually renders. (cherry picked from commit f425112b63c615070b0e09f185ea40b7485a7589) --- .../alchemy/admin/attachments_controller.rb | 9 +++++++ .../admin/attachments/_files_list.html.erb | 2 +- .../admin/attachments_controller_spec.rb | 26 +++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/app/controllers/alchemy/admin/attachments_controller.rb b/app/controllers/alchemy/admin/attachments_controller.rb index ee17ad588e..91d1f32c2c 100644 --- a/app/controllers/alchemy/admin/attachments_controller.rb +++ b/app/controllers/alchemy/admin/attachments_controller.rb @@ -41,6 +41,15 @@ def index .page(params[:page] || 1) .per(items_per_page) + # Preload deletable ids for the current page in a single query so the + # view can decide which delete buttons to enable without calling + # +deletable?+ (a two-query check) per row. We pass the already-loaded + # ids (via +map(&:id)+) rather than the relation itself, because passing + # a paginated relation produces +IN (SELECT ... LIMIT n)+, which older + # MariaDB versions reject. + @deletable_attachment_ids = + Attachment.where(id: @attachments.map(&:id)).deletable.pluck(:id).to_set + if in_overlay? archive_overlay end diff --git a/app/views/alchemy/admin/attachments/_files_list.html.erb b/app/views/alchemy/admin/attachments/_files_list.html.erb index 43d8aed398..023f7f96e0 100644 --- a/app/views/alchemy/admin/attachments/_files_list.html.erb +++ b/app/views/alchemy/admin/attachments/_files_list.html.erb @@ -61,7 +61,7 @@ file_attribute: 'file' %> <% end %> <% table.with_action(:destroy) do |attachment| %> - <% if attachment.deletable? %> + <% if @deletable_attachment_ids.include?(attachment.id) %> <%= link_to_confirm_dialog render_icon(:minus), Alchemy.t(:confirm_to_delete_file), diff --git a/spec/controllers/alchemy/admin/attachments_controller_spec.rb b/spec/controllers/alchemy/admin/attachments_controller_spec.rb index e345d4fe41..1696e10008 100644 --- a/spec/controllers/alchemy/admin/attachments_controller_spec.rb +++ b/spec/controllers/alchemy/admin/attachments_controller_spec.rb @@ -70,6 +70,32 @@ module Alchemy end end + describe "@deletable_attachment_ids" do + let!(:deletable_attachment) { create(:alchemy_attachment) } + let!(:referenced_attachment) { create(:alchemy_attachment) } + + before do + create( + :alchemy_ingredient_richtext, + value: %(x) + ) + end + + it "is a Set of ids of attachments on the current page that are deletable" do + get :index + expect(assigns(:deletable_attachment_ids)).to be_a(Set) + expect(assigns(:deletable_attachment_ids)).to include(deletable_attachment.id) + expect(assigns(:deletable_attachment_ids)).not_to include(referenced_attachment.id) + end + + it "is scoped to the current page only" do + get :index, params: {per_page: 1} + # With per_page: 1 only one attachment is on the current page, + # so the set must have at most one element. + expect(assigns(:deletable_attachment_ids).size).to be <= 1 + end + end + describe "by_file_type filter" do let!(:png) { create(:alchemy_attachment) } From da990954e132fcecf6131789f70838482a28ec0b Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 24 Apr 2026 22:19:01 +0200 Subject: [PATCH 4/4] perf(Admin::PicturesController): Preload deletable ids Rendering the picture archive used to call Picture#deletable? per row to decide which delete buttons to show. Each call issued up to two queries (the polymorphic related_ingredients check and the LIKE scan for ingredient-value references), so a page of N pictures cost up to 2N extra queries in addition to the main fetch. The controller now runs the deletable scope once for the ids on the current page and stores the result in a Set. Because the scope relation is chained as the subquery source of an IN clause, Rails applies the same Ransack filter, sort, and pagination on the database side without materializing the ids Ruby-side, keeping the check tight to the rows the view actually renders. The same preload is wired to the update action since it re-renders the picture partial via turbo stream. (cherry picked from commit f32d7734917608fd95937f5925cc6afeb7ba3d17) # Conflicts: # app/controllers/alchemy/admin/pictures_controller.rb --- .../alchemy/admin/pictures_controller.rb | 25 +++++++++++++++++-- .../alchemy/admin/pictures/_picture.html.erb | 2 +- .../alchemy/admin/pictures_controller_spec.rb | 21 ++++++++++++++++ 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/app/controllers/alchemy/admin/pictures_controller.rb b/app/controllers/alchemy/admin/pictures_controller.rb index 6fb268efc9..e9943dce02 100644 --- a/app/controllers/alchemy/admin/pictures_controller.rb +++ b/app/controllers/alchemy/admin/pictures_controller.rb @@ -10,6 +10,8 @@ class PicturesController < Alchemy::Admin::ResourcesController helper "alchemy/admin/tags" + before_action :load_pictures, only: :index + before_action :load_resource, only: [:edit, :update, :url, :destroy] @@ -21,6 +23,12 @@ class PicturesController < Alchemy::Admin::ResourcesController @picture = Picture.find(params[:id]) end + # Preload deletable ids for the current page (index) or the current + # resource (update, which re-renders the +_picture+ partial via a + # turbo stream). One query replaces the per-row +deletable?+ check + # that would otherwise fire for every picture the view renders. + before_action :load_deletable_picture_ids, only: [:index, :update] + add_alchemy_filter :by_file_format, type: :select, options: ->(query) do Alchemy::Picture.file_formats(query.result) end @@ -32,8 +40,6 @@ class PicturesController < Alchemy::Admin::ResourcesController helper_method :picture_offset def index - @pictures = filtered_pictures(page: params[:page]) - if in_overlay? archive_overlay end @@ -162,6 +168,21 @@ def items_per_page_options private + def load_pictures + @pictures = filtered_pictures(page: params[:page]) + end + + # Preload deletable ids in a single query so the view can decide which + # delete buttons to enable without calling +deletable?+ (a two-query + # check) per row. We pass already-loaded ids (via +map(&:id)+) rather + # than the relation itself, because passing a paginated relation + # produces +IN (SELECT ... LIMIT n)+, which older MariaDB versions + # reject. + def load_deletable_picture_ids + ids = @pictures ? @pictures.map(&:id) : [@picture.id] + @deletable_picture_ids = Picture.where(id: ids).deletable.pluck(:id).to_set + end + def picture_offset ((params[:page] || 1).to_i - 1) * items_per_page end diff --git a/app/views/alchemy/admin/pictures/_picture.html.erb b/app/views/alchemy/admin/pictures/_picture.html.erb index 2672d70551..3d85aa7a52 100644 --- a/app/views/alchemy/admin/pictures/_picture.html.erb +++ b/app/views/alchemy/admin/pictures/_picture.html.erb @@ -2,7 +2,7 @@ <%= check_box_tag "picture_ids[]", picture.id %> - <% if picture.deletable? && can?(:destroy, picture) %> + <% if @deletable_picture_ids.include?(picture.id) && can?(:destroy, picture) %>
<%= link_to_confirm_dialog( diff --git a/spec/controllers/alchemy/admin/pictures_controller_spec.rb b/spec/controllers/alchemy/admin/pictures_controller_spec.rb index c22c13a28f..80503de84e 100644 --- a/spec/controllers/alchemy/admin/pictures_controller_spec.rb +++ b/spec/controllers/alchemy/admin/pictures_controller_spec.rb @@ -141,6 +141,27 @@ module Alchemy end end end + + describe "@deletable_picture_ids" do + let!(:deletable_picture) { create(:alchemy_picture) } + let!(:referenced_picture) { create(:alchemy_picture) } + + before do + create(:alchemy_ingredient_picture, related_object: referenced_picture) + end + + it "is a Set of ids of pictures on the current page that are deletable" do + get :index + expect(assigns(:deletable_picture_ids)).to be_a(Set) + expect(assigns(:deletable_picture_ids)).to include(deletable_picture.id) + expect(assigns(:deletable_picture_ids)).not_to include(referenced_picture.id) + end + + it "is scoped to the current page only" do + get :index, params: {per_page: 1} + expect(assigns(:deletable_picture_ids).size).to be <= 1 + end + end end describe "#create" do