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

#2853 The AdaptiveImageServlet should send redirects to blobs #2871

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

joerghoh
Copy link
Collaborator

Q                       A
Fixed Issues? Fixes #2853
Patch: Bug Fix?
Minor: New Feature?
Major: Breaking Change?
Tests Added + Pass? Yes
Documentation Provided Yes (code comments and or markdown)
Any Dependency Changes?
License Apache License, Version 2.0

The AdaptiveImageServlet always streams its result through the JVM, even if its an already existing rendition with a matching blob in the blobstore. In AEM CS can delivered more efficiently with sending a redirect to the blobstore, and then the Fastly CDN follows this redirect and pulls the data directly from the blobstore. Then AEM does not need to stream that binary through the JVM.

This patch improves the handling not to stream the binary when possible. The feature is configurable and off by default.

(A word to the tests: In JUnit5 it's not possible to parametrize entire test classes anymore, which would have been my preferred way for the tests. Instead you would have to parametrize the tests on their own. To avoid that I duplicated the tests which were failing with the new feature turned and tagged the duplicates accordingly. Now the setup() can initialize the AdaptiveImageServlet accordingly. Happy to learn about a way, which allows to run each tests with the new feature turned off and on, without annotating each test.

Existing tests were not changed, to make sure that all the changes did not affect the default behavior.

Copy link

sonarcloud bot commented Sep 29, 2024

@kwin
Copy link
Contributor

kwin commented Sep 29, 2024

@joerghoh
Copy link
Collaborator Author

@kwin I want to get rid of that stream approach where possible, not encourage it :-)

@kwin
Copy link
Contributor

kwin commented Sep 30, 2024

@joerghoh Please read both documentation and code more thoroughly. That streamrendererservlet does the redirect exactly as you implemented (despite its name): https://github.com/apache/sling-org-apache-sling-servlets-get/blob/507a84c5cc2ede84710dd719f97bb483629b969e/src/main/java/org/apache/sling/servlets/get/impl/helpers/StreamRenderer.java#L159

@joerghoh
Copy link
Collaborator Author

@kwin Unfortunately just sending the link to the raw URL of the blob is not enough in this case.

@kwin
Copy link
Contributor

kwin commented Sep 30, 2024

@joerghoh
Copy link
Collaborator Author

joerghoh commented Oct 3, 2024

Yes, that's correct, I will adapt the PR accordingly. Thanks @kwin for the pointer, I was not aware that we have in Sling that code already.

Need to wait for #2875 to be merged, before I can continue here.

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

Successfully merging this pull request may close these issues.

AdaptiveImageServlet should not stream binaries
2 participants