-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
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! |
@Ad12y it has a PR in progress but it's stalled, you can try making a fresh one (likely based on it). |
Currently, the HTTP compression middleware sets the
Accept-Encoding
header togzip,deflate
orgzip,deflate,br
depending on whether or not brotlipy is installed.However, if the user manually sets the
Accept-Encoding
header to a value containingbr
, and does not havebrotlipy
installed, the server can send a response compressed withbr
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 theAccept-Encoding
header, I think we should:brotlipy
is not installed.brotlipy
is not installed.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.
The text was updated successfully, but these errors were encountered: