From da6a24b66281bb9a9ebbd0a0bded6521b2166d24 Mon Sep 17 00:00:00 2001 From: Pablo Hoffman Date: Sat, 17 Jan 2009 20:40:07 +0000 Subject: [PATCH] More Request/Response cleanup: * made status attribute an int * made engine use __str__ to display crawled requests * HTTP cache now inherits Response class to change __str__ * added tests to check that the class is preserved on .copy() (for both Requests and Responses) * removed custom cached attribute (and passed to a Response.meta item) * removed some custom (and seldom used) methods from Response class: version(), info() * reinforced the privacy of the ResponseBody class, by renaming it to _ResponseBody and added a warning that it may be removed in the future * added tests for Request & Response to_string() methods * fixed minor (and harmless) bug in to_string() methods --HG-- extra : convert_revision : svn%3Ab85faa78-f9eb-468e-a121-7cced6da292c%40737 --- .../contrib/downloadermiddleware/cache.py | 14 +++- .../contrib/downloadermiddleware/debug.py | 13 ++- .../scrapy/contrib/history/middleware.py | 6 +- .../trunk/scrapy/contrib/history/scheduler.py | 9 +- .../trunk/scrapy/core/downloader/handlers.py | 2 +- scrapy/trunk/scrapy/core/engine.py | 2 +- scrapy/trunk/scrapy/http/__init__.py | 4 +- scrapy/trunk/scrapy/http/request.py | 10 ++- scrapy/trunk/scrapy/http/response.py | 82 +++++++++---------- scrapy/trunk/scrapy/replay/__init__.py | 3 +- scrapy/trunk/scrapy/tests/test_adaptors.py | 2 +- scrapy/trunk/scrapy/tests/test_engine.py | 4 +- .../trunk/scrapy/tests/test_http_request.py | 18 ++++ .../trunk/scrapy/tests/test_http_response.py | 25 +++++- 14 files changed, 122 insertions(+), 72 deletions(-) diff --git a/scrapy/trunk/scrapy/contrib/downloadermiddleware/cache.py b/scrapy/trunk/scrapy/contrib/downloadermiddleware/cache.py index ea69fa876..f809a9f50 100644 --- a/scrapy/trunk/scrapy/contrib/downloadermiddleware/cache.py +++ b/scrapy/trunk/scrapy/contrib/downloadermiddleware/cache.py @@ -15,6 +15,14 @@ from scrapy.core.exceptions import NotConfigured, HttpException, IgnoreRequest from scrapy.utils.request import request_fingerprint from scrapy.conf import settings +class CachedResponse(Response): + + def __init__(self, *args, **kwargs): + Response.__init__(self, *args, **kwargs) + self.meta['cached'] = True + + def __str__(self): + return "(cached) " + Response.__str__(self) class CacheMiddleware(object): def __init__(self): @@ -39,7 +47,6 @@ class CacheMiddleware(object): log.msg("Corrupt cache for %s" % request.url, log.WARNING) response = False if response: - response.cached = True if not 200 <= int(response.status) < 300: raise HttpException(response.status, None, response) return response @@ -50,7 +57,7 @@ class CacheMiddleware(object): if not is_cacheable(request): return response - if isinstance(response, Response) and not response.cached: + if isinstance(response, Response) and not response.meta.get('cached'): key = request_fingerprint(request) domain = spider.domain_name self.cache.store(domain, key, request, response) @@ -153,8 +160,7 @@ class Cache(object): headers = Headers(responseheaders) status = metadata['status'] - response = Response(domain=domain, url=url, headers=headers, status=status, body=responsebody) - response.cached = True + response = CachedResponse(domain=domain, url=url, headers=headers, status=status, body=responsebody) return response def store(self, domain, key, request, response): diff --git a/scrapy/trunk/scrapy/contrib/downloadermiddleware/debug.py b/scrapy/trunk/scrapy/contrib/downloadermiddleware/debug.py index 6e13dc1b8..f398fe078 100644 --- a/scrapy/trunk/scrapy/contrib/downloadermiddleware/debug.py +++ b/scrapy/trunk/scrapy/contrib/downloadermiddleware/debug.py @@ -1,20 +1,19 @@ from scrapy import log +from scrapy.core.exceptions import NotConfigured from scrapy.conf import settings class CrawlDebug(object): + def __init__(self): - self.enabled = settings.getbool('CRAWL_DEBUG') + raise NotConfigured def process_request(self, request, spider): - if self.enabled: - log.msg("Crawling %s" % repr(request), domain=spider.domain_name, level=log.DEBUG) + log.msg("Crawling %s" % repr(request), domain=spider.domain_name, level=log.DEBUG) def process_exception(self, request, exception, spider): - if self.enabled: - log.msg("Crawl exception %s in %s" % (exception, repr(request)), domain=spider.domain_name, level=log.DEBUG) + log.msg("Crawl exception %s in %s" % (exception, repr(request)), domain=spider.domain_name, level=log.DEBUG) def process_response(self, request, response, spider): - if self.enabled: - log.msg("Fetched %s from %s" % (response.info(), repr(request)), domain=spider.domain_name, level=log.DEBUG) + log.msg("Fetched %s from %s" % (response, repr(request)), domain=spider.domain_name, level=log.DEBUG) return response diff --git a/scrapy/trunk/scrapy/contrib/history/middleware.py b/scrapy/trunk/scrapy/contrib/history/middleware.py index 9fae23bdd..18c1ec4a0 100644 --- a/scrapy/trunk/scrapy/contrib/history/middleware.py +++ b/scrapy/trunk/scrapy/contrib/history/middleware.py @@ -35,7 +35,7 @@ class HistoryMiddleware(object): def process_response(self, request, response, spider): version = request.context.get('history_response_version') - if version == response.version(): + if version == self.get_version(response): del request.content['history_response_version'] hist = self.historydata.version_info(domain, version) if hist: @@ -66,11 +66,13 @@ class HistoryMiddleware(object): if response: redirect_url = response.url parentkey = urlkey(response.request.headers.get('referer')) if response.request else None - version = response.version() + version = self.get_version(response) else: redirect_url, parentkey, version = url, None, None self.historydata.store(domain, key, url, parentkey, version, post_version) + def get_version(self, response): + key = hashlib.sha1(response.body.to_string()).hexdigest() def urlkey(url): """Generate a 'key' for a given url diff --git a/scrapy/trunk/scrapy/contrib/history/scheduler.py b/scrapy/trunk/scrapy/contrib/history/scheduler.py index 14137c2f5..835a5467a 100644 --- a/scrapy/trunk/scrapy/contrib/history/scheduler.py +++ b/scrapy/trunk/scrapy/contrib/history/scheduler.py @@ -2,6 +2,7 @@ WARNING: This Scheduler code is obsolete and needs to be rewritten """ +import hashlib from datetime import datetime from twisted.internet import defer @@ -72,7 +73,8 @@ class RulesScheduler(Scheduler): def callback(pagedata): """process other callback if we pass the checks""" - if version == pagedata.version(): + + if version == self.get_version(pagedata): hist = self.historydata.version_info(domain, version) if hist: versionkey, created = hist @@ -83,7 +85,7 @@ class RulesScheduler(Scheduler): message = "skipping %s: unchanged for %s" % (pagedata.url, delta) raise IgnoreRequest(message) self.record_visit(domain, request.url, pagedata.url, - pagedata.parent, pagedata.version(), + pagedata.parent, self.get(pagedata), post_version) return pagedata @@ -98,3 +100,6 @@ class RulesScheduler(Scheduler): #request.prepend_callback(d) return request + + def get_version(self, response): + key = hashlib.sha1(response.body.to_string()).hexdigest() diff --git a/scrapy/trunk/scrapy/core/downloader/handlers.py b/scrapy/trunk/scrapy/core/downloader/handlers.py index 0a8fd3d2a..59bb137a6 100644 --- a/scrapy/trunk/scrapy/core/downloader/handlers.py +++ b/scrapy/trunk/scrapy/core/downloader/handlers.py @@ -47,7 +47,7 @@ def download_http(request, spider): def _response(body): body = body or '' - status = factory.status + status = int(factory.status) headers = Headers(factory.response_headers) r = Response(domain=spider.domain_name, url=request.url, status=status, headers=headers, body=body) signals.send_catch_log(signal=signals.request_uploaded, sender='download_http', request=request, spider=spider) diff --git a/scrapy/trunk/scrapy/core/engine.py b/scrapy/trunk/scrapy/core/engine.py index 163bc622e..96165f5b5 100644 --- a/scrapy/trunk/scrapy/core/engine.py +++ b/scrapy/trunk/scrapy/core/engine.py @@ -353,7 +353,7 @@ class ExecutionEngine(object): if isinstance(response, Response): response.request = request # tie request to obtained response cached = 'cached' if response.cached else 'live' - log.msg("Crawled %s <%s> from <%s>" % (cached, response.url, request.headers.get('referer')), level=log.DEBUG, domain=domain) + log.msg("Crawled %s from <%s>" % (response, request.headers.get('referer')), level=log.DEBUG, domain=domain) return response elif isinstance(response, Request): redirected = response # proper alias diff --git a/scrapy/trunk/scrapy/http/__init__.py b/scrapy/trunk/scrapy/http/__init__.py index d9fef2626..d1a3f7c32 100644 --- a/scrapy/trunk/scrapy/http/__init__.py +++ b/scrapy/trunk/scrapy/http/__init__.py @@ -2,10 +2,10 @@ Module containing all HTTP related classes Use this module (instead of the more specific ones) when importing Headers, -Request, Response, ResponseBody and Url outside this module. +Request, Response and Url outside this module. """ from scrapy.http.url import Url from scrapy.http.headers import Headers from scrapy.http.request import Request -from scrapy.http.response import Response, ResponseBody +from scrapy.http.response import Response diff --git a/scrapy/trunk/scrapy/http/request.py b/scrapy/trunk/scrapy/http/request.py index a3c777ec2..6174a0c31 100644 --- a/scrapy/trunk/scrapy/http/request.py +++ b/scrapy/trunk/scrapy/http/request.py @@ -63,9 +63,10 @@ class Request(object): url = property(lambda x: x._url, set_url) def __str__(self): - if self.method != 'GET': - return "<(%s) %s>" % (self.method, self.url) - return "<%s>" % self.url + if self.method == 'GET': + return "<%s>" % self.url + else: + return "<%s %s>" % (self.method, self.url) def __len__(self): """Return raw HTTP request size""" @@ -103,7 +104,8 @@ class Request(object): s = "%s %s HTTP/1.1\r\n" % (self.method, self.url) s += "Host: %s\r\n" % self.url.hostname - s += self.headers.to_string() + "\r\n" + if self.headers: + s += self.headers.to_string() + "\r\n" s += "\r\n" if self.body: s += self.body diff --git a/scrapy/trunk/scrapy/http/response.py b/scrapy/trunk/scrapy/http/response.py index 589a33935..65fae7414 100644 --- a/scrapy/trunk/scrapy/http/response.py +++ b/scrapy/trunk/scrapy/http/response.py @@ -6,17 +6,15 @@ See documentation in docs/ref/request-response.rst """ import re -import hashlib import copy +from types import NoneType +from twisted.web.http import RESPONSES from BeautifulSoup import UnicodeDammit from scrapy.http.url import Url from scrapy.http.headers import Headers -from twisted.web import http -reason_phrases = http.RESPONSES - class Response(object): _ENCODING_RE = re.compile(r'charset=([\w-]+)', re.I) @@ -26,20 +24,17 @@ class Response(object): self.url = Url(url) self.headers = Headers(headers or {}) self.status = status - # ResponseBody is not meant to be used directly (use .replace instead) - assert(isinstance(body, basestring) or body is None) - self.body = ResponseBody(body, self.headers_encoding()) + if body is not None: + assert isinstance(body, basestring), \ + "body must be basestring, got %s" % type(body).__name__ + self.body = _ResponseBody(body, self.headers_encoding()) + else: + self.body = None self.cached = False - self.request = None # request which originated this response + self.request = None self.meta = {} self.cache = {} - def version(self): - """A hash of the contents of this response""" - if not hasattr(self, '_version'): - self._version = hashlib.sha1(self.body.to_string()).hexdigest() - return self._version - def headers_encoding(self): content_type = self.headers.get('Content-Type') if content_type: @@ -52,39 +47,36 @@ class Response(object): (repr(self.domain), repr(self.url), repr(self.headers), repr(self.status), repr(self.body)) def __str__(self): - version = '%s..%s' % (self.version()[:4], self.version()[-4:]) - return "" % (self.status, self.url, version) + if self.status == 200: + return "<%s>" % (self.url) + else: + return "<%d %s>" % (self.status, self.url) def __len__(self): """Return raw HTTP response size""" return len(self.to_string()) - def info(self): - return ">> newresp = oldresp.replace(body="New body") """ - def sameheaders(): - return copy.deepcopy(self.headers) - def samebody(): - return copy.deepcopy(self.body) - newresp = Response(kw.get('domain', self.domain), - kw.get('url', self.url), - headers=kw.get('headers', sameheaders()), - status=kw.get('status', self.status), - body=kw.get('body')) - # Response.__init__ forbids the use of ResponseBody instances - if 'body' not in kw: - newresp.body = samebody() - newresp.meta = self.meta.copy() - return newresp + new = self.__class__(domain=domain or self.domain, + url=url or self.url, + status=status or self.status, + headers=headers or copy.deepcopy(self.headers), + body=body) + if body is None: + new.body = copy.deepcopy(self.body) + new.meta = self.meta.copy() + return new def to_string(self): """ @@ -93,18 +85,24 @@ class Response(object): received (that's not exposed by Twisted). """ - s = "HTTP/1.1 %s %s\r\n" % (self.status, reason_phrases[int(self.status)]) - s += self.headers.to_string() + "\r\n" + s = "HTTP/1.1 %s %s\r\n" % (self.status, RESPONSES[self.status]) + if self.headers: + s += self.headers.to_string() + "\r\n" s += "\r\n" if self.body: s += self.body.to_string() s += "\r\n" return s -class ResponseBody(object): - """The body of an HTTP response +class _ResponseBody(object): + """The body of an HTTP response. + + WARNING: This is a private class and could be removed in the future without + previous notice. Do not use it this class from outside this module, use + the Response class instead. - This handles conversion to unicode and various character encodings. + Currently, the main purpose of this class is to handle conversion to + unicode and various character encodings. """ _template = r'''%s\s*=\s*["']?\s*%s\s*["']?''' @@ -182,7 +180,7 @@ class ResponseBody(object): return proposed def __repr__(self): - return "ResponseBody(content=%s, declared_encoding=%s)" % (repr(self._content), repr(self.declared_encoding)) + return "_ResponseBody(content=%s, declared_encoding=%s)" % (repr(self._content), repr(self.declared_encoding)) def __str__(self): return self.to_string() diff --git a/scrapy/trunk/scrapy/replay/__init__.py b/scrapy/trunk/scrapy/replay/__init__.py index 507c999dd..7abee0dc7 100644 --- a/scrapy/trunk/scrapy/replay/__init__.py +++ b/scrapy/trunk/scrapy/replay/__init__.py @@ -6,6 +6,7 @@ import shutil import tempfile import tarfile import copy +import hashlib from pydispatch import dispatcher @@ -132,7 +133,7 @@ class Replay(object): self.passed_new[str(item.guid)] = item def response_received(self, response, spider): - key = response.version() + key = hashlib.sha1(response.body.to_string()).hexdigest() if (self.recording or self.updating) and key: self.responses_old[key] = response.copy() elif key: diff --git a/scrapy/trunk/scrapy/tests/test_adaptors.py b/scrapy/trunk/scrapy/tests/test_adaptors.py index 0b830d2b0..7d1543db7 100644 --- a/scrapy/trunk/scrapy/tests/test_adaptors.py +++ b/scrapy/trunk/scrapy/tests/test_adaptors.py @@ -42,7 +42,7 @@ class AdaptorsTestCase(unittest.TestCase): def get_selector(self, domain, url, sample_filename, headers=None, selector=HtmlXPathSelector): sample_filename = os.path.join(self.samplesdir, sample_filename) body = file(sample_filename).read() - response = Response(domain=domain, url=url, headers=Headers(headers), status='200', body=body) + response = Response(domain=domain, url=url, headers=Headers(headers), status=200, body=body) return selector(response) def test_extract(self): diff --git a/scrapy/trunk/scrapy/tests/test_engine.py b/scrapy/trunk/scrapy/tests/test_engine.py index f64a8062a..2ae84d9d9 100644 --- a/scrapy/trunk/scrapy/tests/test_engine.py +++ b/scrapy/trunk/scrapy/tests/test_engine.py @@ -149,9 +149,9 @@ class EngineTest(unittest.TestCase): for response, spider in session.respplug: if session.getpath(response.url) == '/item999.html': - self.assertEqual('404', response.status) + self.assertEqual(404, response.status) if session.getpath(response.url) == '/redirect': - self.assertEqual('302', response.status) + self.assertEqual(302, response.status) self.assertEqual(response.domain, spider.domain_name) def test_item_data(self): diff --git a/scrapy/trunk/scrapy/tests/test_http_request.py b/scrapy/trunk/scrapy/tests/test_http_request.py index b9b6d793c..3da9da4d1 100644 --- a/scrapy/trunk/scrapy/tests/test_http_request.py +++ b/scrapy/trunk/scrapy/tests/test_http_request.py @@ -94,5 +94,23 @@ class RequestTest(unittest.TestCase): assert r1.meta is not r2.meta, "meta must be a shallow copy, not identical" self.assertEqual(r1.meta, r2.meta) + def test_copy_inherited_classes(self): + """Test Request children copies preserve their class""" + + class CustomRequest(Request): + pass + + r1 = CustomRequest('example.com', 'http://www.example.com') + r2 = r1.copy() + + assert type(r2) is CustomRequest + + def test_to_string(self): + r1 = Request("http://www.example.com") + self.assertEqual(r1.to_string(), 'GET http://www.example.com HTTP/1.1\r\nHost: www.example.com\r\n\r\n') + + r1 = Request("http://www.example.com", method='POST', headers={"Content-type": "text/html"}, body="Some body") + self.assertEqual(r1.to_string(), 'POST http://www.example.com HTTP/1.1\r\nHost: www.example.com\r\nContent-Type: text/html\r\n\r\nSome body\r\n') + if __name__ == "__main__": unittest.main() diff --git a/scrapy/trunk/scrapy/tests/test_http_response.py b/scrapy/trunk/scrapy/tests/test_http_response.py index 4fd165453..fa7600da7 100644 --- a/scrapy/trunk/scrapy/tests/test_http_response.py +++ b/scrapy/trunk/scrapy/tests/test_http_response.py @@ -1,7 +1,9 @@ import unittest -from scrapy.http import Response, ResponseBody +from scrapy.http import Response +from scrapy.http.response import _ResponseBody class ResponseTest(unittest.TestCase): + def test_init(self): # Response requires domain and url self.assertRaises(Exception, Response) @@ -10,7 +12,6 @@ class ResponseTest(unittest.TestCase): # body can be str or None but not ResponseBody self.assertTrue(isinstance(Response('example.com', 'http://example.com/', body=None), Response)) self.assertTrue(isinstance(Response('example.com', 'http://example.com/', body='body'), Response)) - self.assertRaises(AssertionError, Response, 'example.com', 'http://example.com/', body=ResponseBody('body', 'utf-8')) # test presence of all optional parameters self.assertTrue(isinstance(Response('example.com', 'http://example.com/', headers={}, status=200, body=None), Response)) @@ -29,12 +30,30 @@ class ResponseTest(unittest.TestCase): assert r1.meta is not r2.meta, "meta must be a shallow copy, not identical" self.assertEqual(r1.meta, r2.meta) + def test_copy_inherited_classes(self): + """Test Response children copies preserve their class""" + + class CustomResponse(Response): + pass + + r1 = CustomResponse('example.com', 'http://www.example.com') + r2 = r1.copy() + + assert type(r2) is CustomResponse + + def test_to_string(self): + r1 = Response('example.com', "http://www.example.com") + self.assertEqual(r1.to_string(), 'HTTP/1.1 200 OK\r\n\r\n') + + r1 = Response('example.com', "http://www.example.com", status=404, headers={"Content-type": "text/html"}, body="Some body") + self.assertEqual(r1.to_string(), 'HTTP/1.1 404 Not Found\r\nContent-Type: text/html\r\n\r\nSome body\r\n') + class ResponseBodyTest(unittest.TestCase): unicode_string = u'\u043a\u0438\u0440\u0438\u043b\u043b\u0438\u0447\u0435\u0441\u043a\u0438\u0439 \u0442\u0435\u043a\u0441\u0442' def test_encoding(self): original_string = self.unicode_string.encode('cp1251') - cp1251_body = ResponseBody(original_string, 'cp1251') + cp1251_body = _ResponseBody(original_string, 'cp1251') # check to_unicode self.assertTrue(isinstance(cp1251_body.to_unicode(), unicode))