Fix FEED_URI_PARAMS: custom params throws KeyError (#4966)

* fix FEED_URI_PARAMS: custom params throws KeyError closes #4962

* another try FEED_URI_PARAMS

* add warning message and change default function

* Add tests for FEED_URI_PARAMS

* FEED_URI_PARAMS: warn if the params dict has been modified in-place

* [Doc] FEED_URI_PARAMS: modifying params in-place is deprecated

* Remove whileline

* Rename parameters for lambda function

* Type hints for FeedExporter._get_uri_params

Co-authored-by: Adrián Chaves <adrian@chaves.io>
Co-authored-by: Eugenio Lacuesta <eugenio.lacuesta@gmail.com>

docs/topics/feed-exports.rst

@@ -322,7 +322,7 @@ Post-Processing
 
 Scrapy provides an option to activate plugins to post-process feeds before they are exported
 to feed storages. In addition to using :ref:`builtin plugins <builtin-plugins>`, you
-can create your own :ref:`plugins <custom-plugins>`. 
+can create your own :ref:`plugins <custom-plugins>`.
 
 These plugins can be activated through the ``postprocessing`` option of a feed.
 The option must be passed a list of post-processing plugins in the order you want
@@ -366,7 +366,7 @@ Each plugin is a class that must implement the following methods:
 
     Close the target file object.
 
-To pass a parameter to your plugin, use :ref:`feed options <feed-options>`. You 
+To pass a parameter to your plugin, use :ref:`feed options <feed-options>`. You
 can then access those parameters from the ``__init__`` method of your plugin.
 
 
@@ -744,6 +744,9 @@ The function signature should be as follows:
    :param spider: source spider of the feed items
    :type spider: scrapy.Spider
 
+   .. caution:: The function should return a new dictionary, modifying
+                the received ``params`` in-place is deprecated.
+
 For example, to include the :attr:`name <scrapy.Spider.name>` of the
 source spider in the feed URI:
 

scrapy/extensions/feedexport.py

@@ -11,14 +11,14 @@ import sys
 import warnings
 from datetime import datetime
 from tempfile import NamedTemporaryFile
-from typing import Any, Optional, Tuple
+from typing import Any, Callable, Optional, Tuple, Union
 from urllib.parse import unquote, urlparse
 
 from twisted.internet import defer, threads
 from w3lib.url import file_uri_to_path
 from zope.interface import implementer, Interface
 
-from scrapy import signals
+from scrapy import signals, Spider
 from scrapy.exceptions import NotConfigured, ScrapyDeprecationWarning
 from scrapy.extensions.postprocessing import PostProcessingManager
 from scrapy.utils.boto import is_botocore_available
@@ -524,7 +524,12 @@ class FeedExporter:
             raise TypeError(f"{feedcls.__qualname__}.{method_name} returned None")
         return instance
 
-    def _get_uri_params(self, spider, uri_params, slot=None):
+    def _get_uri_params(
+        self,
+        spider: Spider,
+        uri_params_function: Optional[Union[str, Callable[[dict, Spider], dict]]],
+        slot: Optional[_FeedSlot] = None,
+    ) -> dict:
         params = {}
         for k in dir(spider):
             params[k] = getattr(spider, k)
@@ -532,9 +537,18 @@ class FeedExporter:
         params['time'] = utc_now.replace(microsecond=0).isoformat().replace(':', '-')
         params['batch_time'] = utc_now.isoformat().replace(':', '-')
         params['batch_id'] = slot.batch_id + 1 if slot is not None else 1
-        uripar_function = load_object(uri_params) if uri_params else lambda x, y: None
-        uripar_function(params, spider)
-        return params
+        original_params = params.copy()
+        uripar_function = load_object(uri_params_function) if uri_params_function else lambda params, _: params
+        new_params = uripar_function(params, spider)
+        if new_params is None or original_params != params:
+            warnings.warn(
+                'Modifying the params dictionary in-place in the function defined in '
+                'the FEED_URI_PARAMS setting or in the uri_params key of the FEEDS '
+                'setting is deprecated. The function must return a new dictionary '
+                'instead.',
+                category=ScrapyDeprecationWarning
+            )
+        return new_params if new_params is not None else params
 
     def _load_filter(self, feed_options):
         # load the item filter if declared else load the default filter class

tests/test_feedexport.py

@@ -2608,3 +2608,166 @@ class FTPFeedStoragePreFeedOptionsTest(unittest.TestCase):
                     ),
                 )
             )
+
+
+class URIParamsTest:
+
+    spider_name = "uri_params_spider"
+
+    def build_settings(self, uri='file:///tmp/foobar', uri_params=None):
+        raise NotImplementedError
+
+    def test_default(self):
+        settings = self.build_settings(
+            uri='file:///tmp/%(name)s',
+        )
+        crawler = get_crawler(settings_dict=settings)
+        feed_exporter = FeedExporter.from_crawler(crawler)
+        spider = scrapy.Spider(self.spider_name)
+        spider.crawler = crawler
+        with warnings.catch_warnings(record=True) as w:
+            feed_exporter.open_spider(spider)
+            messages = tuple(
+                str(item.message) for item in w
+                if item.category is ScrapyDeprecationWarning
+            )
+            self.assertEqual(messages, tuple())
+
+        self.assertEqual(
+            feed_exporter.slots[0].uri,
+            f'file:///tmp/{self.spider_name}'
+        )
+
+    def test_none(self):
+        def uri_params(params, spider):
+            pass
+
+        settings = self.build_settings(
+            uri='file:///tmp/%(name)s',
+            uri_params=uri_params,
+        )
+        crawler = get_crawler(settings_dict=settings)
+        feed_exporter = FeedExporter.from_crawler(crawler)
+        spider = scrapy.Spider(self.spider_name)
+        spider.crawler = crawler
+        with warnings.catch_warnings(record=True) as w:
+            feed_exporter.open_spider(spider)
+            messages = tuple(
+                str(item.message) for item in w
+                if item.category is ScrapyDeprecationWarning
+            )
+            self.assertEqual(
+                messages,
+                (
+                    (
+                        'Modifying the params dictionary in-place in the '
+                        'function defined in the FEED_URI_PARAMS setting or '
+                        'in the uri_params key of the FEEDS setting is '
+                        'deprecated. The function must return a new '
+                        'dictionary instead.'
+                    ),
+                )
+            )
+
+        self.assertEqual(
+            feed_exporter.slots[0].uri,
+            f'file:///tmp/{self.spider_name}'
+        )
+
+    def test_empty_dict(self):
+        def uri_params(params, spider):
+            return {}
+
+        settings = self.build_settings(
+            uri='file:///tmp/%(name)s',
+            uri_params=uri_params,
+        )
+        crawler = get_crawler(settings_dict=settings)
+        feed_exporter = FeedExporter.from_crawler(crawler)
+        spider = scrapy.Spider(self.spider_name)
+        spider.crawler = crawler
+        with warnings.catch_warnings(record=True) as w:
+            with self.assertRaises(KeyError):
+                feed_exporter.open_spider(spider)
+            messages = tuple(
+                str(item.message) for item in w
+                if item.category is ScrapyDeprecationWarning
+            )
+            self.assertEqual(messages, tuple())
+
+    def test_params_as_is(self):
+        def uri_params(params, spider):
+            return params
+
+        settings = self.build_settings(
+            uri='file:///tmp/%(name)s',
+            uri_params=uri_params,
+        )
+        crawler = get_crawler(settings_dict=settings)
+        feed_exporter = FeedExporter.from_crawler(crawler)
+        spider = scrapy.Spider(self.spider_name)
+        spider.crawler = crawler
+        with warnings.catch_warnings(record=True) as w:
+            feed_exporter.open_spider(spider)
+            messages = tuple(
+                str(item.message) for item in w
+                if item.category is ScrapyDeprecationWarning
+            )
+            self.assertEqual(messages, tuple())
+
+        self.assertEqual(
+            feed_exporter.slots[0].uri,
+            f'file:///tmp/{self.spider_name}'
+        )
+
+    def test_custom_param(self):
+        def uri_params(params, spider):
+            return {**params, 'foo': self.spider_name}
+
+        settings = self.build_settings(
+            uri='file:///tmp/%(foo)s',
+            uri_params=uri_params,
+        )
+        crawler = get_crawler(settings_dict=settings)
+        feed_exporter = FeedExporter.from_crawler(crawler)
+        spider = scrapy.Spider(self.spider_name)
+        spider.crawler = crawler
+        with warnings.catch_warnings(record=True) as w:
+            feed_exporter.open_spider(spider)
+            messages = tuple(
+                str(item.message) for item in w
+                if item.category is ScrapyDeprecationWarning
+            )
+            self.assertEqual(messages, tuple())
+
+        self.assertEqual(
+            feed_exporter.slots[0].uri,
+            f'file:///tmp/{self.spider_name}'
+        )
+
+
+class URIParamsSettingTest(URIParamsTest, unittest.TestCase):
+
+    def build_settings(self, uri='file:///tmp/foobar', uri_params=None):
+        extra_settings = {}
+        if uri_params:
+            extra_settings['FEED_URI_PARAMS'] = uri_params
+        return {
+            'FEED_URI': uri,
+            **extra_settings,
+        }
+
+
+class URIParamsFeedOptionTest(URIParamsTest, unittest.TestCase):
+
+    def build_settings(self, uri='file:///tmp/foobar', uri_params=None):
+        options = {
+            'format': 'jl',
+        }
+        if uri_params:
+            options['uri_params'] = uri_params
+        return {
+            'FEEDS': {
+                uri: options,
+            },
+        }