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

[bug]: responses with 404 code should not be cached #2551

Closed
1 of 8 tasks
mercs600 opened this issue Jul 13, 2020 · 3 comments · Fixed by #2778
Closed
1 of 8 tasks

[bug]: responses with 404 code should not be cached #2551

mercs600 opened this issue Jul 13, 2020 · 3 comments · Fixed by #2778
Assignees
Labels
bug Something isn't working help wanted Eligible for community contribution. Priority: P2 Progress: done Severity: S2

Comments

@mercs600
Copy link
Contributor

mercs600 commented Jul 13, 2020

Describe the bug
Hi guys. I see we use apicache connected with hastly to optimize and cache images. We face with the issue caching of 404 responses on media proxy. I mean when something is wrong with the media server and proxy is not able to create image then we get 404, but after when media server is back and images are available, then cache is still valid and we see 404 error page still.

The second issue is about webp support and caching:

When you make a deploy and you open the website on Chrome first (which supports webp) you will be able to see webp images. Then if you open the same page on the safari (which doesn't supports webp) you won't be able to see images because they were cached as webp with the first hit ( on chrome)

To reproduce
Steps to reproduce the behavior:

  1. When you use image optimize onboard
  2. Open some image which doesn't exist on your media server
  3. You should see 404 code response and cache headers
  4. Create this missing image on backend server
  5. You should see 404 code response

Expected behavior
Images with 404 and error headers should not be cached.

Screenshots
Zrzut ekranu 2020-07-13 o 10 38 12

Possible solutions
My solution is simple according to documentation of apicache, but should we done it here or maybe it should be fixed on server side, because this solution is transfering the headers ?

  cacheMiddleware = cache(cacheExpires, hastily.hasSupportedExtension, {
            debug: cacheDebug,
            statusCodes: {
                include: [200, 304]
              },
        });

EDIT

include, exclude option in statusCode brings no results until we changed second parameter of apicache instance this is
hastily.hasSupportedExtension:

(req, res) => hastily.hasSupportedExtension(req) && res.statusCode === 200

Can I change this setup from my project level - webpackConfig or somewhere ? Because now I have to modify packages/pwa-buildpack/lib/Utilities/addImgOptMiddleware.js to make it works.

Please let us know what packages this bug is in regards to:

  • venia-concept
  • venia-ui
  • pwa-buildpack
  • peregrine
  • pwa-devdocs
  • upward-js
  • upward-spec
  • create-pwa
@mercs600 mercs600 added the bug Something isn't working label Jul 13, 2020
@m2-community-project m2-community-project bot added this to Ready for Grooming in Community Issue Tracking Jul 13, 2020
@awilcoxa
Copy link

Created PWA-749 in backlog for grooming

@awilcoxa awilcoxa added Priority: P2 Severity: S2 help wanted Eligible for community contribution. labels Jul 13, 2020
@tjwiebell tjwiebell added this to Prioritized in Community Backlog Jul 14, 2020
@zetlen
Copy link
Contributor

zetlen commented Aug 6, 2020

@mercs600 Thanks for your patience. Maybe the simplest solution here would be to remove the apicache middleware entirely. The image opt middleware is NOT meant to directly handle HTTP requests in production; caching should be handled in another layer, ideally the web server or CDN.

The only reason we'd keep this middleware is if there's a severe performance degradation when you remove it. Can you do me a favor, @mercs600, and remove the apicache middleware entirely, and see what happens to your development and staging server performance?

@larsroettig
Copy link
Member

@magento I am working on this

@m2-community-project m2-community-project bot moved this from Ready for Grooming to Dev In Progress in Community Issue Tracking Sep 2, 2020
@tjwiebell tjwiebell moved this from Prioritized to In progress in Community Backlog Sep 4, 2020
@m2-community-project m2-community-project bot moved this from Dev In Progress to Pull Request In Progress in Community Issue Tracking Sep 27, 2020
@m2-community-project m2-community-project bot moved this from Pull Request In Progress to Ready for Grooming in Community Issue Tracking Oct 2, 2020
@tjwiebell tjwiebell moved this from In progress to Prioritized in Community Backlog Oct 6, 2020
@tjwiebell tjwiebell added the hold On hold until another condition is fulfilled. label Oct 6, 2020
@tjwiebell tjwiebell moved this from Prioritized to Backlog in Community Backlog Oct 6, 2020
mercs600 added a commit to mercs600/pwa-studio that referenced this issue Oct 13, 2020
@m2-community-project m2-community-project bot moved this from Ready for Grooming to Pull Request In Progress in Community Issue Tracking Oct 13, 2020
@tjwiebell tjwiebell moved this from Backlog to In progress in Community Backlog Oct 20, 2020
@tjwiebell tjwiebell moved this from In progress to In Review in Community Backlog Oct 20, 2020
@tjwiebell tjwiebell removed the hold On hold until another condition is fulfilled. label Oct 20, 2020
Community Issue Tracking automation moved this from Pull Request In Progress to Done Oct 28, 2020
dpatil-magento added a commit that referenced this issue Oct 28, 2020
* fix: fix unsoported webp for safari #2551

* - Use accept header as cache key
- Update test

Co-authored-by: Tommy Wiebell <twiebell@adobe.com>
Co-authored-by: Devagouda <40405790+dpatil-magento@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Eligible for community contribution. Priority: P2 Progress: done Severity: S2
Projects
Development

Successfully merging a pull request may close this issue.

5 participants