-
Notifications
You must be signed in to change notification settings - Fork 753
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
base: main
Are you sure you want to change the base?
Conversation
Quality Gate passedIssues Measures |
Why not leveraging https://sling.apache.org/documentation/bundles/rendering-content-default-get-servlets.html#streamrendererservlet via request dispatching? |
@kwin I want to get rid of that stream approach where possible, not encourage it :-) |
@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 |
@kwin Unfortunately just sending the link to the raw URL of the blob is not enough in this case. |
To me the URI being generated in https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/15bfe9a6281618e5f14419c3979194cb5d0148f0/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/BinaryDownloadUriProvider.java#L111 and exposed via |
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.