Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ActiveStorage asset linking performance #12576

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 41 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
8abc53b
Improve ActiveStorage asset linking performance
ahukkanen Mar 7, 2024
2989dff
Merge branch 'develop' into fix/storage-service-uris
ahukkanen Mar 19, 2024
217d3ea
Fix issues serving image variants directly through the storage service
ahukkanen Mar 19, 2024
c46b7ea
Fix and extend the storage asset router specs
ahukkanen Mar 19, 2024
8c6b036
Fix typo
ahukkanen Mar 19, 2024
d4e5c25
Merge branch 'develop' into fix/storage-service-uris
ahukkanen Mar 20, 2024
597084c
Define the current host for the assets for all specs
ahukkanen Mar 20, 2024
4f1065f
Fix spelling issues
ahukkanen Mar 20, 2024
c894275
Merge branch 'develop' into fix/storage-service-uris
ahukkanen Apr 1, 2024
a508e4d
Improve the storage router logic for local disk storage
ahukkanen Apr 4, 2024
64acf57
Add the ActiveStorage::SetCurrent concern to devise controllers
ahukkanen Apr 4, 2024
0e9967e
Fix specs broken due to the changes in the asset URL generation
ahukkanen Apr 4, 2024
d628f2b
Merge branch 'develop' into fix/storage-service-uris
ahukkanen Apr 4, 2024
2451ad2
Fix spelling in variable name
ahukkanen Apr 4, 2024
5b96d9f
Different strategy to escape the string included in the regexp
ahukkanen Apr 4, 2024
c93d2b3
Fix id documents verification flaky spec
ahukkanen Apr 4, 2024
379d140
Fix further specs expecting blob URLs
ahukkanen Apr 4, 2024
2eddbfc
Use different matching strategy for questionnaire file answers
ahukkanen Apr 4, 2024
0c8a1c3
Fix flaky specs for inviting process users
ahukkanen Apr 4, 2024
1efd289
Fix assembly serializer spec for checking the file URLs
ahukkanen Apr 4, 2024
cdb375c
Consider that the asset can be `nil` in the generators spec
ahukkanen Apr 4, 2024
cbaf4c0
Fix participatory process group type spec for checking the file URLs
ahukkanen Apr 4, 2024
bd07cf1
Test the participatory process presenter without setting the current …
ahukkanen Apr 4, 2024
456ce1e
Ensure that `VariantWithRecord` also exists at the storage service
ahukkanen Apr 8, 2024
cbb1de8
Merge branch 'develop' into fix/storage-service-uris
ahukkanen Apr 8, 2024
dcedcc9
Method naming and add documentation to the new method
ahukkanen Apr 8, 2024
fe155a6
Add a note about checking that the variant exists at the storage
ahukkanen Apr 8, 2024
7968922
Add a spec for unexisting and processed variant with record
ahukkanen Apr 8, 2024
f6e7e62
Use the `.to be_present` matcher instead of `.not_to be_nil`
ahukkanen Apr 8, 2024
44ee5b2
Improve the regexp matching in the asset router
ahukkanen Apr 10, 2024
31c4b84
Fix passing incompatible URL options to `ActiveStorage::Variant#url`
ahukkanen Apr 10, 2024
9a833a3
Merge branch 'develop' into fix/storage-service-uris
ahukkanen Apr 10, 2024
9ffd03a
Simplify logic
ahukkanen Apr 10, 2024
32ab6f1
Apply suggestions from code review
ahukkanen Apr 16, 2024
d5366b7
Merge branch 'develop' into fix/storage-service-uris
ahukkanen Apr 17, 2024
2001860
Fix unescaped dots in the spec regexps
ahukkanen Apr 17, 2024
accbe48
Merge branch 'fix/storage-service-uris' of github.com:decidim/decidim…
ahukkanen Apr 17, 2024
fb1a97c
Fix unescaped host name dots in the spec regexps
ahukkanen Apr 17, 2024
3d5f19d
Fix rubocop violation
ahukkanen Apr 17, 2024
9272758
Fix error when trying to display image with empty source
ahukkanen Apr 17, 2024
ac22423
Use `Regexp.escape` instead of escaping dots only
ahukkanen Apr 17, 2024
ad148df
Fix endless loop in the `InitiativeGCell`
ahukkanen Apr 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class ApplicationController < ::DecidimController
include Headers::ContentSecurityPolicy
include DisableRedirectionToExternalHost
include Decidim::Admin::Concerns::HasBreadcrumbItems
include ActiveStorage::SetCurrent

helper Decidim::Admin::ApplicationHelper
helper Decidim::Admin::AttributesDisplayHelper
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<div id="minimap" class="grid grid-cols-1 md:grid-cols-2">
<div class="minimap-tab">
<% if current_organization.favicon.attached? %>
<%= image_tag current_organization.attached_uploader(:favicon).path, class: "minimap-favicon" %>
<%= image_tag current_organization.attached_uploader(:favicon).url, class: "minimap-favicon" %>
<% else %>
<%= content_tag :div, nil, { class: "minimap-favicon placeholder", style: "width: #{image_width(current_organization, :favicon) * size_factor}px; height: #{image_height(current_organization, :favicon) * size_factor}px;" } do %>
<span><%= t("favicon", scope: "activemodel.attributes.organization") %></span>
Expand All @@ -13,7 +13,7 @@

<div class="minimap-header">
<% if current_organization.logo.present? %>
<%= image_tag current_organization.attached_uploader(:logo).path, class: "minimap-logo" %>
<%= image_tag current_organization.attached_uploader(:logo).url, class: "minimap-logo" %>
<% else %>
<%= content_tag :div, nil, { class: "minimap-logo placeholder", style: "width: #{image_width(current_organization, :logo) * size_factor}px; height: #{image_height(current_organization, :logo) * size_factor}px;" } do %>
<span><%= t("logo", scope: "activemodel.attributes.organization") %></span>
Expand All @@ -26,7 +26,7 @@

<div class="minimap-footer">
<% if current_organization.official_img_footer.attached? %>
<%= image_tag current_organization.attached_uploader(:official_img_footer).path, class: "minimap-official_img_footer" %>
<%= image_tag current_organization.attached_uploader(:official_img_footer).url, class: "minimap-official_img_footer" %>
<% else %>
<%= content_tag :div, nil, { class: "minimap-official_img_footer placeholder", style: "width: #{image_width(current_organization, :official_img_footer) * size_factor}px; height: #{image_height(current_organization, :official_img_footer) * size_factor}px;" } do %>
<span><%= t("official_img_footer", scope: "activemodel.attributes.organization") %></span>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<div class="layout-nav">
<%= link_to decidim_admin.root_path, class: "logo hidden md:block", data: { "external-link": false }, aria: { label: t("decidim.accessibility.logo", organization: current_organization.name) } do %>
<% if current_organization.logo.present? %>
<%= image_tag current_organization.attached_uploader(:logo).path, alt: current_organization.name %>
<%= image_tag current_organization.attached_uploader(:logo).url, alt: current_organization.name %>
<% else %>
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 134 109">
<g fill="#fff">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,19 @@
expect(page).to have_css("input#attachment_description_en[value='#{translated(attachment.description, locale: :en)}']")
expect(page).to have_css("input#attachment_weight[value='#{attachment.weight}']")
expect(page).to have_select("attachment_attachment_collection_id", selected: translated(attachment_collection.name, locale: :en))
expect(page).to have_css("img[src~='#{attachment.url}']")

# The image's URL changes every time it is requested because the disk
# service generates a unique URL based on the expiry time of the link.
# This expiry time is calculated at the time when the URL is requested
# which is why it changes every time to different URL. This changes the
# JSON encoded file identifier which includes the expiry time as well as
# the digest of the URL because the digest is calculated based on the
# passed data.
filename = attachment.file.blob.filename
within %([data-active-uploads] [data-filename="#{filename}"]) do
src = page.find("img")["src"]
expect(src).to be_blob_url(attachment.file.blob)
end
end

it "can add attachments without a collection to a process" do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ module Decidim::Admin
command.call
organization.reload

expect(organization.attached_uploader(:favicon).variant_path(:small)).to be_present
expect(organization.attached_uploader(:favicon).variant_url(:small)).to be_present
end
end
end
Expand Down
7 changes: 5 additions & 2 deletions decidim-admin/spec/system/admin_manages_organization_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -146,16 +146,19 @@
context "when the admin terms of service content has an image with an alt tag" do
let(:another_organization) { create(:organization) }
let(:image) { create(:attachment, attached_to: another_organization) }
let(:image_url) { image.attached_uploader(:file).url(host: organization_host) }
let(:organization_host) { "example.lvh.me" }
let(:organization) do
create(
:organization,
host: organization_host,
admin_terms_of_service_body: Decidim::Faker::Localized.localized { terms_content }
)
end
let(:terms_content) do
<<~HTML.gsub(/\n\s*/, "")
<p>Paragraph</p>
<div class="editor-content-image" data-image=""><img src="#{image.url}" alt="foo bar"></div>
<div class="editor-content-image" data-image=""><img src="#{image_url}" alt="foo bar"></div>
HTML
end
let(:terms_content_editor) do
Expand All @@ -171,7 +174,7 @@
<span data-image-resizer-dimension="width" data-image-resizer-dimension-value="512"></span>
×
<span data-image-resizer-dimension="height" data-image-resizer-dimension-value="342"></span></div>
<div class="editor-content-image" data-image=""><img src="#{image.url}" alt="foo bar"></div>
<div class="editor-content-image" data-image=""><img src="#{image_url}" alt="foo bar"></div>
</div>
</div>
HTML
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ def resource_path
Decidim::Assemblies::Engine.routes.url_helpers.assembly_path(model)
end

def resource_image_path
model.attached_uploader(:hero_image).path
def resource_image_url
model.attached_uploader(:hero_image).url
end

def metadata_cell
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% add_decidim_meta_tags({
title: translated_attribute(current_participatory_space.title),
image_url: current_participatory_space.attached_uploader(:hero_image).path,
image_url: current_participatory_space.attached_uploader(:hero_image).url,
description: translated_attribute(current_participatory_space.short_description),
url: assembly_url(current_participatory_space)
}) %>
Expand Down
4 changes: 2 additions & 2 deletions decidim-assemblies/lib/decidim/api/assembly_type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ class AssemblyType < Decidim::Api::Types::BaseObject
field :children, [Decidim::Assemblies::AssemblyType, { null: true }], "Children of this assembly", null: false

def hero_image
object.attached_uploader(:hero_image).path
object.attached_uploader(:hero_image).url
end

def banner_image
object.attached_uploader(:banner_image).path
object.attached_uploader(:banner_image).url
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ module Decidim

describe "when images are attached" do
it "resolves hero_image_url" do
expect(subject.hero_image_url).to eq("http://#{organization_host}:#{Capybara.server_port}#{assembly.attached_uploader(:hero_image).path}")
expect(subject.hero_image_url).to be_blob_url(assembly.hero_image.blob)
end

it "resolves banner_image_url" do
expect(subject.banner_image_url).to eq("http://#{organization_host}:#{Capybara.server_port}#{assembly.attached_uploader(:banner_image).path}")
expect(subject.banner_image_url).to be_blob_url(assembly.banner_image.blob)
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,10 @@ module Decidim
subject { described_class.new(assembly_member).avatar_url }

context "when user is present" do
let(:user) { build(:user, name: "Julia G.", nickname: "julia_g") }
let(:user) { create(:user, name: "Julia G.", nickname: "julia_g") }
let(:assembly_member) { build(:assembly_member, full_name: "Full name", user:) }

it { is_expected.to eq user.attached_uploader(:avatar).path }
it { is_expected.to be_blob_url(user.avatar.blob) }
end

context "when no user is present" do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ module Decidim::Assemblies
expect(serialized).to include(weight: resource.weight)
expect(serialized).to include(short_description: resource.short_description)
expect(serialized).to include(description: resource.description)
expect(serialized).to include(remote_hero_image_url: Decidim::Assemblies::AssemblyPresenter.new(resource).hero_image_url)
expect(serialized).to include(remote_banner_image_url: Decidim::Assemblies::AssemblyPresenter.new(resource).banner_image_url)
expect(serialized[:remote_hero_image_url]).to be_blob_url(resource.hero_image.blob)
expect(serialized[:remote_banner_image_url]).to be_blob_url(resource.banner_image.blob)
expect(serialized).to include(promoted: resource.promoted)
expect(serialized).to include(developer_group: resource.developer_group)
expect(serialized).to include(meta_scope: resource.meta_scope)
Expand Down Expand Up @@ -137,7 +137,7 @@ module Decidim::Assemblies
expect(serialized_assembly_attachment).to include(title: attachment.title)
expect(serialized_assembly_attachment).to include(weight: attachment.weight)
expect(serialized_assembly_attachment).to include(description: attachment.description)
expect(serialized_assembly_attachment).to include(remote_file_url: Decidim::AttachmentPresenter.new(resource.attachments.first).attachment_file_url)
expect(serialized_assembly_attachment[:remote_file_url]).to be_blob_url(resource.attachments.first.file.blob)
end
end
end
Expand Down
13 changes: 11 additions & 2 deletions decidim-assemblies/spec/shared/manage_assemblies_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,17 @@

expect(page).to have_admin_callout("successfully")

expect(page).to have_css("img[src*='#{assembly.attached_uploader(:hero_image).path}']")
expect(page).to have_css("img[src*='#{assembly.attached_uploader(:banner_image).path}']")
hero_blob = assembly.hero_image.blob
within %([data-active-uploads] [data-filename="#{hero_blob.filename}"]) do
src = page.find("img")["src"]
expect(src).to be_blob_url(hero_blob)
end

banner_blob = assembly.hero_image.blob
within %([data-active-uploads] [data-filename="#{banner_blob.filename}"]) do
src = page.find("img")["src"]
expect(src).to be_blob_url(banner_blob)
end
end
end

Expand Down
4 changes: 2 additions & 2 deletions decidim-assemblies/spec/types/assembly_type_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,15 @@ module Assemblies
let(:query) { "{ heroImage }" }

it "returns the hero image field" do
expect(response["heroImage"]).to eq(model.attached_uploader(:hero_image).path)
expect(response["heroImage"]).to be_blob_url(model.hero_image.blob)
end
end

describe "bannerImage" do
let(:query) { "{ bannerImage }" }

it "returns the banner image field" do
expect(response["bannerImage"]).to eq(model.attached_uploader(:banner_image).path)
expect(response["bannerImage"]).to be_blob_url(model.banner_image.blob)
end
end

Expand Down
12 changes: 8 additions & 4 deletions decidim-assemblies/spec/types/integration_schema_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
"updatedAt" => assembly.assembly_type.updated_at.iso8601.to_s.gsub("Z", "+00:00")
},
"attachments" => [],
"bannerImage" => assembly.attached_uploader(:banner_image).path,
"categories" => [],
"children" => [],
"childrenCount" => 0,
Expand All @@ -40,7 +39,6 @@
"facebookHandler" => assembly.facebook_handler,
"githubHandler" => assembly.github_handler,
"hashtag" => assembly.hashtag,
"heroImage" => assembly.attached_uploader(:hero_image).path,
"id" => assembly.id.to_s,
"includedAt" => assembly.included_at.to_date.to_s,
"instagramHandler" => assembly.instagram_handler,
Expand Down Expand Up @@ -218,7 +216,10 @@
end

it "returns the correct response" do
expect(response["assemblies"].first).to eq(assembly_data)
data = response["assemblies"].first
expect(data).to include(assembly_data)
expect(data["bannerImage"]).to be_blob_url(assembly.banner_image.blob)
expect(data["heroImage"]).to be_blob_url(assembly.hero_image.blob)
end

it_behaves_like "implements stats type" do
Expand Down Expand Up @@ -372,7 +373,10 @@
end

it "returns the correct response" do
expect(response["assembly"]).to eq(assembly_data)
data = response["assembly"]
expect(data).to include(assembly_data)
expect(data["bannerImage"]).to be_blob_url(assembly.banner_image.blob)
expect(data["heroImage"]).to be_blob_url(assembly.hero_image.blob)
end

it_behaves_like "implements stats type" do
Expand Down
6 changes: 1 addition & 5 deletions decidim-blogs/app/cells/decidim/blogs/post_g_cell.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@ class PostGCell < Decidim::CardGCell

private

def has_image?
resource_image_path.present?
end

def show_description?
true
end
Expand All @@ -21,7 +17,7 @@ def metadata_cell
"decidim/blogs/post_metadata_g"
end

def resource_image_path
def resource_image_url
return if photo.blank?

photo.url
Expand Down
6 changes: 1 addition & 5 deletions decidim-blogs/app/cells/decidim/blogs/post_l_cell.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@ class PostLCell < Decidim::CardLCell

private

def has_image?
true
end

def has_description?
true
end
Expand All @@ -25,7 +21,7 @@ def metadata_cell
"decidim/blogs/post_metadata"
end

def resource_image_path
def resource_image_url
return if photo.blank?

photo.url
Expand Down
2 changes: 1 addition & 1 deletion decidim-blogs/spec/types/integration_schema_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"endorsements" => post.endorsements.map do |endo|
{
"__typename" => "User",
"avatarUrl" => endo.author.attached_uploader(:avatar).path(variant: :thumb),
"avatarUrl" => endo.author.attached_uploader(:avatar).variant_url(:thumb),
"badge" => "",
"deleted" => false,
"id" => endo.author.id.to_s,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ def resource_path
Decidim::Conferences::Engine.routes.url_helpers.conference_path(model)
end

def resource_image_path
model.attached_uploader(:hero_image).path
def resource_image_url
model.attached_uploader(:hero_image).url
end

def metadata_cell
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<div class="conference__grid-item-img">
<%= image_tag model.attached_uploader(:logo).path(variant: :medium), alt: "logo" %>
<%= image_tag model.attached_uploader(:logo).variant_url(:medium), alt: "logo" %>
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def name
def logo
return unless model.logo.attached?

"<div class='card p-m'> #{image_tag model.attached_uploader(:logo).path(variant: :medium), alt: "logo"} </div>"
"<div class='card p-m'> #{image_tag model.attached_uploader(:logo).variant_url(:medium), alt: "logo"} </div>"
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
<% end %>
</td>
<td>
<% if partner.attached_uploader(:logo).path %>
<%= image_tag(partner.attached_uploader(:logo).path(variant: :thumb)) %>
<% if partner.logo.attached? %>
<%= image_tag(partner.attached_uploader(:logo).variant_url(:thumb)) %>
<% end %>
</td>
<td class="table-list__actions">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<div class="diploma__content">
<div>
<div class="diploma__logo">
<%= wicked_pdf_image_tag @conference.attached_uploader(:main_logo).url(variant: :thumb, host: @conference.organization.host) %>
<%= wicked_pdf_image_tag @conference.attached_uploader(:main_logo).variant_url(:thumb) %>
</div>
<div class="diploma__name">
<h2><strong><%= translated_attribute(@conference.title) %></strong></h2>
Expand All @@ -14,7 +14,7 @@
<div class="diploma__attendance">
<strong><%= t("decidim.conferences.admin.send_conference_diploma_mailer.diploma_user.attendance_verified_by") %></strong>
<div>
<%= wicked_pdf_image_tag @conference.attached_uploader(:signature).url(variant: :thumb, host: @conference.organization.host) %>
<%= wicked_pdf_image_tag @conference.attached_uploader(:signature).variant_url(:thumb) %>
</div>
<%= l(@conference.sign_date, format: :decidim_short) %>, <%= @conference.signature_name %>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<section style="background-image:url('<%= current_participatory_space.attached_uploader(:banner_image).path %>');" data-conference-hero>
<section style="background-image:url('<%= current_participatory_space.attached_uploader(:banner_image).url %>');" data-conference-hero>
<div class="conference__hero">
<div class="conference__hero-text">
<h1 class="h1 text-5xl">
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% add_decidim_meta_tags({
title: translated_attribute(current_participatory_space.title),
image_url: current_participatory_space.attached_uploader(:hero_image).path,
image_url: current_participatory_space.attached_uploader(:hero_image).url,
description: translated_attribute(current_participatory_space.short_description),
url: conference_url(current_participatory_space)
}) %>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class ConferencePartnerType < Decidim::Api::Types::BaseObject
field :updated_at, Decidim::Core::DateTimeType, "The time this partner was updated", null: true

def logo
object.attached_uploader(:logo).path
object.attached_uploader(:logo).url
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class ConferenceSpeakerType < Decidim::Api::Types::BaseObject
field :updated_at, Decidim::Core::DateTimeType, "The time this member was updated", null: true

def avatar
object.attached_uploader(:avatar).path
object.attached_uploader(:avatar).url
end
end
end
Expand Down