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,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
+import collections
 import io
 import itertools
 import json
 import logging
 import os
+import posixpath
 import sys
+import urllib
 import zipfile
 
-import requests

[Sebastian Noack, 2015/07/08 13:03:19]

I see, we use "requests", because urllib doesn't have built-in support for file
uploads. However, have you looked into using urllib3 directly? Note that
"requests" is just a thing wrapper around urllib3.

[Sebastian Noack, 2015/07/08 14:23:00]

I just realized that simply using built-in urllib2 with a handwritten request
body, doesn't seem too complicated either:

boundary = '--' + mimetools.choose_boundary()
data = [boundary, 'Content-Disposition: form-data; name="language"', '', locale]
for i, filename in enumerate(filenames):
    data.append(boundary)
    data.append('Content-Disposition: file; name="files[%d]"; filename="%d"' %
(i, i))
    data.append('')
    with open(filename, 'rb') as file:
        data.append(file.read())
data.append(boundary + '--')
data.append('')
body = '\r\n'.join(data)

request = urllib2.Request(url)
request.add_header('Content-Type', 'multipart/form-data; boundary=' + boundary)
request.add_header('Content-Length', len(body))
request.set_data(body)
response = urllib2.urlopen(request)

[kzar, 2015/07/08 15:26:41]

I discussed using the requests library with Wladimir in #adblockplus before I
started writing this code. I don't think it's helpful to continue that
discussion now, after weeks have passed and the code has been written, refined
and tested.

[Sebastian Noack, 2015/07/08 15:43:50]

I'm certainly curious about the reason, you decided for to use "requests". This
library seems to be an unneeded wrapper around "urllib3", which seems to be
unneeded for the use case here, in the first place.

Usually, we (and in particular Wladimir) are opposed to introduce third-party
modules. And if we introduce a new dependency there need to be a good reason to
justify that. I don't care that you discussed that with Wladimir already. Since
I wasn't involved (or invited) to the discussion back then, I will revisit it.

[Wladimir Palant, 2015/07/08 23:11:05]

Doing multipart encoding manually is very awkward. While we do that in
buildtools to avoid introducing a dependency, here we have something that will
be run by the server rather than contributors - so a dependency is better than
adding code complexity.

We use requests because that's what Stack Overflow recommends :). I wasn't aware
that it is a wrapper around urllib3 and indeed - using urllib3 would make more
sense IMHO. While the requests API might be more convenient, just using
urllib3.filepost.encode_multipart_formdata() along with built-in APIs that any
Python developer knows will make this code more maintainable.

[Sebastian Noack, 2015/07/09 21:26:55]

Personally, I'd still prefer to go with built-in urllib2 and the implementation
I suggested above. The implementation in buildtools is rather hacky, I know.
However, I think the snippet I posted above would be reasonable. Also I don't
think that it would be justified to use a third-party library just to avoid
these few lines. However, it's not important enough to argue about.
Yeah, if you both disagree to go the way I suggested above, I agree that using
urllib3 or maybe even better, as you suggested, just urllib/urllib2 in
combination with urllib3.filepost.encode_multipart_formdata() would certainly be
better than using "requests".

[Wladimir Palant, 2015/07/10 21:24:04]

Nope, they are both hacky and I'd definitely prefer some code for this that we
don't own.

[kzar, 2015/07/11 19:21:17]

Done.

+import urllib3
 
 import cms.utils
 from cms.sources import FileSource
 
 logger = logging.getLogger("cms.bin.translate")
 
 class CrowdinAPI:
   FILES_PER_REQUEST = 20
 
-  def __init__(self, api_key, project_name, defaultlocale):
+  def __init__(self, api_key, project_name):
     self.api_key = api_key
     self.project_name = project_name
-    self.defaultlocale = defaultlocale

[Wladimir Palant, 2015/07/08 23:11:06]

It doesn't look like this field is ever used - and it seems misplaced here,
irrelevant for API requests.

[kzar, 2015/07/11 19:21:16]

Done.

-
-  def request(self, request_method, api_endpoint, **kwargs):
-    url = "https://api.crowdin.com/api/project/%s/%s?key=%s&json=1" % (
-      self.project_name, api_endpoint, self.api_key
+    self.connection = urllib3.connection_from_url("https://api.crowdin.com/")
+
+  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)
     )
     try:
-      response = requests.request(request_method, url, **kwargs)
-      response.raise_for_status()
-    except requests.exceptions.HTTPError as e:
-      logger.error("API call to %s failed:\n%s" % (url, response.text))
+      response = self.connection.request(
+        request_method, str(url), **kwargs
+      )
+    except urllib3.exceptions.HTTPError:
+      logger.error("Connection to API endpoint %s failed", url)
       raise
-    except requests.exceptions.ConnectionError:

[Wladimir Palant, 2015/07/08 23:11:06]

There are more exception classes - ConnectionError was merely an example but
there is also Timeout for example. All of them seem to inherit from RequestError
however so catching this one will work.

[kzar, 2015/07/11 19:21:17]

Done.

-      logger.error("Connection to API failed for endpoint %s" % url)
-      raise
+    if response.status < 200 or response.status >= 300:
+      logger.error("API call to %s failed:\n%s", url, response.data)
+      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"),),
+      fields=fields, preload_content=False
+    )
 
     try:
-      return response.json()
+      return json.load(response)
     except ValueError:
-      logger.error("Invalid response returned by API endpoint %s" % url)
+      logger.error("Invalid response returned by API endpoint %s", url)
       raise
 
 
 def grouper(iterable, n):
   iterator = iter(iterable)
   while True:
     chunk = tuple(itertools.islice(iterator, n))
-    if chunk:

[Sebastian Noack, 2015/07/08 13:03:19]

Nit: You could get rid of the else block:

if not chunk:
  break
yield chunk

[kzar, 2015/07/11 19:21:17]

Done.

-      yield chunk
-    else:
+    if not chunk:
       break
+    yield chunk
 
 def extract_strings(source, defaultlocale):
   logger.info("Extracting page strings (please be patient)...")
   page_strings = {}
 
-  def record_string(page, name, default, comment, fixed_strings):
-    store = page_strings.setdefault(page, {})

[Sebastian Noack, 2015/07/08 13:03:20]

Do we care to not unnecessarily change the order of strings, e.g. to keep diffs
compact? Then we should use an OrderedDict.

[Wladimir Palant, 2015/07/08 23:11:06]

Not necessarily relevant for diffs but Crowdin will use the order we define
here. So - yes, OrderedDict sounds like a good idea.

However, the current code will create a dictionary instance on each call, most
of the time it will be thrown away. So maybe a slightly less elegant approach is
better:

  try:
    store = page_strings[page]
  except KeyError:
    store = page_strings[page] = OrderedDict()

[kzar, 2015/07/11 19:21:16]

Done.

-    store[name] = {"message": default}
+  def record_string(page, locale, name, value, comment, fixed_strings):
+    if locale != defaultlocale:
+      return
+
+    try:
+      store = page_strings[page]
+    except KeyError:
+      store = page_strings[page] = collections.OrderedDict()
+
+    store[name] = {"message": value}
 
     if fixed_strings:
       comment = comment + "\n" if comment else ""

[Sebastian Noack, 2015/07/08 13:03:20]

Nit. The ternary operator just adds uneeded complexity here. I'd go for:

if comment:
  comment += "\n"

[kzar, 2015/07/11 19:21:17]

`comment` might be None, hence this logic.

-      comment += ", ".join("{%d}: %s" % (i, s)

[Sebastian Noack, 2015/07/08 13:03:20]

Nit: No reason to pack/unpack the sequence here:

.. % x for x in enumerate(..

[kzar, 2015/07/11 19:21:17]

Done.

-                           for i, s in enumerate(fixed_strings, 1))
+      comment += ", ".join("{%d}: %s" % i_s
+                           for i_s in enumerate(fixed_strings, 1))
     if comment:
       store[name]["description"] = comment
 
   for page, format in source.list_pages():
     cms.utils.process_page(source, defaultlocale, page,
-                           format=format, record_default_strings=record_string)

[Sebastian Noack, 2015/07/09 21:26:55]

Recording the default strings is what we do here. But the converter should be
agnostic of it. For it it's just a callback to call on every string. And that is
what the argument name should reflect.

[kzar, 2015/07/11 19:21:16]

Done.

+                           format=format, localized_string_callback=record_string)
   return page_strings
 
 def configure_locales(crowdin_api, required_locales, enabled_locales,
                       defaultlocale):
   logger.info("Checking which locales are supported by Crowdin...")
   response = crowdin_api.request("GET", "supported-languages")
 
   supported_locales = {l["crowdin_code"] for l in response}
   skipped_locales = required_locales - supported_locales
 
   if skipped_locales:
-    logger.warning("Ignoring locales that Crowdin doesn't support: %s" % (
-      ", ".join(skipped_locales)
-    ))
+    logger.warning("Ignoring locales that Crowdin doesn't support: %s",
+                   ", ".join(skipped_locales))
     required_locales -= skipped_locales
-
-  # It's useful to have a list of all locales to skip
-  skipped_locales.add(defaultlocale)
 
   if not required_locales.issubset(enabled_locales):
     logger.info("Enabling the required locales for the Crowdin project...")
     crowdin_api.request(
       "POST", "edit-project",
-      data={"languages[]": list(enabled_locales | required_locales)}
+      data={"languages": enabled_locales | required_locales}
     )
 
-  return required_locales, skipped_locales
+  return required_locales
 
 def list_remote_files(project_info):
   def parse_file_node(node, path=""):
     if node["node_type"] == "file":
       remote_files.add(path + node["name"])
     elif node["node_type"] == "directory":
       dir_name = path + node["name"]
       remote_directories.add(dir_name)
       for file in node.get("files", []):
         parse_file_node(file, dir_name + "/")
@@ skipping 10 matching lines @@
   for page, strings in page_strings.iteritems():
     if strings:
       local_files.add(page + ".json")
       while "/" in page:
         page = page.rsplit("/", 1)[0]
         local_directories.add(page)
   return local_files, local_directories
 
 def create_directories(crowdin_api, directories):
   for directory in directories:
-    logger.info("Creating directory %s" % directory)
+    logger.info("Creating directory %s", directory)
     crowdin_api.request("POST", "add-directory", data={"name": directory})
 
 def add_update_files(crowdin_api, api_endpoint, message, files, page_strings):
-  for group in grouper(files, CrowdinAPI.FILES_PER_REQUEST):

[Wladimir Palant, 2015/07/08 23:11:08]

No need to assume that crowdin_api is a CrowdinAPI instance. Also no need to
assume that this constant is identical for all instances. Just use
crowdin_api.FILES_PER_REQUEST. 

[kzar, 2015/07/11 19:21:16]

Done.

-    files = {}
+  for group in grouper(files, crowdin_api.FILES_PER_REQUEST):
+    files = []
     for file_name in group:
       page = os.path.splitext(file_name)[0]
-      files["files[%s]" % file_name] = (file_name, json.dumps(page_strings[page]))
+      files.append((file_name, json.dumps(page_strings[page]), "application/json"))
       del page_strings[page]
-    logger.info(message % len(files))
+    logger.info(message, len(files))
     crowdin_api.request("POST", api_endpoint, files=files)
 
 def upload_new_files(crowdin_api, new_files, page_strings):
   add_update_files(crowdin_api, "add-file", "Uploading %d new pages...",
                    new_files, page_strings)
 
 def update_existing_files(crowdin_api, existing_files, page_strings):
   add_update_files(crowdin_api, "update-file", "Updating %d existing pages...",
                    existing_files, page_strings)
 
 def upload_translations(crowdin_api, source_dir, new_files, required_locales):
   def open_locale_files(locale, files):
-    for file in files:
-      path = os.path.join(source_dir, "locales", locale, file)
+    for file_name in files:
+      path = os.path.join(source_dir, "locales", locale, file_name)
       if os.path.isfile(path):
-        yield ("files[%s]" % file, open(path, "r"))
+        with open(path, "rb") as f:
+          yield (file_name, f.read(), "application/json")
 
   if new_files:
     for locale in required_locales:
       for files in grouper(open_locale_files(locale, new_files),

[Sebastian Noack, 2015/07/08 13:03:20]

You should better first get the chunk of filenames and then open the files in
that chunk, rather than opening all files in advance.

[kzar, 2015/07/11 19:21:16]

`open_locale_files` is a generator and I consume the groups lazily so I don't
think the files are opened in advance.

-                           CrowdinAPI.FILES_PER_REQUEST):

[Wladimir Palant, 2015/07/08 23:11:05]

As above, crowdin_api.FILES_PER_REQUEST please.

[kzar, 2015/07/11 19:21:16]

Done.

-        try:
-          logger.info("Uploading %d existing translation "
-                      "files for locale %s..." % (len(files), locale))
-          crowdin_api.request("POST", "upload-translation", files=dict(files),
-                              data={"language": locale})
-        finally:
-          for file_name, file in files:
-            file.close()
+                           crowdin_api.FILES_PER_REQUEST):
+        logger.info("Uploading %d existing translation "
+                    "files for locale %s...", len(files), locale)
+        crowdin_api.request("POST", "upload-translation", files=files,
+                            data={"language": locale})
 
 def remove_old_files(crowdin_api, old_files):
   for file_name in old_files:
-    logger.info("Removing old file %s" % file_name)
+    logger.info("Removing old file %s", file_name)
     crowdin_api.request("POST", "delete-file", data={"file": file_name})
 
 def remove_old_directories(crowdin_api, old_directories):
   for directory in reversed(sorted(old_directories, key=len)):
-    logger.info("Removing old directory %s" % directory)
+    logger.info("Removing old directory %s", directory)
     crowdin_api.request("POST", "delete-directory", data={"name": directory})
 
-def download_translations(crowdin_api, source_dir,
-                          skipped_locales, required_locales):
+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 = requests.get(
-    "https://api.crowdin.com/api/project/%s/download/all.zip?key=%s" % (
-      crowdin_api.project_name, crowdin_api.api_key
-    )
-  )
-  response.raise_for_status()
+  response = crowdin_api.raw_request("GET", "download/all.zip")
+
   logger.info("Extracting translations archive...")
-  with zipfile.ZipFile(io.BytesIO(response.content), "r") as 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:
-        # Don't delete locale files for unsupported locales or the default
-        dirs[:] = [d for d in dirs if d not in skipped_locales]
+        dirs[:] = [d for d in dirs if d in required_locales]
       for f in files:
-        if f.endswith(".json"):
+        if f.lower().endswith(".json"):
           os.remove(os.path.join(root, f))
     # Then extract the new ones in place
     for member in archive.namelist():
-      path, file_name = os.path.split(member)
-      ext = os.path.splitext(file_name)[1]
-      locale = os.path.normpath(path).split(os.sep)[0]
-      if ext == ".json" and locale in required_locales:
+      path, file_name = posixpath.split(member)
+      ext = posixpath.splitext(file_name)[1]
+      locale = path.split(posixpath.sep)[0]
+      if ext.lower() == ".json" and locale in required_locales:
         archive.extract(member, locale_path)

[Wladimir Palant, 2015/07/08 23:11:08]

Please use posixpath module here rather than os.path - archive paths don't
follow OS path conventions. This should also eliminate the need for calling
normpath.

[kzar, 2015/07/11 19:21:16]

Done.

 
 def crowdin_sync(source_dir, crowdin_api_key):
   with FileSource(source_dir) as source:
     config = source.read_config()
     defaultlocale = config.get("general", "defaultlocale")
     crowdin_project_name = config.get("general", "crowdin-project-name")
 
-    crowdin_api = CrowdinAPI(crowdin_api_key, crowdin_project_name,
-                             defaultlocale)
+    crowdin_api = CrowdinAPI(crowdin_api_key, crowdin_project_name)
 
     logger.info("Requesting project information...")
     project_info = crowdin_api.request("GET", "info")
     page_strings = extract_strings(source, defaultlocale)
 
     required_locales = {l for l in source.list_locales() if l != defaultlocale}
     enabled_locales = {l["code"] for l in project_info["languages"]}
 
-  required_locales, skipped_locales = configure_locales(
-    crowdin_api, required_locales, enabled_locales, defaultlocale
-  )
+  required_locales = configure_locales(crowdin_api, required_locales,
+                                       enabled_locales, defaultlocale)
+
   remote_files, remote_directories = list_remote_files(project_info)
   local_files, local_directories = list_local_files(page_strings)
 
   # Avoid deleting all remote content if there was a problem listing local files
   if not local_files:
     logger.error("No existing strings found, maybe the project directory is "
                  "not set up correctly? Aborting!")
     sys.exit(1)
 
   new_files = local_files - remote_files
   new_directories = local_directories - remote_directories
   create_directories(crowdin_api, new_directories)
   upload_new_files(crowdin_api, new_files, page_strings)
   upload_translations(crowdin_api, source_dir, new_files, required_locales)
 
   existing_files = local_files - new_files
   update_existing_files(crowdin_api, existing_files, page_strings)
 
   old_files = remote_files - local_files
   old_directories = remote_directories - local_directories
   remove_old_files(crowdin_api, old_files)
   remove_old_directories(crowdin_api, old_directories)
 
-  download_translations(crowdin_api, source_dir,
-                        skipped_locales, required_locales)
+  download_translations(crowdin_api, source_dir, required_locales)
   logger.info("Crowdin sync completed.")
 
 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)