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.

Sign up for GitHub

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

Jump to bottom

fix sitemap.xml.gz not is not compressed #4622 #4661

Closed
wants to merge 6 commits into from
Closed

fix sitemap.xml.gz not is not compressed #4622 #4661

wants to merge 6 commits into from

Conversation

dobri1408
Copy link
Contributor

Fixes #4622

@dobri1408 dobri1408 requested a review from avoinea April 6, 2023 15:23
@netlify
Copy link

netlify bot commented Apr 6, 2023 •
edited
Loading

✅ Deploy Preview for volto canceled.

Name Link
🔨 Latest commit a59e56e
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/64305b4c7fd7b60008ccfa74

@cypress
Copy link

cypress bot commented Apr 6, 2023 •
edited
Loading

Passing run #4836 ↗︎

0 489 20 0 Flakiness 0

Details:

changelog
Project: Volto Commit: a3dced9106
Status: Passed Duration: 18:23 💡
Started: Apr 6, 2023 3:28 PM Ended: Apr 6, 2023 3:46 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

avoinea
avoinea approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@avoinea avoinea left a comment

Choose a reason for hiding this comment

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

$ file sitemap.xml.gz 
sitemap.xml.gz: gzip compressed data, from Unix, original size modulo 2^32 423340

@avoinea avoinea requested a review from sneridagh April 6, 2023 15:52
davisagli
davisagli reviewed Apr 6, 2023
View reviewed changes
Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

Why does it work with gz and not gzip? https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding mentions gzip but not gz

@davisagli
Copy link
Member

@avoinea How did you retrieve the file for the test? The success may depend on the specifics of the tool you used and how it interprets the header.

@avoinea
Copy link
Member

avoinea commented Apr 7, 2023

@dobri1408 @davisagli 🤔

Maybe that's why it works now. As gz is not a possible value for Content-Encoding, it's ignored and fallbacks to whatever file provides.

@davisagli
Copy link
Member

davisagli commented Apr 7, 2023 •
edited
Loading

@avoinea Ah, that's it.

Content-Encoding is for temporary encoding during transfer (normally we see it added by a frontend webserver gzipping an HTML response to send fewer bytes over the network). Content-Encoding: gzip tells the HTTP client it needs to ungzip the body of the response in order to get the original payload. So it does that and the resulting file is no longer gzipped, even though it has the .gz extension.

We should just not set Content-Encoding here.

davisagli
davisagli reviewed Apr 7, 2023
View reviewed changes
src/express-middleware/sitemap.js
@@ -5,7 +5,7 @@ export const sitemap = function (req, res, next) {
generateSitemap(req).then((sitemap) => {
if (Buffer.isBuffer(sitemap)) {
res.set('Content-Type', 'application/x-gzip');
res.set('Content-Encoding', 'gzip');
res.set('Content-Encoding', 'compress');
Copy link
Member

Choose a reason for hiding this comment

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

@dobri1408 This isn't correct either. We need to remove the Content-Encoding header entirely. Because the gzip compression is supposed to remain after the file is downloaded, not only during transfer.

@dobri1408
Copy link
Contributor Author

@davisagli Thank you for the review
I will test without the header and open a new pr, because I did a wrong commit, by mistake. I will put you and @avoinea as reviewers.

@dobri1408 dobri1408 closed this Apr 7, 2023
@dobri1408
Copy link
Contributor Author

#4663

@avoinea avoinea deleted the sitemap-error branch April 10, 2023 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davisagli davisagli davisagli left review comments

@avoinea avoinea avoinea approved these changes

@sneridagh sneridagh Awaiting requested review from sneridagh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sitemap.xml.gz is not compressed. Not gzip format
3 participants
@dobri1408 @davisagli @avoinea