Merge branch 'metadata-sort-and-refactor' into 'master'

metadata: remove obsolete quirks; add sorting and removal of empty values

Closes #332

See merge request fdroid/fdroidserver!1365

.gitlab-ci.yml

@@ -58,9 +58,6 @@ metadata_v0:
     - ../tests/dump_internal_metadata_format.py
     - sed -i
           -e '/RequiresRoot:/d'
-          -e "/buildozer/d"
-          -e '/^comments\W /d'
-          -e 's,maven\(\W\) false,maven\1 null,'
           metadata/dump_*/*.yaml
     - diff -uw metadata/dump_*
 

fdroidserver/metadata.py

@@ -1070,6 +1070,26 @@ def post_parse_yaml_metadata(yamldata):
         }
 
 
+def _format_multiline(value):
+    """TYPE_MULTILINE with newlines in them are saved as YAML literal strings."""
+    if '\n' in value:
+        return ruamel.yaml.scalarstring.preserve_literal(str(value))
+    return str(value)
+
+
+def _format_list(value):
+    """TYPE_LIST should not contain null values."""
+    return [v for v in value if v]
+
+
+def _format_script(value):
+    """TYPE_SCRIPT with one value are converted to YAML string values."""
+    value = [v for v in value if v]
+    if len(value) == 1:
+        return value[0]
+    return value
+
+
 def _format_stringmap(appid, field, stringmap, versionCode=None):
     """Format TYPE_STRINGMAP taking into account localized files in the metadata dir.
 
@@ -1127,7 +1147,7 @@ def _format_stringmap(appid, field, stringmap, versionCode=None):
             make_list = False
             break
     if make_list:
-        return outlist
+        return sorted(outlist, key=str.lower)
     return stringmap
 
 
@@ -1142,48 +1162,34 @@ def _del_duplicated_NoSourceSince(app):
             del app['AntiFeatures'][key]
 
 
-def _field_to_yaml(typ, value):
-    """Convert data to YAML 1.2 format that keeps the right TYPE_*."""
-    if typ == TYPE_STRING:
-        return str(value)
-    elif typ == TYPE_INT:
-        return int(value)
-    elif typ == TYPE_MULTILINE:
-        if '\n' in value:
-            return ruamel.yaml.scalarstring.preserve_literal(str(value))
-        else:
-            return str(value)
-    elif typ == TYPE_SCRIPT:
-        if type(value) == list:
-            if len(value) == 1:
-                return value[0]
-            else:
-                return value
-    else:
-        return value
-
-
 def _builds_to_yaml(app):
+    """Reformat Builds: flags for output to YAML 1.2.
+
+    This will strip any flag/value that is not set or is empty.
+    TYPE_BOOL fields are removed when they are false.  0 is valid
+    value, it should not be stripped, so there are special cases to
+    handle that.
+
+    """
     builds = ruamel.yaml.comments.CommentedSeq()
     for build in app.get('Builds', []):
-        if not isinstance(build, Build):
-            build = Build(build)
         b = ruamel.yaml.comments.CommentedMap()
         for field in build_flags:
-            if hasattr(build, field):
-                value = getattr(build, field)
-                if field == 'gradle' and value == ['off']:
-                    value = [
-                        ruamel.yaml.scalarstring.SingleQuotedScalarString('off')
-                    ]
-                typ = flagtype(field)
-                # don't check value == True for TYPE_INT as it could be 0
-                if value and typ == TYPE_STRINGMAP:
-                    v = _format_stringmap(app['id'], field, value, build['versionCode'])
-                    if v:
-                        b[field] = v
-                elif value is not None and (typ == TYPE_INT or value):
-                    b.update({field: _field_to_yaml(typ, value)})
+            v = build.get(field)
+            if v is None or v is False or v == '' or v == dict() or v == list():
+                continue
+            _flagtype = flagtype(field)
+            if _flagtype == TYPE_MULTILINE:
+                v = _format_multiline(v)
+            elif _flagtype == TYPE_LIST:
+                v = _format_list(v)
+            elif _flagtype == TYPE_SCRIPT:
+                v = _format_script(v)
+            elif _flagtype == TYPE_STRINGMAP:
+                v = _format_stringmap(app['id'], field, v, build['versionCode'])
+
+            if v or v == 0:
+                b[field] = v
 
         builds.append(b)
 
@@ -1205,11 +1211,12 @@ def _app_to_yaml(app):
         else:
             value = app.get(field)
             if value or field == 'Builds':
+                _fieldtype = fieldtype(field)
                 if field == 'Builds':
                     if app.get('Builds'):
                         cm.update({field: _builds_to_yaml(app)})
-                elif field == 'CurrentVersionCode':
-                    cm[field] = _field_to_yaml(TYPE_INT, value)
+                elif field == 'Categories':
+                    cm[field] = sorted(value, key=str.lower)
                 elif field == 'AntiFeatures':
                     v = _format_stringmap(app['id'], field, value)
                     if v:
@@ -1217,11 +1224,20 @@ def _app_to_yaml(app):
                 elif field == 'AllowedAPKSigningKeys':
                     value = [str(i).lower() for i in value]
                     if len(value) == 1:
-                        cm[field] = _field_to_yaml(TYPE_STRING, value[0])
+                        cm[field] = value[0]
                     else:
-                        cm[field] = _field_to_yaml(TYPE_LIST, value)
+                        cm[field] = value
+                elif _fieldtype == TYPE_MULTILINE:
+                    v = _format_multiline(value)
+                    if v:
+                        cm[field] = v
+                elif _fieldtype == TYPE_SCRIPT:
+                    v = _format_script(value)
+                    if v:
+                        cm[field] = v
                 else:
-                    cm[field] = _field_to_yaml(fieldtype(field), value)
+                    if value:
+                        cm[field] = value
 
                 if insert_newline:
                     # we need to prepend a newline in front of this field

tests/metadata.TestCase

@@ -717,10 +717,10 @@ class MetadataTest(unittest.TestCase):
         app.UpdateCheckMode = 'Tags'
         build = fdroidserver.metadata.Build()
         build.versionName = 'Unknown'  # taken from fdroidserver/import.py
-        build.versionCode = '0'  # taken from fdroidserver/import.py
+        build.versionCode = 0  # taken from fdroidserver/import.py
         build.disable = 'Generated by import.py ...'
         build.commit = 'Unknown'
-        build.gradle = [True]
+        build.gradle = ['yes']
         app['Builds'] = [build]
 
         fdroidserver.metadata.write_yaml(mf, app)
@@ -745,7 +745,7 @@ class MetadataTest(unittest.TestCase):
                     disable: Generated by import.py ...
                     commit: Unknown
                     gradle:
-                      - true
+                      - yes
 
                 AutoUpdateMode: None
                 UpdateCheckMode: Tags
@@ -1886,14 +1886,14 @@ class MetadataTest(unittest.TestCase):
         app = metadata.App({'Builds': [metadata.Build({'rm': []})]})
         self.assertEqual(dict(), metadata._app_to_yaml(app)['Builds'][0])
 
-    def test_app_to_yaml_build_list_string(self):
-        app = metadata.App({'Builds': [metadata.Build({'rm': 'one'})]})
-        self.assertEqual({'rm': 'one'}, metadata._app_to_yaml(app)['Builds'][0])
-
     def test_app_to_yaml_build_list_one(self):
         app = metadata.App({'Builds': [metadata.Build({'rm': ['one']})]})
         self.assertEqual({'rm': ['one']}, metadata._app_to_yaml(app)['Builds'][0])
 
+    def test_app_to_yaml_build_list_two(self):
+        app = metadata.App({'Builds': [metadata.Build({'rm': ['1', '2']})]})
+        self.assertEqual({'rm': ['1', '2']}, metadata._app_to_yaml(app)['Builds'][0])
+
     def test_app_to_yaml_build_list(self):
         app = metadata.App({'Builds': [metadata.Build({'rm': ['b2', 'NO1']})]})
         self.assertEqual({'rm': ['b2', 'NO1']}, metadata._app_to_yaml(app)['Builds'][0])
@@ -1920,6 +1920,53 @@ class MetadataTest(unittest.TestCase):
         cm = metadata._app_to_yaml(metadata.App({'CurrentVersionCode': 0}))
         self.assertFalse('CurrentVersionCode' in cm)
 
+    def test_format_multiline(self):
+        self.assertEqual(metadata._format_multiline('description'), 'description')
+
+    def test_format_multiline_empty(self):
+        self.assertEqual(metadata._format_multiline(''), '')
+
+    def test_format_multiline_newline_char(self):
+        self.assertEqual(metadata._format_multiline('one\\ntwo'), 'one\\ntwo')
+
+    def test_format_multiline_newlines(self):
+        self.assertEqual(
+            metadata._format_multiline(
+                textwrap.dedent(
+                    """
+                    one
+                    two
+                    three
+                    """
+                )
+            ),
+            '\none\ntwo\nthree\n',
+        )
+
+    def test_format_list_empty(self):
+        self.assertEqual(metadata._format_list(['', None]), list())
+
+    def test_format_list_one_empty(self):
+        self.assertEqual(metadata._format_list(['foo', None]), ['foo'])
+
+    def test_format_list_two(self):
+        self.assertEqual(metadata._format_list(['2', '1']), ['2', '1'])
+
+    def test_format_list_newline(self):
+        self.assertEqual(metadata._format_list(['one\ntwo']), ['one\ntwo'])
+
+    def test_format_list_newline_char(self):
+        self.assertEqual(metadata._format_list(['one\\ntwo']), ['one\\ntwo'])
+
+    def test_format_script_empty(self):
+        self.assertEqual(metadata._format_script(['', None]), list())
+
+    def test_format_script_newline(self):
+        self.assertEqual(metadata._format_script(['one\ntwo']), 'one\ntwo')
+
+    def test_format_script_newline_char(self):
+        self.assertEqual(metadata._format_script(['one\\ntwo']), 'one\\ntwo')
+
     def test_format_stringmap_empty(self):
         self.assertEqual(
             metadata._format_stringmap('🔥', 'test', dict()),
@@ -2019,6 +2066,89 @@ class MetadataTest(unittest.TestCase):
                 appid, field, {afname: {'uz': 'a', locale: 'b', 'zh': 'c'}}, versionCode
             )
 
+    def test_app_to_yaml_one_category(self):
+        """Categories does not get simplified to string when outputting YAML."""
+        self.assertEqual(
+            metadata._app_to_yaml({'Categories': ['one']}),
+            {'Categories': ['one']},
+        )
+
+    def test_app_to_yaml_categories(self):
+        """Sort case-insensitive before outputting YAML."""
+        self.assertEqual(
+            metadata._app_to_yaml({'Categories': ['c', 'a', 'B']}),
+            {'Categories': ['a', 'B', 'c']},
+        )
+
+    def test_builds_to_yaml_gradle_yes(self):
+        app = {'Builds': [{'versionCode': 0, 'gradle': ['yes']}]}
+        self.assertEqual(
+            metadata._builds_to_yaml(app), [{'versionCode': 0, 'gradle': ['yes']}]
+        )
+
+    def test_builds_to_yaml_gradle_off(self):
+        app = {'Builds': [{'versionCode': 0, 'gradle': ['off']}]}
+        self.assertEqual(
+            metadata._builds_to_yaml(app), [{'versionCode': 0, 'gradle': ['off']}]
+        )
+
+    def test_builds_to_yaml_gradle_true(self):
+        app = {'Builds': [{'versionCode': 0, 'gradle': ['true']}]}
+        self.assertEqual(
+            metadata._builds_to_yaml(app), [{'versionCode': 0, 'gradle': ['true']}]
+        )
+
+    def test_builds_to_yaml_gradle_false(self):
+        app = {'Builds': [{'versionCode': 0, 'gradle': ['false']}]}
+        self.assertEqual(
+            metadata._builds_to_yaml(app), [{'versionCode': 0, 'gradle': ['false']}]
+        )
+
+    def test_builds_to_yaml_stripped(self):
+        self.assertEqual(
+            metadata._builds_to_yaml(
+                {
+                    'Builds': [
+                        metadata.Build({'versionCode': 0, 'rm': [None], 'init': ['']})
+                    ]
+                }
+            ),
+            [{'versionCode': 0}],
+        )
+
+    def test_builds_to_yaml(self):
+        """Include one of each flag type with a valid value."""
+        app = {
+            'Builds': [
+                metadata.Build(
+                    {
+                        'versionCode': 0,
+                        'gradle': ['free'],
+                        'rm': ['0', '2'],
+                        'submodules': True,
+                        'timeout': 0,
+                        'init': ['false', 'two'],
+                    }
+                )
+            ]
+        }
+        # check that metadata.Build() inited flag values
+        self.assertEqual(app['Builds'][0]['scanignore'], list())
+        # then unchanged values should be removed by _builds_to_yaml
+        self.assertEqual(
+            metadata._builds_to_yaml(app),
+            [
+                {
+                    'versionCode': 0,
+                    'gradle': ['free'],
+                    'rm': ['0', '2'],
+                    'submodules': True,
+                    'timeout': 0,
+                    'init': ['false', 'two'],
+                }
+            ],
+        )
+
 
 class PostMetadataParseTest(unittest.TestCase):
     """Test the functions that post process the YAML input.