From 3783470337e022fba84c6f0f2a1be941d75302ee Mon Sep 17 00:00:00 2001 From: Adrian Chaves Date: Thu, 30 Jul 2026 14:13:47 +0200 Subject: [PATCH 1/3] Support feed modes --- docs/topics/commands.rst | 2 +- docs/topics/feed-exports.rst | 120 +++++-- docs/topics/jobs.rst | 9 + scrapy/extensions/feedexport.py | 213 ++++++++++-- scrapy/settings/default_settings.py | 1 + .../templates/project/module/settings.py.tmpl | 1 + scrapy/utils/conf.py | 26 +- scrapy/utils/ftp.py | 25 +- tests/test_command_crawl.py | 2 +- tests/test_command_runspider.py | 2 +- tests/test_feedexport.py | 317 +++++++++++++++++- tests/test_feedexport_storages.py | 201 ++++++++--- tests/test_utils_conf.py | 55 ++- 13 files changed, 857 insertions(+), 117 deletions(-) diff --git a/docs/topics/commands.rst b/docs/topics/commands.rst index ee2c3a3cd..28525779d 100644 --- a/docs/topics/commands.rst +++ b/docs/topics/commands.rst @@ -271,7 +271,7 @@ Supported options: * ``-a NAME=VALUE``: set a spider argument (may be repeated) -* ``--output FILE`` or ``-o FILE``: append scraped items to the end of FILE (use - for stdout). To define the output format, set a colon at the end of the output URI (i.e. ``-o FILE:FORMAT``) +* ``--output FILE`` or ``-o FILE``: dump scraped items into FILE (use - for stdout), handling an existing FILE as the :setting:`FEED_MODE` setting indicates. To define the output format, set a colon at the end of the output URI (i.e. ``-o FILE:FORMAT``) * ``--overwrite-output FILE`` or ``-O FILE``: dump scraped items into FILE, overwriting any existing file. To define the output format, set a colon at the end of the output URI (i.e. ``-O FILE:FORMAT``) diff --git a/docs/topics/feed-exports.rst b/docs/topics/feed-exports.rst index 66768c97b..37b72d827 100644 --- a/docs/topics/feed-exports.rst +++ b/docs/topics/feed-exports.rst @@ -167,6 +167,9 @@ Note that for the local filesystem storage (only) you can omit the scheme if you specify a path (e.g. ``/tmp/export.csv``). Alternatively you can also use a :class:`pathlib.Path` object. +Supported :ref:`modes `: ``"append"``, ``"create"`` and +``"overwrite"``. If :setting:`FEED_MODE` is ``None``, ``"append"`` is used. + .. _topics-feed-storage-ftp: FTP @@ -183,11 +186,16 @@ FTP supports two different connection modes: `active or passive mode by default. To use the active connection mode instead, set the :setting:`FEED_STORAGE_FTP_ACTIVE` setting to ``True``. -The default value for the ``overwrite`` key in the :setting:`FEEDS` for this -storage backend is: ``True``. +Supported :ref:`modes `: ``"append"``, ``"create"`` and +``"overwrite"``. If :setting:`FEED_MODE` is ``None``, ``"overwrite"`` is used. -.. caution:: The value ``True`` in ``overwrite`` will cause you to lose the - previous version of your data. +.. note:: Some FTP servers may not support appending to files (the ``APPE`` FTP + command). + +.. note:: FTP has no atomic create-if-absent command, so ``"create"`` is + best-effort: the target is checked for existence right before it is + written. Servers that do not support the ``SIZE`` command are + reported as not having the target. This storage backend uses :ref:`delayed file delivery `. @@ -225,11 +233,11 @@ feeds using these settings: - :setting:`AWS_ENDPOINT_URL` - :setting:`AWS_REGION_NAME` -The default value for the ``overwrite`` key in the :setting:`FEEDS` for this -storage backend is: ``True``. +Supported :ref:`modes `: ``"create"`` and ``"overwrite"``. If +:setting:`FEED_MODE` is ``None``, ``"overwrite"`` is used. -.. caution:: The value ``True`` in ``overwrite`` will cause you to lose the - previous version of your data. +``"create"`` mode `does not support files larger than 5 GB +`__. This storage backend uses :ref:`delayed file delivery `. @@ -256,11 +264,8 @@ You can set a *Project ID* and *Access Control List (ACL)* through the following - :setting:`FEED_STORAGE_GCS_ACL` - :setting:`GCS_PROJECT_ID` -The default value for the ``overwrite`` key in the :setting:`FEEDS` for this -storage backend is: ``True``. - -.. caution:: The value ``True`` in ``overwrite`` will cause you to lose the - previous version of your data. +Supported :ref:`modes `: ``"create"`` and ``"overwrite"``. If +:setting:`FEED_MODE` is ``None``, ``"overwrite"`` is used. This storage backend uses :ref:`delayed file delivery `. @@ -277,6 +282,8 @@ The feeds are written to the standard output of the Scrapy process. - Example URI: ``stdout:`` - Required external libraries: none +The :ref:`mode ` is ignored by this storage backend. + .. _delayed-file-delivery: @@ -405,6 +412,7 @@ These are the settings used for configuring the feed exports: - :setting:`FEEDS` (mandatory) - :setting:`FEED_EXPORT_ENCODING` +- :setting:`FEED_MODE` - :setting:`FEED_STORE_EMPTY` - :setting:`FEED_EXPORT_FIELDS` - :setting:`FEED_EXPORT_INDENT` @@ -493,24 +501,9 @@ as a fallback value if that key is not provided for a specific feed definition: - ``item_export_kwargs``: :class:`dict` with keyword arguments for the corresponding :ref:`item exporter class `. -- ``overwrite``: whether to overwrite the file if it already exists - (``True``) or append to its content (``False``). +- ``mode``: falls back to :setting:`FEED_MODE`. - The default value depends on the :ref:`storage backend - `: - - - :ref:`topics-feed-storage-fs`: ``False`` - - - :ref:`topics-feed-storage-ftp`: ``True`` - - .. note:: Some FTP servers may not support appending to files (the - ``APPE`` FTP command). - - - :ref:`topics-feed-storage-s3`: ``True`` (appending is not supported) - - - :ref:`topics-feed-storage-gcs`: ``True`` (appending is not supported) - - - :ref:`topics-feed-storage-stdout`: ``False`` (overwriting is not supported) + .. versionadded:: VERSION - ``store_empty``: falls back to :setting:`FEED_STORE_EMPTY`. @@ -561,6 +554,44 @@ Currently implemented only by :class:`~scrapy.exporters.JsonItemExporter` and :class:`~scrapy.exporters.XmlItemExporter`, i.e. when you are exporting to ``.json`` or ``.xml``. +.. _feed-mode: +.. setting:: FEED_MODE + +FEED_MODE +--------- + +.. versionadded:: VERSION + +Default: ``"create"`` (:ref:`fallback `: ``None``) + +What to do when the target of a feed already exists. + +Supported values, which match the corresponding :func:`open` modes: + +- ``"append"`` (``a``): append to the existing target. + +- ``"create"`` (``x``): do not write, keeping the existing target untouched. + +- ``"overwrite"`` (``w``): replace the existing target. + +Can be overridden with the ``mode`` :ref:`feed option `. + +Not every :ref:`storage backend ` supports every +mode; see the documentation of your storage backend. + +If ``None``, the mode depends on the backend being used, e.g. appending for the +:ref:`local filesystem ` and overwriting for :ref:`S3 +`. + +When the mode of a feed is ``"create"`` and its target already exists, Scrapy +logs an error, increases the ``feedexport/conflicts/`` +:ref:`stat `, and does not write that file, i.e. the items that it +would have contained are lost. + +.. note:: When resuming a crawl (see :setting:`JOBDIR`), the feed of the + previous run already exists, so a feed that must survive a resumed crawl + needs the ``"append"`` mode. + .. setting:: FEED_STORE_EMPTY FEED_STORE_EMPTY @@ -570,8 +601,8 @@ Default: ``True`` Whether to export empty feeds (i.e. feeds with no items). If ``False``, and there are no items to export, no new files are created and -existing files are not modified, even if the :ref:`overwrite feed option -` is enabled. +existing files are not modified, even if the ``mode`` :ref:`feed option +` is ``"overwrite"``. .. setting:: FEED_STORAGES @@ -581,7 +612,30 @@ FEED_STORAGES Default: ``{}`` A dict containing additional feed storage backends supported by your project. -The keys are URI schemes and the values are paths to storage classes. +The keys are URI schemes and the values are paths to storage classes, which must +follow this protocol: + +.. autoclass:: FeedStorageProtocol + :members: + + .. attribute:: supported_modes + :type: frozenset[str] + + Optional set of supported values of the ``mode`` :ref:`feed option + `, e.g. ``frozenset({"create", "overwrite"})``. + + Setting ``mode`` to an unsupported value raises an exception, which + prevents the crawl from running. A storage class that does not define + this attribute is assumed to predate the ``mode`` feed option, and + setting that option on a feed that uses it logs a warning about the + option being possibly ignored. + + A storage class that declares support for ``"create"`` must enforce it + when writing the feed, ideally atomically, raising + :exc:`FileExistsError` if the target exists. Scrapy then handles that + file as described in :setting:`FEED_MODE`. + + .. versionadded:: VERSION .. setting:: FEED_STORAGE_FTP_ACTIVE diff --git a/docs/topics/jobs.rst b/docs/topics/jobs.rst index c9916110d..544f2cd03 100644 --- a/docs/topics/jobs.rst +++ b/docs/topics/jobs.rst @@ -83,6 +83,15 @@ stopping it cleanly. Forced, sudden or otherwise unclean shutdown can lead to data corruption in the job directory, which may prevent the spider from resuming correctly. +Feed exports +------------ + +When a job is resumed, the :ref:`feeds ` of the previous +run already exist, so their :ref:`mode ` determines what happens to +the items of the resumed run: ``"append"`` adds them to those feeds, +``"overwrite"`` replaces the items of the previous run with them, and +``"create"`` does not write them anywhere. + Cookies expiration ------------------ diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py index c2997921d..835cd0377 100644 --- a/scrapy/extensions/feedexport.py +++ b/scrapy/extensions/feedexport.py @@ -79,6 +79,38 @@ UriParamsCallableT: TypeAlias = Callable[ ] +FEED_MODES: frozenset[str] = frozenset({"append", "create", "overwrite"}) + + +def _get_mode(storage: Any, feed_options: dict[str, Any] | None, legacy: str) -> str: + """Return the mode of *storage*, based on the *mode* feed option, or on the + deprecated *overwrite* feed option, or, if neither is set, on *legacy*, + which must be the mode that the storage used before the *mode* feed option + existed.""" + feed_options = feed_options or {} + mode = feed_options.get("mode") + if mode is None: + overwrite = feed_options.get("overwrite") + mode = legacy if overwrite is None else "overwrite" if overwrite else "append" + _check_mode(mode, storage) + return mode + + +def _check_mode(mode: str, storage: Any, uri: str | None = None) -> None: + if mode not in FEED_MODES: + raise ValueError( + f"Invalid feed mode: {mode!r}. Supported modes: " + f"{', '.join(sorted(FEED_MODES))}." + ) + supported: frozenset[str] | None = getattr(storage, "supported_modes", None) + if supported is not None and mode not in supported: + suffix = f" (feed URI: {uri})" if uri else "" + raise ValueError( + f"{type(storage).__name__} does not support the {mode!r} feed " + f"mode{suffix}. Supported modes: {', '.join(sorted(supported))}." + ) + + class ItemFilter: """ This will be used by FeedExporter to decide if an item should be allowed @@ -157,6 +189,9 @@ class BlockingFeedStorage(ABC): class StdoutFeedStorage: + # The mode is irrelevant here: writing to a stream cannot destroy data. + supported_modes: frozenset[str] = FEED_MODES + def __init__( self, uri: str, @@ -167,13 +202,6 @@ class StdoutFeedStorage: if not _stdout: _stdout = sys.stdout.buffer self._stdout: IO[bytes] = _stdout - if feed_options and feed_options.get("overwrite", False) is True: - logger.warning( - "Standard output (stdout) storage does not support " - "overwriting. To suppress this warning, remove the " - "overwrite option from your FEEDS setting, or set " - "it to False." - ) def open(self, spider: Spider) -> IO[bytes]: return self._stdout @@ -182,18 +210,27 @@ class StdoutFeedStorage: pass +_WRITE_MODES: dict[str, OpenBinaryMode] = { + "append": "ab", + "create": "xb", + "overwrite": "wb", +} + + class FileFeedStorage: + supported_modes: frozenset[str] = FEED_MODES + def __init__(self, uri: str, *, feed_options: dict[str, Any] | None = None): self.path: str = file_uri_to_path(uri) if uri.startswith("file:") else uri - feed_options = feed_options or {} - self.write_mode: OpenBinaryMode = ( - "wb" if feed_options.get("overwrite", False) else "ab" - ) + self.write_mode: OpenBinaryMode = _WRITE_MODES[ + _get_mode(self, feed_options, "append") + ] def open(self, spider: Spider) -> IO[bytes]: dirname = Path(self.path).parent if dirname and not dirname.exists(): dirname.mkdir(parents=True) + # pylint: disable-next=unspecified-encoding # binary mode return Path(self.path).open(self.write_mode) def store(self, file: IO[bytes]) -> Deferred[None] | None: @@ -202,6 +239,8 @@ class FileFeedStorage: class S3FeedStorage(BlockingFeedStorage): + supported_modes: frozenset[str] = frozenset({"create", "overwrite"}) + def __init__( self, uri: str, @@ -239,12 +278,7 @@ class S3FeedStorage(BlockingFeedStorage): region_name=self.region_name, ) - if feed_options and feed_options.get("overwrite", True) is False: - logger.warning( - "S3 does not support appending to files. To " - "suppress this warning, remove the overwrite " - "option from your FEEDS setting or set it to True." - ) + self._mode: str = _get_mode(self, feed_options, "overwrite") @classmethod def from_crawler( @@ -266,14 +300,37 @@ class S3FeedStorage(BlockingFeedStorage): ) def _store_in_thread(self, file: IO[bytes]) -> None: + from botocore.exceptions import ClientError # noqa: PLC0415 + file.seek(0) + extra_args: dict[str, Any] = {"ACL": self.acl} if self.acl else {} try: - if self.acl: + if self._mode == "create": + # upload_fileobj() does not allow IfNoneMatch + # (https://github.com/boto/boto3/issues/4366). + try: + self.s3_client.put_object( + Bucket=self.bucketname, + Key=self.keyname, + Body=file, + IfNoneMatch="*", + **extra_args, + ) + except ClientError as error: + if ( + error.response.get("Error", {}).get("Code") + == "PreconditionFailed" + ): + raise FileExistsError( + f"s3://{self.bucketname}/{self.keyname} already exists" + ) from error + raise + elif extra_args: self.s3_client.upload_fileobj( Bucket=self.bucketname, Key=self.keyname, Fileobj=file, - ExtraArgs={"ACL": self.acl}, + ExtraArgs=extra_args, ) else: self.s3_client.upload_fileobj( @@ -286,6 +343,8 @@ class S3FeedStorage(BlockingFeedStorage): class GCSFeedStorage(BlockingFeedStorage): + supported_modes: frozenset[str] = frozenset({"create", "overwrite"}) + def __init__( self, uri: str, @@ -301,12 +360,7 @@ class GCSFeedStorage(BlockingFeedStorage): self.bucket_name: str = u.hostname self.blob_name: str = u.path[1:] # remove first "/" - if feed_options and feed_options.get("overwrite", True) is False: - logger.warning( - "GCS does not support appending to files. To " - "suppress this warning, remove the overwrite " - "option from your FEEDS setting or set it to True." - ) + self._mode: str = _get_mode(self, feed_options, "overwrite") @classmethod def from_crawler( @@ -323,20 +377,34 @@ class GCSFeedStorage(BlockingFeedStorage): feed_options=feed_options, ) + def _get_blob(self) -> Any: + from google.cloud.storage import Client # noqa: PLC0415 + + client = Client(project=self.project_id) + bucket = client.get_bucket(self.bucket_name) + return bucket.blob(self.blob_name) + def _store_in_thread(self, file: IO[bytes]) -> None: + from google.api_core.exceptions import PreconditionFailed # noqa: PLC0415 + file.seek(0) try: - from google.cloud.storage import Client # noqa: PLC0415 - - client = Client(project=self.project_id) - bucket = client.get_bucket(self.bucket_name) - blob = bucket.blob(self.blob_name) - blob.upload_from_file(file, predefined_acl=self.acl) + kwargs = {"if_generation_match": 0} if self._mode == "create" else {} + try: + self._get_blob().upload_from_file( + file, predefined_acl=self.acl, **kwargs + ) + except PreconditionFailed as error: + raise FileExistsError( + f"gs://{self.bucket_name}/{self.blob_name} already exists" + ) from error finally: file.close() class FTPFeedStorage(BlockingFeedStorage): + supported_modes: frozenset[str] = FEED_MODES + def __init__( self, uri: str, @@ -353,7 +421,16 @@ class FTPFeedStorage(BlockingFeedStorage): self.password: str = unquote(u.password or "") self.path: str = u.path self.use_active_mode: bool = use_active_mode - self.overwrite: bool = not feed_options or feed_options.get("overwrite", True) + self._mode: str = _get_mode(self, feed_options, "overwrite") + + @property + def overwrite(self) -> bool: + warnings.warn( + "FTPFeedStorage.overwrite is deprecated, use the mode feed option instead.", + ScrapyDeprecationWarning, + stacklevel=2, + ) + return self._mode != "append" @classmethod def from_crawler( @@ -378,7 +455,7 @@ class FTPFeedStorage(BlockingFeedStorage): username=self.username, password=self.password, use_active_mode=self.use_active_mode, - overwrite=self.overwrite, + mode=self._mode, ) @@ -416,6 +493,7 @@ class FeedSlot: self.crawler: Crawler = crawler # flags self.itemcount: int = 0 + self._skipped: bool = False self._exporting: bool = False self._fileloaded: bool = False @@ -517,6 +595,19 @@ class FeedExporter: self.exporters: dict[str, type[BaseItemExporter]] = self._load_components( "FEED_EXPORTERS" ) + if any( + feed_options.get("mode") is None for feed_options in self.feeds.values() + ): + warnings.warn( + "The default value of the FEED_MODE setting will change from " + "None to 'create' in a future Scrapy version, i.e. Scrapy will " + "stop writing feeds whose target already exists. Explicitly " + "set FEED_MODE, or the mode feed option of every feed, to " + "silence this warning.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + for uri, feed_options in self.feeds.items(): if not self._storage_supported(uri, feed_options): raise NotConfigured @@ -538,6 +629,23 @@ class FeedExporter: ) ) + def _skip(self, slot: FeedSlot) -> None: + """Report that the target of *slot* already exists, and mark *slot* so + that nothing is written to it.""" + slot._skipped = True + logger.error( + "Not writing %(uri)s because it already exists and the feed mode " + "is 'create'; its items are lost. To write it instead, remove the " + "target, or set the mode feed option or the FEED_MODE setting to " + "'overwrite' or 'append' (-O implies 'overwrite').", + {"uri": slot.uri}, + extra={"spider": slot.spider}, + ) + assert self.crawler.stats + self.crawler.stats.inc_value( + f"feedexport/conflicts/{type(slot.storage).__name__}" + ) + async def close_spider(self, spider: Spider) -> None: for slot in self.slots: self._schedule_slot_close(slot, spider) @@ -586,6 +694,8 @@ class FeedExporter: return slot_.file async def _close_slot(self, slot: FeedSlot, spider: Spider) -> None: + if slot._skipped: + return if slot.itemcount: # Normal case @@ -603,6 +713,9 @@ class FeedExporter: assert self.crawler.stats try: await ensure_awaitable(slot.storage.store(self._get_file(slot))) + except FileExistsError: + self._skip(slot) + self.crawler.stats.inc_value(f"feedexport/failed_count/{slot_type}") except Exception: logger.error( "Error storing %s", @@ -661,9 +774,16 @@ class FeedExporter: ) # if slot doesn't accept item, continue with next slot continue - slot.start_exporting() - assert slot.exporter - slot.exporter.export_item(item) + if not slot._skipped: + try: + slot.start_exporting() + except FileExistsError: + self._skip(slot) + if not slot._skipped: + assert slot.exporter + slot.exporter.export_item(item) + # Skipped items are counted, so that the following files of this + # feed still cover the same items as those of any other feed. slot.itemcount += 1 # create new slot for each slot with itemcount == FEED_EXPORT_BATCH_ITEM_COUNT and close the old one if ( @@ -725,7 +845,8 @@ class FeedExporter: scheme = urlparse(uri).scheme if scheme in self.storages or PureWindowsPath(uri).drive: try: - self._get_storage(uri, feed_options) + storage = self._get_storage(uri, feed_options) + self._check_storage_mode(uri, storage, feed_options) return True except NotConfigured as e: logger.error( @@ -736,6 +857,24 @@ class FeedExporter: logger.error("Unknown feed storage scheme: %(scheme)s", {"scheme": scheme}) return False + @staticmethod + def _check_storage_mode( + uri: str, storage: FeedStorageProtocol, feed_options: dict[str, Any] + ) -> None: + mode = feed_options.get("mode") + if mode is None: + # Every storage keeps its own historical mode. + return + if getattr(storage, "supported_modes", None) is None: + logger.warning( + "%(storage)s does not declare which feed modes it supports, so " + "the mode feed option of the %(uri)s feed (%(mode)r) may be " + "ignored.", + {"storage": type(storage).__name__, "uri": uri, "mode": mode}, + ) + return + _check_mode(mode, storage, uri) + def _get_storage( self, uri: str, feed_options: dict[str, Any] ) -> FeedStorageProtocol: diff --git a/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py index 993f2436d..dbd595de0 100644 --- a/scrapy/settings/default_settings.py +++ b/scrapy/settings/default_settings.py @@ -370,6 +370,7 @@ FEED_EXPORTERS_BASE = { "pickle": "scrapy.exporters.PickleItemExporter", } FEED_FORMAT = "jsonlines" +FEED_MODE = None FEED_STORE_EMPTY = True FEED_STORAGES = {} FEED_STORAGES_BASE = { diff --git a/scrapy/templates/project/module/settings.py.tmpl b/scrapy/templates/project/module/settings.py.tmpl index 0432a7231..eb78ea468 100644 --- a/scrapy/templates/project/module/settings.py.tmpl +++ b/scrapy/templates/project/module/settings.py.tmpl @@ -85,3 +85,4 @@ DOWNLOAD_DELAY = 1 # Set settings whose default value is deprecated to a future-proof value FEED_EXPORT_ENCODING = "utf-8" +FEED_MODE = "create" diff --git a/scrapy/utils/conf.py b/scrapy/utils/conf.py index 4850b370b..18fbaf7b0 100644 --- a/scrapy/utils/conf.py +++ b/scrapy/utils/conf.py @@ -3,12 +3,13 @@ from __future__ import annotations import numbers import os import sys +import warnings from configparser import ConfigParser from operator import itemgetter from pathlib import Path from typing import TYPE_CHECKING, Any, cast -from scrapy.exceptions import UsageError +from scrapy.exceptions import ScrapyDeprecationWarning, UsageError from scrapy.settings import BaseSettings from scrapy.utils.deprecate import update_classpath from scrapy.utils.python import without_none_values @@ -128,6 +129,25 @@ def feed_complete_default_values_from_settings( feed: dict[str, Any], settings: BaseSettings ) -> dict[str, Any]: out = feed.copy() + if "overwrite" in out: + warnings.warn( + "The overwrite feed option is deprecated, use the mode feed option" + " instead: mode='overwrite' instead of overwrite=True, and" + " mode='append' instead of overwrite=False.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + if out.get("mode") is not None: + raise ValueError( + "The overwrite and mode feed options are mutually exclusive," + " please set only the mode feed option." + ) + out["mode"] = "overwrite" if out.pop("overwrite") else "append" + out.setdefault("mode", settings["FEED_MODE"]) + if out["mode"] in {"append", "overwrite"}: + # Kept for feed storages that were written before the mode feed option + # existed and hence only look for the overwrite feed option. + out["overwrite"] = out["mode"] == "overwrite" out.setdefault("batch_item_count", settings.getint("FEED_EXPORT_BATCH_ITEM_COUNT")) out.setdefault("encoding", settings["FEED_EXPORT_ENCODING"]) out.setdefault("fields", settings.getdictorlist("FEED_EXPORT_FIELDS") or None) @@ -188,7 +208,9 @@ def feed_process_params_from_cli( check_valid_format(feed_format) result[feed_uri] = {"format": feed_format} if overwrite: - result[feed_uri]["overwrite"] = True + # -O unambiguously means overwriting, so it ignores FEED_MODE. -o + # does not, because appending is not supported by every storage. + result[feed_uri]["mode"] = "overwrite" # FEEDS setting should take precedence over the matching CLI options result.update(settings.getdict("FEEDS")) diff --git a/scrapy/utils/ftp.py b/scrapy/utils/ftp.py index a3e7a4306..b0d6222c7 100644 --- a/scrapy/utils/ftp.py +++ b/scrapy/utils/ftp.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import posixpath from contextlib import closing from ftplib import FTP, error_perm @@ -19,6 +21,16 @@ def ftp_makedirs_cwd(ftp: FTP, path: str, first_call: bool = True) -> None: ftp.cwd(path) +def _ftp_file_exists_in_cwd(ftp: FTP, filename: str) -> bool: + try: + ftp.voidcmd("TYPE I") + return ftp.size(filename) is not None + except error_perm: + # The file does not exist, or the server does not support the SIZE + # command, or it does not allow us to use it. + return False + + def ftp_store_file( *, path: str, @@ -29,10 +41,19 @@ def ftp_store_file( password: str, use_active_mode: bool = False, overwrite: bool = True, + mode: str | None = None, ) -> None: """Opens a FTP connection with passed credentials,sets current directory to the directory extracted from given path, then uploads the file to server + + *mode* may be ``"append"``, ``"create"`` or ``"overwrite"``. It takes + precedence over *overwrite*, which only remains for backward compatibility. + + ``"create"`` is best-effort: FTP has no atomic create-if-absent command, so + the file is checked for existence right before it is written. """ + if mode is None: + mode = "overwrite" if overwrite else "append" with FTP() as ftp, closing(file): ftp.connect(host, port) ftp.login(username, password) @@ -41,5 +62,7 @@ def ftp_store_file( file.seek(0) dirname, filename = posixpath.split(path) ftp_makedirs_cwd(ftp, dirname) - command = "STOR" if overwrite else "APPE" + if mode == "create" and _ftp_file_exists_in_cwd(ftp, filename): + raise FileExistsError(f"{path} already exists") + command = "APPE" if mode == "append" else "STOR" ftp.storbinary(f"{command} {filename}", file) diff --git a/tests/test_command_crawl.py b/tests/test_command_crawl.py index 70c26e6d0..c85f0a7f7 100644 --- a/tests/test_command_crawl.py +++ b/tests/test_command_crawl.py @@ -81,7 +81,7 @@ class MySpider(scrapy.Spider): args = ["-O", "example.json"] log = self.get_log(spider_code, proj_path, args=args) assert ( - '[myspider] DEBUG: FEEDS: {"example.json": {"format": "json", "overwrite": true}}' + '[myspider] DEBUG: FEEDS: {"example.json": {"format": "json", "mode": "overwrite"}}' in log ) with j.open(encoding="utf-8") as f2: diff --git a/tests/test_command_runspider.py b/tests/test_command_runspider.py index 2b410b5c6..d834a3a3c 100644 --- a/tests/test_command_runspider.py +++ b/tests/test_command_runspider.py @@ -263,7 +263,7 @@ class MySpider(scrapy.Spider): args = ["-O", "example.json"] log = self.get_log(tmp_path, spider_code, args=args) assert ( - '[myspider] DEBUG: FEEDS: {"example.json": {"format": "json", "overwrite": true}}' + '[myspider] DEBUG: FEEDS: {"example.json": {"format": "json", "mode": "overwrite"}}' in log ) with (tmp_path / "example.json").open(encoding="utf-8") as f2: diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index 92c414bef..7b693616e 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -10,6 +10,7 @@ from logging import getLogger from pathlib import Path from typing import IO, TYPE_CHECKING, Any from unittest import mock +from urllib.parse import urljoin import lxml.etree import pytest @@ -17,9 +18,10 @@ from w3lib.url import file_uri_to_path import scrapy from scrapy import Spider, signals -from scrapy.exceptions import NotConfigured +from scrapy.exceptions import NotConfigured, ScrapyDeprecationWarning from scrapy.exporters import CsvItemExporter, JsonItemExporter from scrapy.extensions.feedexport import ( + FEED_MODES, BlockingFeedStorage, FeedExporter, FeedSlot, @@ -28,6 +30,7 @@ from scrapy.extensions.feedexport import ( ) from scrapy.utils.python import to_unicode from scrapy.utils.test import get_crawler +from tests.mockserver.http import MockServer from tests.spiders import ItemSpider from tests.utils.bases.feedexport import TestFeedExportBase from tests.utils.decorators import coroutine_test, inline_callbacks_test @@ -36,6 +39,8 @@ from tests.utils.feedexport import MyItem, MyItem2, path_to_url, printf_escape if TYPE_CHECKING: from collections.abc import Awaitable, Callable, Iterable + from scrapy.crawler import Crawler + class FromCrawlerMixin: init_with_crawler = False @@ -77,6 +82,36 @@ class FailingBlockingFeedStorage(DummyBlockingFeedStorage): raise OSError("Cannot store") +class DelayedFileStorage(BlockingFeedStorage): + """Feed storage that, like the S3 or GCS ones, can only detect a conflict + when the feed is delivered, at the end of the crawl or of a batch.""" + + supported_modes = FEED_MODES + + def __init__(self, uri: str, *, feed_options: dict[str, Any] | None = None): + self.path = Path(uri.replace("delayed://", "/")) + self.mode = (feed_options or {}).get("mode") + + def _store_in_thread(self, file: IO[bytes]) -> None: + file.seek(0) + try: + if self.mode == "create" and self.path.exists(): + raise FileExistsError(str(self.path)) + self.path.write_bytes(file.read()) + finally: + file.close() + + +class CreateOnlyFileStorage(FileFeedStorage): + supported_modes = frozenset({"create"}) + + +class LegacyFileStorage(FileFeedStorage): + """Feed storage that predates the mode feed option.""" + + supported_modes = None # type: ignore[assignment] + + class LogOnStoreFileStorage: """ This storage logs inside `store` method. @@ -1289,6 +1324,286 @@ class TestFeedExporterSignals: assert self.feed_exporter_closed_received +class TestFeedMode: + """End-to-end tests of the mode feed option and the FEED_MODE setting.""" + + mockserver: MockServer + + @classmethod + def setup_class(cls): + cls.mockserver = MockServer() + cls.mockserver.__enter__() + + @classmethod + def teardown_class(cls): + cls.mockserver.__exit__(None, None, None) + + @pytest.fixture(autouse=True) + def _temp_dir(self, tmp_path: Path) -> None: + self.temp_dir = tmp_path + + def _path(self, content: bytes | None = None) -> Path: + path = self.temp_dir / "items.jl" + if content is not None: + path.write_bytes(content) + return path + + async def _crawl( + self, + path: Path, + mode: str | None = None, + settings: dict[str, Any] | None = None, + item_count: int = 1, + scheme: str | None = None, + ) -> Crawler: + class TestSpider(scrapy.Spider): + name = "testspider" + start_urls = [self.mockserver.url("/")] + + def parse(self, response): + for _ in range(item_count): + yield {"foo": "bar"} + + feed_options: dict[str, Any] = {"format": "jl"} + if mode is not None: + feed_options["mode"] = mode + if scheme is not None: + uri = f"{scheme}:/{path}" + elif "%(batch_id)d" in str(path): + # A batch URI template must keep its %(batch_id)d placeholder, so it + # is neither quoted nor printf-escaped. + uri = urljoin("file:", str(path)) + else: + uri = printf_escape(path_to_url(path)) + crawler = get_crawler( + TestSpider, + { + "FEEDS": {uri: feed_options}, + **(settings or {}), + }, + ) + await crawler.crawl_async() + return crawler + + @coroutine_test + async def test_create(self) -> None: + path = self._path() + await self._crawl(path, "create") + assert path.read_bytes() == b'{"foo": "bar"}\n' + + @coroutine_test + async def test_create_existing(self, caplog: pytest.LogCaptureFixture) -> None: + path = self._path(b"old content") + with caplog.at_level(logging.ERROR): + crawler = await self._crawl(path, "create") + assert path.read_bytes() == b"old content" + assert "because it already exists" in caplog.text + assert crawler.stats + stats = crawler.stats.get_stats() + assert stats.get("feedexport/conflicts/FileFeedStorage") == 1 + assert "feedexport/success_count/FileFeedStorage" not in stats + + @coroutine_test + async def test_create_existing_multiple_feeds(self) -> None: + """A conflicting target only affects its own feed: the crawl runs and + the other feeds are written.""" + existing = self._path(b"old content") + missing = self.temp_dir / "missing.jl" + crawler = await self._crawl( + existing, + settings={ + "FEEDS": { + printf_escape(path_to_url(existing)): {"format": "jl"}, + printf_escape(path_to_url(missing)): {"format": "jl"}, + }, + "FEED_MODE": "create", + }, + ) + assert existing.read_bytes() == b"old content" + assert missing.read_bytes() == b'{"foo": "bar"}\n' + assert crawler.stats + stats = crawler.stats.get_stats() + assert stats.get("feedexport/conflicts/FileFeedStorage") == 1 + assert stats.get("feedexport/success_count/FileFeedStorage") == 1 + assert stats.get("finish_reason") == "finished" + + @coroutine_test + async def test_create_existing_all_feeds(self, caplog: pytest.LogCaptureFixture): + """A crawl that cannot write any feed still runs, like any other crawl + whose feeds cannot be written.""" + existing1 = self._path(b"old content") + existing2 = self.temp_dir / "items2.jl" + existing2.write_bytes(b"old content") + with caplog.at_level(logging.ERROR): + crawler = await self._crawl( + existing1, + settings={ + "FEEDS": { + printf_escape(path_to_url(existing1)): {"format": "jl"}, + printf_escape(path_to_url(existing2)): {"format": "jl"}, + }, + "FEED_MODE": "create", + }, + ) + assert existing1.read_bytes() == b"old content" + assert existing2.read_bytes() == b"old content" + assert caplog.text.count("because it already exists") == 2 + assert crawler.stats + stats = crawler.stats.get_stats() + assert stats.get("feedexport/conflicts/FileFeedStorage") == 2 + assert stats.get("finish_reason") == "finished" + assert "feedexport/success_count/FileFeedStorage" not in stats + + @coroutine_test + async def test_overwrite_existing(self) -> None: + path = self._path(b"old content") + await self._crawl(path, "overwrite") + assert path.read_bytes() == b'{"foo": "bar"}\n' + + @coroutine_test + async def test_append_existing(self) -> None: + path = self._path(b"old content\n") + await self._crawl(path, "append") + assert path.read_bytes() == b'old content\n{"foo": "bar"}\n' + + @coroutine_test + async def test_unset_mode(self) -> None: + path = self._path() + with pytest.warns(ScrapyDeprecationWarning, match="FEED_MODE"): + await self._crawl(path) + assert path.read_bytes() == b'{"foo": "bar"}\n' + + @coroutine_test + async def test_unset_mode_existing_target(self) -> None: + path = self._path(b"old content\n") + with pytest.warns(ScrapyDeprecationWarning, match="FEED_MODE"): + await self._crawl(path) + # The legacy behavior is kept. + assert path.read_bytes() == b'old content\n{"foo": "bar"}\n' + + @coroutine_test + async def test_explicit_mode_existing_target(self, recwarn) -> None: + path = self._path(b"old content\n") + await self._crawl(path, "append") + assert not [ + warning + for warning in recwarn + if issubclass(warning.category, ScrapyDeprecationWarning) + and "FEED_MODE" in str(warning.message) + ] + + @coroutine_test + async def test_create_existing_later_batch( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """When the target of a later batch exists, that batch is skipped, and + the following ones are still written.""" + (self.temp_dir / "items-2.jl").write_bytes(b"old content") + with caplog.at_level(logging.ERROR): + crawler = await self._crawl( + self.temp_dir / "items-%(batch_id)d.jl", + "create", + {"FEED_EXPORT_BATCH_ITEM_COUNT": 1}, + item_count=3, + ) + assert (self.temp_dir / "items-1.jl").read_bytes() == b'{"foo": "bar"}\n' + assert (self.temp_dir / "items-2.jl").read_bytes() == b"old content" + # The skipped items are counted, so the following batch covers the same + # items that it would have covered otherwise. + assert (self.temp_dir / "items-3.jl").read_bytes() == b'{"foo": "bar"}\n' + assert not (self.temp_dir / "items-4.jl").exists() + # A single error, instead of one per item after the conflicting one. + assert caplog.text.count("because it already exists") == 1 + assert crawler.stats + stats = crawler.stats.get_stats() + assert stats.get("feedexport/conflicts/FileFeedStorage") == 1 + assert stats.get("feedexport/success_count/FileFeedStorage") == 2 + assert stats.get("finish_reason") == "finished" + + @coroutine_test + async def test_create_existing_later_batch_delayed( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """When the feed is only delivered at the end of each batch, the + conflict is detected then, and only that batch is lost.""" + (self.temp_dir / "items-2.jl").write_bytes(b"old content") + with caplog.at_level(logging.ERROR): + crawler = await self._crawl( + self.temp_dir / "items-%(batch_id)d.jl", + "create", + { + "FEED_EXPORT_BATCH_ITEM_COUNT": 1, + "FEED_STORAGES": { + "delayed": "tests.test_feedexport.DelayedFileStorage" + }, + }, + item_count=3, + scheme="delayed", + ) + assert (self.temp_dir / "items-1.jl").read_bytes() == b'{"foo": "bar"}\n' + assert (self.temp_dir / "items-2.jl").read_bytes() == b"old content" + assert (self.temp_dir / "items-3.jl").read_bytes() == b'{"foo": "bar"}\n' + assert caplog.text.count("because it already exists") == 1 + assert crawler.stats + stats = crawler.stats.get_stats() + assert stats.get("feedexport/conflicts/DelayedFileStorage") == 1 + assert stats.get("feedexport/failed_count/DelayedFileStorage") == 1 + assert stats.get("feedexport/success_count/DelayedFileStorage") == 2 + + @coroutine_test + async def test_feed_mode_setting(self) -> None: + path = self._path(b"old content") + await self._crawl(path, settings={"FEED_MODE": "overwrite"}) + assert path.read_bytes() == b'{"foo": "bar"}\n' + + @coroutine_test + async def test_mode_overrides_feed_mode_setting(self) -> None: + path = self._path(b"old content\n") + await self._crawl(path, "append", {"FEED_MODE": "overwrite"}) + assert path.read_bytes() == b'old content\n{"foo": "bar"}\n' + + +class TestFeedModeInit: + def test_invalid_mode(self): + """An invalid mode prevents the crawl from running.""" + settings = { + "FEEDS": {"file:///tmp/items.json": {"format": "json", "mode": "x"}} + } + with pytest.raises(ValueError, match="Invalid feed mode: 'x'"): + get_crawler(settings_dict=settings) + + def test_unsupported_mode(self): + settings = { + "FEEDS": {"file:///tmp/items.json": {"format": "json"}}, + "FEED_STORAGES": {"file": "tests.test_feedexport.CreateOnlyFileStorage"}, + "FEED_MODE": "append", + } + with pytest.raises( + ValueError, + match="CreateOnlyFileStorage does not support the 'append' feed mode", + ): + get_crawler(settings_dict=settings) + + def test_undeclared_mode_support(self, caplog: pytest.LogCaptureFixture): + settings = { + "FEEDS": {"file:///tmp/items.json": {"format": "json"}}, + "FEED_STORAGES": {"file": "tests.test_feedexport.LegacyFileStorage"}, + "FEED_MODE": "create", + } + with caplog.at_level(logging.WARNING): + get_crawler(settings_dict=settings) + assert "does not declare which feed modes it supports" in caplog.text + + def test_undeclared_mode_support_unset_mode(self, caplog: pytest.LogCaptureFixture): + settings = { + "FEEDS": {"file:///tmp/items.json": {"format": "json"}}, + "FEED_STORAGES": {"file": "tests.test_feedexport.LegacyFileStorage"}, + } + with caplog.at_level(logging.WARNING): + get_crawler(settings_dict=settings) + assert "does not declare which feed modes it supports" not in caplog.text + + class TestFeedExportInit: def test_unsupported_storage(self): settings = { diff --git a/tests/test_feedexport_storages.py b/tests/test_feedexport_storages.py index 66488540f..d085c3cf6 100644 --- a/tests/test_feedexport_storages.py +++ b/tests/test_feedexport_storages.py @@ -14,7 +14,9 @@ import pytest from w3lib.url import path_to_file_uri import scrapy +from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.extensions.feedexport import ( + FEED_MODES, BlockingFeedStorage, FileFeedStorage, FTPFeedStorage, @@ -68,11 +70,43 @@ class TestFileFeedStorage: def test_overwrite(self, tmp_path): path = tmp_path / "file.txt" - self._store(path, {"overwrite": True}) + self._store(path, {"mode": "overwrite"}) self._assert_stores( - FileFeedStorage(str(path), feed_options={"overwrite": True}), path + FileFeedStorage(str(path), feed_options={"mode": "overwrite"}), path ) + def test_create(self, tmp_path): + path = tmp_path / "file.txt" + self._assert_stores( + FileFeedStorage(str(path), feed_options={"mode": "create"}), path + ) + + def test_create_existing(self, tmp_path): + path = tmp_path / "file.txt" + path.write_bytes(b"content") + storage = FileFeedStorage(str(path), feed_options={"mode": "create"}) + with pytest.raises(FileExistsError): + storage.open(scrapy.Spider("default")) + assert path.read_bytes() == b"content" + + @pytest.mark.parametrize( + ("feed_options", "expected_write_mode"), + [ + (None, "ab"), + ({}, "ab"), + ({"mode": "create"}, "xb"), + ({"overwrite": True}, "wb"), + ({"overwrite": False}, "ab"), + ], + ) + def test_mode(self, tmp_path, feed_options, expected_write_mode): + storage = FileFeedStorage(str(tmp_path / "file.txt"), feed_options=feed_options) + assert storage.write_mode == expected_write_mode + + def test_invalid_mode(self, tmp_path): + with pytest.raises(ValueError, match="Invalid feed mode: 'x'"): + FileFeedStorage(str(tmp_path / "file.txt"), feed_options={"mode": "x"}) + @staticmethod def _assert_stores( storage: FileFeedStorage, path: Path, expected_content: bytes = b"content" @@ -113,19 +147,20 @@ class TestFTPFeedStorage: file.write(content) await maybe_deferred_to_future(storage.store(file)) - def _assert_stored(self, path: Path, content): + def _assert_stored(self, path: Path, content, unlink: bool = True): assert path.exists() try: assert path.read_bytes() == content finally: - path.unlink() + if unlink: + path.unlink() @coroutine_test async def test_append(self): with MockFTPServer() as ftp_server: filename = "file" url = ftp_server.url(filename) - feed_options = {"overwrite": False} + feed_options = {"mode": "append"} await self._store(url, b"foo", feed_options=feed_options) await self._store(url, b"bar", feed_options=feed_options) self._assert_stored(ftp_server.path / filename, b"foobar") @@ -139,13 +174,25 @@ class TestFTPFeedStorage: await self._store(url, b"bar") self._assert_stored(ftp_server.path / filename, b"bar") + @coroutine_test + async def test_create(self): + with MockFTPServer() as ftp_server: + filename = "file" + url = ftp_server.url(filename) + feed_options = {"mode": "create"} + await self._store(url, b"foo", feed_options=feed_options) + self._assert_stored(ftp_server.path / filename, b"foo", unlink=False) + with pytest.raises(FileExistsError): + await self._store(url, b"bar", feed_options=feed_options) + self._assert_stored(ftp_server.path / filename, b"foo") + @coroutine_test async def test_append_active_mode(self): with MockFTPServer() as ftp_server: settings = {"FEED_STORAGE_FTP_ACTIVE": True} filename = "file" url = ftp_server.url(filename) - feed_options = {"overwrite": False} + feed_options = {"mode": "append"} await self._store(url, b"foo", feed_options=feed_options, settings=settings) await self._store(url, b"bar", feed_options=feed_options, settings=settings) self._assert_stored(ftp_server.path / filename, b"foobar") @@ -160,6 +207,13 @@ class TestFTPFeedStorage: await self._store(url, b"bar", settings=settings) self._assert_stored(ftp_server.path / filename, b"bar") + def test_overwrite_deprecated(self): + storage = FTPFeedStorage.from_crawler(get_crawler(), "ftp://localhost/file") + with pytest.warns( + ScrapyDeprecationWarning, match="FTPFeedStorage.overwrite is deprecated" + ): + assert storage.overwrite is True + def test_uri_auth_quote(self): # RFC3986: 3.2.1. User Information pw_quoted = quote(string.punctuation, safe="") @@ -415,21 +469,59 @@ class TestS3FeedStorage: acl = storage.s3_client.upload_fileobj.call_args[1]["ExtraArgs"]["ACL"] assert acl == "custom-acl" - def test_overwrite_default(self, caplog: pytest.LogCaptureFixture) -> None: - S3FeedStorage( - "s3://mybucket/export.csv", "access_key", "secret_key", "custom-acl" - ) - assert "S3 does not support appending to files" not in caplog.text + def test_mode_append(self) -> None: + with pytest.raises( + ValueError, match="S3FeedStorage does not support the 'append' feed mode" + ): + S3FeedStorage( + "s3://mybucket/export.csv", + "access_key", + "secret_key", + "custom-acl", + feed_options={"mode": "append"}, + ) - def test_overwrite_false(self, caplog: pytest.LogCaptureFixture) -> None: - S3FeedStorage( + @coroutine_test + async def test_store_create(self) -> None: + storage = S3FeedStorage( "s3://mybucket/export.csv", "access_key", "secret_key", "custom-acl", - feed_options={"overwrite": False}, + feed_options={"mode": "create"}, ) - assert "S3 does not support appending to files" in caplog.text + storage.s3_client = mock.MagicMock() + file = BytesIO(b"test file") + stored = storage.store(file) + assert stored is not None + await maybe_deferred_to_future(stored) + storage.s3_client.upload_fileobj.assert_not_called() + assert storage.s3_client.put_object.call_args == mock.call( + Bucket="mybucket", + Key="export.csv", + Body=file, + IfNoneMatch="*", + ACL="custom-acl", + ) + + @coroutine_test + async def test_store_create_existing(self) -> None: + from botocore.exceptions import ClientError # noqa: PLC0415 + + storage = S3FeedStorage( + "s3://mybucket/export.csv", + "access_key", + "secret_key", + feed_options={"mode": "create"}, + ) + storage.s3_client = mock.MagicMock() + storage.s3_client.put_object.side_effect = ClientError( + {"Error": {"Code": "PreconditionFailed"}}, "PutObject" + ) + stored = storage.store(BytesIO(b"test file")) + assert stored is not None + with pytest.raises(FileExistsError): + await maybe_deferred_to_future(stored) class TestGCSFeedStorage: @@ -503,20 +595,56 @@ class TestGCSFeedStorage: blob_mock.upload_from_file.assert_called_once_with(f, predefined_acl=acl) f.close.assert_called_once_with() - def test_overwrite_default(self, caplog: pytest.LogCaptureFixture): - with caplog.at_level(logging.DEBUG): - GCSFeedStorage("gs://mybucket/export.csv", "myproject-123", "custom-acl") - assert "GCS does not support appending to files" not in caplog.text - - def test_overwrite_false(self, caplog: pytest.LogCaptureFixture): - with caplog.at_level(logging.DEBUG): + def test_mode_append(self): + with pytest.raises( + ValueError, match="GCSFeedStorage does not support the 'append' feed mode" + ): GCSFeedStorage( "gs://mybucket/export.csv", "myproject-123", "custom-acl", - feed_options={"overwrite": False}, + feed_options={"mode": "append"}, ) - assert "GCS does not support appending to files" in caplog.text + + @coroutine_test + async def test_store_create(self): + pytest.importorskip("google.cloud.storage") + + (client_mock, _, blob_mock) = mock_google_cloud_storage() + with mock.patch("google.cloud.storage.Client") as m: + m.return_value = client_mock + f = mock.Mock() + storage = GCSFeedStorage( + "gs://mybucket/export.csv", + "myproject-123", + "publicRead", + feed_options={"mode": "create"}, + ) + await maybe_deferred_to_future(storage.store(f)) + blob_mock.upload_from_file.assert_called_once_with( + f, predefined_acl="publicRead", if_generation_match=0 + ) + f.close.assert_called_once_with() + + @coroutine_test + async def test_store_create_existing(self): + pytest.importorskip("google.cloud.storage") + from google.api_core.exceptions import PreconditionFailed # noqa: PLC0415 + + (client_mock, _, blob_mock) = mock_google_cloud_storage() + blob_mock.upload_from_file.side_effect = PreconditionFailed("exists") + with mock.patch("google.cloud.storage.Client") as m: + m.return_value = client_mock + f = mock.Mock() + storage = GCSFeedStorage( + "gs://mybucket/export.csv", + "myproject-123", + None, + feed_options={"mode": "create"}, + ) + with pytest.raises(FileExistsError): + await maybe_deferred_to_future(storage.store(f)) + f.close.assert_called_once_with() class TestStdoutFeedStorage: @@ -528,18 +656,15 @@ class TestStdoutFeedStorage: storage.store(file) assert out.getvalue() == b"content" - def test_overwrite_default(self, caplog: pytest.LogCaptureFixture): + @pytest.mark.parametrize("mode", sorted(FEED_MODES)) + def test_mode_ignored(self, mode: str, caplog: pytest.LogCaptureFixture): + out = BytesIO() with caplog.at_level(logging.DEBUG): - StdoutFeedStorage("stdout:") - assert ( - "Standard output (stdout) storage does not support overwriting" - not in caplog.text - ) - - def test_overwrite_true(self, caplog: pytest.LogCaptureFixture): - with caplog.at_level(logging.DEBUG): - StdoutFeedStorage("stdout:", feed_options={"overwrite": True}) - assert ( - "Standard output (stdout) storage does not support overwriting" - in caplog.text - ) + storage = StdoutFeedStorage( + "stdout:", _stdout=out, feed_options={"mode": mode} + ) + file = storage.open(scrapy.Spider("default")) + file.write(b"content") + storage.store(file) + assert out.getvalue() == b"content" + assert not caplog.text diff --git a/tests/test_utils_conf.py b/tests/test_utils_conf.py index 518cc3518..c1e8bd64a 100644 --- a/tests/test_utils_conf.py +++ b/tests/test_utils_conf.py @@ -4,7 +4,7 @@ from typing import Any import pytest -from scrapy.exceptions import UsageError +from scrapy.exceptions import ScrapyDeprecationWarning, UsageError from scrapy.settings import BaseSettings, Settings from scrapy.utils.conf import ( arglist_to_dict, @@ -98,11 +98,60 @@ class TestFeedExportConfig: def test_feed_export_config_overwrite(self): settings = Settings() assert { - "output.json": {"format": "json", "overwrite": True} + "output.json": {"format": "json", "mode": "overwrite"} } == feed_process_params_from_cli( settings, [], overwrite_output=["output.json"] ) + def test_feed_complete_default_values_mode_from_settings(self): + settings = Settings({"FEED_MODE": "create"}) + new_feed = feed_complete_default_values_from_settings({}, settings) + assert new_feed["mode"] == "create" + assert "overwrite" not in new_feed + + @pytest.mark.parametrize( + ("mode", "overwrite"), + [ + ("append", False), + ("overwrite", True), + ], + ) + def test_feed_complete_default_values_mode_sets_overwrite(self, mode, overwrite): + """The deprecated overwrite feed option is kept in sync for the sake of + feed storages that predate the mode feed option.""" + settings = Settings({"FEED_MODE": mode}) + new_feed = feed_complete_default_values_from_settings({}, settings) + assert new_feed["mode"] == mode + assert new_feed["overwrite"] is overwrite + + @pytest.mark.parametrize( + ("overwrite", "mode"), + [ + (True, "overwrite"), + (False, "append"), + ], + ) + def test_feed_complete_default_values_overwrite_deprecated(self, overwrite, mode): + settings = Settings() + with pytest.warns( + ScrapyDeprecationWarning, match="overwrite feed option is deprecated" + ): + new_feed = feed_complete_default_values_from_settings( + {"overwrite": overwrite}, settings + ) + assert new_feed["mode"] == mode + assert new_feed["overwrite"] is overwrite + + def test_feed_complete_default_values_overwrite_and_mode(self): + settings = Settings() + with ( + pytest.raises(ValueError, match="mutually exclusive"), + pytest.warns(ScrapyDeprecationWarning), + ): + feed_complete_default_values_from_settings( + {"overwrite": True, "mode": "create"}, settings + ) + def test_output_and_overwrite_output(self): with pytest.raises(UsageError): feed_process_params_from_cli( @@ -123,6 +172,7 @@ class TestFeedExportConfig: ) new_feed = feed_complete_default_values_from_settings(feed, settings) assert new_feed == { + "mode": None, "encoding": "custom encoding", "fields": ["f1", "f2", "f3"], "indent": 42, @@ -148,6 +198,7 @@ class TestFeedExportConfig: ) new_feed = feed_complete_default_values_from_settings(feed, settings) assert new_feed == { + "mode": None, "encoding": "other encoding", "fields": None, "indent": 42, From 9dd3401acc3094c19b132adb5c88121e702aab13 Mon Sep 17 00:00:00 2001 From: Adrian Chaves Date: Thu, 30 Jul 2026 14:44:56 +0200 Subject: [PATCH 2/3] Fix tests on Windows --- tests/test_feedexport.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index 7b693616e..7e4bbe363 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -10,7 +10,6 @@ from logging import getLogger from pathlib import Path from typing import IO, TYPE_CHECKING, Any from unittest import mock -from urllib.parse import urljoin import lxml.etree import pytest @@ -89,7 +88,7 @@ class DelayedFileStorage(BlockingFeedStorage): supported_modes = FEED_MODES def __init__(self, uri: str, *, feed_options: dict[str, Any] | None = None): - self.path = Path(uri.replace("delayed://", "/")) + self.path = Path(uri.split("://", 1)[1]) self.mode = (feed_options or {}).get("mode") def _store_in_thread(self, file: IO[bytes]) -> None: @@ -1368,11 +1367,13 @@ class TestFeedMode: if mode is not None: feed_options["mode"] = mode if scheme is not None: - uri = f"{scheme}:/{path}" + # as_posix() keeps the URI valid on Windows, where paths have a + # drive and backslashes. + uri = f"{scheme}://{path.as_posix()}" elif "%(batch_id)d" in str(path): # A batch URI template must keep its %(batch_id)d placeholder, so it - # is neither quoted nor printf-escaped. - uri = urljoin("file:", str(path)) + # is used as a path, which is neither quoted nor printf-escaped. + uri = str(path) else: uri = printf_escape(path_to_url(path)) crawler = get_crawler( From 12e2a1156603dfe1f91b0f4cfa7bb19618b00c41 Mon Sep 17 00:00:00 2001 From: Adrian Chaves Date: Thu, 30 Jul 2026 16:15:15 +0200 Subject: [PATCH 3/3] Complete coverage --- tests/test_feedexport.py | 5 +++-- tests/test_feedexport_storages.py | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index 7e4bbe363..daa3659b9 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -1396,9 +1396,10 @@ class TestFeedMode: async def test_create_existing(self, caplog: pytest.LogCaptureFixture) -> None: path = self._path(b"old content") with caplog.at_level(logging.ERROR): - crawler = await self._crawl(path, "create") + crawler = await self._crawl(path, "create", item_count=3) assert path.read_bytes() == b"old content" - assert "because it already exists" in caplog.text + # Reported once, not once per item. + assert caplog.text.count("because it already exists") == 1 assert crawler.stats stats = crawler.stats.get_stats() assert stats.get("feedexport/conflicts/FileFeedStorage") == 1 diff --git a/tests/test_feedexport_storages.py b/tests/test_feedexport_storages.py index d085c3cf6..40f5853b5 100644 --- a/tests/test_feedexport_storages.py +++ b/tests/test_feedexport_storages.py @@ -523,6 +523,25 @@ class TestS3FeedStorage: with pytest.raises(FileExistsError): await maybe_deferred_to_future(stored) + @coroutine_test + async def test_store_create_error(self) -> None: + from botocore.exceptions import ClientError # noqa: PLC0415 + + storage = S3FeedStorage( + "s3://mybucket/export.csv", + "access_key", + "secret_key", + feed_options={"mode": "create"}, + ) + storage.s3_client = mock.MagicMock() + storage.s3_client.put_object.side_effect = ClientError( + {"Error": {"Code": "AccessDenied"}}, "PutObject" + ) + stored = storage.store(BytesIO(b"test file")) + assert stored is not None + with pytest.raises(ClientError): + await maybe_deferred_to_future(stored) + class TestGCSFeedStorage: def test_parse_settings(self):