Repository: python-babel/babel
Pull request #532 (2017-09-26 17:32:18)

pofile.py: Added new exception called PoFileError and thrown if flagged
This new exception is thrown when the po parser finds an invalid pofile.
This helps handle invalid po files that are parsed. Invalid po files may cause
other possible errors such as a UnicodeEncodeError.

Closes https://github.com/python-babel/babel/issues/531

codecov-io: # [Codecov](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=h1) Report
> Merging [#532](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=desc) into [master](https://codecov.io/gh/python-babel/babel/commit/f29eccd6b6edd031e43ccd1bcf92513221b66df6?src=pr&el=desc) will **decrease** coverage by `1.15%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/python-babel/babel/pull/532/graphs/tree.svg?height=150&width=650&src=pr&token=lEYaWCjCnu)](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
- Coverage      90%   88.85%   -1.16%     
==========================================
  Files          24       24              
  Lines        4024     4029       +5     
==========================================
- Hits         3622     3580      -42     
- Misses        402      449      +47
```


| [Impacted Files](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [babel/messages/pofile.py](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree#diff-YmFiZWwvbWVzc2FnZXMvcG9maWxlLnB5) | `96.45% <100%> (-0.3%)` | :arrow_down: |
| [babel/\_compat.py](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree#diff-YmFiZWwvX2NvbXBhdC5weQ==) | `53.48% <0%> (-46.52%)` | :arrow_down: |
| [babel/localtime/\_unix.py](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree#diff-YmFiZWwvbG9jYWx0aW1lL191bml4LnB5) | `11.53% <0%> (-10.26%)` | :arrow_down: |
| [babel/localtime/\_win32.py](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree#diff-YmFiZWwvbG9jYWx0aW1lL193aW4zMi5weQ==) | `59.25% <0%> (-3.71%)` | :arrow_down: |
| [babel/localtime/\_\_init\_\_.py](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree#diff-YmFiZWwvbG9jYWx0aW1lL19faW5pdF9fLnB5) | `62.5% <0%> (-2.5%)` | :arrow_down: |
| [babel/messages/catalog.py](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree#diff-YmFiZWwvbWVzc2FnZXMvY2F0YWxvZy5weQ==) | `92.23% <0%> (-2.39%)` | :arrow_down: |
| [babel/util.py](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree#diff-YmFiZWwvdXRpbC5weQ==) | `90.78% <0%> (-1.32%)` | :arrow_down: |
| [babel/messages/extract.py](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree#diff-YmFiZWwvbWVzc2FnZXMvZXh0cmFjdC5weQ==) | `94.18% <0%> (-1.1%)` | :arrow_down: |
| [babel/localedata.py](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree#diff-YmFiZWwvbG9jYWxlZGF0YS5weQ==) | `94.82% <0%> (-0.87%)` | :arrow_down: |
| [babel/support.py](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree#diff-YmFiZWwvc3VwcG9ydC5weQ==) | `82.28% <0%> (-0.4%)` | :arrow_down: |
| ... and [1 more](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=footer). Last update [f29eccd...4ac2700](https://codecov.io/gh/python-babel/babel/pull/532?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Bedrock02: @akx thanks for the quick feedback. I applied the suggested changes in a separate commit. After changes come to a final state I could squash to one commit.
akx: @Bedrock02 This looks good now :) Mind rebasing on top of current master and squashing the two commits while you're at it?
Bedrock02: @akx branch has been rebased on top of the current master and I squashed the two commits. Let me know if there is anything else that I should do.
akx: @Bedrock02 Can you rebase once more on top of the current master so we get rid of the CI failure (as since #546 was just merged we don't support Python 2.6 anymore)? Thank you!

Patch:
diff --git a/babel/messages/pofile.py b/babel/messages/pofile.py
index 696ec3e..beb6f35 100644
--- a/babel/messages/pofile.py
+++ b/babel/messages/pofile.py
@@ -19,6 +19,7 @@ from babel.util import wraptext
 from babel._compat import text_type
 
 
+
 def unescape(string):
     r"""Reverse `escape` the given string.
 
@@ -73,6 +74,15 @@ def denormalize(string):
         return unescape(string)
 
 
+class PoFileError(Exception):
+    """Exception thrown by PoParser when an invalid po file is encountered."""
+    def __init__(self, message, catalog, line, lineno):
+        super(PoFileError, self).__init__('{message} on {lineno}'.format(message=message, lineno=lineno))
+        self.catalog = catalog
+        self.line = line
+        self.lineno = lineno
+
+
 class _NormalizedString(object):
 
     def __init__(self, *args):
@@ -104,11 +114,12 @@ class PoFileParser(object):
         'msgid_plural',
     ]
 
-    def __init__(self, catalog, ignore_obsolete=False):
+    def __init__(self, catalog, ignore_obsolete=False, abort_invalid=False):
         self.catalog = catalog
         self.ignore_obsolete = ignore_obsolete
         self.counter = 0
         self.offset = 0
+        self.abort_invalid = abort_invalid
         self._reset_message_state()
 
     def _reset_message_state(self):
@@ -276,11 +287,13 @@ class PoFileParser(object):
             self._add_message()
 
     def _invalid_pofile(self, line, lineno, msg):
+        if self.abort_invalid:
+            raise PoFileError(msg, self.catalog, line, lineno)
         print("WARNING:", msg)
         print("WARNING: Problem on line {0}: {1}".format(lineno + 1, line))
 
 
-def read_po(fileobj, locale=None, domain=None, ignore_obsolete=False, charset=None):
+def read_po(fileobj, locale=None, domain=None, ignore_obsolete=False, charset=None, abort_invalid=False):
     """Read messages from a ``gettext`` PO (portable object) file from the given
     file-like object and return a `Catalog`.
 
@@ -325,9 +338,10 @@ def read_po(fileobj, locale=None, domain=None, ignore_obsolete=False, charset=No
     :param domain: the message domain
     :param ignore_obsolete: whether to ignore obsolete messages in the input
     :param charset: the character set of the catalog.
+    :param abort_invalid: abort read if po file is invalid
     """
     catalog = Catalog(locale=locale, domain=domain, charset=charset)
-    parser = PoFileParser(catalog, ignore_obsolete)
+    parser = PoFileParser(catalog, ignore_obsolete, abort_invalid=abort_invalid)
     parser.parse(fileobj)
     return catalog
 


Test patch:
diff --git a/tests/messages/test_pofile.py b/tests/messages/test_pofile.py
index f6cd66d..002954f 100644
--- a/tests/messages/test_pofile.py
+++ b/tests/messages/test_pofile.py
@@ -13,6 +13,7 @@
 
 from datetime import datetime
 import unittest
+import sys
 
 from babel.core import Locale
 from babel.messages.catalog import Catalog, Message
@@ -20,7 +21,6 @@ from babel.messages import pofile
 from babel.util import FixedOffsetTimezone
 from babel._compat import StringIO, BytesIO
 
-
 class ReadPoTestCase(unittest.TestCase):
 
     def test_preserve_locale(self):
@@ -429,6 +429,70 @@ msgstr[2] "Vohs [text]"
         self.assertEqual("", message.string[1])
         self.assertEqual("Vohs [text]", message.string[2])
 
+    def test_abort_invalid_po_file(self):
+        invalid_po = '''
+            msgctxt ""
+            "{\"checksum\": 2148532640, \"cxt\": \"collector_thankyou\", \"id\": "
+            "270005359}"
+            msgid ""
+            "Thank you very much for your time.\n"
+            "If you have any questions regarding this survey, please contact Fulano "
+            "at nadie@blah.com"
+            msgstr "Merci de prendre le temps de remplir le sondage.
+            Pour toute question, veuillez communiquer avec Fulano  à nadie@blah.com
+            "
+        '''
+        invalid_po_2 = '''
+            msgctxt ""
+            "{\"checksum\": 2148532640, \"cxt\": \"collector_thankyou\", \"id\": "
+            "270005359}"
+            msgid ""
+            "Thank you very much for your time.\n"
+            "If you have any questions regarding this survey, please contact Fulano "
+            "at fulano@blah.com."
+            msgstr "Merci de prendre le temps de remplir le sondage.
+            Pour toute question, veuillez communiquer avec Fulano a fulano@blah.com
+            "
+            '''
+        # Catalog not created, throws Unicode Error
+        buf = StringIO(invalid_po)
+        output = None
+
+        # This should only be thrown under py27
+        if sys.version_info.major == 2:
+            with self.assertRaises(UnicodeEncodeError):
+                output = pofile.read_po(buf, locale='fr', abort_invalid=False)
+            assert not output
+        else:
+            output = pofile.read_po(buf, locale='fr', abort_invalid=False)
+            assert isinstance(output, Catalog)
+
+        # Catalog not created, throws PoFileError
+        buf = StringIO(invalid_po_2)
+        output = None
+        with self.assertRaises(pofile.PoFileError) as e:
+            output = pofile.read_po(buf, locale='fr', abort_invalid=True)
+        assert not output
+
+        # Catalog is created with warning, no abort
+        buf = StringIO(invalid_po_2)
+        output = pofile.read_po(buf, locale='fr', abort_invalid=False)
+        assert isinstance(output, Catalog)
+
+        # Catalog not created, aborted with PoFileError
+        buf = StringIO(invalid_po_2)
+        output = None
+        with self.assertRaises(pofile.PoFileError) as e:
+            output = pofile.read_po(buf, locale='fr', abort_invalid=True)
+        assert not output
+
+    def test_invalid_pofile_with_abort_flag(self):
+        parser = pofile.PoFileParser(None, abort_invalid=True)
+        lineno = 10
+        line = 'Algo esta mal'
+        msg = 'invalid file'
+        with self.assertRaises(pofile.PoFileError) as e:
+            parser._invalid_pofile(line, lineno, msg)
 
 class WritePoTestCase(unittest.TestCase):