Repository: scrapy/scrapy

Issue:
Give `BaseDupeFilter` access to spider-object
I am in a situation where a single item gets defined over a sequence of multiple pages, passing values between the particular callbacks using the `meta`-dict. I believe this is a common approach among scrapy-users.

However, it feels like this approach is difficult to get right. With the default implementation of `RFPDupefilter`, my callback-chain is teared apart quite easy, as fingerprints don't take the meta-dict into account. The corresponding requests are thrown away, the information in the meta-dict which made this request unique is lost.

I have currently implemented by own meta-aware DupeFilter, but I am still facing the problem that it lacks access to the specific spider in use - and only the Spider really knows the meta-attributes that make a request unique. I could now take it a step further and implement my own scheduler, but I'm afraid that all these custom extensions make my code very brittle wrt future versions of scrapy.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/scrapy/pull/2956?src=pr&el=h1) Report
> Merging [#2956](https://codecov.io/gh/scrapy/scrapy/pull/2956?src=pr&el=desc) into [master](https://codecov.io/gh/scrapy/scrapy/commit/f729d74886be2290fcbbcaa21d366b770ff21008?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `71.42%`.


```diff
@@            Coverage Diff             @@
##           master    #2956      +/-   ##
==========================================
- Coverage   84.59%   84.58%   -0.02%     
==========================================
  Files         164      164              
  Lines        9249     9255       +6     
  Branches     1376     1378       +2     
==========================================
+ Hits         7824     7828       +4     
- Misses       1167     1168       +1     
- Partials      258      259       +1
```

| [Impacted Files](https://codecov.io/gh/scrapy/scrapy/pull/2956?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scrapy/dupefilters.py](https://codecov.io/gh/scrapy/scrapy/pull/2956?src=pr&el=tree#diff-c2NyYXB5L2R1cGVmaWx0ZXJzLnB5) | `90.56% <100%> (+0.36%)` | :arrow_up: |
| [scrapy/core/scheduler.py](https://codecov.io/gh/scrapy/scrapy/pull/2956?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvc2NoZWR1bGVyLnB5) | `61.68% <60%> (-0.46%)` | :arrow_down: |

johtso: Any update on this?
kmike: @johtso thanks for the ping :)

@elacuesta I think this PR looks good, it needs just a small testing tweak. It'd be also good to add a test for dupefilters without from_crawler/from_settings methods. What's the reason for supporting them, by the way?
elacuesta: Hello Mikhail! I think I addressed your latest comments (setting the string to compare in the class itself, add a test for dupefilters created without from_crawler/from_settings methods). Not sure what would be the case for the directly created dupefilters, maybe someone needs to do some initialization but doesn't need the crawler nor the settings and just a child class with a custom constructor is enough?
kmike: Thanks @elacuesta! Test coverage is not complete because tests still don't check that dupefilter without from_crawler / from_settings methods work: dupefilter you're using inherits from a base class which has these methods. 
elacuesta: I didn't realize that before, thanks! I changed the test case, the new class doesn't implement any of the dupefilter methods but I think that's not the point of the test, it's just to ensure the right class is used.
elacuesta: Ping @kmike :innocent: 
kmike: This looks good 👍  

However, I'd prefer to avoid copy-paste, and use the same function to create middlewares/extensions and dupefilters. Such function can be found in https://github.com/scrapy/scrapy/pull/1605, which is almost ready to merge as well; I wonder if you're up to finishing it, and using as a base for your PR :)
elacuesta: Updated to use `scrapy.utils.misc.create_instance`. The diff shows unrelated changes but that should go away after merging #3348
dangra: hi @elacuesta, can you rebase now that #3348 is merged? thanks

Patch:
diff --git a/scrapy/core/scheduler.py b/scrapy/core/scheduler.py
index a54b4daf0..eb790a67e 100644
--- a/scrapy/core/scheduler.py
+++ b/scrapy/core/scheduler.py
@@ -4,7 +4,7 @@ import logging
 from os.path import join, exists
 
 from scrapy.utils.reqser import request_to_dict, request_from_dict
-from scrapy.utils.misc import load_object
+from scrapy.utils.misc import load_object, create_instance
 from scrapy.utils.job import job_dir
 
 logger = logging.getLogger(__name__)
@@ -26,7 +26,7 @@ class Scheduler(object):
     def from_crawler(cls, crawler):
         settings = crawler.settings
         dupefilter_cls = load_object(settings['DUPEFILTER_CLASS'])
-        dupefilter = dupefilter_cls.from_settings(settings)
+        dupefilter = create_instance(dupefilter_cls, settings, crawler)
         pqclass = load_object(settings['SCHEDULER_PRIORITY_QUEUE'])
         dqclass = load_object(settings['SCHEDULER_DISK_QUEUE'])
         mqclass = load_object(settings['SCHEDULER_MEMORY_QUEUE'])


Test patch:
diff --git a/tests/test_dupefilters.py b/tests/test_dupefilters.py
index 2d1a4bfff..db69597a2 100644
--- a/tests/test_dupefilters.py
+++ b/tests/test_dupefilters.py
@@ -5,11 +5,60 @@ import shutil
 
 from scrapy.dupefilters import RFPDupeFilter
 from scrapy.http import Request
+from scrapy.core.scheduler import Scheduler
 from scrapy.utils.python import to_bytes
+from scrapy.utils.job import job_dir
+from scrapy.utils.test import get_crawler
+
+
+class FromCrawlerRFPDupeFilter(RFPDupeFilter):
+
+    @classmethod
+    def from_crawler(cls, crawler):
+        debug = crawler.settings.getbool('DUPEFILTER_DEBUG')
+        df = cls(job_dir(crawler.settings), debug)
+        df.method = 'from_crawler'
+        return df
+
+
+class FromSettingsRFPDupeFilter(RFPDupeFilter):
+
+    @classmethod
+    def from_settings(cls, settings):
+        debug = settings.getbool('DUPEFILTER_DEBUG')
+        df = cls(job_dir(settings), debug)
+        df.method = 'from_settings'
+        return df
+
+
+class DirectDupeFilter(object):
+    method = 'n/a'
 
 
 class RFPDupeFilterTest(unittest.TestCase):
 
+    def test_df_from_crawler_scheduler(self):
+        settings = {'DUPEFILTER_DEBUG': True,
+                    'DUPEFILTER_CLASS': __name__  + '.FromCrawlerRFPDupeFilter'}
+        crawler = get_crawler(settings_dict=settings)
+        scheduler = Scheduler.from_crawler(crawler)
+        self.assertTrue(scheduler.df.debug)
+        self.assertEqual(scheduler.df.method, 'from_crawler')
+
+    def test_df_from_settings_scheduler(self):
+        settings = {'DUPEFILTER_DEBUG': True,
+                    'DUPEFILTER_CLASS': __name__  + '.FromSettingsRFPDupeFilter'}
+        crawler = get_crawler(settings_dict=settings)
+        scheduler = Scheduler.from_crawler(crawler)
+        self.assertTrue(scheduler.df.debug)
+        self.assertEqual(scheduler.df.method, 'from_settings')
+
+    def test_df_direct_scheduler(self):
+        settings = {'DUPEFILTER_CLASS': __name__  + '.DirectDupeFilter'}
+        crawler = get_crawler(settings_dict=settings)
+        scheduler = Scheduler.from_crawler(crawler)
+        self.assertEqual(scheduler.df.method, 'n/a')
+
     def test_filter(self):
         dupefilter = RFPDupeFilter()
         dupefilter.open()