Merge pull request rails#42535 from crawler/active-storage-strict-loading-support

Active storage representations controllers strict_loading_by_default support

Author: pixeltrix

activestorage/CHANGELOG.md

@@ -1,8 +1,12 @@
+*   Add support of `strict_loading_by_default` to `ActiveStorage::Representations` controllers
+
+    *Anton Topchii*, *Andrew White*
+
 *   Allow to detach an attachment when record is not persisted
 
     *Jacopo Beschi*
 
-*   Use libvips instead of ImageMagick to analyze images when `active_storage.variant_processor = vips`  
+*   Use libvips instead of ImageMagick to analyze images when `active_storage.variant_processor = vips`
 
     *Breno Gazzola*
 

activestorage/app/controllers/active_storage/representations/base_controller.rb

@@ -6,6 +6,10 @@ class ActiveStorage::Representations::BaseController < ActiveStorage::BaseContro
   before_action :set_representation
 
   private
+    def blob_scope
+      ActiveStorage::Blob.scope_for_strict_loading
+    end
+
     def set_representation
       @representation = @blob.representation(params[:variation_key]).processed
     rescue ActiveSupport::MessageVerifier::InvalidSignature

activestorage/app/controllers/concerns/active_storage/set_blob.rb

@@ -9,8 +9,12 @@ module ActiveStorage::SetBlob #:nodoc:
 
   private
     def set_blob
-      @blob = ActiveStorage::Blob.find_signed!(params[:signed_blob_id] || params[:signed_id])
+      @blob = blob_scope.find_signed!(params[:signed_blob_id] || params[:signed_id])
     rescue ActiveSupport::MessageVerifier::InvalidSignature
       head :not_found
     end
+
+    def blob_scope
+      ActiveStorage::Blob
+    end
 end

activestorage/app/models/active_storage/blob.rb

@@ -148,6 +148,14 @@ def combine_signed_id_purposes(purpose) #:nodoc:
     def signed_id_verifier #:nodoc:
       @signed_id_verifier ||= ActiveStorage.verifier
     end
+
+    def scope_for_strict_loading #:nodoc:
+      if strict_loading_by_default? && ActiveStorage.track_variants
+        includes(variant_records: { image_attachment: :blob }, preview_image_attachment: :blob)
+      else
+        all
+      end
+    end
   end
 
   # Returns a signed ID for this blob that's suitable for reference on the client-side without fear of tampering.

activestorage/test/controllers/representations/proxy_controller_test.rb

@@ -41,6 +41,29 @@ class ActiveStorage::Representations::ProxyControllerWithVariantsTest < ActionDi
   end
 end
 
+class ActiveStorage::Representations::ProxyControllerWithVariantsWithStrictLoadingTest < ActionDispatch::IntegrationTest
+  setup do
+    @blob = create_file_blob filename: "racecar.jpg"
+    @blob.variant(resize: "100x100").processed
+  end
+
+  test "showing existing variant record"  do
+    with_strict_loading_by_default do
+      get rails_blob_representation_proxy_url(
+        filename: @blob.filename,
+        signed_blob_id: @blob.signed_id,
+        variation_key: ActiveStorage::Variation.encode(resize: "100x100"))
+    end
+    assert_response :ok
+    assert_match(/^inline/, response.headers["Content-Disposition"])
+
+    @blob.reload # became free of strict_loading?
+    image = read_image(@blob.variant(resize: "100x100"))
+    assert_equal 100, image.width
+    assert_equal 67, image.height
+  end
+end
+
 class ActiveStorage::Representations::ProxyControllerWithPreviewsTest < ActionDispatch::IntegrationTest
   setup do
     @blob = create_file_blob filename: "report.pdf", content_type: "application/pdf"
@@ -80,3 +103,28 @@ class ActiveStorage::Representations::ProxyControllerWithPreviewsTest < ActionDi
     assert_response :not_found
   end
 end
+
+class ActiveStorage::Representations::ProxyControllerWithPreviewsWithStrictLoadingTest < ActionDispatch::IntegrationTest
+  setup do
+    @blob = create_file_blob filename: "report.pdf", content_type: "application/pdf"
+    @blob.preview(resize: "100x100").processed
+  end
+
+  test "showing existing preview record" do
+    with_strict_loading_by_default do
+      get rails_blob_representation_proxy_url(
+        filename: @blob.filename,
+        signed_blob_id: @blob.signed_id,
+        variation_key: ActiveStorage::Variation.encode(resize: "100x100"))
+    end
+
+    assert_response :ok
+    assert_match(/^inline/, response.headers["Content-Disposition"])
+    @blob.reload # became free of strict_loading?
+    assert_predicate @blob.preview_image, :attached?
+
+    image = read_image(@blob.preview_image.variant(resize: "100x100").processed)
+    assert_equal 77, image.width
+    assert_equal 100, image.height
+  end
+end

activestorage/test/controllers/representations/redirect_controller_test.rb

@@ -42,6 +42,31 @@ class ActiveStorage::Representations::RedirectControllerWithVariantsTest < Actio
   end
 end
 
+class ActiveStorage::Representations::RedirectControllerWithVariantsWithStrictLoadingTest < ActionDispatch::IntegrationTest
+  setup do
+    @blob = create_file_blob filename: "racecar.jpg"
+    @blob.variant(resize: "100x100").processed
+  end
+
+  test "showing existing variant record inline" do
+    with_strict_loading_by_default do
+      get rails_blob_representation_url(
+        filename: @blob.filename,
+        signed_blob_id: @blob.signed_id,
+        variation_key: ActiveStorage::Variation.encode(resize: "100x100"))
+    end
+
+    assert_redirected_to(/racecar\.jpg/)
+    follow_redirect!
+    assert_match(/^inline/, response.headers["Content-Disposition"])
+
+    @blob.reload # became free of strict_loading?
+    image = read_image(@blob.variant(resize: "100x100"))
+    assert_equal 100, image.width
+    assert_equal 67, image.height
+  end
+end
+
 class ActiveStorage::Representations::RedirectControllerWithPreviewsTest < ActionDispatch::IntegrationTest
   setup do
     @blob = create_file_blob filename: "report.pdf", content_type: "application/pdf"
@@ -81,3 +106,29 @@ class ActiveStorage::Representations::RedirectControllerWithPreviewsTest < Actio
     assert_response :not_found
   end
 end
+
+class ActiveStorage::Representations::RedirectControllerWithPreviewsWithStrictLoadingTest < ActionDispatch::IntegrationTest
+  setup do
+    @blob = create_file_blob filename: "report.pdf", content_type: "application/pdf"
+    @blob.preview(resize: "100x100").processed
+  end
+
+  test "showing existing preview record inline" do
+    with_strict_loading_by_default do
+      get rails_blob_representation_url(
+        filename: @blob.filename,
+        signed_blob_id: @blob.signed_id,
+        variation_key: ActiveStorage::Variation.encode(resize: "100x100"))
+    end
+
+    assert_predicate @blob.preview_image, :attached?
+    assert_redirected_to(/report\.png/)
+    follow_redirect!
+    assert_match(/^inline/, response.headers["Content-Disposition"])
+
+    @blob.reload # became free of strict_loading?
+    image = read_image(@blob.preview_image.variant(resize: "100x100"))
+    assert_equal 77, image.width
+    assert_equal 100, image.height
+  end
+end

activestorage/test/models/blob_test.rb

@@ -286,6 +286,33 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
+  test "scope_for_strict_loading adds includes only when track_variants and strict_loading_by_default" do
+    assert_empty(
+      ActiveStorage::Blob.scope_for_strict_loading.includes_values,
+      "Expected ActiveStorage::Blob.scope_for_strict_loading have no includes"
+    )
+
+    with_strict_loading_by_default do
+      includes_values = ActiveStorage::Blob.scope_for_strict_loading.includes_values
+
+      assert(
+        includes_values.any? { |values| values[:variant_records] == { image_attachment: :blob } },
+        "Expected ActiveStorage::Blob.scope_for_strict_loading to have variant_records included"
+      )
+      assert(
+        includes_values.any? { |values| values[:preview_image_attachment] == :blob },
+        "Expected ActiveStorage::Blob.scope_for_strict_loading to have preview_image_attachment included"
+      )
+
+      without_variant_tracking do
+        assert_empty(
+          ActiveStorage::Blob.scope_for_strict_loading.includes_values,
+          "Expected ActiveStorage::Blob.scope_for_strict_loading have no includes"
+        )
+      end
+    end
+  end
+
   private
     def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil, service_name: :local)
       filename ||= blob.filename

activestorage/test/test_helper.rb

@@ -125,6 +125,20 @@ def with_service(service_name)
     ensure
       ActiveStorage::Blob.service = previous_service
     end
+
+    def with_strict_loading_by_default(&block)
+      strict_loading_was = ActiveRecord::Base.strict_loading_by_default
+      ActiveRecord::Base.strict_loading_by_default = true
+      yield
+      ActiveRecord::Base.strict_loading_by_default = strict_loading_was
+    end
+
+    def without_variant_tracking(&block)
+      variant_tracking_was = ActiveStorage.track_variants
+      ActiveStorage.track_variants = false
+      yield
+      ActiveStorage.track_variants = variant_tracking_was
+    end
 end
 
 require "global_id"