Skip to content
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.

Sign up for GitHub

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

Jump to bottom

new allow_offsite parameter in OffsiteMiddleware #6151

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

new allow_offsite parameter in OffsiteMiddleware #6151

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
21bddc2
new 'allow_offsite' parameter in OffsiteMiddleware
BurnzZ Nov 22, 2023
eb5afd8
document deprecated dont_filter flag in OffsiteMiddleware
BurnzZ Nov 28, 2023
acba118
avoid deprecating dont_filter in OffsiteMiddleware
BurnzZ Dec 13, 2023
7b02385
Merge branch 'master' into allow_offsite
wRAR May 16, 2024
0c44e98
Copy the code to the downloader mw.
wRAR May 16, 2024
ad5eff1
Add tests for allow_offsite in the downloader mw.
wRAR May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions docs/topics/spider-middleware.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,9 +345,14 @@ OffsiteMiddleware
:attr:`~scrapy.Spider.allowed_domains` attribute, or the
attribute is empty, the offsite middleware will allow all requests.

If the request has the :attr:`~scrapy.Request.dont_filter` attribute
set, the offsite middleware will allow the request even if its domain is not
listed in allowed domains.
If ``allow_offsite`` is set to ``True`` in :attr:`Request.meta`, then the
offsite middleware will allow the request even if its domain is not listed
in allowed domains.
BurnzZ marked this conversation as resolved.
Show resolved Hide resolved

.. caution:: Setting :attr:`~scrapy.Request.dont_filter` to ``True`` also
causes the offsite middleware to allow the request. However,
this is deprecated. Use ``allow_offsite`` instead in
:attr:`Request.meta`.


RefererMiddleware
Expand Down
11 changes: 10 additions & 1 deletion scrapy/spidermiddlewares/offsite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

from scrapy import Spider, signals
from scrapy.crawler import Crawler
from scrapy.exceptions import ScrapyDeprecationWarning
from scrapy.http import Request, Response
from scrapy.statscollectors import StatsCollector
from scrapy.utils.httpobj import urlparse_cached
Expand Down Expand Up @@ -50,7 +51,15 @@
def _filter(self, request: Any, spider: Spider) -> bool:
if not isinstance(request, Request):
return True
if request.dont_filter or self.should_follow(request, spider):
if request.dont_filter:
warnings.warn(

Check warning on line 55 in scrapy/spidermiddlewares/offsite.py

View check run for this annotation

Codecov / codecov/patch

scrapy/spidermiddlewares/offsite.py#L55 

Added line #L55 was not covered by tests
"The dont_filter filter flag is deprecated in OffsiteMiddleware. "
"Set 'allow_offsite' to True in Request.meta instead.",
ScrapyDeprecationWarning,
stacklevel=2,
)
return True

Check warning on line 61 in scrapy/spidermiddlewares/offsite.py

View check run for this annotation

Codecov / codecov/patch

scrapy/spidermiddlewares/offsite.py#L61 

Added line #L61 was not covered by tests
Copy link
Member

@kmike kmike Dec 11, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I wonder if it'd be better to just keep supporting dont_filter, without a deprecation warning.

First, the flag says "dont_filter", and we respect it, and don't filter out the request. An argument can be made though that "don't filter" only applies to deduplication filter, not to other types of filtering. I think that's valid, But that's not the current flag behavior, and also it's not in the flag name (it's not "dont_deduplicate").

Second, the user doesn't control all the don't_filter flags, Scrapy and other components can be setting this flag. For example, the default start_requests implementation uses dont_filter=True.

It seems it's not possibile to "deprecate don_filter flag in OffsiteMiddleware", because the user might be setting this flag not for the OffsiteMiddleware, but for other Scrapy components, but the request may still end up in OffsiteMiddleware.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. In that case, we need to clarify that the dont_filter value also affects some middlewares as well and not just for the Scheduler.

if request.meta.get("allow_offsite") or self.should_follow(request, spider):
return True
domain = urlparse_cached(request).hostname
if domain and domain not in self.domains_seen:
Expand Down
1 change: 1 addition & 0 deletions tests/test_spidermiddleware_offsite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def test_process_spider_output(self):
Request("http://scrapy.org/1"),
Request("http://sub.scrapy.org/1"),
Request("http://offsite.tld/letmepass", dont_filter=True),
Request("http://offsite-2.tld/allow", meta={"allow_offsite": True}),
Request("http://scrapy.test.org/"),
Request("http://scrapy.test.org:8000/"),
]
Expand Down