From f3b75c940d2fefc3ff5bb4435e507c5959ff903c Mon Sep 17 00:00:00 2001 From: MrMenezes Date: Fri, 21 Oct 2016 18:02:16 -0300 Subject: [PATCH 1/8] Fix warning to duplicated spider. Issue 2181 --- scrapy/spiderloader.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scrapy/spiderloader.py b/scrapy/spiderloader.py index d4f0f663f..e6c3e64a5 100644 --- a/scrapy/spiderloader.py +++ b/scrapy/spiderloader.py @@ -23,7 +23,12 @@ class SpiderLoader(object): def _load_spiders(self, module): for spcls in iter_spider_classes(module): - self._spiders[spcls.name] = spcls + if spcls.name in self._spiders.keys(): + import warnings + warnings.warn("There are several spiders with the same name (" + spcls.name + + "), this can cause unexpected behavior", UserWarning) + self._spiders[spcls.name] = spcls + def _load_all_spiders(self): for name in self.spider_modules: From 6abd9ba843e54ecb869af9571192c3f33375a2b6 Mon Sep 17 00:00:00 2001 From: Erick Date: Fri, 21 Oct 2016 18:24:59 -0300 Subject: [PATCH 2/8] Fix warning to duplicated spider. Issue 2181 --- scrapy/spiderloader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scrapy/spiderloader.py b/scrapy/spiderloader.py index e6c3e64a5..6093c07c6 100644 --- a/scrapy/spiderloader.py +++ b/scrapy/spiderloader.py @@ -27,7 +27,7 @@ class SpiderLoader(object): import warnings warnings.warn("There are several spiders with the same name (" + spcls.name + "), this can cause unexpected behavior", UserWarning) - self._spiders[spcls.name] = spcls + self._spiders[spcls.name] = spcls def _load_all_spiders(self): From 5be5ef57f38b5d6f8ed01ccf9c9cac1e1e02fe03 Mon Sep 17 00:00:00 2001 From: Paul Tremberth Date: Wed, 23 Nov 2016 11:01:31 +0100 Subject: [PATCH 3/8] Remove extra blank line --- scrapy/spiderloader.py | 1 - 1 file changed, 1 deletion(-) diff --git a/scrapy/spiderloader.py b/scrapy/spiderloader.py index 6093c07c6..1322c01d1 100644 --- a/scrapy/spiderloader.py +++ b/scrapy/spiderloader.py @@ -28,7 +28,6 @@ class SpiderLoader(object): warnings.warn("There are several spiders with the same name (" + spcls.name + "), this can cause unexpected behavior", UserWarning) self._spiders[spcls.name] = spcls - def _load_all_spiders(self): for name in self.spider_modules: From e71803c833edd67400848a3b0d4dbb01e0ec80f7 Mon Sep 17 00:00:00 2001 From: Paul Tremberth Date: Wed, 23 Nov 2016 11:52:02 +0100 Subject: [PATCH 4/8] Add tests for duplicate spider name warnings --- tests/test_spiderloader/__init__.py | 49 +++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/test_spiderloader/__init__.py b/tests/test_spiderloader/__init__.py index b2ad93b3f..ac5f0ddab 100644 --- a/tests/test_spiderloader/__init__.py +++ b/tests/test_spiderloader/__init__.py @@ -101,3 +101,52 @@ class SpiderLoaderTest(unittest.TestCase): spiders = spider_loader.list() self.assertEqual(spiders, []) + + +class DuplicateSpiderNameLoaderTest(unittest.TestCase): + + def setUp(self): + orig_spiders_dir = os.path.join(module_dir, 'test_spiders') + self.tmpdir = self.mktemp() + os.mkdir(self.tmpdir) + self.spiders_dir = os.path.join(self.tmpdir, 'test_spiders_xxx') + shutil.copytree(orig_spiders_dir, self.spiders_dir) + sys.path.append(self.tmpdir) + self.settings = Settings({'SPIDER_MODULES': ['test_spiders_xxx']}) + + def tearDown(self): + del sys.modules['test_spiders_xxx'] + sys.path.remove(self.tmpdir) + + def test_dupename_warning(self): + # copy 1 spider module so as to have duplicate spider name + shutil.copyfile(os.path.join(self.tmpdir, 'test_spiders_xxx/spider3.py'), + os.path.join(self.tmpdir, 'test_spiders_xxx/spider3dupe.py')) + + with warnings.catch_warnings(record=True) as w: + spider_loader = SpiderLoader.from_settings(self.settings) + + self.assertEqual(len(w), 1) + self.assertIn("several spiders with the same name (spider3)", str(w[0].message)) + + spiders = set(spider_loader.list()) + self.assertEqual(spiders, set(['spider1', 'spider2', 'spider3'])) + + def test_multiple_dupename_warning(self): + # copy 2 spider modules so as to have duplicate spider name + # This should issue 2 warning, 1 for each duplicate spider name + shutil.copyfile(os.path.join(self.tmpdir, 'test_spiders_xxx/spider1.py'), + os.path.join(self.tmpdir, 'test_spiders_xxx/spider1dupe.py')) + shutil.copyfile(os.path.join(self.tmpdir, 'test_spiders_xxx/spider2.py'), + os.path.join(self.tmpdir, 'test_spiders_xxx/spider2dupe.py')) + + with warnings.catch_warnings(record=True) as w: + spider_loader = SpiderLoader.from_settings(self.settings) + + self.assertEqual(len(w), 2) + msgs = sorted(str(wrn.message) for wrn in w) + self.assertIn("several spiders with the same name (spider1)", msgs[0]) + self.assertIn("several spiders with the same name (spider2)", msgs[1]) + + spiders = set(spider_loader.list()) + self.assertEqual(spiders, set(['spider1', 'spider2', 'spider3'])) From 12a8ddecab8c64923d1789571955699270255211 Mon Sep 17 00:00:00 2001 From: Paul Tremberth Date: Thu, 2 Mar 2017 13:03:18 +0100 Subject: [PATCH 5/8] Fix tests --- tests/test_spiderloader/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_spiderloader/__init__.py b/tests/test_spiderloader/__init__.py index ac5f0ddab..302bf9a10 100644 --- a/tests/test_spiderloader/__init__.py +++ b/tests/test_spiderloader/__init__.py @@ -130,7 +130,7 @@ class DuplicateSpiderNameLoaderTest(unittest.TestCase): self.assertIn("several spiders with the same name (spider3)", str(w[0].message)) spiders = set(spider_loader.list()) - self.assertEqual(spiders, set(['spider1', 'spider2', 'spider3'])) + self.assertEqual(spiders, set(['spider1', 'spider2', 'spider3', 'spider4'])) def test_multiple_dupename_warning(self): # copy 2 spider modules so as to have duplicate spider name @@ -149,4 +149,4 @@ class DuplicateSpiderNameLoaderTest(unittest.TestCase): self.assertIn("several spiders with the same name (spider2)", msgs[1]) spiders = set(spider_loader.list()) - self.assertEqual(spiders, set(['spider1', 'spider2', 'spider3'])) + self.assertEqual(spiders, set(['spider1', 'spider2', 'spider3', 'spider4'])) From a017f7b93298f8fcf5252afd8674466de95f04f9 Mon Sep 17 00:00:00 2001 From: Paul Tremberth Date: Tue, 7 Mar 2017 14:44:27 +0100 Subject: [PATCH 6/8] Warn about modules where duplicate spider names were found --- scrapy/spiderloader.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/scrapy/spiderloader.py b/scrapy/spiderloader.py index 1322c01d1..27a909c9f 100644 --- a/scrapy/spiderloader.py +++ b/scrapy/spiderloader.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- from __future__ import absolute_import +from collections import defaultdict import traceback import warnings @@ -19,14 +20,21 @@ class SpiderLoader(object): def __init__(self, settings): self.spider_modules = settings.getlist('SPIDER_MODULES') self._spiders = {} + self._found = defaultdict(list) self._load_all_spiders() def _load_spiders(self, module): for spcls in iter_spider_classes(module): + self._found[spcls.name].append((module.__name__, spcls.__name__)) if spcls.name in self._spiders.keys(): import warnings - warnings.warn("There are several spiders with the same name (" + spcls.name + - "), this can cause unexpected behavior", UserWarning) + msg = ("There are several spiders with the same name {!r}:\n" + "{}\n This can cause unexpected behavior.".format( + spcls.name, + "\n".join( + " {1} (in {0})".format(mod, cls) + for (mod, cls) in self._found[spcls.name]))) + warnings.warn(msg, UserWarning) self._spiders[spcls.name] = spcls def _load_all_spiders(self): From 978306a2236403f6275bad11e85d1e42ca37d6f1 Mon Sep 17 00:00:00 2001 From: Paul Tremberth Date: Tue, 7 Mar 2017 14:48:16 +0100 Subject: [PATCH 7/8] Fix dupe spider name warning string tests --- tests/test_spiderloader/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_spiderloader/__init__.py b/tests/test_spiderloader/__init__.py index 302bf9a10..4600e53dc 100644 --- a/tests/test_spiderloader/__init__.py +++ b/tests/test_spiderloader/__init__.py @@ -127,7 +127,7 @@ class DuplicateSpiderNameLoaderTest(unittest.TestCase): spider_loader = SpiderLoader.from_settings(self.settings) self.assertEqual(len(w), 1) - self.assertIn("several spiders with the same name (spider3)", str(w[0].message)) + self.assertIn("several spiders with the same name 'spider3'", str(w[0].message)) spiders = set(spider_loader.list()) self.assertEqual(spiders, set(['spider1', 'spider2', 'spider3', 'spider4'])) @@ -145,8 +145,8 @@ class DuplicateSpiderNameLoaderTest(unittest.TestCase): self.assertEqual(len(w), 2) msgs = sorted(str(wrn.message) for wrn in w) - self.assertIn("several spiders with the same name (spider1)", msgs[0]) - self.assertIn("several spiders with the same name (spider2)", msgs[1]) + self.assertIn("several spiders with the same name 'spider1'", msgs[0]) + self.assertIn("several spiders with the same name 'spider2'", msgs[1]) spiders = set(spider_loader.list()) self.assertEqual(spiders, set(['spider1', 'spider2', 'spider3', 'spider4'])) From 0b9a18e1a120751b8a2b2f1cc04c62b97967d612 Mon Sep 17 00:00:00 2001 From: Paul Tremberth Date: Tue, 7 Mar 2017 15:41:17 +0100 Subject: [PATCH 8/8] Warn only once for all spiders --- scrapy/spiderloader.py | 22 +++++++++++++--------- tests/test_spiderloader/__init__.py | 13 ++++++++----- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/scrapy/spiderloader.py b/scrapy/spiderloader.py index 27a909c9f..486a4637e 100644 --- a/scrapy/spiderloader.py +++ b/scrapy/spiderloader.py @@ -23,18 +23,21 @@ class SpiderLoader(object): self._found = defaultdict(list) self._load_all_spiders() + def _check_name_duplicates(self): + dupes = ["\n".join(" {cls} named {name!r} (in {module})".format( + module=mod, cls=cls, name=name) + for (mod, cls) in locations) + for name, locations in self._found.items() + if len(locations)>1] + if dupes: + msg = ("There are several spiders with the same name:\n\n" + "{}\n\n This can cause unexpected behavior.".format( + "\n\n".join(dupes))) + warnings.warn(msg, UserWarning) + def _load_spiders(self, module): for spcls in iter_spider_classes(module): self._found[spcls.name].append((module.__name__, spcls.__name__)) - if spcls.name in self._spiders.keys(): - import warnings - msg = ("There are several spiders with the same name {!r}:\n" - "{}\n This can cause unexpected behavior.".format( - spcls.name, - "\n".join( - " {1} (in {0})".format(mod, cls) - for (mod, cls) in self._found[spcls.name]))) - warnings.warn(msg, UserWarning) self._spiders[spcls.name] = spcls def _load_all_spiders(self): @@ -47,6 +50,7 @@ class SpiderLoader(object): "Check SPIDER_MODULES setting".format( modname=name, tb=traceback.format_exc())) warnings.warn(msg, RuntimeWarning) + self._check_name_duplicates() @classmethod def from_settings(cls, settings): diff --git a/tests/test_spiderloader/__init__.py b/tests/test_spiderloader/__init__.py index 4600e53dc..673a2d302 100644 --- a/tests/test_spiderloader/__init__.py +++ b/tests/test_spiderloader/__init__.py @@ -127,7 +127,9 @@ class DuplicateSpiderNameLoaderTest(unittest.TestCase): spider_loader = SpiderLoader.from_settings(self.settings) self.assertEqual(len(w), 1) - self.assertIn("several spiders with the same name 'spider3'", str(w[0].message)) + msg = str(w[0].message) + self.assertIn("several spiders with the same name", msg) + self.assertIn("'spider3'", msg) spiders = set(spider_loader.list()) self.assertEqual(spiders, set(['spider1', 'spider2', 'spider3', 'spider4'])) @@ -143,10 +145,11 @@ class DuplicateSpiderNameLoaderTest(unittest.TestCase): with warnings.catch_warnings(record=True) as w: spider_loader = SpiderLoader.from_settings(self.settings) - self.assertEqual(len(w), 2) - msgs = sorted(str(wrn.message) for wrn in w) - self.assertIn("several spiders with the same name 'spider1'", msgs[0]) - self.assertIn("several spiders with the same name 'spider2'", msgs[1]) + self.assertEqual(len(w), 1) + msg = str(w[0].message) + self.assertIn("several spiders with the same name", msg) + self.assertIn("'spider1'", msg) + self.assertIn("'spider2'", msg) spiders = set(spider_loader.list()) self.assertEqual(spiders, set(['spider1', 'spider2', 'spider3', 'spider4']))