Repository: scrapy/w3lib

Issue:
Bug in NEW add_or_replace_parameter (w3lib 1.20.0)
Hi everybody,

New add_or_replace_parameter doesn't work correctly for list parameters in url e.g.:
```
>>> from w3lib.url import add_or_replace_parameter
>>> add_or_replace_parameter('http://example.com/?a=1&a=2', 'b', 1)
'http://example.com/?a=2&b=1'
```
I wait `http://example.com/?a=1&a=2&b=1` but get `http://example.com/?a=2&b=1`, also I don't have this problem with w3lib 1.19.0.



Discussion:

Patch:
diff --git a/.bumpversion.cfg b/.bumpversion.cfg
index 55eacaa..9224b24 100644
--- a/.bumpversion.cfg
+++ b/.bumpversion.cfg
@@ -1,5 +1,5 @@
 [bumpversion]
-current_version = 1.20.0
+current_version = 1.21.0
 commit = True
 tag = True
 tag_name = v{new_version}
diff --git a/NEWS b/NEWS
index 51ae0a5..8df497d 100644
--- a/NEWS
+++ b/NEWS
@@ -1,6 +1,20 @@
 w3lib release notes
 ===================
 
+1.21.0 (2019-08-09)
+-------------------
+
+- Add the ``encoding`` and ``path_encoding`` parameters to
+  :func:`w3lib.url.safe_download_url` (issue #118)
+- :func:`w3lib.url.safe_url_string` now also removes tabs and new lines
+  (issue #133)
+- :func:`w3lib.html.remove_comments` now also removes truncated comments
+  (issue #129)
+- :func:`w3lib.html.remove_tags_with_content` no longer removes tags which
+  start with the same text as one of the specified tags (issue #114)
+- Recommend pytest instead of nose to run tests (issue #124)
+
+
 1.20.0 (2019-01-11)
 -------------------
 
@@ -9,6 +23,7 @@ w3lib release notes
 - Add `w3lib.url.add_or_replace_parameters` helper (issue #117)
 - Documentation fixes (issue #115)
 
+
 1.19.0 (2018-01-25)
 -------------------
 
diff --git a/docs/conf.py b/docs/conf.py
index fb7b59b..fa4a1bc 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -53,7 +53,7 @@ copyright = u'2014, w3lib developers'
 # built documents.
 #
 # The full version, including alpha/beta/rc tags.
-release = '1.20.0'
+release = '1.21.0'
 # The short X.Y version.
 version = '.'.join(release.split('.')[:2])
 
diff --git a/docs/index.rst b/docs/index.rst
index 104ff28..4ce80ff 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -39,7 +39,7 @@ Install
 Tests
 =====
 
-`nose`_ is the preferred way to run tests. Just run: ``nosetests`` from the
+`pytest`_ is the preferred way to run tests. Just run: ``pytest`` from the
 root directory to execute tests using the default Python interpreter.
 
 `tox`_ could be used to run tests for all supported Python versions.
@@ -48,7 +48,7 @@ the root directory - tests will be executed for all available
 Python interpreters.
 
 .. _tox: http://tox.testrun.org
-.. _nose: http://readthedocs.org/docs/nose/en/latest/
+.. _pytest: https://docs.pytest.org/en/latest/
 
 
 Changelog
@@ -74,4 +74,3 @@ Indices and tables
 * :ref:`genindex`
 * :ref:`modindex`
 * :ref:`search`
-
diff --git a/setup.py b/setup.py
index 2ae088c..1f7bdda 100644
--- a/setup.py
+++ b/setup.py
@@ -3,7 +3,7 @@ from setuptools import setup, find_packages
 
 setup(
     name='w3lib',
-    version='1.20.0',
+    version='1.21.0',
     license='BSD',
     description='Library of web-related functions',
     author='Scrapy project',
diff --git a/w3lib/__init__.py b/w3lib/__init__.py
index 2d4cccd..13704bc 100644
--- a/w3lib/__init__.py
+++ b/w3lib/__init__.py
@@ -1,3 +1,3 @@
-__version__ = "1.20.0"
+__version__ = "1.21.0"
 version_info = tuple(int(v) if v.isdigit() else v
                      for v in __version__.split('.'))
diff --git a/w3lib/encoding.py b/w3lib/encoding.py
index c7ac567..6eccb27 100644
--- a/w3lib/encoding.py
+++ b/w3lib/encoding.py
@@ -109,7 +109,7 @@ DEFAULT_ENCODING_TRANSLATION = {
 }
 
 def _c18n_encoding(encoding):
-    """Cannonicalize an encoding name
+    """Canonicalize an encoding name
 
     This performs normalization and translates aliases using python's
     encoding aliases
diff --git a/w3lib/html.py b/w3lib/html.py
index 9990a35..a323b7b 100644
--- a/w3lib/html.py
+++ b/w3lib/html.py
@@ -122,7 +122,7 @@ def replace_tags(text, token='', encoding=None):
     return _tag_re.sub(token, to_unicode(text, encoding))
 
 
-_REMOVECOMMENTS_RE = re.compile(u'<!--.*?-->', re.DOTALL)
+_REMOVECOMMENTS_RE = re.compile(u'<!--.*?(?:-->|$)', re.DOTALL)
 def remove_comments(text, encoding=None):
     """ Remove HTML Comments.
 
@@ -220,7 +220,7 @@ def remove_tags_with_content(text, which_ones=(), encoding=None):
 
     text = to_unicode(text, encoding)
     if which_ones:
-        tags = '|'.join([r'<%s.*?</%s>|<%s\s*/>' % (tag, tag, tag) for tag in which_ones])
+        tags = '|'.join([r'<%s\b.*?</%s>|<%s\s*/>' % (tag, tag, tag) for tag in which_ones])
         retags = re.compile(tags, re.DOTALL | re.IGNORECASE)
         text = retags.sub(u'', text)
     return text
diff --git a/w3lib/url.py b/w3lib/url.py
index f55d057..05514df 100644
--- a/w3lib/url.py
+++ b/w3lib/url.py
@@ -9,7 +9,7 @@ import re
 import posixpath
 import warnings
 import string
-from collections import namedtuple, OrderedDict
+from collections import namedtuple
 import six
 from six.moves.urllib.parse import (urljoin, urlsplit, urlunsplit,
                                     urldefrag, urlencode, urlparse,
@@ -34,14 +34,20 @@ EXTRA_SAFE_CHARS = b'|'  # see https://github.com/scrapy/w3lib/pull/25
 
 _safe_chars = RFC3986_RESERVED + RFC3986_UNRESERVED + EXTRA_SAFE_CHARS + b'%'
 
-def safe_url_string(url, encoding='utf8', path_encoding='utf8'):
+_ascii_tab_newline_re = re.compile(r'[\t\n\r]')  # see https://infra.spec.whatwg.org/#ascii-tab-or-newline
+
+def safe_url_string(url, encoding='utf8', path_encoding='utf8', quote_path=True):
     """Convert the given URL into a legal URL by escaping unsafe characters
-    according to RFC-3986.
+    according to RFC-3986. Also, ASCII tabs and newlines are removed
+    as per https://url.spec.whatwg.org/#url-parsing.
 
     If a bytes URL is given, it is first converted to `str` using the given
-    encoding (which defaults to 'utf-8'). 'utf-8' encoding is used for
-    URL path component (unless overriden by path_encoding), and given
-    encoding is used for query string or form data.
+    encoding (which defaults to 'utf-8'). If quote_path is True (default), 
+    path_encoding ('utf-8' by default) is used to encode URL path component
+    which is then quoted. Otherwise, if quote_path is False, path component
+    is not encoded or quoted. Given encoding is used for query string 
+    or form data.
+
     When passing an encoding, you should use the encoding of the
     original page (the page from which the URL was extracted from).
 
@@ -56,8 +62,8 @@ def safe_url_string(url, encoding='utf8', path_encoding='utf8'):
     #     encoded with the supplied encoding (or UTF8 by default)
     #   - if the supplied (or default) encoding chokes,
     #     percent-encode offending bytes
-    parts = urlsplit(to_unicode(url, encoding=encoding,
-                                errors='percentencode'))
+    decoded = to_unicode(url, encoding=encoding, errors='percentencode')
+    parts = urlsplit(_ascii_tab_newline_re.sub('', decoded))
 
     # IDNA encoding can fail for too long labels (>63 characters)
     # or missing labels (e.g. http://.example.com)
@@ -66,15 +72,18 @@ def safe_url_string(url, encoding='utf8', path_encoding='utf8'):
     except UnicodeError:
         netloc = parts.netloc
 
+    # default encoding for path component SHOULD be UTF-8
+    if quote_path:
+        path = quote(to_bytes(parts.path, path_encoding), _safe_chars)
+    else:
+        path = to_native_str(parts.path)
+    
     # quote() in Python2 return type follows input type;
     # quote() in Python3 always returns Unicode (native str)
     return urlunsplit((
         to_native_str(parts.scheme),
         to_native_str(netloc).rstrip(':'),
-
-        # default encoding for path component SHOULD be UTF-8
-        quote(to_bytes(parts.path, path_encoding), _safe_chars),
-
+        path,
         # encoding of query and fragment follows page encoding
         # or form-charset (if known and passed)
         quote(to_bytes(parts.query, encoding), _safe_chars),
@@ -84,7 +93,7 @@ def safe_url_string(url, encoding='utf8', path_encoding='utf8'):
 
 _parent_dirs = re.compile(r'/?(\.\./)+')
 
-def safe_download_url(url):
+def safe_download_url(url, encoding='utf8', path_encoding='utf8'):
     """ Make a url for download. This will call safe_url_string
     and then strip the fragment, if one exists. The path will
     be normalised.
@@ -92,11 +101,11 @@ def safe_download_url(url):
     If the path is outside the document root, it will be changed
     to be within the document root.
     """
-    safe_url = safe_url_string(url)
+    safe_url = safe_url_string(url, encoding, path_encoding)
     scheme, netloc, path, query, _ = urlsplit(safe_url)
     if path:
         path = _parent_dirs.sub('', posixpath.normpath(path))
-        if url.endswith('/') and not path.endswith('/'):
+        if safe_url.endswith('/') and not path.endswith('/'):
             path += '/'
     else:
         path = '/'
@@ -199,13 +208,21 @@ def url_query_cleaner(url, parameterlist=(), sep='&', kvsep='=', remove=False, u
         url += '#' + fragment
     return url
 
-
 def _add_or_replace_parameters(url, params):
     parsed = urlsplit(url)
-    args = parse_qsl(parsed.query, keep_blank_values=True)
-
-    new_args = OrderedDict(args)
-    new_args.update(params)
+    current_args = parse_qsl(parsed.query, keep_blank_values=True)
+
+    new_args = []
+    seen_params = set()
+    for name, value in current_args:
+        if name not in params:
+            new_args.append((name, value))
+        elif name not in seen_params:
+            new_args.append((name, params[name]))
+            seen_params.add(name)
+
+    not_modified_args = [(name, value) for name, value in params.items() if name not in seen_params]
+    new_args += not_modified_args
 
     query = urlencode(new_args)
     return urlunsplit(parsed._replace(query=query))


Test patch:
diff --git a/tests/test_html.py b/tests/test_html.py
index 68133cb..97b2f56 100644
--- a/tests/test_html.py
+++ b/tests/test_html.py
@@ -106,6 +106,8 @@ class RemoveCommentsTest(unittest.TestCase):
         self.assertEqual(remove_comments(b"test <!--textcoment--> whatever"), u'test  whatever')
         self.assertEqual(remove_comments(b"test <!--\ntextcoment\n--> whatever"), u'test  whatever')
 
+        self.assertEqual(remove_comments(b"test <!--"), u'test ')
+
 
 class RemoveTagsTest(unittest.TestCase):
     def test_returns_unicode(self):
@@ -184,6 +186,10 @@ class RemoveTagsWithContentTest(unittest.TestCase):
         # text with empty tags
         self.assertEqual(remove_tags_with_content(u'<br/>a<br />', which_ones=('br',)), u'a')
 
+    def test_tags_with_shared_prefix(self):
+        # https://github.com/scrapy/w3lib/issues/114
+        self.assertEqual(remove_tags_with_content(u'<span></span>

', which_ones=('s',)), u'<span></span>')
+
 
 class ReplaceEscapeCharsTest(unittest.TestCase):
     def test_returns_unicode(self):
diff --git a/tests/test_url.py b/tests/test_url.py
index d20612c..3ff544e 100644
--- a/tests/test_url.py
+++ b/tests/test_url.py
@@ -59,9 +59,34 @@ class UrlTests(unittest.TestCase):
 
         self.assertTrue(isinstance(safe_url_string(b'http://example.com/'), str))
 
+    def test_safe_url_string_remove_ascii_tab_and_newlines(self):
+        self.assertEqual(safe_url_string("http://example.com/test\n.html"),
+                                         "http://example.com/test.html")
+        self.assertEqual(safe_url_string("http://example.com/test\t.html"),
+                                         "http://example.com/test.html")
+        self.assertEqual(safe_url_string("http://example.com/test\r.html"),
+                                         "http://example.com/test.html")
+        self.assertEqual(safe_url_string("http://example.com/test\r.html\n"),
+                                         "http://example.com/test.html")
+        self.assertEqual(safe_url_string("http://example.com/test\r\n.html\t"),
+                                         "http://example.com/test.html")
+        self.assertEqual(safe_url_string("http://example.com/test\a\n.html"),
+                                         "http://example.com/test%07.html")
+
     def test_safe_url_string_unsafe_chars(self):
         safeurl = safe_url_string(r"http://localhost:8001/unwise{,},|,\,^,[,],`?|=[]&[]=|")
         self.assertEqual(safeurl, r"http://localhost:8001/unwise%7B,%7D,|,%5C,%5E,[,],%60?|=[]&[]=|")
+        
+    def test_safe_url_string_quote_path(self):
+        safeurl = safe_url_string(u'http://google.com/"hello"', quote_path=True)
+        self.assertEqual(safeurl, u'http://google.com/%22hello%22')
+        
+        safeurl = safe_url_string(u'http://google.com/"hello"', quote_path=False)
+        self.assertEqual(safeurl, u'http://google.com/"hello"')
+        
+        safeurl = safe_url_string(u'http://google.com/"hello"')
+        self.assertEqual(safeurl, u'http://google.com/%22hello%22')
+        
 
     def test_safe_url_string_with_query(self):
         safeurl = safe_url_string(u"http://www.example.com/£?unit=µ")
@@ -203,6 +228,19 @@ class UrlTests(unittest.TestCase):
                          'http://www.example.org/image')
         self.assertEqual(safe_download_url('http://www.example.org/dir/'),
                          'http://www.example.org/dir/')
+        self.assertEqual(safe_download_url(b'http://www.example.org/dir/'),
+                         'http://www.example.org/dir/')
+
+        # Encoding related tests
+        self.assertEqual(safe_download_url(b'http://www.example.org?\xa3',
+                         encoding='latin-1', path_encoding='latin-1'),
+                         'http://www.example.org/?%A3')
+        self.assertEqual(safe_download_url(b'http://www.example.org?\xc2\xa3',
+                         encoding='utf-8', path_encoding='utf-8'),
+                         'http://www.example.org/?%C2%A3')
+        self.assertEqual(safe_download_url(b'http://www.example.org/\xc2\xa3?\xc2\xa3',
+                         encoding='utf-8', path_encoding='latin-1'),
+                         'http://www.example.org/%A3?%C2%A3')
 
     def test_is_url(self):
         self.assertTrue(is_url('http://www.example.org'))
@@ -283,6 +321,12 @@ class UrlTests(unittest.TestCase):
         self.assertEqual(add_or_replace_parameter(url, 'pageurl', 'test'),
                          'http://example.com/?version=1&pageurl=test&param2=value2')
 
+        url = 'http://domain/test?arg1=v1&arg2=v2&arg1=v3'
+        self.assertEqual(add_or_replace_parameter(url, 'arg4', 'v4'),
+                         'http://domain/test?arg1=v1&arg2=v2&arg1=v3&arg4=v4')
+        self.assertEqual(add_or_replace_parameter(url, 'arg1', 'v3'),
+                         'http://domain/test?arg1=v3&arg2=v2')
+
     def test_add_or_replace_parameters(self):
         url = 'http://domain/test'
         self.assertEqual(add_or_replace_parameters(url, {'arg': 'v'}),
@@ -292,6 +336,17 @@ class UrlTests(unittest.TestCase):
                          'http://domain/test?arg1=v1&arg2=v2&arg3=v3&arg4=v4')
         self.assertEqual(add_or_replace_parameters(url, {'arg4': 'v4', 'arg3': 'v3new'}),
                          'http://domain/test?arg1=v1&arg2=v2&arg3=v3new&arg4=v4')
+        url = 'http://domain/test?arg1=v1&arg2=v2&arg1=v3'
+        self.assertEqual(add_or_replace_parameters(url, {'arg4': 'v4'}),
+                         'http://domain/test?arg1=v1&arg2=v2&arg1=v3&arg4=v4')
+        self.assertEqual(add_or_replace_parameters(url, {'arg1': 'v3'}),
+                         'http://domain/test?arg1=v3&arg2=v2')
+
+    def test_add_or_replace_parameters_does_not_change_input_param(self):
+        url = 'http://domain/test?arg=original'
+        input_param = {'arg': 'value'}
+        new_url = add_or_replace_parameters(url, input_param)  # noqa
+        self.assertEqual(input_param, {'arg': 'value'})
 
     def test_url_query_cleaner(self):
         self.assertEqual('product.html',