Issue 2625 - [cms] Crowdin synchronisation script

cms/bin/translate.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2015 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,
@@ skipping 23 matching lines @@
 logger = logging.getLogger("cms.bin.translate")
 
 class CrowdinAPI:
   FILES_PER_REQUEST = 20
 
   def __init__(self, api_key, project_name):
     self.api_key = api_key
     self.project_name = project_name
     self.connection = urllib3.connection_from_url("https://api.crowdin.com/")
 
-  def raw_request(self, request_method, api_endpoint, query_params, **kwargs):
+  def raw_request(self, request_method, api_endpoint, query_params=(), **kwargs):
     url = "/api/project/%s/%s?%s" % (
       urllib.quote(self.project_name),
       urllib.quote(api_endpoint),
-      urllib.urlencode([("key", self.api_key)] + query_params)
+      urllib.urlencode((("key", self.api_key),) + query_params)
     )
     try:
       response = self.connection.request(
         request_method, str(url), **kwargs

[Sebastian Noack, 2015/07/15 10:31:03]

Converting url to an str object seems to be unnecessary. I don't see any obvious
code path where it isn't one. And even then urllib3 doesn't seem to bother
whether it gets an str or unicode object here.

[kzar, 2015/07/15 11:09:03]

For some reason the URL was ending up being unicode, perhaps from the project
name pulled out of the config? Anyway it turns out if the URL passed to httplib
is unicode then the whole request becomes unicode. Then later when httplib tries
to append the body of the request an encoding exception could be thrown if there
are any non-ascii characters in the body (e.g. existing Russian translations).
This was the simplest way I found to make things work consistently.

[Sebastian Noack, 2015/07/15 11:27:11]

So far for urllib3. This is certainly a silly behavior, if not a footgun. But
yeah, nothing else/better we could do about it.

       )
-
-      if response.status < 200 or response.status >= 300:
-        raise urllib3.exceptions.HTTPError(response.status)
-
-      return response

[Sebastian Noack, 2015/07/15 10:31:03]

Nit: The return doesn't need to be in the try..catch block

[kzar, 2015/07/15 11:09:03]

Done.

     except urllib3.exceptions.HTTPError:
+      logger.error("Connection to API endpoint %s failed", url)
+      raise
+    if response.status < 200 or response.status >= 300:
       logger.error("API call to %s failed:\n%s", url, response.data)

[Sebastian Noack, 2015/07/15 10:31:03]

If self.connect.request() fails, "response" wouldn't be deifned.

[kzar, 2015/07/15 11:09:03]

Whoops, Done.

-      raise
+      raise urllib3.exceptions.HTTPError(response.status)
+    return response
 
   def request(self, request_method, api_endpoint, data=None, files=None):
     fields = []
     if data:
       for name, value in data.iteritems():
         if isinstance(value, basestring):
           fields.append((name, value))
         else:
           fields.extend((name + "[]", v) for v in value)
     if files:
       fields.extend(("files[%s]" % f[0], f) for f in files)
 
     response = self.raw_request(
-      request_method, api_endpoint, [("json", "1")],
+      request_method, api_endpoint, (("json", "1"),),
       fields=fields, preload_content=False
     )
 
     try:
       return json.load(response)
     except ValueError:
       logger.error("Invalid response returned by API endpoint %s", url)
       raise
 
 
@@ skipping 132 matching lines @@
     crowdin_api.request("POST", "delete-directory", data={"name": directory})
 
 def download_translations(crowdin_api, source_dir, required_locales):
   logger.info("Requesting generation of fresh translations archive...")
   result = crowdin_api.request("GET", "export")
   if result.get("success", {}).get("status") == "skipped":
     logger.warning("Archive generation skipped, either "
                    "no changes or API usage excessive")
 
   logger.info("Downloading translations archive...")
-  response = crowdin_api.raw_request("GET", "download/all.zip", [])
+  response = crowdin_api.raw_request("GET", "download/all.zip")
 
   logger.info("Extracting translations archive...")
   with zipfile.ZipFile(io.BytesIO(response.data), "r") as archive:
     locale_path = os.path.join(source_dir, "locales")
     # First clear existing translation files
     for root, dirs, files in os.walk(locale_path, topdown=True):
       if root == locale_path:
         dirs[:] = [d for d in dirs if d in required_locales]
       for f in files:
         if f.lower().endswith(".json"):
@@ skipping 53 matching lines @@
 if __name__ == "__main__":
   if len(sys.argv) < 3:
     print >>sys.stderr, "Usage: python -m cms.bin.translate www_directory crowdin_project_api_key [logging_level]"
     sys.exit(1)
 
   logging.basicConfig()
   logger.setLevel(sys.argv[3] if len(sys.argv) > 3 else logging.INFO)
 
   source_dir, crowdin_api_key = sys.argv[1:3]
   crowdin_sync(source_dir, crowdin_api_key)