Warn about br handling if brotlipy is not installed

[Gallaecio]

Currently, the HTTP compression middleware sets the Accept-Encoding header to gzip,deflate or gzip,deflate,br depending on whether or not brotlipy is installed.

However, if the user manually sets the Accept-Encoding header to a value containing br, and does not have brotlipy installed, the server can send a response compressed with br and Scrapy will silently fail to decompress it, so the response content will not be readable.

I think we should improve a bit the handling of this scenario.

If the user sets br manually on the Accept-Encoding header, I think we should:

• Log a warning if brotlipy is not installed.
• Have the middleware cause an error if the server sends the response br-encoded, and brotlipy is not installed.
• Log an information message the first time that the user sends a response with a manually-set Accept-Encoding header which is identical to the one that the HTTP compression middleware would define. It should not be a warning because the user does not need to do anything about it. It should only be displayed once because there is a chance the user is doing it on purpose (e.g. if the default headers define a different value but the user wishes to override that for some requests with the default value.

The 3rd bullet point may not be that useful, but I think the 1st and 2nd are.

[Gallaecio]

That tool sounds very interesting!

Regarding this specific issue, it could be argued that it is indeed a good first issue. I’m unsure, though.

[Dushatar]

Hello. Me and two others would like to take a look at this issue as part of a course we are taking @Gallaecio

Hopefully we can make some progress in the, not so long, time allocated to the assignment.

[Gallaecio]

@Dushatar Please, have a go at it, and let us know if you need any help :slightly_smiling_face:

[Dushatar]

We did not have much time designated for this in the course, it was mostly just to get the feel for open source projects. We pushed some code that does what was requested (as we understood it), but I am not sure if the way we did it is as intended.

It (1) logs a warning if brotlipy is not installed and (2) raise a ImportError exception if a user is sent an br-encoded response while not having brotlipy installed.

[Gallaecio]

It’s quite a good start, but it would need some adjustments to get merged. It should definitely make things easier for the next person that comes around.

[Anupam-USP]

I am very new, sorry to ask silly questions but after changing files on my machine how can I check if this is causing some errors or is it ok to push and later create a pr?

[Gallaecio]

After changing files on my machine how can I check if this is causing some errors or is it ok to push and later create a pr?

You can run automated tests locally. But feel free to create a pull request and let the Continuous Integration system run tests automatically in multiple different environments. If your changes are not ready for a review yet, you can mark your pull request as a draft when creating it, and later mark it as ready for review.

[PluT00]

Is this one still relevant? I'd like to work on it!

[Gallaecio]

There is https://github.com/scrapy/scrapy/pull/5480 already, pending review. You could review it and give your thoughts.

[wRAR]

Alternatively: #4698

[Ad12y]

Is this still relevant? If yes, I would like to work on it!

[wRAR]

@Ad12y it has a PR in progress but it's stalled, you can try making a fresh one (likely based on it).