Issue 4767 - Improve error reporting in update_update_manifests

sitescripts/extensions/utils.py

Index: sitescripts/extensions/utils.py
===================================================================
--- a/sitescripts/extensions/utils.py
+++ b/sitescripts/extensions/utils.py
@@ -23,9 +23,11 @@
 import urlparse
 import urllib
 import xml.dom.minidom as dom
+import sys
 from ConfigParser import SafeConfigParser, NoOptionError
 from StringIO import StringIO
 from sitescripts.utils import get_config
+from xml.parsers.expat import ExpatError
 
 PACKAGE_SUFFIXES = {
     'gecko': '.xpi',
@@ -259,16 +261,24 @@
     """
     gets download link for a Gecko add-on from the Mozilla Addons site
     """
-    url = 'https://services.addons.mozilla.org/en-US/firefox/api/1/addon/%s' % _urlencode(galleryID)
-    document = dom.parse(_urlopen(url))
-    linkTags = document.getElementsByTagName('install')
-    linkTag = linkTags[0] if len(linkTags) > 0 else None
-    versionTags = document.getElementsByTagName('version')
-    versionTag = versionTags[0] if len(versionTags) > 0 else None
-    if linkTag and versionTag and linkTag.firstChild and versionTag.firstChild:
-        return (linkTag.firstChild.data, versionTag.firstChild.data)
-    else:
-        return (None, None)
+    try:
+        url = 'https://services.addons.mozilla.org/en-US/firefox/api/1/addon/%s' % _urlencode(galleryID)
+        document = dom.parse(_urlopen(url))

[Vasily Kuznetsov, 2017/01/12 11:12:32]

Actually we don't need to put the whole block into try/except to catch
ExpatError. Expat errors are only created by `dom.parse`. So you can put only
this one line into try/except.

If you want to re-download and re-parse once (as we dicussed over IRC), it would
probably make more sense to move this line into a separate function similar to
`_urlopen`.

+        linkTags = document.getElementsByTagName('install')
+        linkTag = linkTags[0] if len(linkTags) > 0 else None
+        versionTags = document.getElementsByTagName('version')
+        versionTag = versionTags[0] if len(versionTags) > 0 else None
+        if linkTag and versionTag and linkTag.firstChild and versionTag.firstChild:
+            return (linkTag.firstChild.data, versionTag.firstChild.data)
+        else:
+            return (None, None)
+    except ExpatError:
+        page = _urlopen(url)

[Vasily Kuznetsov, 2017/01/12 11:12:32]

This is not a good idea. You're downloading the page again just to put it into
the exception text. First of all, it's wasteful, we've already downloaded it on
line 266, no need to do it again. Second, it can lead to very misleading error
messages if what you download on the second try turns out to be good XML.

So as per comment in line 266, it would be better to move parsing and its error
reporting into a separate function and there do something like:

for i in range(attempts):
    with _urlopen(url) as page:
        content = page.read()
    try:
        return dom.parseString(content)
    except err:
        # We shouldn't raise this immediately since we'll be retrying.
        exception = Exception('error {} in parsing xml:\n{}\nfrom
{}'.format(err, content, url))
raise exception

+        content = document.open()
+        page.close()
+        raise Exception('Error found while parsing xml:\n{0}\nfrom {1} link'
+                        .format(content, galleryID))
+
 
 
 def _getLocalLink(repo):