Repository: scrapy/scrapy (python)

Issue:
Backward incompatibility for relocated paths in settings
Reported by @dangra

This issue manifests when mixing old paths and new ones for extensions and middlewares (this can happen for example while using a newer version of Scrapy in a project that hasn't updated to the new paths yet). Since paths aren't normalized, the same component can be loaded twice.

Take these settings for example:

```python
# scrapy/settings/default_settings.py
EXTENSIONS_BASE = {
    'scrapy.extensions.debug.StackTraceDump': 100,  # new path
} 
```
```python
# myproject/settings.py
EXTENSIONS = {
    'scrapy.contrib.debug.StackTraceDump': 200,  # old path
}
```

While merging both dictionaries to build the list of components, the same StackTraceDump class is going to be loaded twice since it appears in two different keys. 

Pull request description:
Fix #1265
Attempt to solve #1265 by normalizing the paths in `build_component_list`, a helper used to merge component settings with their base settings. This solution is not bulletproof regarding priority sorting, but I think it's as close as we can get it to be. 

Example of a use-case that is still broken:
```python
>>> EXTENSIONS =  {'new_path.Obj': 10,'another_path.Another': 20, 'old_path.Obj': 30}
>>> build_components({}, EXTENSIONS)
>>> ['another_path.Another', 'new_path.Obj']
```

Current implementation will load each object once, but it will give preference to the old path ordering. That isn't always what we want, the problem here is that we can't know which path was updated/added to the dictionary first (Another reason why #1149 is going to be really helpful, though those pairings could still have the same priority). For a concrete example, Scrapy Cloud uses addons with old paths, users won't be able to update them if they use the new paths since the old ones take precedence.

I don't see how we can fix that in Scrapy side, I just hope those are really rare cases (mixing old paths and new ones in the same dictionary should be uncommon) so we can move forward with the fix.

Patch:
diff --git a/scrapy/utils/conf.py b/scrapy/utils/conf.py
index b883923b9..423ca43e2 100644
--- a/scrapy/utils/conf.py
+++ b/scrapy/utils/conf.py
@@ -5,16 +5,30 @@ from operator import itemgetter
 import six
 from six.moves.configparser import SafeConfigParser
 
+from scrapy.utils.deprecate import update_classpath
 
-def build_component_list(base, custom):
+
+def build_component_list(base, custom, convert=update_classpath):
     """Compose a component list based on a custom and base dict of components
     (typically middlewares or extensions), unless custom is already a list, in
     which case it's returned.
     """
+
+    def _check_components(complist):
+        if len({convert(c) for c in complist}) != len(complist):
+            raise ValueError('Some paths in {!r} convert to the same object, '
+                             'please update your settings'.format(complist))
+
     if isinstance(custom, (list, tuple)):
-        return custom
-    compdict = base.copy()
-    compdict.update(custom)
+        _check_components(custom)
+        return type(custom)(convert(c) for c in custom)
+
+    def _map_keys(compdict):
+        _check_components(compdict)
+        return {convert(k): v for k, v in six.iteritems(compdict)}
+
+    compdict = _map_keys(base)
+    compdict.update(_map_keys(custom))
     items = (x for x in six.iteritems(compdict) if x[1] is not None)
     return [x[0] for x in sorted(items, key=itemgetter(1))]
 
diff --git a/scrapy/utils/deprecate.py b/scrapy/utils/deprecate.py
index 6f1940611..37e94fae9 100644
--- a/scrapy/utils/deprecate.py
+++ b/scrapy/utils/deprecate.py
@@ -121,3 +121,37 @@ def _clspath(cls, forced=None):
     if forced is not None:
         return forced
     return '{}.{}'.format(cls.__module__, cls.__name__)
+
+
+DEPRECATION_RULES = [
+    ('scrapy.contrib_exp.downloadermiddleware.decompression.', 'scrapy.downloadermiddlewares.decompression.'),
+    ('scrapy.contrib_exp.iterators.', 'scrapy.utils.iterators.'),
+    ('scrapy.contrib.downloadermiddleware.', 'scrapy.downloadermiddlewares.'),
+    ('scrapy.contrib.exporter.', 'scrapy.exporters.'),
+    ('scrapy.contrib.linkextractors.', 'scrapy.linkextractors.'),
+    ('scrapy.contrib.loader.processor.', 'scrapy.loader.processors.'),
+    ('scrapy.contrib.loader.', 'scrapy.loader.'),
+    ('scrapy.contrib.pipeline.', 'scrapy.pipelines.'),
+    ('scrapy.contrib.spidermiddleware.', 'scrapy.spidermiddlewares.'),
+    ('scrapy.contrib.spiders.', 'scrapy.spiders.'),
+    ('scrapy.contrib.', 'scrapy.extensions.'),
+    ('scrapy.command.', 'scrapy.commands.'),
+    ('scrapy.dupefilter.', 'scrapy.dupefilters.'),
+    ('scrapy.linkextractor.', 'scrapy.linkextractors.'),
+    ('scrapy.spider.', 'scrapy.spiders.'),
+    ('scrapy.squeue.', 'scrapy.squeues.'),
+    ('scrapy.statscol.', 'scrapy.statscollectors.'),
+    ('scrapy.utils.decorator.', 'scrapy.utils.decorators.'),
+    ('scrapy.spidermanager.SpiderManager', 'scrapy.spiderloader.SpiderLoader'),
+]
+
+
+def update_classpath(path):
+    """Update a deprecated path from an object with its new location"""
+    for prefix, replacement in DEPRECATION_RULES:
+        if path.startswith(prefix):
+            new_path = path.replace(prefix, replacement, 1)
+            warnings.warn("`{}` class is deprecated, use `{}` instead".format(path, new_path),
+                          ScrapyDeprecationWarning)
+            return new_path
+    return path


Test patch:
diff --git a/tests/test_utils_conf.py b/tests/test_utils_conf.py
index 35277a423..177d024fb 100644
--- a/tests/test_utils_conf.py
+++ b/tests/test_utils_conf.py
@@ -2,16 +2,41 @@ import unittest
 
 from scrapy.utils.conf import build_component_list, arglist_to_dict
 
-class UtilsConfTestCase(unittest.TestCase):
 
-    def test_build_component_list(self):
+class BuildComponentListTest(unittest.TestCase):
+
+    def test_build_dict(self):
         base = {'one': 1, 'two': 2, 'three': 3, 'five': 5, 'six': None}
         custom = {'two': None, 'three': 8, 'four': 4}
-        self.assertEqual(build_component_list(base, custom),
+        self.assertEqual(build_component_list(base, custom, lambda x: x),
                          ['one', 'four', 'five', 'three'])
 
+    def test_return_list(self):
+        custom = ['a', 'b', 'c']
+        self.assertEqual(build_component_list(None, custom, lambda x: x),
+                         custom)
+
+    def test_map_dict(self):
+        custom = {'one': 1, 'two': 2, 'three': 3}
+        self.assertEqual(build_component_list({}, custom, lambda x: x.upper()),
+                         ['ONE', 'TWO', 'THREE'])
+
+    def test_map_list(self):
         custom = ['a', 'b', 'c']
-        self.assertEqual(build_component_list(base, custom), custom)
+        self.assertEqual(build_component_list(None, custom, lambda x: x.upper()),
+                         ['A', 'B', 'C'])
+
+    def test_duplicate_components_in_dict(self):
+        duplicate_dict = {'one': 1, 'two': 2, 'ONE': 4}
+        self.assertRaises(ValueError,
+                          build_component_list, {}, duplicate_dict, lambda x: x.lower())
+
+    def test_duplicate_components_in_list(self):
+        duplicate_list = ['a', 'b', 'a']
+        self.assertRaises(ValueError,
+                          build_component_list, None, duplicate_list, lambda x: x)
+
+class UtilsConfTestCase(unittest.TestCase):
 
     def test_arglist_to_dict(self):
         self.assertEqual(arglist_to_dict(['arg1=val1', 'arg2=val2']),
diff --git a/tests/test_utils_deprecate.py b/tests/test_utils_deprecate.py
index 3d94bc1a0..41b8100d7 100644
--- a/tests/test_utils_deprecate.py
+++ b/tests/test_utils_deprecate.py
@@ -3,7 +3,7 @@ from __future__ import absolute_import
 import inspect
 import unittest
 import warnings
-from scrapy.utils.deprecate import create_deprecated_class
+from scrapy.utils.deprecate import create_deprecated_class, update_classpath
 
 from tests import mock
 
@@ -248,3 +248,28 @@ class WarnWhenSubclassedTest(unittest.TestCase):
                     pass
 
         self.assertIn("Error detecting parent module", str(w[0].message))
+
+
+@mock.patch('scrapy.utils.deprecate.DEPRECATION_RULES',
+            [('scrapy.contrib.pipeline.', 'scrapy.pipelines.'),
+             ('scrapy.contrib.', 'scrapy.extensions.')])
+class UpdateClassPathTest(unittest.TestCase):
+
+    def test_old_path_gets_fixed(self):
+        with warnings.catch_warnings(record=True) as w:
+            output = update_classpath('scrapy.contrib.debug.Debug')
+        self.assertEqual(output, 'scrapy.extensions.debug.Debug')
+        self.assertEqual(len(w), 1)
+        self.assertIn("scrapy.contrib.debug.Debug", str(w[0].message))
+        self.assertIn("scrapy.extensions.debug.Debug", str(w[0].message))
+
+    def test_sorted_replacement(self):
+        with warnings.catch_warnings(record=True):
+            output = update_classpath('scrapy.contrib.pipeline.Pipeline')
+        self.assertEqual(output, 'scrapy.pipelines.Pipeline')
+
+    def test_unmatched_path_stays_the_same(self):
+        with warnings.catch_warnings(record=True) as w:
+            output = update_classpath('scrapy.unmatched.Path')
+        self.assertEqual(output, 'scrapy.unmatched.Path')
+        self.assertEqual(len(w), 0)