From 47f7da8724b5a453979c2b0d10114c23ac1bd170 Mon Sep 17 00:00:00 2001 From: Mikhail Korobov Date: Tue, 7 Feb 2017 02:03:26 +0500 Subject: [PATCH] canonicalize=False by default for LinkExtractor. Fixes GH-1941. --- docs/topics/link-extractors.rst | 9 +++++-- scrapy/linkextractors/lxmlhtml.py | 22 +++++++++++---- .../link_extractor/sgml_linkextractor.html | 1 + tests/test_linkextractors.py | 27 +++++++++++++++++++ 4 files changed, 52 insertions(+), 7 deletions(-) diff --git a/docs/topics/link-extractors.rst b/docs/topics/link-extractors.rst index 75bdb4142..01d7f0b97 100644 --- a/docs/topics/link-extractors.rst +++ b/docs/topics/link-extractors.rst @@ -51,7 +51,7 @@ LxmlLinkExtractor :synopsis: lxml's HTMLParser-based link extractors -.. class:: LxmlLinkExtractor(allow=(), deny=(), allow_domains=(), deny_domains=(), deny_extensions=None, restrict_xpaths=(), restrict_css=(), tags=('a', 'area'), attrs=('href',), canonicalize=True, unique=True, process_value=None, strip=True) +.. class:: LxmlLinkExtractor(allow=(), deny=(), allow_domains=(), deny_domains=(), deny_extensions=None, restrict_xpaths=(), restrict_css=(), tags=('a', 'area'), attrs=('href',), canonicalize=False, unique=True, process_value=None, strip=True) LxmlLinkExtractor is the recommended link extractor with handy filtering options. It is implemented using lxml's robust HTMLParser. @@ -103,7 +103,12 @@ LxmlLinkExtractor :type attrs: list :param canonicalize: canonicalize each extracted url (using - w3lib.url.canonicalize_url). Defaults to ``True``. + w3lib.url.canonicalize_url). Defaults to ``False``. + Note that canonicalize_url is meant for duplicate checking; + it can change the URL visible at server side, so the response can be + different for requests with canonicalized and raw URLs. If you're + using LinkExtractor to follow links it is more robust to + keep the default ``canonicalize=False``. :type canonicalize: boolean :param unique: whether duplicate filtering should be applied to extracted diff --git a/scrapy/linkextractors/lxmlhtml.py b/scrapy/linkextractors/lxmlhtml.py index c284f1905..a7092f9b8 100644 --- a/scrapy/linkextractors/lxmlhtml.py +++ b/scrapy/linkextractors/lxmlhtml.py @@ -6,6 +6,7 @@ from six.moves.urllib.parse import urljoin import lxml.etree as etree from w3lib.html import strip_html5_whitespace +from w3lib.url import canonicalize_url from scrapy.link import Link from scrapy.utils.misc import arg_to_iter, rel_has_nofollow @@ -29,12 +30,17 @@ def _nons(tag): class LxmlParserLinkExtractor(object): def __init__(self, tag="a", attr="href", process=None, unique=False, - strip=True): + strip=True, canonicalized=False): self.scan_tag = tag if callable(tag) else lambda t: t == tag self.scan_attr = attr if callable(attr) else lambda a: a == attr self.process_attr = process if callable(process) else lambda v: v self.unique = unique self.strip = strip + if canonicalized: + self.link_key = lambda link: link.url + else: + self.link_key = lambda link: canonicalize_url(link.url, + keep_fragments=True) def _iter_links(self, document): for el in document.iter(etree.Element): @@ -82,21 +88,27 @@ class LxmlParserLinkExtractor(object): def _deduplicate_if_needed(self, links): if self.unique: - return unique_list(links, key=lambda link: link.url) + return unique_list(links, key=self.link_key) return links class LxmlLinkExtractor(FilteringLinkExtractor): def __init__(self, allow=(), deny=(), allow_domains=(), deny_domains=(), restrict_xpaths=(), - tags=('a', 'area'), attrs=('href',), canonicalize=True, + tags=('a', 'area'), attrs=('href',), canonicalize=False, unique=True, process_value=None, deny_extensions=None, restrict_css=(), strip=True): tags, attrs = set(arg_to_iter(tags)), set(arg_to_iter(attrs)) tag_func = lambda x: x in tags attr_func = lambda x: x in attrs - lx = LxmlParserLinkExtractor(tag=tag_func, attr=attr_func, - unique=unique, process=process_value, strip=strip) + lx = LxmlParserLinkExtractor( + tag=tag_func, + attr=attr_func, + unique=unique, + process=process_value, + strip=strip, + canonicalized=canonicalize + ) super(LxmlLinkExtractor, self).__init__(lx, allow=allow, deny=deny, allow_domains=allow_domains, deny_domains=deny_domains, diff --git a/tests/sample_data/link_extractor/sgml_linkextractor.html b/tests/sample_data/link_extractor/sgml_linkextractor.html index fbb803f2d..7d5db368a 100644 --- a/tests/sample_data/link_extractor/sgml_linkextractor.html +++ b/tests/sample_data/link_extractor/sgml_linkextractor.html @@ -11,6 +11,7 @@ sample 3 text sample 3 repetition +sample 3 repetition with fragment inner tag href with whitespaces diff --git a/tests/test_linkextractors.py b/tests/test_linkextractors.py index 340c64f35..50484f060 100644 --- a/tests/test_linkextractors.py +++ b/tests/test_linkextractors.py @@ -30,6 +30,7 @@ class Base: Link(url='http://example.com/sample1.html', text=u''), Link(url='http://example.com/sample2.html', text=u'sample 2'), Link(url='http://example.com/sample3.html', text=u'sample 3 text'), + Link(url='http://example.com/sample3.html#foo', text='sample 3 repetition with fragment'), Link(url='http://www.google.com/something', text=u''), Link(url='http://example.com/innertag.html', text=u'inner tag'), Link(url='http://example.com/page%204.html', text=u'href with whitespaces'), @@ -41,6 +42,7 @@ class Base: Link(url='http://example.com/sample1.html', text=u''), Link(url='http://example.com/sample2.html', text=u'sample 2'), Link(url='http://example.com/sample3.html', text=u'sample 3 text'), + Link(url='http://example.com/sample3.html#foo', text='sample 3 repetition with fragment') ]) def test_extract_filter_allow_with_duplicates(self): @@ -50,6 +52,27 @@ class Base: Link(url='http://example.com/sample2.html', text=u'sample 2'), Link(url='http://example.com/sample3.html', text=u'sample 3 text'), Link(url='http://example.com/sample3.html', text=u'sample 3 repetition'), + Link(url='http://example.com/sample3.html#foo', text='sample 3 repetition with fragment') + ]) + + def test_extract_filter_allow_with_duplicates_canonicalize(self): + lx = self.extractor_cls(allow=('sample', ), unique=False, + canonicalize=True) + self.assertEqual([link for link in lx.extract_links(self.response)], [ + Link(url='http://example.com/sample1.html', text=u''), + Link(url='http://example.com/sample2.html', text=u'sample 2'), + Link(url='http://example.com/sample3.html', text=u'sample 3 text'), + Link(url='http://example.com/sample3.html', text=u'sample 3 repetition'), + Link(url='http://example.com/sample3.html', text='sample 3 repetition with fragment') + ]) + + def test_extract_filter_allow_no_duplicates_canonicalize(self): + lx = self.extractor_cls(allow=('sample',), unique=True, + canonicalize=True) + self.assertEqual([link for link in lx.extract_links(self.response)], [ + Link(url='http://example.com/sample1.html', text=u''), + Link(url='http://example.com/sample2.html', text=u'sample 2'), + Link(url='http://example.com/sample3.html', text=u'sample 3 text'), ]) def test_extract_filter_allow_and_deny(self): @@ -73,6 +96,8 @@ class Base: Link(url='http://example.com/sample1.html', text=u''), Link(url='http://example.com/sample2.html', text=u'sample 2'), Link(url='http://example.com/sample3.html', text=u'sample 3 text'), + Link(url='http://example.com/sample3.html#foo', + text='sample 3 repetition with fragment') ]) lx = self.extractor_cls(allow='sample', deny='3') @@ -280,6 +305,7 @@ class Base: Link(url='http://example.com/sample1.html', text=u''), Link(url='http://example.com/sample2.html', text=u'sample 2'), Link(url='http://example.com/sample3.html', text=u'sample 3 text'), + Link(url='http://example.com/sample3.html#foo', text='sample 3 repetition with fragment'), Link(url='http://www.google.com/something', text=u''), Link(url='http://example.com/innertag.html', text=u'inner tag'), Link(url='http://example.com/page%204.html', text=u'href with whitespaces'), @@ -291,6 +317,7 @@ class Base: Link(url='http://example.com/sample2.html', text=u'sample 2'), Link(url='http://example.com/sample2.jpg', text=u''), Link(url='http://example.com/sample3.html', text=u'sample 3 text'), + Link(url='http://example.com/sample3.html#foo', text='sample 3 repetition with fragment'), Link(url='http://www.google.com/something', text=u''), Link(url='http://example.com/innertag.html', text=u'inner tag'), Link(url='http://example.com/page%204.html', text=u'href with whitespaces'),