Warn about br handling if brotlipy is not installed #4697
Comments
|
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. |
|
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. |
|
@Dushatar Please, have a go at it, and let us know if you need any help 🙂 |
|
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. |
|
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. |
|
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? |
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. |
|
Is this one still relevant? I'd like to work on it! |
|
There is #5480 already, pending review. You could review it and give your thoughts. |
|
Alternatively: #4698 |
Solved version of issue scrapy#4697
|
Is this still relevant? If yes, I would like to work on it! |
and others
Currently, the HTTP compression middleware sets the
Accept-Encodingheader togzip,deflateorgzip,deflate,brdepending on whether or not brotlipy is installed.However, if the user manually sets the
Accept-Encodingheader to a value containingbr, and does not havebrotlipyinstalled, the server can send a response compressed withbrand 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
brmanually on theAccept-Encodingheader, I think we should:brotlipyis not installed.brotlipyis not installed.Accept-Encodingheader 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.
The text was updated successfully, but these errors were encountered: