feat: Adding support for brolti #173

nicksrandall · 2020-09-15T02:24:40Z

Fixes #71

TL;DR

There has been several attempts (#172, #158) to add brotli compression support to this library and so far none have been successfully merged (for various reasons). This PR is yet another attempt to support brotli compression in a backwards compatible way while also reducing existing code change as much as possible to ensure stability of the library.

What is the problem that is trying to be solved?

The brotli compression algorithm is generally more efficient than gzip and now supported in recent versions of Node.js and in all major browsers. Http clients (like web browsers) can specify an "Accept-Encoding" header in their requests to share which compression algorithms they support and prefer. According to the spec, when two values have the same preference, the first value will be used (seems reasonable right?).

However, many browsers (including Google Chrome) send the "br" (brotli) value last even though it is generally preferred to the other algorithms (ie they send gzip, deflate, br instead of br, gzip, deflate). This causes the brotli compression algorithm to be de-prioritized and unused.

What can we do about it?

We can follow a well-established convention to deviate from the spec slightly and force the server to choose the "preferred" compression algorithm when the client has (basically) stated that it doesn't not explicitly prefer one algorithm over another.

So, for example, if we get an "Accept-Encoding" header value of gzip, deflate, br we will use br (brotli) because brotli is more efficient over gzip and their preference (set by client) is both 1 (the default value).

However, if we get gzip;q=0.8, br;q=0.1 (q is level of preference from 0 to 1 where 1 is most preferred) we will use 'gzip' because the client has explicitly stated that it is more preferred than brotli.

nicksrandall · 2020-09-15T03:45:25Z

@dougwilson this is ready for review... I think you'll like this PR much better

nicksrandall · 2020-09-17T01:06:28Z

@dougwilson what do you think of the direction in this PR? Anything I can do to help you with the QA/Approval process?

dougwilson · 2020-09-17T01:08:40Z

Hi @nicksrandall apologies if I didn't mention it before: sorry I haven't been able to take a look just yet; I am currently spending all my allocated OS time right now addressing some security issues that have been reported. Please give me about a week to get everything settled and I will be taking a look, thank you 🙏 -- I've noticed you have made about 6+ pushes to this branch even after you noted it was ready for review; it seems like it is still having some edge cases sorted out anyhow 😂

nicksrandall · 2020-10-01T18:05:37Z

Hi @dougwilson not trying to be pushy, just checking in to see where we are at with this PR? Any change you'll have some time to review anytime soon?

nicksrandall · 2020-10-01T18:12:47Z

The only potential issue I can see with this change is that I'm cloning the request object on every request before I sent it to the accepts library to determine which encoding method should be used. As far as I know, the accepts library only uses the headers property on the request object so I could only send the cloned (and altered) headers to the accepts library with the rest of the request object being omitted. Therefore I would have less to clone so that might be a bit more performant but it could break something if accepts library decides to use other parts of the request in the future.

dougwilson · 2020-10-01T18:13:20Z

The security issue is still on going at this time, I apologize. Github is the reporter and I have been working with their security group still on the situation.

dougwilson

I am on my phone, but tried to do review what I could to help move it along. It is not comprehensive and I didn't actually run anything, so please forgive me, but I thought that I could at least use my time here waiting in the dr. office to do something for you since you pinged me :)

dougwilson · 2020-10-01T18:15:50Z

index.js

-        method = accept.encoding(['gzip', 'identity'])
-      }
+      // compression method
+      var accept = accepts(objectAssign({}, res, { headers: headers }))


AFAIK the accepts API expects the request object, not a response object clone.

Good catch, this is a bug (although it didn't actually effect anything because accepts library only reads the headers). This has been fixed.

I'm not sure if this is even cloning the res correctly, anyhow; it seems to only copy over the own, enumerable properties; all the methods from the prototype and such are lost, for instance.

I've updated this code so that we don't pass in the entire (cloned) request and instead with just pass in the cloned headers. Thats all the accepts function reads from anyway.

So that of course does end up violating the API contract of that dependency. We should either hard-pin that dependency so that we have to manually bump the version for any changes since this would now be relying on the internal behavior of that dependency to not access anything in outside of that one property, or perhaps get the dependency to change/clarify the contract to only being the headers. If we do go the pin dependency route, we'll want to check that if it then passes the req object to a dependency of it's own, that that is pinned within that module as well. This will prevent the possibility of sudden breakage when dependencies, dependencies of dependencies, etc. update

dougwilson · 2020-10-01T18:18:17Z

index.js

+ *
+ * @private
+ */
+function prioritize (str) {


Looking at this, it seems like this can be an option added to the accept negotiation logic directly? Like an option to prefer server order over client order? I think that is effectively what this is doing, or am I misunderstanding?

Great idea. I have added a priorizeClient option that will allow users to opt-out of this functionality.

For what it's worth, we were already using this behavior because we were prioritizing GZIP over deflate even when deflate came first. As such, I think "opt-out" vs "opt-in" is acceptable in this case.

I'm sorry, I think I didn't explain very well my thought on this; it wasn't to add a parameter here to this library's API, but rather to the accepts API. As far as adding this as a parameter to this module's API, I'm not sure if it's actually necessary, though if it is going to be surfaced as a switch, I would kind of expect such a switch to default to the spec compliant method. But again, I'm not sure if this actually needs to be a switch for the user; a better user switch would actually be to turn on/off each individual compression method, but that would be out of scope for this PR.

I see what you are saying but we were already out of spec because we were prioritizing gzip over deflate even when deflate came first. The option to be fully spec compliant would be a new feature for this library.

I also will argue that most people do not want the spec compliant behavior and they will prefer the default option that we have here.

I think you are miss understanding what I am saying on this comment again, I'm really sorry. I must not be good at communicating my idea here through writing and that is my bad. I tried twice and it wasn't successful :( Perhaps if you desiure to move this forward would you be willing to do some kind of voice call?

@wesleytodd This was already included in version 2 of accepts, right?

dougwilson · 2020-10-01T18:19:14Z

index.js

+ * @private
+ */
+function sortEncodings (a, b) {
+  if (a.indexOf('br') >= 0) {


The encoding are case insensitive according to the spec, but this us treating them as case sensitive.

Another great catch, I now lowercase the value of the header before I look for any specific values.

This seems to now violate the spec in a different way: the q parameter name is case-sensitive, so br;q=0 header would have br with a quality of 0, but br;Q=0 would have a quality of 1. This change you made will now accidentally treat the latter as the former.

I see what you are saying. I've updated the code so that this sorting function will sort the items in the header but it will not change their original case. It will also work with any casing of encoding values.

Is there a reason why it is using .indexOf instead of just a comparison? I would think .indexOf would identify bro as br or similar. I'm not sure what that would all entail. I glanced again at the spec and this header doesn't allow random parameters to be specified, so it does seem unlikely to get mixed up in that way, but when looking, I realize that this header does accept the special * value. I know this sorting does not take that * into account there, so I wonder if that will cause issues with the logic? I tried a few different headers to see what the outcomes are and was curious if these make sense:

* : Results in br encoding
*, gzip: Results in gzip encoding
gzip, *: Results in gzip encoding

Abhijith-net · 2020-10-20T17:23:49Z

@nicksrandall Thanks for this change, is there a plan for closing this. We are planning to go for production.

index.js

Co-authored-by: Matthias Le Brun <[email protected]>

Kisama · 2021-02-26T00:26:15Z

Is there any plan for close this PR?

I'm waiting for this feature to release

nicksrandall · 2021-03-08T20:42:27Z

I think that #172 is more likely to land. I'll close this PR as soon as that PR lands.

bjohansebas · 2024-11-03T20:24:20Z

Closed in favor of #194, @nicksrandall thank you for the initiative. You can continue helping by reviewing #194 so we can launch it soon.

nicksrandall mentioned this pull request Sep 15, 2020

Added support for brotli ('br') content-encoding #172

Closed

nicksrandall force-pushed the nick/brotli branch 9 times, most recently from ceb509a to 688b0d4 Compare September 15, 2020 03:41

nicksrandall force-pushed the nick/brotli branch 6 times, most recently from 1c28fc3 to df37bab Compare September 15, 2020 14:49

nicksrandall mentioned this pull request Sep 15, 2020

Support for Brotli compression #71

Open

nicksrandall force-pushed the nick/brotli branch from df37bab to dc8e920 Compare September 17, 2020 00:59

dougwilson suggested changes Oct 1, 2020

View reviewed changes

nicksrandall force-pushed the nick/brotli branch from dc8e920 to 6447efa Compare October 1, 2020 18:52

feat: Adding support for brolti

3b31349

nicksrandall force-pushed the nick/brotli branch from 6447efa to 3b31349 Compare October 1, 2020 18:59

nicksrandall requested a review from dougwilson October 5, 2020 19:38

[NR] do not change capitalization of accept-encoding header

c7b68dd

chore: remove semicolons

c772ea4

bloodyowl reviewed Oct 21, 2020

View reviewed changes

index.js Show resolved Hide resolved

Update index.js

0331e38

Co-authored-by: Matthias Le Brun <[email protected]>

dougwilson added enhancement pr labels Dec 19, 2020

bjohansebas mentioned this pull request Sep 18, 2024

Release 1.8.0 #189

Open

bjohansebas closed this Nov 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adding support for brolti #173

feat: Adding support for brolti #173

nicksrandall commented Sep 15, 2020 •

edited

Loading

nicksrandall commented Sep 15, 2020

nicksrandall commented Sep 17, 2020

dougwilson commented Sep 17, 2020 •

edited

Loading

nicksrandall commented Oct 1, 2020

nicksrandall commented Oct 1, 2020

dougwilson commented Oct 1, 2020

dougwilson left a comment

dougwilson Oct 1, 2020

nicksrandall Oct 1, 2020

dougwilson Oct 5, 2020

nicksrandall Oct 8, 2020

dougwilson Oct 8, 2020

dougwilson Oct 1, 2020

nicksrandall Oct 1, 2020

dougwilson Oct 5, 2020

nicksrandall Oct 8, 2020 •

edited

Loading

dougwilson Oct 8, 2020

bjohansebas Sep 19, 2024

dougwilson Oct 1, 2020

nicksrandall Oct 1, 2020

dougwilson Oct 5, 2020

nicksrandall Oct 8, 2020

dougwilson Oct 8, 2020 •

edited

Loading

Abhijith-net commented Oct 20, 2020

Kisama commented Feb 26, 2021

nicksrandall commented Mar 8, 2021

bjohansebas commented Nov 3, 2024

feat: Adding support for brolti #173

feat: Adding support for brolti #173

Conversation

nicksrandall commented Sep 15, 2020 • edited Loading

TL;DR

What is the problem that is trying to be solved?

What can we do about it?

nicksrandall commented Sep 15, 2020

nicksrandall commented Sep 17, 2020

dougwilson commented Sep 17, 2020 • edited Loading

nicksrandall commented Oct 1, 2020

nicksrandall commented Oct 1, 2020

dougwilson commented Oct 1, 2020

dougwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicksrandall Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Abhijith-net commented Oct 20, 2020

Kisama commented Feb 26, 2021

nicksrandall commented Mar 8, 2021

bjohansebas commented Nov 3, 2024

nicksrandall commented Sep 15, 2020 •

edited

Loading

dougwilson commented Sep 17, 2020 •

edited

Loading

nicksrandall Oct 8, 2020 •

edited

Loading

dougwilson Oct 8, 2020 •

edited

Loading