Skip to content

fix(cdn): MERC-9364 Use CDN Original images for webdav - cache control #298

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

christensenep
Copy link

@christensenep christensenep commented Dec 29, 2023

What? Why?

! DO NOT MERGE !

Use CDN Original images for webdav - cache control. Take 2. See bigcommerce/paper#343

If a developer/merchant uses the cdn handlebars helper to inject images from webdav into the theme using e.g.({{cdn "webdav:/img/image.jpg"}}), there is no cache-control header when the image is fetched on the storefront.

Similar to our Image Manager, we should be using the cdn original images (Similar to Jinsoo's PR here MERC-7127)

This PR differs from the previous as it only does this for file suffixes that image manager supports (jpg/jpeg/png/gif). This avoids the issue where the url is changed for e.g. a pdf, and thus creates a broken link.

How was it tested?

Tested locally and in integration, and with unit tests. Have verified that images (png/gif/jpeg/jpg) uploaded to the content directory via webdav can be accessed at their cache-controlled url using the cdn helper (/images/stencil/original/content/<whatever>), while all other file extensions still return the /content/<whatever> path when using the helper.

Behold, the results of outputting some of these urls directly to the DOM:
Screenshot 2024-01-11 at 4 40 25 PM

cc @bigcommerce/storefront-team

@christensenep christensenep changed the title MERC-9364 Use CDN Original images for webdav - cache control fix: MERC-9364 Use CDN Original images for webdav - cache control Dec 29, 2023
@christensenep christensenep force-pushed the MERC-9364 branch 2 times, most recently from 06cc938 to a52423f Compare December 29, 2023 01:02
@christensenep
Copy link
Author

♻️ I have tested this with gifs, jpeg/jpgs and pngs, and verified that the new urls are valid and have cache control headers. I have also verified that other file extensions such as pdfs and, importantly, bmps do not change the url from its current form and are thus still valid. I also verified that bmps do not have the images/stencil/content form of the url available, so I believe this is the correct and exhaustive of extensions we should be doing this for.

@christensenep
Copy link
Author

@bigcommerce/team-merchandising @bigcommerce/team-storefront

jairo-bc
jairo-bc previously approved these changes Jan 15, 2024
@bc-alexsaiannyi bc-alexsaiannyi force-pushed the MERC-9364 branch 2 times, most recently from 82294e2 to 3344da0 Compare March 6, 2025 13:15
@bc-alexsaiannyi bc-alexsaiannyi force-pushed the MERC-9364 branch 2 times, most recently from 8b8ca19 to c9f8d6b Compare March 17, 2025 20:44
@bc-alexsaiannyi bc-alexsaiannyi force-pushed the MERC-9364 branch 3 times, most recently from de37add to 8b2e272 Compare April 14, 2025 10:18
jmwiese
jmwiese previously approved these changes Apr 29, 2025
Copy link
Contributor

@jmwiese jmwiese left a comment

Choose a reason for hiding this comment

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

LGTM

@bc-alexsaiannyi bc-alexsaiannyi changed the title fix: MERC-9364 Use CDN Original images for webdav - cache control [DO NOT CLOSED] fix: MERC-9364 Use CDN Original images for webdav - cache control Apr 30, 2025
@bc-alexsaiannyi bc-alexsaiannyi changed the title [DO NOT CLOSED] fix: MERC-9364 Use CDN Original images for webdav - cache control [DO NOT CLOSE] fix: MERC-9364 Use CDN Original images for webdav - cache control Apr 30, 2025
@bc-alexsaiannyi bc-alexsaiannyi changed the title fix: MERC-9364 Use CDN Original images for webdav - cache control [ DO NOT MERGE] fix: MERC-9364 Use CDN Original images for webdav - cache control May 27, 2025
@bc-alexsaiannyi bc-alexsaiannyi changed the title [ DO NOT MERGE] fix: MERC-9364 Use CDN Original images for webdav - cache control fix(cdn): MERC-9364 Use CDN Original images for webdav - cache control May 28, 2025
@bc-alexsaiannyi bc-alexsaiannyi force-pushed the MERC-9364 branch 14 times, most recently from f766ffe to 1e15c95 Compare May 28, 2025 13:52
jairo-bc
jairo-bc previously approved these changes May 29, 2025
return [cdnUrl, 'content', path].join('/');
const supportedImageExtensions = ['jpg', 'jpeg', 'gif', 'png'];
const unsupportedPathSyntaxes = ['../', './'];
const searchParamIgnoredPath = path.split('?')[0];
Copy link

@bc-alexsaiannyi bc-alexsaiannyi Jun 5, 2025

Choose a reason for hiding this comment

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

we should cover cases when pathname contains searchParams like
'webdav:/img/brands-logos/main.png?cache_bust=1749005114992'
and exclude them from filtering on the next step

const supportedImageExtensions = ['jpg', 'jpeg', 'gif', 'png'];
const unsupportedPathSyntaxes = ['../', './'];
const searchParamIgnoredPath = path.split('?')[0];
const isImage = supportedImageExtensions.some(ext => searchParamIgnoredPath.toLowerCase().endsWith(ext));
Copy link

@bc-alexsaiannyi bc-alexsaiannyi Jun 5, 2025

Choose a reason for hiding this comment

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

previously used method includes() can lead to incorrect results. For example, if provided path looks like 'webdav:banners/menu-giftcard.svg' or 'webdav:/collection/gifts/image.pdf?cache-buster=2025-06-04T00:50:40' then we will get situation where path.toLowerCase().includes(ext)) return true and helper modified path for unsupported content.

Now the logic has been updated, so firstly we exclude searchParams from path and then check if it ends with supported extension.

Copy link
Member

@rtalvarez rtalvarez left a comment

Choose a reason for hiding this comment

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

nice, looks good 👍

@@ -56,7 +56,14 @@ module.exports = globals => {
}

if (protocol === 'webdav:') {
return [cdnUrl, 'content', path].join('/');
const supportedImageExtensions = ['jpg', 'jpeg', 'gif', 'png'];
Copy link

Choose a reason for hiding this comment

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

Should we include . ?

Choose a reason for hiding this comment

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

I think no, Jay. On L61 I exclude any searchParams from path and what is left I check if it ends with supported extension. So for example User provides something like: webdav:/collection/gifts/image.pdf?cache-buster=2025-06-04T00:50:40.
We then check first if it webdav, then go and look for any searchParams and drop them so receive path: /collection/gifts/image.pdf and then check if it ends with supported extensions.

@bc-alexsaiannyi
Copy link

jfyi, I would like to reiterate and summarise changes related to webdav functionality:

  1. we currently support 4 types of images : jpeg, jpg, png and gif.
  2. Content like svg or pdf files won't be affected.
  3. We only provide cache-controlled url for images uploaded to content directory. That also means all previously uploaded content (images) to other directories won't be affected.

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

Successfully merging this pull request may close these issues.

7 participants