Repository: scrapy/scrapy
Pull request #2769 (2017-05-31 18:08:20)

[MRG+1] Add verification to check if Request callback is callable
This PR intends to fix https://github.com/scrapy/scrapy/issues/2766.

When users pass a string to `Request.__init__`'s `callback` parameter, they end up getting a Twisted exception, which doesn't help to uncover this, which is a rather simple issue.

This PR changes that, so that users get a proper message when passing something that's not callable to the `callback` and `errback` parameters.


Patch:
diff --git a/docs/intro/install.rst b/docs/intro/install.rst
index 47af8292e..9cec2eaee 100644
--- a/docs/intro/install.rst
+++ b/docs/intro/install.rst
@@ -107,7 +107,7 @@ Python virtualenvs can be created to use Python 2 by default, or Python 3 by def
 
 .. _virtualenv: https://virtualenv.pypa.io
 .. _virtualenv installation instructions: https://virtualenv.pypa.io/en/stable/installation/
-.. _virtualenvwrapper: https://virtualenvwrapper.readthedocs.io/en/latest/install.html
+.. _virtualenvwrapper: http://virtualenvwrapper.readthedocs.io/en/latest/install.html
 .. _user guide: https://virtualenv.pypa.io/en/stable/userguide/
 
 
diff --git a/docs/news.rst b/docs/news.rst
index 577c93b8e..e0f8eee0b 100644
--- a/docs/news.rst
+++ b/docs/news.rst
@@ -12,7 +12,7 @@ but quite a few handy improvements nonetheless.
 Scrapy now supports anonymous FTP sessions with customizable user and
 password via the new :setting:`FTP_USER` and :setting:`FTP_PASSWORD` settings.
 And if you're using Twisted version 17.1.0 or above, FTP is now available
-with Python 3.
+with Python 3. 
 
 There's a new :meth:`response.follow <scrapy.http.TextResponse.follow>` method
 for creating requests; **it is now a recommended way to create Requests
@@ -407,7 +407,7 @@ Refactoring
 
 - ``canonicalize_url`` has been moved to `w3lib.url`_ (:issue:`2168`).
 
-.. _w3lib.url: https://w3lib.readthedocs.io/en/latest/w3lib.html#w3lib.url.canonicalize_url
+.. _w3lib.url: http://w3lib.readthedocs.io/en/latest/w3lib.html#w3lib.url.canonicalize_url
 
 Tests & Requirements
 ~~~~~~~~~~~~~~~~~~~~
diff --git a/docs/topics/commands.rst b/docs/topics/commands.rst
index 8de858f8a..3e69c4e6f 100644
--- a/docs/topics/commands.rst
+++ b/docs/topics/commands.rst
@@ -543,7 +543,7 @@ Example::
 
     COMMANDS_MODULE = 'mybot.commands'
 
-.. _Deploying your project: https://scrapyd.readthedocs.io/en/latest/deploy.html
+.. _Deploying your project: http://scrapyd.readthedocs.org/en/latest/deploy.html
 
 Register commands via setup.py entry points
 -------------------------------------------
diff --git a/docs/topics/deploy.rst b/docs/topics/deploy.rst
index f4186ea7a..bc48ddce7 100644
--- a/docs/topics/deploy.rst
+++ b/docs/topics/deploy.rst
@@ -50,10 +50,10 @@ them as needed - the configuration is read from the ``scrapy.cfg`` file
 just like ``scrapyd-deploy``.
 
 .. _Scrapyd: https://github.com/scrapy/scrapyd
-.. _Deploying your project: https://scrapyd.readthedocs.io/en/latest/deploy.html
+.. _Deploying your project: https://scrapyd.readthedocs.org/en/latest/deploy.html
 .. _Scrapy Cloud: http://scrapinghub.com/scrapy-cloud/
 .. _scrapyd-client: https://github.com/scrapy/scrapyd-client
 .. _shub: http://doc.scrapinghub.com/shub.html
-.. _scrapyd-deploy documentation: https://scrapyd.readthedocs.io/en/latest/deploy.html
+.. _scrapyd-deploy documentation: http://scrapyd.readthedocs.org/en/latest/deploy.html
 .. _Scrapy Cloud documentation: http://doc.scrapinghub.com/scrapy-cloud.html
 .. _Scrapinghub: http://scrapinghub.com/
diff --git a/docs/topics/item-pipeline.rst b/docs/topics/item-pipeline.rst
index ac0a5973b..33e4d7429 100644
--- a/docs/topics/item-pipeline.rst
+++ b/docs/topics/item-pipeline.rst
@@ -208,7 +208,7 @@ and Deferred callback fires, it saves item to a file and adds filename to an ite
             item["screenshot_filename"] = filename
             return item
 
-.. _Splash: https://splash.readthedocs.io/en/stable/
+.. _Splash: http://splash.readthedocs.io/en/stable/
 .. _Deferred: https://twistedmatrix.com/documents/current/core/howto/defer.html
 
 Duplicates filter
diff --git a/docs/topics/scrapyd.rst b/docs/topics/scrapyd.rst
index a3d6f7698..57921b901 100644
--- a/docs/topics/scrapyd.rst
+++ b/docs/topics/scrapyd.rst
@@ -10,4 +10,4 @@ Scrapyd has been moved into a separate project.
 
 Its documentation is now hosted at:
 
-    https://scrapyd.readthedocs.io/en/latest/
+    http://scrapyd.readthedocs.org/en/latest/
diff --git a/docs/topics/spiders.rst b/docs/topics/spiders.rst
index 6ac946003..49c0cefb5 100644
--- a/docs/topics/spiders.rst
+++ b/docs/topics/spiders.rst
@@ -80,7 +80,7 @@ scrapy.Spider
        allowed to crawl. Requests for URLs not belonging to the domain names
        specified in this list (or their subdomains) won't be followed if
        :class:`~scrapy.spidermiddlewares.offsite.OffsiteMiddleware` is enabled.
-
+       
        Let's say your target url is ``https://www.example.com/1.html``,
        then add ``'example.com'`` to the list.
 
@@ -756,4 +756,4 @@ Combine SitemapSpider with other sources of urls::
 .. _Sitemap index files: http://www.sitemaps.org/protocol.html#index
 .. _robots.txt: http://www.robotstxt.org/
 .. _TLD: https://en.wikipedia.org/wiki/Top-level_domain
-.. _Scrapyd documentation: https://scrapyd.readthedocs.io/en/latest/
+.. _Scrapyd documentation: http://scrapyd.readthedocs.org/en/latest/
diff --git a/scrapy/commands/parse.py b/scrapy/commands/parse.py
index 5264982b6..a90095146 100644
--- a/scrapy/commands/parse.py
+++ b/scrapy/commands/parse.py
@@ -142,7 +142,8 @@ class Command(ScrapyCommand):
                 logger.error('Unable to find spider for: %(url)s',
                              {'url': url})
 
-        request = Request(url, opts.callback)
+        # Request requires callback argument as callable or None, not string
+        request = Request(url, None)
         _start_requests = lambda s: [self.prepare_request(s, request, opts)]
         self.spidercls.start_requests = _start_requests
 
@@ -164,7 +165,9 @@ class Command(ScrapyCommand):
             # determine real callback
             cb = response.meta['_callback']
             if not cb:
-                if opts.rules and self.first_response == response:
+                if opts.callback:
+                    cb = opts.callback
+                elif opts.rules and self.first_response == response:
                     cb = self.get_callback_from_rules(spider, response)
 
                     if not cb:
diff --git a/scrapy/http/request/__init__.py b/scrapy/http/request/__init__.py
index 1435d91de..13a92ffa0 100644
--- a/scrapy/http/request/__init__.py
+++ b/scrapy/http/request/__init__.py
@@ -27,6 +27,10 @@ class Request(object_ref):
         assert isinstance(priority, int), "Request priority not an integer: %r" % priority
         self.priority = priority
 
+        if callback is not None and not callable(callback):
+            raise TypeError('callback must be a callable, got %s' % type(callback).__name__)
+        if errback is not None and not callable(errback):
+            raise TypeError('errback must be a callable, got %s' % type(errback).__name__)
         assert callback or not errback, "Cannot use errback without a callback"
         self.callback = callback
         self.errback = errback


Test patch:
diff --git a/tests/test_http_request.py b/tests/test_http_request.py
index bbce537f4..9b0ee63dc 100644
--- a/tests/test_http_request.py
+++ b/tests/test_http_request.py
@@ -235,6 +235,26 @@ class RequestTest(unittest.TestCase):
         self.assertRaises(AttributeError, setattr, r, 'url', 'http://example2.com')
         self.assertRaises(AttributeError, setattr, r, 'body', 'xxx')
 
+    def test_callback_is_callable(self):
+        def a_function():
+            pass
+        r = self.request_class('http://example.com')
+        self.assertIsNone(r.callback)
+        r = self.request_class('http://example.com', a_function)
+        self.assertIs(r.callback, a_function)
+        with self.assertRaises(TypeError):
+            self.request_class('http://example.com', 'a_function')
+
+    def test_errback_is_callable(self):
+        def a_function():
+            pass
+        r = self.request_class('http://example.com')
+        self.assertIsNone(r.errback)
+        r = self.request_class('http://example.com', a_function, errback=a_function)
+        self.assertIs(r.errback, a_function)
+        with self.assertRaises(TypeError):
+            self.request_class('http://example.com', a_function, errback='a_function')
+
 
 class FormRequestTest(RequestTest):
 
diff --git a/tests/test_utils_reqser.py b/tests/test_utils_reqser.py
index 073baadc2..dcc070b8f 100644
--- a/tests/test_utils_reqser.py
+++ b/tests/test_utils_reqser.py
@@ -17,8 +17,8 @@ class RequestSerializationTest(unittest.TestCase):
 
     def test_all_attributes(self):
         r = Request("http://www.example.com",
-            callback='parse_item',
-            errback='handle_error',
+            callback=self.spider.parse_item,
+            errback=self.spider.handle_error,
             method="POST",
             body=b"some body",
             headers={'content-encoding': 'text/html; charset=latin-1'},
@@ -27,7 +27,7 @@ class RequestSerializationTest(unittest.TestCase):
             priority=20,
             meta={'a': 'b'},
             flags=['testFlag'])
-        self._assert_serializes_ok(r)
+        self._assert_serializes_ok(r, spider=self.spider)
 
     def test_latin1_body(self):
         r = Request("http://www.example.com", body=b"\xa3")