scrapy
scrapy copied to clipboard
Refactor cookie handling in CookiesMiddleware
- Refactoring cookie handling in CookiesMiddleware
- Adding test case for off-domain jar storage
Can close #5841
Codecov Report
Merging #5946 (c1a5e15) into master (4dacad0) will decrease coverage by
46.39%. Report is 366 commits behind head on master. The diff coverage is65.84%.
:exclamation: Current head c1a5e15 differs from pull request most recent head 4d58842. Consider uploading reports for the commit 4d58842 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## master #5946 +/- ##
===========================================
- Coverage 88.85% 42.47% -46.39%
===========================================
Files 162 163 +1
Lines 11057 11550 +493
Branches 1801 1880 +79
===========================================
- Hits 9825 4906 -4919
- Misses 954 6269 +5315
- Partials 278 375 +97
| Files Changed | Coverage Δ | |
|---|---|---|
| scrapy/__init__.py | 84.21% <0.00%> (ø) |
|
| scrapy/cmdline.py | 0.00% <0.00%> (-67.97%) |
:arrow_down: |
| scrapy/commands/shell.py | 0.00% <0.00%> (-92.86%) |
:arrow_down: |
| scrapy/downloadermiddlewares/cookies.py | 37.50% <0.00%> (-60.40%) |
:arrow_down: |
| scrapy/downloadermiddlewares/decompression.py | 32.75% <0.00%> (-67.25%) |
:arrow_down: |
| scrapy/downloadermiddlewares/httpcache.py | 33.73% <ø> (-60.25%) |
:arrow_down: |
| scrapy/linkextractors/__init__.py | 75.00% <ø> (-25.00%) |
:arrow_down: |
| scrapy/mail.py | 38.88% <0.00%> (-39.77%) |
:arrow_down: |
| scrapy/squeues.py | 54.32% <ø> (-45.68%) |
:arrow_down: |
| scrapy/contracts/__init__.py | 18.51% <20.00%> (-65.08%) |
:arrow_down: |
| ... and 74 more |
I don’t think the changes to existing tests are correct. I think we need to change the implementation instead to make sure they continue to pass as they were.
Also, the cookie processing method affects both requests and responses, so it would be good to make sure we have a test to make sure that, if we get a Set-Cookie header from a request with a cookie set for an unrelated domain, that the cookie is not added to the cookiejar. We trust users, not servers.
@emarondan I have made a small change to test_process_response_unrelated_domain, to cover the issue I fear we might introduce here. When receiving a cookie in a response for a different domain, the cookie should be ignored altogether; we should not add it to the cookie jar, not for the actual response domain, not for the cookie domain.
Please, see if you can find a way, however unclean, to get test_off_domain_jar_storage to pass without breaking the new test_process_response_unrelated_domain in the process.