Allow not to dedupe urls with different fragments

[boogheta]

w3lib's canonicalize_url has an option to keep fragments but it is not accessible from scrapy's request_fingerprint method, so it's impossible without changing scrapy's code to make a custom dedupe filter which would keep urls identical except for fragments.

We need this in our webcrawler Hyphe for our future headless crawling feature using chromedriver (currently developed in medialab/hyphe#288 ) to crawl modern websites with routing included in the fragment.

For what I understand the solution discussed in #4067 using dont_filter=False on each query would completely disable deduping for the spider which is not an option for a crawler that adds new links from each crawled page as it would then run indefinitely in loop

cc @arnaudmolo

[elacuesta]

The feature makes sense and the implementation looks good IMHO. Current test failure is caused by #4014 so it's fine. Could you add a test to https://github.com/scrapy/scrapy/blob/1.7.4/tests/test_utils_request.py?

[codecov]

Codecov Report

Merging #4104 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4104      +/-   ##
==========================================
+ Coverage   85.68%   85.69%   +<.01%     
==========================================
  Files         165      165              
  Lines        9734     9735       +1     
  Branches     1463     1463              
==========================================
+ Hits         8341     8342       +1     
  Misses       1136     1136              
  Partials      257      257

Impacted Files	Coverage Δ
scrapy/utils/request.py	100% <100%> (ø)	⬆️
scrapy/loader/__init__.py	95.36% <0%> (ø)	⬆️

[boogheta]

Hi @elacuesta and thanks for the quick feedback and the test addition recommendation: it made me realise my naive approach was not taking account of the fingerprint cache, I repushed with a slight modification to do so, with according tests

[elacuesta]

Great, thanks for the follow-up 👍

[Gallaecio]

it's impossible without changing scrapy's code to make a custom dedupe filter which would keep urls identical except for fragments.

Isn’t this true even after these changes? Don’t you still need to create your own duplicate filter that uses request_fingerprint with keep_fragments=True and assign it to DUPEFILTER_CLASS?

I’m hesitant about this because it may complicate further the solution to #900.

[boogheta]

Hi @Gallaecio, we've ran tests on our codebase with the above change and it works with it in our case.

It is true we need anyway to create our own duplicate filter that uses request_fingerprint with keep_fragments=True, the problem the PR solves it that doing so was not possible otherwise, since the keep_fragments argument could not be passed to canonicalize_url throuh request_fingerprint.

I didn't see the discussion about #900 before and I understand indeed that this change should be integrated within it when it is implemented but I don't think it would too much of a hassle to do, and I'll be happy to participate then ! :)

[Gallaecio]

Could you update the docstring of the function? There’s a mention to the include_headers parameter towards the end, it would be good for it to also mention the new parameter in a similar way.

[boogheta]

Ha yes indeed, sorry I forgot, will do right away!

[boogheta]

Done!

[boogheta]

Thank you!
May I ask if we can expect it within a release soon?

[Gallaecio]

Well, that depends on your definition of soon 🙂. The current plans are for the next release to be 2.0, sometime in 2020 Q1.

[boogheta]

Ah that's far indeed ;)
And I must say a bit frightening considering that according to semantic versioning, 2.0 usually means breaking changes by then.
I saw you released a 1.8.0 just before merging, wouldn't it make sense to see a few other minor releases with bugfixes and slight improvements such as a 1.8.1 by then?

[Gallaecio]

In patch versions we usually include only important bugfixes, so I’m sorry to say that, even if there is a 1.8.1 version, it will probably not include this change 🙁

[boogheta]

I don't understand, isn't it the whole point of making minor releases to include small changes along with bugfixes? What would be the problem to include such changes that only add functionality without breaking anything? It would feel very frustrating to contribute only to see changes completely overwritten and broken by the time they finally end up in a release :-(

[Gallaecio]

As I said, we tend to stick to important bugfixes, to keep patch releases small and simple. But I guess we can evaluate including this change in 1.8.1 if we decide to have a 1.8.1 release. I’ll create a milestone so we do not forget.

[boogheta]

Thanks for taking it into consideration!

[elacuesta]

@boogheta In the meantime, is it possible for you to install using the commit hash, i.e. pip install git+https://github.com/scrapy/scrapy.git@6d6da78eda3cc0bba1bfdf70194fdf655fac8aeb?

[boogheta]

Yes, this is probably what we will end up doing in our repo by then, although this is not a very good practice and I know some colleagues will shame us about doing so ;)