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

[ActiveStorage] Prevent AS::Preview#processed to generate a variant for an empty transformation #50046

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

chaadow
Copy link
Contributor

@chaadow chaadow commented Nov 13, 2023

Followup #50044 cc @jonathanhefner

Motivation / Background

if an empty hash is passed to a preview call (blob.preview({})) We go through the original preview instead of regenerating a variation based on the original preview image which would result in a performance penalty.

Detail

Calls to #url or #key would now use the original blob.preview_image instead of generating a variant that has an empty transformation.

==> Resulting in saving both a MiniMagic/vips call. + a network/IO call to the storage service

Additional information

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 chaadow force-pushed the refactor_preview_tests_and_code branch 6 times, most recently from b4d327a to 2686bfb Compare November 13, 2023 19:37
@chaadow chaadow force-pushed the refactor_preview_tests_and_code branch 3 times, most recently from 332b7ea to df01397 Compare November 13, 2023 23:41
activestorage/app/models/active_storage/preview.rb Outdated Show resolved Hide resolved
activestorage/app/models/active_storage/preview.rb Outdated Show resolved Hide resolved
activestorage/app/models/active_storage/preview.rb Outdated Show resolved Hide resolved
activestorage/app/models/active_storage/preview.rb Outdated Show resolved Hide resolved
activestorage/app/models/active_storage/preview.rb Outdated Show resolved Hide resolved
activestorage/test/models/preview_test.rb Outdated Show resolved Hide resolved
activestorage/test/models/preview_test.rb Outdated Show resolved Hide resolved
activestorage/test/models/preview_test.rb Outdated Show resolved Hide resolved
activestorage/test/models/preview_test.rb Outdated Show resolved Hide resolved
activestorage/CHANGELOG.md Outdated Show resolved Hide resolved
@chaadow chaadow force-pushed the refactor_preview_tests_and_code branch 2 times, most recently from 7bb9a6b to d58c1b6 Compare November 14, 2023 19:13
@chaadow chaadow force-pushed the refactor_preview_tests_and_code branch 3 times, most recently from 998f2f0 to a4b36fe Compare November 14, 2023 19:49
@chaadow chaadow force-pushed the refactor_preview_tests_and_code branch from a4b36fe to fd721c7 Compare November 14, 2023 21:44
@chaadow
Copy link
Contributor Author

chaadow commented Nov 14, 2023

I've squashed and rebased with main.

@chaadow chaadow force-pushed the refactor_preview_tests_and_code branch from fd721c7 to 1a2e644 Compare November 14, 2023 21:54
@chaadow chaadow force-pushed the refactor_preview_tests_and_code branch 2 times, most recently from b0841d8 to 7493b6d Compare November 15, 2023 13:23
Copy link
Member

@jonathanhefner jonathanhefner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you recently pushed many documentation changes. Would you mind splitting those out into separate PRs unless they are directly related to this change? It makes things easier to review. Also, keep in mind that this is an optimization for a corner case, so I don't think we need to extensively document it (if at all).

activestorage/test/models/preview_test.rb Outdated Show resolved Hide resolved
@chaadow chaadow force-pushed the refactor_preview_tests_and_code branch from 7493b6d to f4a0bf5 Compare November 15, 2023 19:54
@chaadow
Copy link
Contributor Author

chaadow commented Nov 15, 2023

Would you mind splitting those out into separate PRs unless they are directly related to this change?

@jonathanhefner I changed all of processed, url, key and download, and they're publicly documented API, I thought I would update the documentation to reflect the new reality of these methods. Even though it's an "edge case", I thought documentation should elicit all possible cases.

Moreover, contextually speaking, since it's a followup from our past Pull request that we co-authored, in which we changed #processed but we forgot to update the documentation, (and I also re-changed this method again), I thought I might update to further help the readers with multiple examples that could potentially help them by adding more context.

I will revert and create another PR.

Thanks again for the prompt and exhaustive review!

if an empty hash is passed to a preview call (`blob.preview({})`)
We go through the original preview instead of regenerating a variation
based on the original preview image which would result in a performance
penalty
@chaadow chaadow force-pushed the refactor_preview_tests_and_code branch from f4a0bf5 to eae1965 Compare November 15, 2023 20:01
@chaadow
Copy link
Contributor Author

chaadow commented Nov 15, 2023

@jonathanhefner I've applied the last changes, squashed the commits, and rebased with main.

@jonathanhefner jonathanhefner merged commit f98bb7e into rails:main Nov 15, 2023
4 checks passed
@jonathanhefner
Copy link
Member

Thank you, @chaadow! ✔️

@nicowenterodt
Copy link
Contributor

Thanks from me as well. Very useful 👏 @chaadow @jonathanhefner


preview.processed

freeze_time { assert_equal blob.preview_image.url, preview.url }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chaadow I've stumbled upon this now, why is this here? If I remove it the tests still pass for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants