#404 Fix for multiple files for *_file fields

 * Introduce to use ',' to separate multiple files

Author: chinyeungli

src/attributecode/gen.py

@@ -2,7 +2,7 @@
 # -*- coding: utf8 -*-
 
 # ============================================================================
-#  Copyright (c) 2013-2018 nexB Inc. http://www.nexb.com/ - All rights reserved.
+#  Copyright (c) 2013-2019 nexB Inc. http://www.nexb.com/ - All rights reserved.
 #  Licensed under the Apache License, Version 2.0 (the "License");
 #  you may not use this file except in compliance with the License.
 #  You may obtain a copy of the License at
@@ -36,6 +36,7 @@
 from attributecode import util
 from attributecode.util import add_unc
 from attributecode.util import csv
+from attributecode.util import file_fields
 from attributecode.util import to_posix
 from attributecode.util import UNC_PREFIX_POSIX
 from attributecode.util import unique
@@ -96,6 +97,24 @@ def check_duplicated_about_resource(inventory_dict):
     return errors
 
 
+def check_newline_in_file_field(inventory_dict):
+    """
+    Return a list of errors for newline characters detected in *_file fields.
+    """
+    errors = []
+    for component in inventory_dict:
+        for k in component.keys():
+            if k in file_fields:
+                try:
+                    if '\n' in component[k]:
+                        msg = ("New line character detected in '%s' for '%s' which is not supported."
+                                "\nPlease use ',' to declare multiple files.") % (k, component['about_resource'])
+                        errors.append(Error(CRITICAL, msg))
+                except:
+                    pass
+    return errors
+
+
 # TODO: this should be either the CSV or the ABOUT files but not both???
 def load_inventory(location, base_dir, reference_dir=None):
     """
@@ -126,6 +145,10 @@ def load_inventory(location, base_dir, reference_dir=None):
         if dup_about_resource_err:
             errors.extend(dup_about_resource_err)
             return errors, abouts
+        newline_in_file = check_newline_in_file_field(inventory)
+        if newline_in_file:
+            errors.extend(newline_in_file)
+            return errors, abouts
     except Exception as e:
         # TODO: why catch ALL Exception
         msg = "The essential field 'about_resource' is not found in the <input>"
@@ -164,7 +187,7 @@ def load_inventory(location, base_dir, reference_dir=None):
             else:
                 updated_resource_value = basename(resource_path)
             fields['about_resource'] = updated_resource_value
-        
+
         ld_errors = about.load_dict(
             fields,
             base_dir,

src/attributecode/model.py

@@ -64,6 +64,7 @@
 from attributecode.util import boolean_fields
 from attributecode.util import copy_license_notice_files
 from attributecode.util import csv
+from attributecode.util import file_fields
 from attributecode.util import filter_errors
 from attributecode.util import is_valid_name
 from attributecode.util import on_windows
@@ -438,66 +439,68 @@ def _validate(self, *args, **kwargs):
         # dict of normalized paths to a location or None
         paths = OrderedDict()
 
-        for path in self.value:
-            path = path.strip()
-            path = util.to_posix(path)
-
-            # normalize eventual / to .
-            # and a succession of one or more ////// to . too
-            if path.strip() and not path.strip(posixpath.sep):
-                path = '.'
-
-            # removing leading and trailing path separator
-            # path are always relative
-            path = path.strip(posixpath.sep)
-
-            # the license files, if need to be copied, are located under the path
-            # set from the 'license-text-location' option, so the tool should check
-            # at the 'license-text-location' instead of the 'base_dir'
-            if not (self.base_dir or self.reference_dir):
-                msg = (u'Field %(name)s: Unable to verify path: %(path)s:'
-                       u' No base directory provided' % locals())
-                errors.append(Error(ERROR, msg))
-                location = None
-                paths[path] = location
-                continue
-
-            if self.reference_dir:
-                location = posixpath.join(self.reference_dir, path)
-            else:
-                # The 'about_resource' should be a joined path with
-                # the 'about_file_path' and the 'base_dir
-                if not self.running_inventory and self.about_file_path:
-                    # Get the parent directory of the 'about_file_path'
-                    afp_parent = posixpath.dirname(self.about_file_path)
-
-                    # Create a relative 'about_resource' path by joining the
-                    # parent of the 'about_file_path' with the value of the
-                    # 'about_resource'
-                    arp = posixpath.join(afp_parent, path)
-                    normalized_arp = posixpath.normpath(arp).strip(posixpath.sep)
-                    location = posixpath.join(self.base_dir, normalized_arp)
-                else:
-                    location = posixpath.join(self.base_dir, path)
-
-            location = util.to_native(location)
-            location = os.path.abspath(os.path.normpath(location))
-            location = util.to_posix(location)
-            location = add_unc(location)
-
-            if not os.path.exists(location):
-                # We don't want to show the UNC_PREFIX in the error message
-                location = util.to_posix(location.strip(UNC_PREFIX))
-                msg = (u'Field %(name)s: Path %(location)s not found'
-                       % locals())
-                # We want to show INFO error for 'about_resource'
-                if name == u'about_resource':
-                    errors.append(Error(INFO, msg))
+        for path_value in self.value:
+            p = path_value.split(',')
+            for path in p:
+                path = path.strip()
+                path = util.to_posix(path)
+
+                # normalize eventual / to .
+                # and a succession of one or more ////// to . too
+                if path.strip() and not path.strip(posixpath.sep):
+                    path = '.'
+        
+                # removing leading and trailing path separator
+                # path are always relative
+                path = path.strip(posixpath.sep)
+        
+                # the license files, if need to be copied, are located under the path
+                # set from the 'license-text-location' option, so the tool should check
+                # at the 'license-text-location' instead of the 'base_dir'
+                if not (self.base_dir or self.reference_dir):
+                    msg = (u'Field %(name)s: Unable to verify path: %(path)s:'
+                           u' No base directory provided' % locals())
+                    errors.append(Error(ERROR, msg))
+                    location = None
+                    paths[path] = location
+                    continue
+        
+                if self.reference_dir:
+                    location = posixpath.join(self.reference_dir, path)
                 else:
-                    errors.append(Error(CRITICAL, msg))
-                location = None
-
-            paths[path] = location
+                    # The 'about_resource' should be a joined path with
+                    # the 'about_file_path' and the 'base_dir
+                    if not self.running_inventory and self.about_file_path:
+                        # Get the parent directory of the 'about_file_path'
+                        afp_parent = posixpath.dirname(self.about_file_path)
+        
+                        # Create a relative 'about_resource' path by joining the
+                        # parent of the 'about_file_path' with the value of the
+                        # 'about_resource'
+                        arp = posixpath.join(afp_parent, path)
+                        normalized_arp = posixpath.normpath(arp).strip(posixpath.sep)
+                        location = posixpath.join(self.base_dir, normalized_arp)
+                    else:
+                        location = posixpath.join(self.base_dir, path)
+        
+                location = util.to_native(location)
+                location = os.path.abspath(os.path.normpath(location))
+                location = util.to_posix(location)
+                location = add_unc(location)
+        
+                if not os.path.exists(location):
+                    # We don't want to show the UNC_PREFIX in the error message
+                    location = util.to_posix(location.strip(UNC_PREFIX))
+                    msg = (u'Field %(name)s: Path %(location)s not found'
+                           % locals())
+                    # We want to show INFO error for 'about_resource'
+                    if name == u'about_resource':
+                        errors.append(Error(INFO, msg))
+                    else:
+                        errors.append(Error(CRITICAL, msg))
+                    location = None
+        
+                paths[path] = location
 
         self.value = paths
         return errors
@@ -516,7 +519,6 @@ def _validate(self, *args, **kwargs):
         errors = super(AboutResourceField, self)._validate(*args, ** kwargs)
         return errors
 
-
 class FileTextField(PathField):
     """
     A path field pointing to one or more text files such as license files.
@@ -529,10 +531,7 @@ def _validate(self, *args, **kwargs):
         of errors. base_dir is the directory used to resolve a file location
         from a path.
         """
-
         errors = super(FileTextField, self)._validate(*args, ** kwargs)
-        super(FileTextField, self)._validate(*args, ** kwargs)
-
         # a FileTextField is a PathField
         # self.value is a paths to location ordered dict
         # we will replace the location with the text content
@@ -560,7 +559,6 @@ def _validate(self, *args, **kwargs):
         self.errors = errors
         return errors
 
-
 class BooleanField(SingleLineField):
     """
     An flag field with a boolean value. Validated value is False, True or None.
@@ -976,7 +974,6 @@ def load_dict(self, fields_dict, base_dir, running_inventory=False, reference_di
             if not value:
                 # never return empty or absent fieds
                 continue
-
             if key == u'licenses':
                 # FIXME: use a license object instead
                 lic_key, lic_name, lic_file, lic_url = ungroup_licenses(value)
@@ -1023,7 +1020,6 @@ def dumps(self):
         license_name = []
         license_file = []
         license_url = []
-        file_fields = ['about_resource', 'notice_file', 'changelog_file', 'author_file']
         bool_fields = ['redistribute', 'attribute', 'track_changes', 'modified', 'internal_use_only']
         for field in self.all_fields():
             if not field.value and not field.name in bool_fields:
@@ -1034,15 +1030,20 @@ def dumps(self):
             elif field.name == 'license_name' and field.value:
                 license_name = field.value
             elif field.name == 'license_file' and field.value:
-                license_file = field.value.keys()
+                # Restore the original_value as it was parsed for
+                # validation purpose
+                if field.original_value:
+                    license_file = field.original_value.split('\n')
+                else:
+                    license_file = field.value.keys()
             elif field.name == 'license_url' and field.value:
                 license_url = field.value
 
             # No multiple 'about_resource' reference supported.
             # Take the first element (should only be one) in the list for the
             # value of 'about_resource'
             elif field.name in file_fields and field.value:
-                data[field.name] = list(field.value.keys())[0]
+                data[field.name] = field.original_value
             elif field.name in bool_fields and not field.value == None:
                 data[field.name] = field.value
             else:
@@ -1193,8 +1194,17 @@ def about_object_to_list_of_dictionary(abouts):
     """
     serialized = []
     for about in abouts:
+        # Restore the *_file value to the original value
+        # The *_file's original_value may be parsed (i.e. split(',))
+        # for validation purpose.  
+        about.license_file.value = about.license_file.original_value
+        about.notice_file.value = about.notice_file.original_value 
+        about.changelog_file.value = about.changelog_file.original_value
+        about.author_file.value = about.author_file.original_value
+
         # TODO: this wholeblock should be under sd_dict()
         ad = about.as_dict()
+
         # Update the 'about_resource' field with the relative path
         # from the output location
         try:
@@ -1207,7 +1217,8 @@ def about_object_to_list_of_dictionary(abouts):
                     for resource in about_resource:
                         updated_about_resource = posixpath.normpath(posixpath.join(afp_parent, resource))
                         if resource == u'.':
-                            updated_about_resource = updated_about_resource + '/'
+                            if not updated_about_resource == '/': 
+                                updated_about_resource = updated_about_resource + '/'
                     ad['about_resource'] = OrderedDict([(updated_about_resource, None)])
                     del ad['about_file_path']
                 serialized.append(ad)

src/attributecode/util.py

@@ -53,6 +53,7 @@
 
 # boolean field name
 boolean_fields = ['redistribute', 'attribute', 'track_change', 'modified', 'internal_use_only']
+file_fields = ['about_resource', 'notice_file', 'changelog_file', 'author_file']
 
 def to_posix(path):
     """
@@ -487,14 +488,13 @@ def ungroup_licenses(licenses):
 # FIXME: add docstring
 def format_about_dict_for_csv_output(about_dictionary_list):
     csv_formatted_list = []
-    file_fields = ['license_file', 'notice_file', 'changelog_file', 'author_file']
     for element in about_dictionary_list:
         row_list = OrderedDict()
         for key in element:
             if element[key]:
                 if isinstance(element[key], list):
                     row_list[key] = u'\n'.join((element[key]))
-                elif key == u'about_resource' or key in file_fields:
+                elif key == u'about_resource':
                     row_list[key] = u'\n'.join((element[key].keys()))
                 else:
                     row_list[key] = element[key]
@@ -505,7 +505,6 @@ def format_about_dict_for_csv_output(about_dictionary_list):
 # FIXME: add docstring
 def format_about_dict_for_json_output(about_dictionary_list):
     licenses = ['license_key', 'license_name', 'license_file', 'license_url']
-    file_fields = ['notice_file', 'changelog_file', 'author_file']
     json_formatted_list = []
     for element in about_dictionary_list:
         row_list = OrderedDict()
@@ -526,11 +525,9 @@ def format_about_dict_for_json_output(about_dictionary_list):
                     elif key == 'license_name':
                         license_name = element[key]
                     elif key == 'license_file':
-                        license_file = element[key].keys()
+                        license_file = element[key]
                     elif key == 'license_url':
                         license_url = element[key]
-                elif key in file_fields:
-                    row_list[key] = element[key].keys()
                 else:
                     row_list[key] = element[key]
 

tests/test_gen.py

@@ -56,6 +56,18 @@ def test_check_duplicated_about_resource(self):
         result = gen.check_duplicated_about_resource(test_dict)
         assert expected == result
 
+    def test_check_newline_in_file_field(self):
+        test_dict = [
+            {'about_resource': '/test/test.c', 'name': 'test.c', 'notice_file': 'NOTICE\nNOTICE2'},
+            {'about_resource': '/test/abc/', 'version': '1.0', 'name': 'abc'},
+            {'about_resource': '/test/test.c', 'version': '1.04', 'name': 'test1.c'}]
+        expected = [
+            Error(CRITICAL,
+                  "New line character detected in 'notice_file' for '/test/test.c' which is not supported."
+                  "\nPlease use ',' to declare multiple files.")]
+        result = gen.check_newline_in_file_field(test_dict)
+        assert expected == result
+
     def test_load_inventory(self):
         location = get_test_loc('test_gen/inv.csv')
         base_dir = get_temp_dir()