Issue 4767 - Improve error reporting in update_update_manifests

sitescripts/extensions/utils.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2016 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
@@ skipping 233 matching lines @@
 
 
 def _urlopen(url, attempts=3):
     """
     Tries to open a particular URL, retries on failure.
     """
     for i in range(attempts):
         try:
             return urllib.urlopen(url)
         except IOError as e:
-            error = e
+            error = Exception('Error {0} while opening {1} url'
+                              .format(e, url))
             time.sleep(5)
     raise error
 
 
+def _parseXMLDocument(url, attempts=2):
+    for i in range(attempts):
+        page = _urlopen(url)
+        content = page.read()
+        page.close()

[Sebastian Noack, 2017/01/17 09:33:32]

The close() should have gone into a finally block, or even better using the with
statement:

  with contextlib.closing(_urlopen(url)) as page:
      content = page.read()

[Sebastian Noack, 2017/01/17 10:16:56]

Yes, in Python 2, the repsonse object returned by urlopen() doesn't implement
__enter__/__exit__. So you either have to use the contextlib.closing() decorator
before passing it to the with statement or use a traditionally try-finally
block. I suggest doing the former, and removing the decorator once we migrate to
Python 3.

Also, please reply inline to inline comment.

[Sebastian Noack, 2017/01/17 10:20:15]

Well, alternatively you could use try-finally. But IMO, the with-statement is
nicer. For reference, here is how we did it in jsHydra where we support both
Python 2+3: https://hg.adblockplus.org/jshydra/file/tip/abp_rewrite.py#l11

[Vasily Kuznetsov, 2017/01/17 10:30:34]

Yeah, with is nicer, especially having Python 3 in mind.

+        try:
+            return dom.parseString(content)
+        except ExpatError as err:
+            exception = Exception('Error {0} while parsing xml:\n{1}\nfrom {2}'
+                                  .format(err, content, url))
+    raise exception
+
+
 def _getMozillaDownloadLink(galleryID):
     """
     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))
+    document = _parseXMLDocument(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)
 
 
@@ skipping 39 matching lines @@
     """
     gets the download links for all extensions and puts them into the config
     object
     """
     for repo in Configuration.getRepositoryConfigurations():
         try:
             (downloadURL, version) = _getDownloadLink(repo)
             if downloadURL is None:
                 raise Exception('No download link found for repo: ' +
                                 repo.repositoryName)
-        except ExpatError:
-            traceback.print_exc()
-            print "Error found while parsing xml from %s link" % repo.repositoryName

[Jon Sonesen, 2017/01/09 07:48:22]

Here you should be using the .format() method for string formatting as per our
style guidelines. Guideline text:

 "Use the + operator when concatenating exactly two strings, use the format()
method for more complex string formatting, use the join() method when
concatenating pre-existing sequences."

[Jon Sonesen, 2017/01/09 08:44:10]

Oops, I also just noticed that you are using double quotes. We are using single
quotes so that string literals match the repr 

[f.nicolaisen, 2017/01/10 08:58:04]

This adds some context (i.e. which repo) to the error, which is good.

However, in the case of the stacktrace shown in the issue, it is a special case
for _getMozillaDownloadLink - meaning that some external service is returning
something we cannot parse:

    ExpatError: mismatched tag: line 6, column 2

I'm guessing that the mozilla service returns something different than we
expect. I would like to know what this is, so that we know more about what
action to take next time it occurs. For example, maybe they return an xml
payload saying <response msg="This service is deprecated, please switch to using
http://blabla.mozilla.org">.

Perhaps it is even possible to test cover this by mocking the external call to
deliver something unexpected like that (ref test_updateManifests.py).

However, I think with these considerations we are going outside the scope of
'infrastructure' and deeper into sitescripts territory. @Vasily/Jon is this
something you want to adopt and roll out in the near future?

-            continue
         except:
             traceback.print_exc()
             continue
         if not result.has_section(repo.repositoryName):
             result.add_section(repo.repositoryName)
         result.set(repo.repositoryName, 'downloadURL', downloadURL)
         result.set(repo.repositoryName, 'version', version)
 
         qrcode = _getQRCode(downloadURL)
         if qrcode is not None:
@@ skipping 46 matching lines @@
     if not extensions:
         return
 
     updates = {}
     for extension in extensions:
         updates[extension['basename']] = {
             'url': extension['updateURL'],
             'version': extension['version']
         }
     writeLibabpUpdateManifest(path, updates)