Merge pull request #4254 from elacuesta/spider.parse

Change Scraper API to call internal `_parse` method

docs/topics/spiders.rst

@@ -360,9 +360,10 @@ CrawlSpider
 
    This spider also exposes an overrideable method:
 
-   .. method:: parse_start_url(response)
+   .. method:: parse_start_url(response, **kwargs)
 
-      This method is called for the start_urls responses. It allows to parse
+      This method is called for each response produced for the URLs in
+      the spider's ``start_urls`` attribute. It allows to parse
       the initial responses and must return either an
       :ref:`item object <topics-items>`, a :class:`~scrapy.http.Request`
       object, or an iterable containing any of them.
@@ -388,11 +389,6 @@ Crawling rules
    object will contain the text of the link that produced the :class:`~scrapy.http.Request`
    in its ``meta`` dictionary (under the ``link_text`` key)
 
-   .. warning:: When writing crawl spider rules, avoid using ``parse`` as
-       callback, since the :class:`CrawlSpider` uses the ``parse`` method
-       itself to implement its logic. So if you override the ``parse`` method,
-       the crawl spider will no longer work.
-
    ``cb_kwargs`` is a dict containing the keyword arguments to be passed to the
    callback function.
 
@@ -418,6 +414,11 @@ Crawling rules
    It receives a :class:`Twisted Failure <twisted.python.failure.Failure>`
    instance as first parameter.
 
+
+.. warning:: Because of its internal implementation, you must explicitly set
+   callbacks for new requests when writing :class:`CrawlSpider`-based spiders;
+   unexpected behaviour can occur otherwise.
+
    .. versionadded:: 2.0
       The *errback* parameter.
 
@@ -451,6 +452,11 @@ Let's now take a look at an example CrawlSpider with rules::
             item['name'] = response.xpath('//td[@id="item_name"]/text()').get()
             item['description'] = response.xpath('//td[@id="item_description"]/text()').get()
             item['link_text'] = response.meta['link_text']
+            url = response.xpath('//td[@id="additional_data"]/@href').get()
+            return response.follow(url, self.parse_additional_page, cb_kwargs=dict(item=item))
+
+        def parse_additional_page(self, response, item):
+            item['additional_data'] = response.xpath('//p[@id="additional_data"]/text()').get()
             return item
 
 
@@ -544,6 +550,11 @@ XMLFeedSpider
         those results. It must return a list of results (items or requests).
 
 
+.. warning:: Because of its internal implementation, you must explicitly set
+   callbacks for new requests when writing :class:`XMLFeedSpider`-based spiders;
+   unexpected behaviour can occur otherwise.
+
+
 XMLFeedSpider example
 ~~~~~~~~~~~~~~~~~~~~~
 

scrapy/core/scraper.py

@@ -148,7 +148,7 @@ class Scraper:
     def call_spider(self, result, request, spider):
         result.request = request
         dfd = defer_result(result)
-        callback = request.callback or spider.parse
+        callback = request.callback or spider._parse
         warn_on_generator_with_return_value(spider, callback)
         warn_on_generator_with_return_value(spider, request.errback)
         dfd.addCallbacks(callback=callback,

scrapy/spiders/__init__.py

@@ -86,7 +86,10 @@ class Spider(object_ref):
         )
         return Request(url, dont_filter=True)
 
-    def parse(self, response):
+    def _parse(self, response, **kwargs):
+        return self.parse(response, **kwargs)
+
+    def parse(self, response, **kwargs):
         raise NotImplementedError('{}.parse callback is not defined'.format(self.__class__.__name__))
 
     @classmethod

scrapy/spiders/crawl.py

@@ -78,10 +78,15 @@ class CrawlSpider(Spider):
         super(CrawlSpider, self).__init__(*a, **kw)
         self._compile_rules()
 
-    def parse(self, response):
-        return self._parse_response(response, self.parse_start_url, cb_kwargs={}, follow=True)
+    def _parse(self, response, **kwargs):
+        return self._parse_response(
+            response=response,
+            callback=self.parse_start_url,
+            cb_kwargs=kwargs,
+            follow=True,
+        )
 
-    def parse_start_url(self, response):
+    def parse_start_url(self, response, **kwargs):
         return []
 
     def process_results(self, response, results):

scrapy/spiders/feed.py

@@ -61,7 +61,7 @@ class XMLFeedSpider(Spider):
             for result_item in self.process_results(response, ret):
                 yield result_item
 
-    def parse(self, response):
+    def _parse(self, response, **kwargs):
         if not hasattr(self, 'parse_node'):
             raise NotConfigured('You must define parse_node method in order to scrape this XML feed')
 
@@ -128,7 +128,7 @@ class CSVFeedSpider(Spider):
             for result_item in self.process_results(response, ret):
                 yield result_item
 
-    def parse(self, response):
+    def _parse(self, response, **kwargs):
         if not hasattr(self, 'parse_row'):
             raise NotConfigured('You must define parse_row method in order to scrape this CSV feed')
         response = self.adapt_response(response)

tests/spiders.py

@@ -250,13 +250,40 @@ class DuplicateStartRequestsSpider(MockServerSpider):
         self.visited += 1
 
 
-class CrawlSpiderWithErrback(MockServerSpider, CrawlSpider):
-    name = 'crawl_spider_with_errback'
+class CrawlSpiderWithParseMethod(MockServerSpider, CrawlSpider):
+    """
+    A CrawlSpider which overrides the 'parse' method
+    """
+    name = 'crawl_spider_with_parse_method'
     custom_settings = {
         'RETRY_HTTP_CODES': [],  # no need to retry
     }
     rules = (
-        Rule(LinkExtractor(), callback='callback', errback='errback', follow=True),
+        Rule(LinkExtractor(), callback='parse', follow=True),
+    )
+
+    def start_requests(self):
+        test_body = b"""
+        <html>
+            <head><title>Page title<title></head>
+            <body>
+                <p><a href="/status?n=200">Item 200</a></p>  <!-- callback -->
+                <p><a href="/status?n=201">Item 201</a></p>  <!-- callback -->
+            </body>
+        </html>
+        """
+        url = self.mockserver.url("/alpayload")
+        yield Request(url, method="POST", body=test_body)
+
+    def parse(self, response, foo=None):
+        self.logger.info('[parse] status %i (foo: %s)', response.status, foo)
+        yield Request(self.mockserver.url("/status?n=202"), self.parse, cb_kwargs={"foo": "bar"})
+
+
+class CrawlSpiderWithErrback(CrawlSpiderWithParseMethod):
+    name = 'crawl_spider_with_errback'
+    rules = (
+        Rule(LinkExtractor(), callback='parse', errback='errback', follow=True),
     )
 
     def start_requests(self):
@@ -275,9 +302,6 @@ class CrawlSpiderWithErrback(MockServerSpider, CrawlSpider):
         url = self.mockserver.url("/alpayload")
         yield Request(url, method="POST", body=test_body)
 
-    def callback(self, response):
-        self.logger.info('[callback] status %i', response.status)
-
     def errback(self, failure):
         self.logger.info('[errback] status %i', failure.value.response.status)
 

tests/test_crawl.py

@@ -29,6 +29,7 @@ from tests.spiders import (
     BytesReceivedCallbackSpider,
     BytesReceivedErrbackSpider,
     CrawlSpiderWithErrback,
+    CrawlSpiderWithParseMethod,
     DelaySpider,
     DuplicateStartRequestsSpider,
     FollowAllSpider,
@@ -321,6 +322,28 @@ with multiples lines
         self._assert_retried(log)
         self.assertIn("Got response 200", str(log))
 
+
+class CrawlSpiderTestCase(TestCase):
+
+    def setUp(self):
+        self.mockserver = MockServer()
+        self.mockserver.__enter__()
+        self.runner = CrawlerRunner()
+
+    def tearDown(self):
+        self.mockserver.__exit__(None, None, None)
+
+    @defer.inlineCallbacks
+    def test_crawlspider_with_parse(self):
+        self.runner.crawl(CrawlSpiderWithParseMethod, mockserver=self.mockserver)
+
+        with LogCapture() as log:
+            yield self.runner.join()
+
+        self.assertIn("[parse] status 200 (foo: None)", str(log))
+        self.assertIn("[parse] status 201 (foo: None)", str(log))
+        self.assertIn("[parse] status 202 (foo: bar)", str(log))
+
     @defer.inlineCallbacks
     def test_crawlspider_with_errback(self):
         self.runner.crawl(CrawlSpiderWithErrback, mockserver=self.mockserver)
@@ -328,10 +351,12 @@ with multiples lines
         with LogCapture() as log:
             yield self.runner.join()
 
-        self.assertIn("[callback] status 200", str(log))
-        self.assertIn("[callback] status 201", str(log))
+        self.assertIn("[parse] status 200 (foo: None)", str(log))
+        self.assertIn("[parse] status 201 (foo: None)", str(log))
+        self.assertIn("[parse] status 202 (foo: bar)", str(log))
         self.assertIn("[errback] status 404", str(log))
         self.assertIn("[errback] status 500", str(log))
+        self.assertIn("[errback] status 501", str(log))
 
     @defer.inlineCallbacks
     def test_async_def_parse(self):

tests/test_spider.py

@@ -150,7 +150,7 @@ class XMLFeedSpiderTest(SpiderTest):
 
         for iterator in ('iternodes', 'xml'):
             spider = _XMLSpider('example', iterator=iterator)
-            output = list(spider.parse(response))
+            output = list(spider._parse(response))
             self.assertEqual(len(output), 2, iterator)
             self.assertEqual(output, [
                 {'loc': [u'http://www.example.com/Special-Offers.html'],