Bug 1321279 - Part 1 - Read target FTL files before migrations.

Read target Fluent localization files before migrating translations into
them from legacy resources.  This change also explicitly prohibits
specifying Fluent resources as sources of migration. They could only be
used with the COPY transform anyways.

Author: stasm

fluent/migrate/__init__.py

@@ -1,6 +1,9 @@
 # coding=utf8
 
 from .context import MergeContext                      # noqa: F401
+from .errors import (                                  # noqa: F401
+    MigrationError, NotSupportedError, UnreadableReferenceError
+)
 from .transforms import (                              # noqa: F401
     Source, COPY, REPLACE_IN_TEXT, REPLACE, PLURALS, CONCAT
 )

fluent/migrate/context.py

@@ -18,7 +18,7 @@ def getParser(path):
 from .cldr import get_plural_categories
 from .transforms import Source
 from .merge import merge_resource
-from .util import get_message
+from .errors import NotSupportedError, UnreadableReferenceError
 
 
 class MergeContext(object):
@@ -79,6 +79,10 @@ def read_ftl_resource(self, path):
         f = codecs.open(path, 'r', 'utf8')
         try:
             contents = f.read()
+        except UnicodeDecodeError as err:
+            logger = logging.getLogger('migrate')
+            logger.warn('Unable to read file {}: {}'.format(path, err))
+            raise err
         finally:
             f.close()
 
@@ -94,7 +98,7 @@ def read_ftl_resource(self, path):
             logger = logging.getLogger('migrate')
             for annot in annots:
                 msg = annot.message
-                logger.warn(u'Syntax error in {}: {}'.format(path, msg))
+                logger.warn('Syntax error in {}: {}'.format(path, msg))
 
         return ast
 
@@ -111,43 +115,40 @@ def add_reference(self, path, realpath=None):
         try:
             ast = self.read_ftl_resource(fullpath)
         except IOError as err:
-            logger = logging.getLogger('migrate')
-            logger.error(u'Missing reference file: {}'.format(path))
-            raise err
+            error_message = 'Missing reference file: {}'.format(fullpath)
+            logging.getLogger('migrate').error(error_message)
+            raise UnreadableReferenceError(error_message)
         except UnicodeDecodeError as err:
-            logger = logging.getLogger('migrate')
-            logger.error(u'Error reading file {}: {}'.format(path, err))
-            raise err
+            error_message = 'Error reading file {}: {}'.format(fullpath, err)
+            logging.getLogger('migrate').error(error_message)
+            raise UnreadableReferenceError(error_message)
         else:
             self.reference_resources[path] = ast
 
-    def add_localization(self, path):
-        """Add an existing localization resource.
+    def maybe_add_localization(self, path):
+        """Add a localization resource to migrate translations from.
 
-        If it's an FTL resource, add an FTL AST.  Otherwise, it's a legacy
-        resource.  Use a compare-locales parser to create a dict of (key,
-        string value) tuples.
+        Only legacy resources can be added as migration sources.  The resource
+        may be missing on disk.
+
+        Uses a compare-locales parser to create a dict of (key, string value)
+        tuples.
         """
-        fullpath = os.path.join(self.localization_dir, path)
-        if fullpath.endswith('.ftl'):
-            try:
-                ast = self.read_ftl_resource(fullpath)
-            except IOError:
-                logger = logging.getLogger('migrate')
-                logger.warn(u'Missing localization file: {}'.format(path))
-            except UnicodeDecodeError as err:
-                logger = logging.getLogger('migrate')
-                logger.warn(u'Error reading file {}: {}'.format(path, err))
-            else:
-                self.localization_resources[path] = ast
+        if path.endswith('.ftl'):
+            error_message = (
+                'Migrating translations from Fluent files is not supported '
+                '({})'.format(path))
+            logging.getLogger('migrate').error(error_message)
+            raise NotSupportedError(error_message)
+
+        try:
+            fullpath = os.path.join(self.localization_dir, path)
+            collection = self.read_legacy_resource(fullpath)
+        except IOError:
+            logger = logging.getLogger('migrate')
+            logger.warn('Missing localization file: {}'.format(path))
         else:
-            try:
-                collection = self.read_legacy_resource(fullpath)
-            except IOError:
-                logger = logging.getLogger('migrate')
-                logger.warn(u'Missing localization file: {}'.format(path))
-            else:
-                self.localization_resources[path] = collection
+            self.localization_resources[path] = collection
 
     def add_transforms(self, path, transforms):
         """Define transforms for path.
@@ -175,17 +176,30 @@ def get_sources(acc, cur):
         path_transforms = self.transforms.setdefault(path, [])
         path_transforms += transforms
 
+        if path not in self.localization_resources:
+            fullpath = os.path.join(self.localization_dir, path)
+            try:
+                ast = self.read_ftl_resource(fullpath)
+            except IOError:
+                logger = logging.getLogger('migrate')
+                logger.info(
+                    'Localization file {} does not exist and '
+                    'it will be created'.format(path))
+            except UnicodeDecodeError:
+                logger = logging.getLogger('migrate')
+                logger.warn(
+                    'Localization file {} will be re-created and some '
+                    'translations might be lost'.format(path))
+            else:
+                self.localization_resources[path] = ast
+
     def get_source(self, path, key):
-        """Get an entity value from the localized source.
+        """Get an entity value from a localized legacy source.
 
         Used by the `Source` transform.
         """
-        if path.endswith('.ftl'):
-            resource = self.localization_resources[path]
-            return get_message(resource.body, key)
-        else:
-            resource = self.localization_resources[path]
-            return resource.get(key, None)
+        resource = self.localization_resources[path]
+        return resource.get(key, None)
 
     def merge_changeset(self, changeset=None):
         """Return a generator of FTL ASTs for the changeset.
@@ -200,10 +214,11 @@ def merge_changeset(self, changeset=None):
         """
 
         if changeset is None:
-            # Merge all known legacy translations.
+            # Merge all known legacy translations. Used in tests.
             changeset = {
                 (path, key)
                 for path, strings in self.localization_resources.iteritems()
+                if not path.endswith('.ftl')
                 for key in strings.iterkeys()
             }
 

fluent/migrate/errors.py

@@ -0,0 +1,10 @@
+class MigrationError(ValueError):
+    pass
+
+
+class NotSupportedError(MigrationError):
+    pass
+
+
+class UnreadableReferenceError(MigrationError):
+    pass

fluent/migrate/merge.py

@@ -11,9 +11,10 @@ def merge_resource(ctx, reference, current, transforms, in_changeset):
     """Transform legacy translations into FTL.
 
     Use the `reference` FTL AST as a template.  For each en-US string in the
-    reference, first check if it's in the currently processed changeset with
-    `in_changeset`; then check for an existing translation in the current FTL
-    `localization` or for a migration specification in `transforms`.
+    reference, first check for an existing translation in the current FTL
+    `localization` and return early if it's present; then if the string has
+    a transform defined in the migration specification and if it's in the
+    currently processed changeset, evaluate the transform.
     """
 
     def merge_body(body):

fluent/migrate/transforms.py

@@ -66,6 +66,7 @@
 from __future__ import unicode_literals
 
 import fluent.syntax.ast as FTL
+from .errors import NotSupportedError
 
 
 def pattern_from_text(value):
@@ -113,6 +114,11 @@ class Source(Transform):
     # not be replaced?
 
     def __init__(self, path, key):
+        if path.endswith('.ftl'):
+            raise NotSupportedError(
+                'Migrating translations from Fluent files is not supported '
+                '({})'.format(path))
+
         self.path = path
         self.key = key
 

tests/migrate/fixtures/en-US/privacy.ftl

@@ -0,0 +1,9 @@
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+dnt-description =
+    Send websites a “Do Not Track” signal
+    that you don’t want to be tracked
+dnt-learn-more = Learn more
+dnt-always = Always

tests/migrate/fixtures/pl/existing.ftl

@@ -0,0 +1,7 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+   - License, v. 2.0. If a copy of the MPL was not distributed with this
+   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+
+<!ENTITY doNotTrack.description "Old Description in Polish">
+<!ENTITY doNotTrack.learnMore.label "Więcej informacji">
+<!ENTITY doNotTrack.always.label "Zawsze">

tests/migrate/fixtures/pl/privacy.dtd

@@ -0,0 +1,5 @@
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+dnt-description = New Description in Polish

tests/migrate/fixtures/pl/privacy.ftl

@@ -16,6 +16,8 @@
 
 class MockContext(unittest.TestCase):
     def get_source(self, path, key):
+        # Ignore path (test.properties) and get translations from self.strings
+        # defined in setUp.
         return self.strings.get(key, None).val
 
 
@@ -36,7 +38,7 @@ def test_concat_one(self):
         msg = FTL.Message(
             FTL.Identifier('hello'),
             value=CONCAT(
-                COPY(self.strings, 'hello'),
+                COPY('test.properties', 'hello'),
             )
         )
 
@@ -51,8 +53,8 @@ def test_concat_two(self):
         msg = FTL.Message(
             FTL.Identifier('hello'),
             value=CONCAT(
-                COPY(self.strings, 'hello.start'),
-                COPY(self.strings, 'hello.end'),
+                COPY('test.properties', 'hello.start'),
+                COPY('test.properties', 'hello.end'),
             )
         )
 
@@ -86,8 +88,8 @@ def test_concat_whitespace_begin(self):
         msg = FTL.Message(
             FTL.Identifier('hello'),
             value=CONCAT(
-                COPY(self.strings, 'whitespace.begin.start'),
-                COPY(self.strings, 'whitespace.begin.end'),
+                COPY('test.properties', 'whitespace.begin.start'),
+                COPY('test.properties', 'whitespace.begin.end'),
             )
         )
 
@@ -103,8 +105,8 @@ def test_concat_whitespace_end(self):
         msg = FTL.Message(
             FTL.Identifier('hello'),
             value=CONCAT(
-                COPY(self.strings, 'whitespace.end.start'),
-                COPY(self.strings, 'whitespace.end.end'),
+                COPY('test.properties', 'whitespace.end.start'),
+                COPY('test.properties', 'whitespace.end.end'),
             )
         )
 
@@ -129,11 +131,11 @@ def test_concat_literal(self):
         msg = FTL.Message(
             FTL.Identifier('update-failed'),
             value=CONCAT(
-                COPY(self.strings, 'update.failed.start'),
+                COPY('test.properties', 'update.failed.start'),
                 FTL.TextElement('<a>'),
-                COPY(self.strings, 'update.failed.linkText'),
+                COPY('test.properties', 'update.failed.linkText'),
                 FTL.TextElement('</a>'),
-                COPY(self.strings, 'update.failed.end'),
+                COPY('test.properties', 'update.failed.end'),
             )
         )
 
@@ -157,9 +159,9 @@ def test_concat_replace(self):
         msg = FTL.Message(
             FTL.Identifier('channel-desc'),
             value=CONCAT(
-                COPY(self.strings, 'channel.description.start'),
+                COPY('test.properties', 'channel.description.start'),
                 FTL.Placeable(EXTERNAL_ARGUMENT('channelname')),
-                COPY(self.strings, 'channel.description.end'),
+                COPY('test.properties', 'channel.description.end'),
             )
         )
 
@@ -187,7 +189,7 @@ def test_concat_replace(self):
             FTL.Identifier('community'),
             value=CONCAT(
                 REPLACE(
-                    self.strings,
+                    'test.properties',
                     'community.start',
                     {
                         '&brandShortName;': MESSAGE_REFERENCE(
@@ -197,7 +199,7 @@ def test_concat_replace(self):
                 ),
                 FTL.TextElement('<a>'),
                 REPLACE(
-                    self.strings,
+                    'test.properties',
                     'community.mozillaLink',
                     {
                         '&vendorShortName;': MESSAGE_REFERENCE(
@@ -206,11 +208,11 @@ def test_concat_replace(self):
                     }
                 ),
                 FTL.TextElement('</a>'),
-                COPY(self.strings, 'community.middle'),
+                COPY('test.properties', 'community.middle'),
                 FTL.TextElement('<a>'),
-                COPY(self.strings, 'community.creditsLink'),
+                COPY('test.properties', 'community.creditsLink'),
                 FTL.TextElement('</a>'),
-                COPY(self.strings, 'community.end')
+                COPY('test.properties', 'community.end')
             )
         )