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

fix brotli responses being double-compressed #166

Merged
merged 1 commit into from
May 18, 2021

Conversation

dwickern
Copy link
Contributor

@dwickern dwickern commented May 17, 2021

Related to fastify/fastify-static#158

When used with fastify-static's preCompressed option, this plugin will double-compress .br files.

Currently this plugin avoids double-compressing for gzip and deflate, by inspecting the magic number in few bytes of the stream. However Brotli doesn't have a magic number to look for.

fastify-compress/index.js

Lines 491 to 494 in 714e4a9

switch (isCompressed(data)) {
case 1: return swap(null, new Minipass())
case 2: return swap(null, new Minipass())
}

fastify-compress/index.js

Lines 460 to 465 in 714e4a9

function isCompressed (data) {
if (isGzip(data)) return 1
if (isDeflate(data)) return 2
if (isZip(data)) return 3
return 0
}

Instead we can check for an existing Content-Encoding header on the reply. This has the extra advantage that we don't need to strip off the Content-Length header.

Checklist

if (responseEncoding && responseEncoding !== 'identity') {
// response is already compressed
return next()
}
setVaryHeader(reply)
Copy link
Member

Choose a reason for hiding this comment

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

Should the new block be after setting the setVaryHeader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't fastify-static be responsible for setting the Vary header?

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the answer. Anyway I'm landing - if it becomes a problem I guess we'll get another PR.

@mcollina mcollina merged commit 3d3ac7a into fastify:master May 18, 2021
@mcollina
Copy link
Member

Unfortunately I landed without the test passing. I'm going to rever the commit, would you mind sending a fresh PR?

Thanks!

mcollina added a commit that referenced this pull request May 18, 2021
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.

2 participants