Repository: scrapy/scrapy
Pull request #1498 (2015-09-16 09:56:49)

[MRG+1] #1487 add_scheme_if_missing for `scrapy shell` command
This change addresses the enhancement mentioned in #1487.

The add_scheme_if_missing function uses the parse_url function to parse the url. It checks if the scheme is missing, if it is, 'http' is added as the default scheme. It checks if the netloc field is empty. If it is empty, it replaces it with the path field and changes the path field to empty. This is done because when handing invalid url's like "www.example.com" where the scheme is missing, the parse_url function(which uses urlparse itself) leaves the netloc empty and puts the entire url in path. This leads to incorrect url generation when geturl() is called.

The add_scheme_if_missing function is called by the shell command right after it extracts the url from the arguments list. A check is performed to ensure that a null value is not passed to the function.

The test tests some common url variations.

codecov-io: ## [Current coverage][1] is `82.36%`
> Merging **#1498** into **master** will increase coverage by **+0.02%** as of [`40f351b`][3]



[1]: https://codecov.io/github/scrapy/scrapy?ref=40f351b547bd98ca6c32546de960efc025ac6c8c
[2]: https://codecov.io/github/scrapy/scrapy/features/suggestions?ref=40f351b547bd98ca6c32546de960efc025ac6c8c
[3]: https://codecov.io/github/scrapy/scrapy/commit/40f351b547bd98ca6c32546de960efc025ac6c8c
[4]: https://codecov.io/github/scrapy/scrapy/compare/5279da9916c00c7a6679cfc555f9a2b1863b4821...40f351b547bd98ca6c32546de960efc025ac6c8c

> Powered by [Codecov](https://codecov.io). Updated on successful CI builds.
nramirezuy: This new function should be called `add_http_if_no_scheme`; it just adds `http`.
Preetwinder: Thank you for reviewing my request. I should have tested more rigorously before submitting. I was not aware of the different variations of URLs that kmike mentioned. Also this is the first time I am contributing to an open source project.
While writing more extensive tests, I discovered another failure point due to urlparse incorrectly handling invalid URLs. If the scheme is not present and both the port and the path are specified, an incorrect URL is generated. This happens because urlparse uses the first occurrence of : to parse the scheme. Correcting this would require some hackey code, if we intend to stay within the feature set provided by the urlparse library.
I feel such an addition would negatively affect the readability of the code. I have been unable to find an elegant solution to the problem. It might be preferable to use a version of the code which directly alters the URL, similar to the code provided by kmike on the issue page, since the urlparse library isn't designed to handle invalid urls. Please guide me in regards to which approach is preferable.
Preetwinder: I have proceeded to make the changes mentioned by kmike and nramirezuy. Apart from adding the new tests and the docstring, I have also tested the function against multiple other URL variants separately. 
Preetwinder: Thank you kmike for your feedback and patience. Since the master branch has changed, should I merge or rebase before pushing the new commits?
kmike: Thanks @Preetwinder! Your PR is very good (and it was good from the beginning); +1 to merge it after a few minor changes.

> Since the master branch has changed, should I merge or rebase before pushing the new commits?

I think it is better to rebase.

> While writing more extensive tests, I discovered another failure point due to urlparse incorrectly handling invalid URLs. If the scheme is not present and both the port and the path are specified, an incorrect URL is generated. This happens because urlparse uses the first occurrence of : to parse the scheme. Correcting this would require some hackey code, if we intend to stay within the feature set provided by the urlparse library.

Yeah, urlparse doesn't handle invalid URLs. There is no need to stay within urlparse feature set; I'm [fine](https://github.com/scrapy/scrapy/issues/1306) even with replacing urplarse entirely.

In the end we should provide a function to "normalize" a URL like browsers normalize them when users enter a URL to the address bar. Adding schema is a step this function should be doing.



Patch:
diff --git a/scrapy/commands/shell.py b/scrapy/commands/shell.py
index 95af8586b..92ebbe605 100644
--- a/scrapy/commands/shell.py
+++ b/scrapy/commands/shell.py
@@ -9,6 +9,7 @@ from threading import Thread
 from scrapy.commands import ScrapyCommand
 from scrapy.shell import Shell
 from scrapy.http import Request
+from scrapy.utils.url import add_http_if_no_scheme
 from scrapy.utils.spider import spidercls_for_request, DefaultSpider
 
 
@@ -41,6 +42,8 @@ class Command(ScrapyCommand):
 
     def run(self, args, opts):
         url = args[0] if args else None
+        if url:
+            url = add_http_if_no_scheme(url)
         spider_loader = self.crawler_process.spider_loader
 
         spidercls = DefaultSpider
diff --git a/scrapy/utils/url.py b/scrapy/utils/url.py
index 99f350361..398407a64 100644
--- a/scrapy/utils/url.py
+++ b/scrapy/utils/url.py
@@ -110,3 +110,14 @@ def escape_ajax(url):
     if not frag.startswith('!'):
         return url
     return add_or_replace_parameter(defrag, '_escaped_fragment_', frag[1:])
+
+
+def add_http_if_no_scheme(url):
+    """Add http as the default scheme if it is missing from the url."""
+    if url.startswith('//'):
+        url = 'http:' + url
+        return url
+    parser = parse_url(url)
+    if not parser.scheme or not parser.netloc:
+        url = 'http://' + url
+    return url


Test patch:
diff --git a/tests/test_utils_url.py b/tests/test_utils_url.py
index 7bf0e5b4a..314ccd30f 100644
--- a/tests/test_utils_url.py
+++ b/tests/test_utils_url.py
@@ -4,7 +4,7 @@ import unittest
 import six
 from scrapy.spiders import Spider
 from scrapy.utils.url import (url_is_from_any_domain, url_is_from_spider,
-                              canonicalize_url)
+                              canonicalize_url, add_http_if_no_scheme)
 
 __doctests__ = ['scrapy.utils.url']
 
@@ -187,5 +187,112 @@ class CanonicalizeUrlTest(unittest.TestCase):
                          "http://foo.com/AC%2FDC/")
 
 
+class AddHttpIfNoScheme(unittest.TestCase):
+    
+    def test_add_scheme(self):
+        self.assertEqual(add_http_if_no_scheme('www.example.com'),
+                                               'http://www.example.com')
+
+    def test_without_subdomain(self):
+        self.assertEqual(add_http_if_no_scheme('example.com'),
+                                               'http://example.com')
+
+    def test_path(self):
+        self.assertEqual(add_http_if_no_scheme('www.example.com/some/page.html'),
+                                               'http://www.example.com/some/page.html')
+
+    def test_port(self):
+        self.assertEqual(add_http_if_no_scheme('www.example.com:80'),
+                                               'http://www.example.com:80')
+
+    def test_fragment(self):
+        self.assertEqual(add_http_if_no_scheme('www.example.com/some/page#frag'),
+                                               'http://www.example.com/some/page#frag')
+
+    def test_query(self):
+        self.assertEqual(add_http_if_no_scheme('www.example.com/do?a=1&b=2&c=3'),
+                                               'http://www.example.com/do?a=1&b=2&c=3')
+
+    def test_username_password(self):
+        self.assertEqual(add_http_if_no_scheme('username:password@www.example.com'),
+                                               'http://username:password@www.example.com')
+    
+    def test_complete_url(self):
+        self.assertEqual(add_http_if_no_scheme('username:password@www.example.com:80/some/page/do?a=1&b=2&c=3#frag'),
+                                               'http://username:password@www.example.com:80/some/page/do?a=1&b=2&c=3#frag')
+
+    def test_preserve_http(self):
+        self.assertEqual(add_http_if_no_scheme('http://www.example.com'),
+                                               'http://www.example.com')
+
+    def test_preserve_http_without_subdomain(self):
+        self.assertEqual(add_http_if_no_scheme('http://example.com'),
+                                               'http://example.com')
+
+    def test_preserve_http_path(self):
+        self.assertEqual(add_http_if_no_scheme('http://www.example.com/some/page.html'),
+                                               'http://www.example.com/some/page.html')
+
+    def test_preserve_http_port(self):
+        self.assertEqual(add_http_if_no_scheme('http://www.example.com:80'),
+                                               'http://www.example.com:80')
+
+    def test_preserve_http_fragment(self):
+        self.assertEqual(add_http_if_no_scheme('http://www.example.com/some/page#frag'),
+                                               'http://www.example.com/some/page#frag')
+
+    def test_preserve_http_query(self):
+        self.assertEqual(add_http_if_no_scheme('http://www.example.com/do?a=1&b=2&c=3'),
+                                               'http://www.example.com/do?a=1&b=2&c=3')
+
+    def test_preserve_http_username_password(self):
+        self.assertEqual(add_http_if_no_scheme('http://username:password@www.example.com'),
+                                               'http://username:password@www.example.com')
+
+    def test_preserve_http_complete_url(self):
+        self.assertEqual(add_http_if_no_scheme('http://username:password@www.example.com:80/some/page/do?a=1&b=2&c=3#frag'),
+                                               'http://username:password@www.example.com:80/some/page/do?a=1&b=2&c=3#frag')
+
+    def test_protocol_relative(self):
+        self.assertEqual(add_http_if_no_scheme('//www.example.com'),
+                                               'http://www.example.com')
+
+    def test_protocol_relative_without_subdomain(self):
+        self.assertEqual(add_http_if_no_scheme('//example.com'),
+                                               'http://example.com')
+
+    def test_protocol_relative_path(self):
+        self.assertEqual(add_http_if_no_scheme('//www.example.com/some/page.html'),
+                                               'http://www.example.com/some/page.html')
+
+    def test_protocol_relative_port(self):
+        self.assertEqual(add_http_if_no_scheme('//www.example.com:80'),
+                                               'http://www.example.com:80')
+
+    def test_protocol_relative_fragment(self):
+        self.assertEqual(add_http_if_no_scheme('//www.example.com/some/page#frag'),
+                                               'http://www.example.com/some/page#frag')
+
+    def test_protocol_relative_query(self):
+        self.assertEqual(add_http_if_no_scheme('//www.example.com/do?a=1&b=2&c=3'),
+                                               'http://www.example.com/do?a=1&b=2&c=3')
+
+    def test_protocol_relative_username_password(self):
+        self.assertEqual(add_http_if_no_scheme('//username:password@www.example.com'),
+                                               'http://username:password@www.example.com')
+
+    def test_protocol_relative_complete_url(self):
+        self.assertEqual(add_http_if_no_scheme('//username:password@www.example.com:80/some/page/do?a=1&b=2&c=3#frag'),
+                                               'http://username:password@www.example.com:80/some/page/do?a=1&b=2&c=3#frag')
+
+    def test_preserve_https(self):
+        self.assertEqual(add_http_if_no_scheme('https://www.example.com'),
+                                               'https://www.example.com')
+
+    def test_preserve_ftp(self):
+        self.assertEqual(add_http_if_no_scheme('ftp://www.example.com'),
+                                               'ftp://www.example.com')
+
+
 if __name__ == "__main__":
     unittest.main()