Fix AS:Representations::ProxyController returning the wrong preview

When a blob is a representable of kind `previewable`, the preview
image that's being proxied is always the original preview image,
discarding completely the `variation_key` param passed in the request.

This commit fixes this by editing `Preview` and `VariantWithRecord` to
have full synchronized API with `Variant`. this will then allow the
ProxyController to not call `representable#image` but `representable`
instead.

As all 3 classes are now 100% interchangeable, we can deprecate the use
of `Representable#image`. Users of this class won't need to call this
method as it's become obsolete. and in case of `Preview#image`
erroneous.

Author: chaadow

activestorage/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   Fix `ActiveStorage::Representations::ProxyController` not returning the proper
+    preview image variant for previewable files.
+
+    *Chedli Bourguiba*
+
 *   Fix `ActiveStorage::Representations::ProxyController` to proxy untracked
     variants.
 

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

@@ -12,7 +12,7 @@ class ActiveStorage::Representations::ProxyController < ActiveStorage::Represent
 
   def show
     http_cache_forever public: true do
-      send_blob_stream @representation.image, disposition: params[:disposition]
+      send_blob_stream @representation, disposition: params[:disposition]
     end
   end
 end

activestorage/app/models/active_storage/preview.rb

@@ -31,8 +31,12 @@
 # These libraries are not provided by \Rails. You must install them yourself to use the built-in previewers. Before you
 # install and use third-party software, make sure you understand the licensing implications of doing so.
 class ActiveStorage::Preview
+  include ActiveStorage::Blob::Servable
+
   class UnprocessedError < StandardError; end
 
+  delegate :filename, :content_type, to: :presentation
+
   attr_reader :blob, :variation
 
   def initialize(blob, variation_or_variation_key)

activestorage/app/models/active_storage/variant_with_record.rb

@@ -5,6 +5,8 @@
 # Like an ActiveStorage::Variant, but keeps detail about the variant in the database as an
 # ActiveStorage::VariantRecord. This is only used if +ActiveStorage.track_variants+ is enabled.
 class ActiveStorage::VariantWithRecord
+  include ActiveStorage::Blob::Servable
+
   attr_reader :blob, :variation
   delegate :service, to: :blob
   delegate :content_type, to: :variation

activestorage/test/controllers/representations/proxy_controller_test.rb

@@ -18,10 +18,7 @@ class ActiveStorage::Representations::ProxyControllerWithVariantsTest < ActionDi
 
     assert_response :ok
     assert_match(/^attachment/, response.headers["Content-Disposition"])
-
-    image = read_image(@blob.variant(@transformations))
-    assert_equal 100, image.width
-    assert_equal 67, image.height
+    assert_equal @blob.variant(@transformations).download, response.body
   end
 
   test "showing variant inline" do
@@ -32,10 +29,7 @@ class ActiveStorage::Representations::ProxyControllerWithVariantsTest < ActionDi
 
     assert_response :ok
     assert_match(/^inline/, response.headers["Content-Disposition"])
-
-    image = read_image(@blob.variant(@transformations))
-    assert_equal 100, image.width
-    assert_equal 67, image.height
+    assert_equal @blob.variant(@transformations).download, response.body
   end
 
   test "showing untracked variant" do
@@ -48,7 +42,7 @@ class ActiveStorage::Representations::ProxyControllerWithVariantsTest < ActionDi
 
       assert_response :ok
       assert_match(/^attachment/, response.headers["Content-Disposition"])
-      assert_equal @blob.representation(@transformations).download, response.body
+      assert_equal @blob.variant(@transformations).download, response.body
     end
   end
 
@@ -74,52 +68,46 @@ class ActiveStorage::Representations::ProxyControllerWithVariantsTest < ActionDi
 class ActiveStorage::Representations::ProxyControllerWithVariantsWithStrictLoadingTest < ActionDispatch::IntegrationTest
   setup do
     @blob = create_file_blob filename: "racecar.jpg"
-    @blob.variant(resize_to_limit: [100, 100]).processed
+    @transformations = { resize_to_limit: [100, 100] }
+    @blob.variant(@transformations).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_to_limit: [100, 100]))
+        variation_key: ActiveStorage::Variation.encode(@transformations))
     end
+
     assert_response :ok
     assert_match(/^inline/, response.headers["Content-Disposition"])
-
-    @blob.reload # became free of strict_loading?
-    image = read_image(@blob.variant(resize_to_limit: [100, 100]))
-    assert_equal 100, image.width
-    assert_equal 67, image.height
+    assert_equal @blob.variant(@transformations).download, response.body
   end
 end
 
 class ActiveStorage::Representations::ProxyControllerWithPreviewsTest < ActionDispatch::IntegrationTest
   setup do
     @blob = create_file_blob filename: "report.pdf", content_type: "application/pdf"
+    @transformations = { resize_to_limit: [100, 100] }
   end
 
   test "showing preview inline" do
     get rails_blob_representation_proxy_url(
       filename: @blob.filename,
       signed_blob_id: @blob.signed_id,
-      variation_key: ActiveStorage::Variation.encode(resize_to_limit: [100, 100]))
+      variation_key: ActiveStorage::Variation.encode(@transformations))
 
     assert_response :ok
     assert_match(/^inline/, response.headers["Content-Disposition"])
-
-    assert_predicate @blob.preview_image, :attached?
-
-    image = read_image(@blob.preview_image.variant(resize_to_limit: [100, 100]).processed)
-    assert_equal 77, image.width
-    assert_equal 100, image.height
+    assert_equal @blob.preview(@transformations).download, response.body
   end
 
   test "showing preview with invalid signed blob ID" do
     get rails_blob_representation_proxy_url(
       filename: @blob.filename,
       signed_blob_id: "invalid",
-      variation_key: ActiveStorage::Variation.encode(resize_to_limit: [100, 100]))
+      variation_key: ActiveStorage::Variation.encode(@transformations))
 
     assert_response :not_found
   end
@@ -137,24 +125,20 @@ class ActiveStorage::Representations::ProxyControllerWithPreviewsTest < ActionDi
 class ActiveStorage::Representations::ProxyControllerWithPreviewsWithStrictLoadingTest < ActionDispatch::IntegrationTest
   setup do
     @blob = create_file_blob filename: "report.pdf", content_type: "application/pdf"
-    @blob.preview(resize_to_limit: [100, 100]).processed
+    @transformations = { resize_to_limit: [100, 100] }
+    @blob.preview(@transformations).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_to_limit: [100, 100]))
+        variation_key: ActiveStorage::Variation.encode(@transformations))
     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_to_limit: [100, 100]).processed)
-    assert_equal 77, image.width
-    assert_equal 100, image.height
+    assert_equal @blob.preview(@transformations).download, response.body
   end
 end