Merge pull request #9920 from nsoranzo/release_20.01_fix_check_html

[20.01] Rewrite `check_html()` and `Text.count_data_lines()` to read a chunk of each line

lib/galaxy/datatypes/data.py

@@ -179,12 +179,14 @@ class Data(object):
         """Unimplemented method, allows guessing of metadata from contents of file"""
         return True
 
-    def missing_meta(self, dataset, check=[], skip=[]):
+    def missing_meta(self, dataset, check=None, skip=None):
         """
         Checks for empty metadata values, Returns True if non-optional metadata is missing
         Specifying a list of 'check' values will only check those names provided; when used, optionality is ignored
         Specifying a list of 'skip' items will return True even when a named metadata value is missing
         """
+        if skip is None:
+            skip = []
         if check:
             to_check = ((to_check, dataset.metadata.get(to_check)) for to_check in check)
         else:
@@ -346,7 +348,7 @@ class Data(object):
 
     def __archive_extra_files_path(self, extra_files_path):
         """Yield filepaths and relative filepaths for files in extra_files_path"""
-        for root, dirs, files in os.walk(extra_files_path):
+        for root, _, files in os.walk(extra_files_path):
             for fname in files:
                 fpath = os.path.join(root, fname)
                 rpath = os.path.relpath(fpath, extra_files_path)
@@ -859,13 +861,17 @@ class Text(Data):
         Count the number of lines of data in dataset,
         skipping all blank lines and comments.
         """
+        CHUNK_SIZE = 2 ** 15  # 32Kb
         data_lines = 0
         with compression_utils.get_fileobj(dataset.file_name) as in_file:
             # FIXME: Potential encoding issue can prevent the ability to iterate over lines
             # causing set_meta process to fail otherwise OK jobs. A better solution than
             # a silent try/except is desirable.
             try:
-                for line in in_file:
+                while True:
+                    line = in_file.readline(CHUNK_SIZE)
+                    if not line:
+                        break
                     line = line.strip()
                     if line and not line.startswith('#'):
                         data_lines += 1

lib/galaxy/util/__init__.py

@@ -1037,9 +1037,8 @@ def unicodify(value, encoding=DEFAULT_ENCODING, error='replace', strip_null=Fals
         # Now in Python 3, value is an instance of bytes or str
         if not isinstance(value, text_type):
             value = text_type(value, encoding, error)
-    except Exception:
-        msg = "Value '%s' could not be coerced to Unicode" % value
-        log.exception(msg)
+    except Exception as e:
+        msg = "Value '%s' could not be coerced to Unicode: %s('%s')" % (value, type(e).__name__, e)
         raise Exception(msg)
     if strip_null:
         return value.replace('\0', '')

lib/galaxy/util/checkers.py

@@ -1,10 +1,14 @@
 import gzip
+import io
 import re
 import sys
 import tarfile
 import zipfile
 
-from six import BytesIO
+from six import (
+    BytesIO,
+    StringIO
+)
 from six.moves import filter
 
 from galaxy import util
@@ -21,34 +25,35 @@ else:
     import bz2
 
 HTML_CHECK_LINES = 100
+CHUNK_SIZE = 2 ** 15  # 32Kb
+HTML_REGEXPS = (
+    re.compile(r"<A\s+[^>]*HREF[^>]+>", re.I),
+    re.compile(r"<IFRAME[^>]*>", re.I),
+    re.compile(r"<FRAMESET[^>]*>", re.I),
+    re.compile(r"<META[\W][^>]*>", re.I),
+    re.compile(r"<SCRIPT[^>]*>", re.I),
+)
 
 
-def check_html(file_path, chunk=None):
-    if chunk is None:
-        temp = open(file_path, mode='rb')
-    elif hasattr(chunk, "splitlines"):
-        temp = chunk.splitlines()
+def check_html(name, file_path=True):
+    """
+    Returns True if the file/string contains HTML code.
+    """
+    # Handles files if file_path is True or text if file_path is False
+    if file_path:
+        temp = io.open(name, "r", encoding='utf-8')
     else:
-        temp = chunk
-    regexp1 = re.compile(r"<A\s+[^>]*HREF[^>]+>", re.I)
-    regexp2 = re.compile(r"<IFRAME[^>]*>", re.I)
-    regexp3 = re.compile(r"<FRAMESET[^>]*>", re.I)
-    regexp4 = re.compile(r"<META[\W][^>]*>", re.I)
-    regexp5 = re.compile(r"<SCRIPT[^>]*>", re.I)
-    lineno = 0
-    # TODO: Potentially reading huge lines into string here, this should be
-    # reworked.
-    for line in temp:
-        line = util.unicodify(line)
-        lineno += 1
-        matches = regexp1.search(line) or regexp2.search(line) or regexp3.search(line) or regexp4.search(line) or regexp5.search(line)
-        if matches:
-            if chunk is None:
-                temp.close()
-            return True
-        if HTML_CHECK_LINES and (lineno > HTML_CHECK_LINES):
-            break
-    if chunk is None:
+        temp = StringIO(util.unicodify(name))
+    try:
+        for _ in range(HTML_CHECK_LINES):
+            line = temp.readline(CHUNK_SIZE)
+            if not line:
+                break
+            if any(regexp.search(line) for regexp in HTML_REGEXPS):
+                return True
+    except UnicodeDecodeError:
+        return False
+    finally:
         temp.close()
     return False
 
@@ -89,12 +94,10 @@ def check_gzip(file_path, check_content=True):
     if not check_content:
         return (True, True)
 
-    CHUNK_SIZE = 2 ** 15  # 32Kb
-    gzipped_file = gzip.GzipFile(file_path, mode='rb')
-    chunk = gzipped_file.read(CHUNK_SIZE)
-    gzipped_file.close()
+    with gzip.open(file_path, mode='rb') as gzipped_file:
+        chunk = gzipped_file.read(CHUNK_SIZE)
     # See if we have a compressed HTML file
-    if check_html(file_path, chunk=chunk):
+    if check_html(chunk, file_path=False):
         return (True, False)
     return (True, True)
 
@@ -111,12 +114,10 @@ def check_bz2(file_path, check_content=True):
     if not check_content:
         return (True, True)
 
-    CHUNK_SIZE = 2 ** 15  # reKb
-    bzipped_file = bz2.BZ2File(file_path, mode='rb')
-    chunk = bzipped_file.read(CHUNK_SIZE)
-    bzipped_file.close()
+    with bz2.BZ2File(file_path, mode='rb') as bzipped_file:
+        chunk = bzipped_file.read(CHUNK_SIZE)
     # See if we have a compressed HTML file
-    if check_html(file_path, chunk=chunk):
+    if check_html(chunk, file_path=False):
         return (True, False)
     return (True, True)
 
@@ -128,12 +129,11 @@ def check_zip(file_path, check_content=True, files=1):
     if not check_content:
         return (True, True)
 
-    CHUNK_SIZE = 2 ** 15  # 32Kb
     chunk = None
     for filect, member in enumerate(iter_zip(file_path)):
         handle, name = member
         chunk = handle.read(CHUNK_SIZE)
-        if chunk and check_html(file_path, chunk):
+        if chunk and check_html(chunk, file_path=False):
             return (True, False)
         if filect >= files:
             break

test/unit/util/test_checkers.py

@@ -0,0 +1,19 @@
+import tempfile
+
+from galaxy.util.checkers import check_html
+
+
+def test_check_html():
+    html_text = '<p>\n<a href="url">Link</a>\n</p>\n'
+    assert check_html(html_text, file_path=False)
+    # Test a non-HTML binary string
+    assert not check_html(b'No HTML here\nSecond line\n', file_path=False)
+    with tempfile.NamedTemporaryFile(mode='w') as tmp:
+        tmp.write(html_text)
+        tmp.flush()
+        assert check_html(tmp.name)
+    # Test a non-UTF8 binary file
+    with tempfile.NamedTemporaryFile(mode='wb') as tmp:
+        tmp.write(b'\x1f\x8b')
+        tmp.flush()
+        assert not check_html(tmp.name)