[ActiveStorage] Fix TransformJob to preprocess preview image variant

[chaadow]

Followup #50006 cc @jonathanhefner @nicowenterodt

Motivation / Background

This Pull Request has been created because TransformJob for previewable files only preprocess the preview image, but not the associated preview_image variant.

Detail

This Pull Request changes ActiveStorage::TransformJob to also trigger the preprocessing of the variant. It is also compatible with both previewable and variable records, as they both implement the #key method

Additional information

Every ActiveStorage::Blob has a has_one_attached :preview_image which is technically another AS blob.

Right now ActiveStorage::Preview#processed only generates the preview_image blob, but not the variant that's associated with it (with transformations available in the @variation instance variable)

One way to solve this is to force the ActiveStorage::TransformJob to preprocess the variant as well by calling ActiveStorage::Preview#key ( or ActiveStorage::Variant#key but it just calculates a key here) this would return the key value that's inside the variant, which can be safely discarded in the context of ActiveStorage::TransformJob

The ActiveStorage::Preview#processed method can be subject to a refactor to handle multiple transformations depending on the @variation instance variable ( which might require refactoring the #processed? method )

the ActiveStorage::Preview#key, as well as ActiveStorage::Preview#url, also can be refactored, because right now they perform two things:

1. Generate a variant ( System and network calls )
2. return the key or url

Compared to the ActiveStorage::Variant#key that simply calculates a key

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[chaadow]

since the preview_image does not exist before calling the job, variant(transformations) returns nil, that's why i had to add th &.send

[chaadow]

Since representation makes the assumption that it returns an object that responds to processed, I figured it wouldn't hurt to add this key call
@jonathanhefner

[chaadow]

(also key and url are publicly documented methods are usually used after calling processed cf. guide )

[nicowenterodt]

I can confirm that this works for me. The variant of the preview is now being pre-generated as it should. However I still don't get why simply calling key solves that 🤔

[chaadow]

I can confirm that this works for me. The variant of the preview is now being pre-generated as it should. However I still don't get why simply calling key solves that 🤔

@nicowenterodt A bit magical but if you look at either key or url here, it calls the variant private instance method :

rails/activestorage/app/models/active_storage/preview.rb

Lines 72 to 78 in 9194b33

	def key
	if processed?
	variant.key
	else
	raise UnprocessedError
	end
	end

and variant looks like this:

rails/activestorage/app/models/active_storage/preview.rb

Lines 106 to 108 in 9194b33

	def variant
	image.variant(variation).processed
	end

which itself creates the variant AND calls processed

Since variant is a private method i resorted to calling key. however i could have simply done : send(:variant)

It took me a bit of time to be 100% sure of what's happening. Maybe variant should become a public instance method instead.

EDIT : Oh and variant calls image which is defined as :

rails/activestorage/app/models/active_storage/preview.rb

Lines 54 to 56 in 9194b33

	def image
	blob.preview_image
	end

and preview_image is a has_one_attached on ActiveStorage::Blob. A lot of indirection, but basically that's how simply calling url or key works here.

[nicowenterodt]

@chaadow thanks for pinpointing this one out. 👍

[jonathanhefner]

Thank you for investigating this! I think the best approach is similar to your most recent commit — changing ActiveStorage::Preview#processed to also process the variant. However, as you probably noticed, that causes some tests to fail.

I have been working on a similar solution. I addressed the test failures in #50043 (and #50041), and I've just submitted #50044 with you as a co-author.

[chaadow]

@jonathanhefner thank you. while spelunking the code and tests, I realized the tests in preview_test.rb are a bit misleading, and would love to add new ones as well as change some of them if you don't mind.

Is it possible to push your code onto my PR, or should I create a new PR.
for example this test :

  test "previewing a PDF" do
    blob = create_file_blob(filename: "report.pdf", content_type: "application/pdf")
    # variation = { resize_to_limit: [640, 280] }
    preview = blob.preview({}).processed

    assert_predicate preview.image, :attached?
    assert_equal "report.png", preview.image.filename.to_s
    assert_equal "image/png", preview.image.content_type

    image = read_image(preview.image)
    assert_equal 612, image.width
    assert_equal 792, image.height
  end

1 - At first i thought that { resize_to_limit: [640, 280] } would have any impact on the resulting preview, but you can even put an empty hash, and the test would still pass
==> i thought for these tests to simply change them to an empty hash so that the reader understands that those values have no impact on the test.

2- also i was wondering if we pass an empty hash, should we add a conditional to skip the variant.processed call?

3- Just like this test, I would like to add a new test that read_image on the variant generated, and check its size

[jonathanhefner]

i thought for these tests to simply change them to an empty hash so that the reader understands that those values have no impact on the test.

If I understand correctly, I think the way the test is currently written is better. It tests that the preview image has an expected size even when a variant is specified. In other words, that specifying a variant does not impact the full-sized preview image. But if I am misunderstanding, feel free to open a PR so I can see the proposed change.

also i was wondering if we pass an empty hash, should we add a conditional to skip the variant.processed call?

Maybe...? I would have to dig into it to see whether it would be feasible / good. But feel free to open a PR for that as well, if you think it would be an improvement.