Repository: scrapy/scrapy
Pull request #1847 (2016-03-05 22:41:44)

[MRG+2] added BLOCKING_FEED_STORAGE_PATH to settings
## PR Overview

I'm adding a new setting  BLOCKING_FEED_STORAGE_PATH to configure a folder used to save temporary files before uploading them. 

This setting defaults to None and will pick a system temporary folder. If a value is set, will raise an exception if it's not a valid directory and use it otherwise.

See https://github.com/scrapy/scrapy/issues/1779 for more details


codecov-io: ## [Current coverage][1] is `83.12%`
> Merging **#1847** into **master** will decrease coverage by **-0.06%** as of [`20773d3`][3]



[1]: https://codecov.io/github/scrapy/scrapy?ref=20773d3ff535e587969653025126863fb2a5cdec
[2]: https://codecov.io/github/scrapy/scrapy/features/suggestions?ref=20773d3ff535e587969653025126863fb2a5cdec
[3]: https://codecov.io/github/scrapy/scrapy/commit/20773d3ff535e587969653025126863fb2a5cdec
[4]: https://codecov.io/github/scrapy/scrapy/compare/e122c569fe2e444517ab14d2c7dc8351ce3c3cfa...20773d3ff535e587969653025126863fb2a5cdec

> Powered by [Codecov](https://codecov.io). Updated on successful CI builds.
redapple: @aron-bordin , could you add tests for this feature and setting to `scrapy.tests.feedexport`?
aron-bordin: Hi @redapple , I updated the PR. Please, take a look when possible.
redapple: @aron-bordin , could you add a test for the default case to make Codecov happy? :)

aron-bordin: @redapple updated
redapple: looks good to me. Thanks @aron-bordin !
kmike: The PR looks good :+1: 

But the `BLOCKING_FEED_STORAGE_PATH` name looks confusing, and other feed-related option names start with `FEED_`. What about `FEED_STORAGE_TEMPDIR`? Other ideas?

Patch:
diff --git a/docs/_ext/scrapydocs.py b/docs/_ext/scrapydocs.py
index 83b0d2cc6..f0827f2b1 100644
--- a/docs/_ext/scrapydocs.py
+++ b/docs/_ext/scrapydocs.py
@@ -18,7 +18,7 @@ def is_setting_index(node):
     if node.tagname == 'index':
         # index entries for setting directives look like:
         # [(u'pair', u'SETTING_NAME; setting', u'std:setting-SETTING_NAME', '')]
-        entry_type, info, refid = node['entries'][0][:3]
+        entry_type, info, refid, _ = node['entries'][0]
         return entry_type == 'pair' and info.endswith('; setting')
     return False
 
@@ -30,7 +30,7 @@ def get_setting_target(node):
 
 def get_setting_name_and_refid(node):
     """Extract setting name from directive index node"""
-    entry_type, info, refid = node['entries'][0][:3]
+    entry_type, info, refid, _ = node['entries'][0]
     return info.replace('; setting', ''), refid
 
 
diff --git a/docs/faq.rst b/docs/faq.rst
index 411e043b7..b132a9c2e 100644
--- a/docs/faq.rst
+++ b/docs/faq.rst
@@ -33,7 +33,7 @@ Scrapy is supported under Python 2.7 only.
 Python 2.6 support was dropped starting at Scrapy 0.20.
 
 Does Scrapy work with Python 3?
--------------------------------
+---------------------------------
 
 No, but there are plans to support Python 3.3+.
 At the moment, Scrapy works with Python 2.7.
@@ -57,6 +57,8 @@ focus on the real problems we need to solve.
 We'd be proud if Scrapy serves as an inspiration for other projects. Feel free
 to steal from us!
 
+.. _Django: https://www.djangoproject.com/
+
 Does Scrapy work with HTTP proxies?
 -----------------------------------
 
diff --git a/docs/topics/architecture.rst b/docs/topics/architecture.rst
index b4588652c..aef033dc3 100644
--- a/docs/topics/architecture.rst
+++ b/docs/topics/architecture.rst
@@ -48,7 +48,8 @@ Spiders
 
 Spiders are custom classes written by Scrapy users to parse responses and
 extract items (aka scraped items) from them or additional URLs (requests) to
-follow. For more information see :ref:`topics-spiders`.
+follow. Each spider is able to handle a specific domain (or group of domains).
+For more information see :ref:`topics-spiders`.
 
 Item Pipeline
 -------------
@@ -63,35 +64,18 @@ Downloader middlewares
 
 Downloader middlewares are specific hooks that sit between the Engine and the
 Downloader and process requests when they pass from the Engine to the
-Downloader, and responses that pass from Downloader to the Engine.
-
-Use a Downloader middleware if you need to do one of the following:
-
-* process a request just before it is sent to the Downloader
-  (i.e. right before Scrapy sends the request to the website);
-* change received response before passing it to a spider;
-* send a new Request instead of passing received response to a spider;
-* pass response to a spider without fetching a web page;
-* silently drop some requests.
-
-For more information see :ref:`topics-downloader-middleware`.
+Downloader, and responses that pass from Downloader to the Engine. They provide
+a convenient mechanism for extending Scrapy functionality by plugging custom
+code. For more information see :ref:`topics-downloader-middleware`.
 
 Spider middlewares
 ------------------
 
 Spider middlewares are specific hooks that sit between the Engine and the
 Spiders and are able to process spider input (responses) and output (items and
-requests).
-
-Use a Spider middleware if you need to
-
-* post-process output of spider callbacks - change/add/remove requests or items;
-* post-process start_requests;
-* handle spider exceptions;
-* call errback instead of callback for some of the requests based on response
-  content.
-
-For more information see :ref:`topics-spider-middleware`.
+requests). They provide a convenient mechanism for extending Scrapy
+functionality by plugging custom code. For more information see
+:ref:`topics-spider-middleware`.
 
 Data flow
 =========
@@ -99,31 +83,33 @@ Data flow
 The data flow in Scrapy is controlled by the execution engine, and goes like
 this:
 
-1. The Engine gets the first URLs to crawl from the Spider and schedules them
+1. The Engine opens a domain, locates the Spider that handles that domain, and
+   asks the spider for the first URLs to crawl.
+
+2. The Engine gets the first URLs to crawl from the Spider and schedules them
    in the Scheduler, as Requests.
 
-2. The Engine asks the Scheduler for the next URLs to crawl.
+3. The Engine asks the Scheduler for the next URLs to crawl.
 
-3. The Scheduler returns the next URLs to crawl to the Engine and the Engine
+4. The Scheduler returns the next URLs to crawl to the Engine and the Engine
    sends them to the Downloader, passing through the Downloader Middleware
    (request direction).
 
-4. Once the page finishes downloading the Downloader generates a Response (with
+5. Once the page finishes downloading the Downloader generates a Response (with
    that page) and sends it to the Engine, passing through the Downloader
    Middleware (response direction).
 
-5. The Engine receives the Response from the Downloader and sends it to the
+6. The Engine receives the Response from the Downloader and sends it to the
    Spider for processing, passing through the Spider Middleware (input direction).
 
-6. The Spider processes the Response and returns scraped items and new Requests
+7. The Spider processes the Response and returns scraped items and new Requests
    (to follow) to the Engine.
 
-7. The Engine passes scraped items and new Requests returned by a spider
-   through Spider Middleware (output direction), and then sends processed
-   items to Item Pipelines and processed Requests to the Scheduler.
+8. The Engine sends scraped items (returned by the Spider) to the Item Pipeline
+   and Requests (returned by spider) to the Scheduler
 
-8. The process repeats (from step 1) until there are no more requests from the
-   Scheduler.
+9. The process repeats (from step 2) until there are no more requests from the
+   Scheduler, and the Engine closes the domain.
 
 Event-driven networking
 =======================
diff --git a/docs/topics/email.rst b/docs/topics/email.rst
index 62ebc4c08..2b5812c8d 100644
--- a/docs/topics/email.rst
+++ b/docs/topics/email.rst
@@ -164,7 +164,7 @@ Password to use for SMTP authentication, along with :setting:`MAIL_USER`.
 .. setting:: MAIL_TLS
 
 MAIL_TLS
---------
+---------
 
 Default: ``False``
 
@@ -173,7 +173,7 @@ Enforce using STARTTLS. STARTTLS is a way to take an existing insecure connectio
 .. setting:: MAIL_SSL
 
 MAIL_SSL
---------
+---------
 
 Default: ``False``
 
diff --git a/docs/topics/media-pipeline.rst b/docs/topics/media-pipeline.rst
index a34f4c053..bf5d08d54 100644
--- a/docs/topics/media-pipeline.rst
+++ b/docs/topics/media-pipeline.rst
@@ -29,7 +29,7 @@ the same media to that queue. This avoids downloading the same media more than
 once when it's shared by several items.
 
 Using the Files Pipeline
-========================
+=========================
 
 The typical workflow, when using the :class:`FilesPipeline` goes like
 this:
@@ -104,7 +104,7 @@ behaviour, see :ref:`topics-media-pipeline-override`.
 .. _topics-media-pipeline-enabling:
 
 Enabling your Media Pipeline
-============================
+=============================
 
 .. setting:: IMAGES_STORE
 .. setting:: FILES_STORE
@@ -175,7 +175,7 @@ Additional features
 ===================
 
 File expiration
----------------
+----------------
 
 .. setting:: IMAGES_EXPIRES
 .. setting:: FILES_EXPIRES
diff --git a/docs/topics/request-response.rst b/docs/topics/request-response.rst
index 7b48c4d1c..10affc21f 100644
--- a/docs/topics/request-response.rst
+++ b/docs/topics/request-response.rst
@@ -67,7 +67,6 @@ Request objects
 
             request_with_cookies = Request(url="http://www.example.com",
                                            cookies={'currency': 'USD', 'country': 'UY'})
-
         2. Using a list of dicts::
 
             request_with_cookies = Request(url="http://www.example.com",
diff --git a/docs/topics/selectors.rst b/docs/topics/selectors.rst
index d1845b20c..3c5c170a3 100644
--- a/docs/topics/selectors.rst
+++ b/docs/topics/selectors.rst
@@ -151,7 +151,7 @@ It returns ``None`` if no element was found:
 
 A default return value can be provided as an argument, to be used instead of ``None``:
 
-    >>> response.xpath('//div[@id="not-exists"]/text()').extract_first(default='not-found')
+    >>> sel.xpath('//div[@id="not-exists"]/text()').extract_first(default='not-found')
     'not-found'
 
 Notice that CSS selectors can select text or attribute nodes using CSS3
@@ -738,14 +738,10 @@ simple/convenient XPaths. You can use the
 
 Let's show an example that illustrates this with Github blog atom feed.
 
-.. highlight:: sh
-
 First, we open the shell with the url we want to scrape::
 
     $ scrapy shell https://github.com/blog.atom
 
-.. highlight:: python
-
 Once in the shell we can try selecting all ``<link>`` objects and see that it
 doesn't work (because the Atom XML namespace is obfuscating those nodes)::
 
diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst
index 0716f1bd4..1bf428edb 100644
--- a/docs/topics/settings.rst
+++ b/docs/topics/settings.rst
@@ -178,6 +178,15 @@ Default: ``None``
 The AWS secret key used by code that requires access to `Amazon Web services`_,
 such as the :ref:`S3 feed storage backend <topics-feed-storage-s3>`.
 
+.. setting:: FEED_TEMPDIR
+
+FEED_TEMPDIR
+------------
+
+The Feed Temp dir allows you to set a custom folder to save crawler
+temporary files before uploading with :ref:`FTP feed storage <topics-feed-storage-ftp>` and
+:ref:`Amazon S3 <topics-feed-storage-s3>`.
+
 .. setting:: BOT_NAME
 
 BOT_NAME
@@ -342,7 +351,7 @@ Whether to enable DNS in-memory cache.
 .. setting:: DNSCACHE_SIZE
 
 DNSCACHE_SIZE
--------------
+----------------
 
 Default: ``10000``
 
@@ -351,7 +360,7 @@ DNS in-memory cache size.
 .. setting:: DNS_TIMEOUT
 
 DNS_TIMEOUT
------------
+----------------
 
 Default: ``60``
 
diff --git a/docs/topics/signals.rst b/docs/topics/signals.rst
index 6434c98c5..410717f7d 100644
--- a/docs/topics/signals.rst
+++ b/docs/topics/signals.rst
@@ -203,7 +203,7 @@ request_scheduled
     :type spider: :class:`~scrapy.spiders.Spider` object
 
 request_dropped
----------------
+-----------------
 
 .. signal:: request_dropped
 .. function:: request_dropped(request, spider)
diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py
index 3dab2d77e..5d90ef738 100644
--- a/scrapy/extensions/feedexport.py
+++ b/scrapy/extensions/feedexport.py
@@ -8,7 +8,7 @@ import os
 import sys
 import logging
 import posixpath
-from tempfile import TemporaryFile
+from tempfile import NamedTemporaryFile
 from datetime import datetime
 import six
 from six.moves.urllib.parse import urlparse
@@ -47,7 +47,11 @@ class IFeedStorage(Interface):
 class BlockingFeedStorage(object):
 
     def open(self, spider):
-        return TemporaryFile(prefix='feed-')
+        path = spider.crawler.settings['FEED_TEMPDIR']
+        if path and not os.path.isdir(path):
+            raise OSError('Not a Directory: ' + str(path))
+
+        return NamedTemporaryFile(prefix='feed-', dir=path)
 
     def store(self, file):
         return threads.deferToThread(self._store_in_thread, file)
diff --git a/scrapy/extensions/httpcache.py b/scrapy/extensions/httpcache.py
index 247cac64e..a871cc895 100644
--- a/scrapy/extensions/httpcache.py
+++ b/scrapy/extensions/httpcache.py
@@ -328,7 +328,7 @@ class FilesystemCacheStorage(object):
         metapath = os.path.join(rpath, 'pickled_meta')
         if not os.path.exists(metapath):
             return  # not found
-        mtime = os.stat(metapath).st_mtime
+        mtime = os.stat(rpath).st_mtime
         if 0 < self.expiration_secs < time() - mtime:
             return  # expired
         with self._open(metapath, 'rb') as f:
diff --git a/scrapy/pipelines/files.py b/scrapy/pipelines/files.py
index b1b8404bb..1fd2571e2 100644
--- a/scrapy/pipelines/files.py
+++ b/scrapy/pipelines/files.py
@@ -147,7 +147,7 @@ class S3FilesStore(object):
                 Bucket=self.bucket,
                 Key=key_name,
                 Body=buf,
-                Metadata={k: str(v) for k, v in six.iteritems(meta or {})},
+                Metadata={k: str(v) for k, v in six.iteritems(meta)},
                 ACL=self.POLICY,
                 **extra)
         else:
@@ -364,9 +364,8 @@ class FilesPipeline(MediaPipeline):
     def file_downloaded(self, response, request, info):
         path = self.file_path(request, response=response, info=info)
         buf = BytesIO(response.body)
-        checksum = md5sum(buf)
-        buf.seek(0)
         self.store.persist_file(path, buf, info)
+        checksum = md5sum(buf)
         return checksum
 
     def item_completed(self, results, item, info):
diff --git a/scrapy/responsetypes.py b/scrapy/responsetypes.py
index 075f757af..c667b141d 100644
--- a/scrapy/responsetypes.py
+++ b/scrapy/responsetypes.py
@@ -10,7 +10,7 @@ import six
 
 from scrapy.http import Response
 from scrapy.utils.misc import load_object
-from scrapy.utils.python import binary_is_text, to_bytes, to_native_str
+from scrapy.utils.python import isbinarytext, to_bytes, to_native_str
 
 
 class ResponseTypes(object):
@@ -94,7 +94,7 @@ class ResponseTypes(object):
         cannot be guess using more straightforward methods."""
         chunk = body[:5000]
         chunk = to_bytes(chunk)
-        if not binary_is_text(chunk):
+        if isbinarytext(chunk):
             return self.from_mimetype('application/octet-stream')
         elif b"<html>" in chunk.lower():
             return self.from_mimetype('text/html')
diff --git a/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py
index d449c4891..b9d01e155 100644
--- a/scrapy/settings/default_settings.py
+++ b/scrapy/settings/default_settings.py
@@ -134,6 +134,7 @@ EXTENSIONS_BASE = {
     'scrapy.extensions.throttle.AutoThrottle': 0,
 }
 
+FEED_TEMPDIR = None
 FEED_URI = None
 FEED_URI_PARAMS = None  # a function to extend uri arguments
 FEED_FORMAT = 'jsonlines'
diff --git a/scrapy/utils/python.py b/scrapy/utils/python.py
index 42fbbda7f..1f9d02df5 100644
--- a/scrapy/utils/python.py
+++ b/scrapy/utils/python.py
@@ -174,25 +174,17 @@ def memoizemethod_noargs(method):
         return cache[self]
     return new_method
 
-
 _BINARYCHARS = {six.b(chr(i)) for i in range(32)} - {b"\0", b"\t", b"\n", b"\r"}
 _BINARYCHARS |= {ord(ch) for ch in _BINARYCHARS}
 
-@deprecated("scrapy.utils.python.binary_is_text")
-def isbinarytext(text):
-    """ This function is deprecated.
-    Please use scrapy.utils.python.binary_is_text, which was created to be more
-    clear about the functions behavior: it is behaving inverted to this one. """
-    return not binary_is_text(text)
 
-
-def binary_is_text(data):
-    """ Returns `True` if the given ``data`` argument (a ``bytes`` object)
-    does not contain unprintable control characters.
+def isbinarytext(text):
+    """Return True if the given text is considered binary, or False
+    otherwise, by looking for binary bytes at their chars
     """
-    if not isinstance(data, bytes):
-        raise TypeError("data must be bytes, got '%s'" % type(data).__name__)
-    return all(c not in _BINARYCHARS for c in data)
+    if not isinstance(text, bytes):
+        raise TypeError("text must be bytes, got '%s'" % type(text).__name__)
+    return any(c in _BINARYCHARS for c in text)
 
 
 def get_func_args(func, stripself=False):
diff --git a/scrapy/utils/response.py b/scrapy/utils/response.py
index d9e951463..73db2641e 100644
--- a/scrapy/utils/response.py
+++ b/scrapy/utils/response.py
@@ -47,8 +47,14 @@ def get_meta_refresh(response):
 
 def response_status_message(status):
     """Return status code plus status text descriptive message
+
+    >>> response_status_message(200)
+    '200 OK'
+
+    >>> response_status_message(404)
+    '404 Not Found'
     """
-    return '%s %s' % (status, to_native_str(http.RESPONSES.get(int(status), "Unknown Status")))
+    return '%s %s' % (status, to_native_str(http.RESPONSES.get(int(status))))
 
 
 def response_httprepr(response):
diff --git a/tox.ini b/tox.ini
index f6de64b27..2a8067618 100644
--- a/tox.ini
+++ b/tox.ini
@@ -60,7 +60,6 @@ deps = {[testenv:py33]deps}
 changedir = docs
 deps =
     Sphinx
-    sphinx_rtd_theme
 
 [testenv:docs]
 changedir = {[docs]changedir}


Test patch:
diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py
index c76d26b57..05f19d33e 100644
--- a/tests/test_feedexport.py
+++ b/tests/test_feedexport.py
@@ -18,9 +18,9 @@ from w3lib.url import path_to_file_uri
 import scrapy
 from scrapy.extensions.feedexport import (
     IFeedStorage, FileFeedStorage, FTPFeedStorage,
-    S3FeedStorage, StdoutFeedStorage
-)
-from scrapy.utils.test import assert_aws_environ, get_s3_content_and_delete
+    S3FeedStorage, StdoutFeedStorage,
+    BlockingFeedStorage)
+from scrapy.utils.test import assert_aws_environ, get_s3_content_and_delete, get_crawler
 from scrapy.utils.python import to_native_str
 
 
@@ -87,6 +87,41 @@ class FTPFeedStorageTest(unittest.TestCase):
             self.assertEqual(fp.read(), b"new content")
 
 
+class BlockingFeedStorageTest(unittest.TestCase):
+
+    def get_test_spider(self, settings=None):
+        class TestSpider(scrapy.Spider):
+            name = 'test_spider'
+        crawler = get_crawler(settings_dict=settings)
+        spider = TestSpider.from_crawler(crawler)
+        return spider
+
+    def test_default_temp_dir(self):
+        b = BlockingFeedStorage()
+
+        tmp = b.open(self.get_test_spider())
+        tmp_path = os.path.dirname(tmp.name)
+        self.assertEqual(tmp_path, tempfile.gettempdir())
+
+    def test_temp_file(self):
+        b = BlockingFeedStorage()
+
+        tests_path = os.path.dirname(os.path.abspath(__file__))
+        spider = self.get_test_spider({'FEED_TEMPDIR': tests_path})
+        tmp = b.open(spider)
+        tmp_path = os.path.dirname(tmp.name)
+        self.assertEqual(tmp_path, tests_path)
+
+    def test_invalid_folder(self):
+        b = BlockingFeedStorage()
+
+        tests_path = os.path.dirname(os.path.abspath(__file__))
+        invalid_path = os.path.join(tests_path, 'invalid_path')
+        spider = self.get_test_spider({'FEED_TEMPDIR': invalid_path})
+
+        self.assertRaises(OSError, b.open, spider=spider)
+
+
 class S3FeedStorageTest(unittest.TestCase):
 
     @defer.inlineCallbacks
diff --git a/tests/test_utils_python.py b/tests/test_utils_python.py
index 9a0cc975d..4f0834902 100644
--- a/tests/test_utils_python.py
+++ b/tests/test_utils_python.py
@@ -5,7 +5,7 @@ from itertools import count
 import six
 
 from scrapy.utils.python import (
-    memoizemethod_noargs, binary_is_text, equal_attributes,
+    memoizemethod_noargs, isbinarytext, equal_attributes,
     WeakKeyCache, stringify_dict, get_func_args, to_bytes, to_unicode,
     without_none_values)
 
@@ -71,18 +71,18 @@ class MemoizedMethodTest(unittest.TestCase):
         assert one is not three
 
 
-class BinaryIsTextTest(unittest.TestCase):
-    def test_binaryistext(self):
-        assert binary_is_text(b"hello")
+class IsBinaryTextTest(unittest.TestCase):
+    def test_isbinarytext(self):
+        assert not isbinarytext(b"hello")
 
     def test_utf_16_strings_contain_null_bytes(self):
-        assert binary_is_text(u"hello".encode('utf-16'))
+        assert not isbinarytext(u"hello".encode('utf-16'))
 
     def test_one_with_encoding(self):
-        assert binary_is_text(b"<div>Price \xa3</div>")
+        assert not isbinarytext(b"<div>Price \xa3</div>")
 
     def test_real_binary_bytes(self):
-        assert not binary_is_text(b"\x02\xa3")
+        assert isbinarytext(b"\x02\xa3")
 
 
 
diff --git a/tests/test_utils_response.py b/tests/test_utils_response.py
index bea4dade3..1d1638e06 100644
--- a/tests/test_utils_response.py
+++ b/tests/test_utils_response.py
@@ -5,7 +5,7 @@ from six.moves.urllib.parse import urlparse
 from scrapy.http import Response, TextResponse, HtmlResponse
 from scrapy.utils.python import to_bytes
 from scrapy.utils.response import (response_httprepr, open_in_browser,
-                                   get_meta_refresh, get_base_url, response_status_message)
+                                   get_meta_refresh, get_base_url)
 
 __doctests__ = ['scrapy.utils.response']
 
@@ -78,8 +78,3 @@ class ResponseUtilsTest(unittest.TestCase):
         resp2 = HtmlResponse("http://www.example.com", body=b"""
         <html><body>blahablsdfsal&amp;</body></html>""")
         self.assertEqual(get_base_url(resp2), "http://www.example.com")
-
-    def test_response_status_message(self):
-        self.assertEqual(response_status_message(200), '200 OK')
-        self.assertEqual(response_status_message(404), '404 Not Found')
-        self.assertEqual(response_status_message(573), "573 Unknown Status")