Repository: spdx/tools-python

Issue:
Make properties optional that are not mandatory anymore in 2.3
  - [x] package concluded license
  - [x] package declared license
  - [x] package copyright text
  - [x] license information from files
  - [x] package verification code
  - [x] package has files
  - [x] file concluded license
  - [x] license information in file
  - [x] file copyright text
  - [x] snippet concluded license
  - [x] snippet copyright text
  - [x] license information in snippet

Discussion:

Patch:
diff --git a/spdx/file.py b/spdx/file.py
index df3ac88..7910e36 100644
--- a/spdx/file.py
+++ b/spdx/file.py
@@ -134,7 +134,7 @@ class File(object):
         return messages
 
     def validate_copyright(self, messages):
-        if not isinstance(
+        if self.copyright and not isinstance(
             self.copyright,
             (str, utils.NoAssert, utils.SPDXNone),
         ):
@@ -156,20 +156,26 @@ class File(object):
         return messages
 
     def validate_licenses_in_file(self, messages):
-        # FIXME: what are we testing the length of a list? or?
-        if len(self.licenses_in_file) == 0:
-            messages.append("File must have at least one license in file.")
+        for license_in_file in self.licenses_in_file:
+            if not isinstance(
+                license_in_file, (utils.SPDXNone, utils.NoAssert, document.License)
+            ):
+                messages.append(
+                    "License in file must be instance of "
+                    "spdx.utils.SPDXNone or spdx.utils.NoAssert or "
+                    "spdx.document.License"
+                )
 
         return messages
 
     def validate_concluded_license(self, messages):
-        # FIXME: use isinstance instead??
-        if not isinstance(
-            self.conc_lics, (document.License, utils.NoAssert, utils.SPDXNone)
+        if self.conc_lics and not isinstance(
+            self.conc_lics, (utils.SPDXNone, utils.NoAssert, document.License)
         ):
             messages.append(
-                "File concluded license must be one of "
-                "document.License, utils.NoAssert or utils.SPDXNone"
+                "File concluded license must be instance of "
+                "spdx.utils.SPDXNone or spdx.utils.NoAssert or "
+                "spdx.document.License"
             )
 
         return messages
diff --git a/spdx/package.py b/spdx/package.py
index 0114b06..d6727bd 100644
--- a/spdx/package.py
+++ b/spdx/package.py
@@ -164,7 +164,6 @@ class Package(object):
         self.validate_mandatory_str_fields(messages)
         self.validate_files(messages)
         self.validate_pkg_ext_refs(messages)
-        self.validate_mandatory_fields(messages)
         self.validate_optional_fields(messages)
         messages.pop_context()
 
@@ -204,22 +203,7 @@ class Package(object):
                 "spdx.utils.NoAssert or spdx.creationinfo.Creator"
             )
 
-        return messages
-
-    def validate_pkg_ext_refs(self, messages):
-        for ref in self.pkg_ext_refs:
-            if isinstance(ref, ExternalPackageRef):
-                messages = ref.validate(messages)
-            else:
-                messages.append(
-                    "External package references must be of the type "
-                    "spdx.package.ExternalPackageRef and not " + str(type(ref))
-                )
-
-        return messages
-
-    def validate_mandatory_fields(self, messages):
-        if not isinstance(
+        if self.conc_lics and not isinstance(
             self.conc_lics, (utils.SPDXNone, utils.NoAssert, document.License)
         ):
             messages.append(
@@ -228,7 +212,7 @@ class Package(object):
                 "spdx.document.License"
             )
 
-        if not isinstance(
+        if self.license_declared and not isinstance(
             self.license_declared, (utils.SPDXNone, utils.NoAssert, document.License)
         ):
             messages.append(
@@ -237,7 +221,6 @@ class Package(object):
                 "spdx.document.License"
             )
 
-        # FIXME: this is obscure and unreadable
         license_from_file_check = lambda prev, el: prev and isinstance(
             el, (document.License, utils.SPDXNone, utils.NoAssert)
         )
@@ -248,20 +231,24 @@ class Package(object):
                 "spdx.document.License"
             )
 
-        if not self.licenses_from_files and self.are_files_analyzed:
-            messages.append("Package licenses_from_files can not be empty")
+        return messages
+
+    def validate_pkg_ext_refs(self, messages):
+        for ref in self.pkg_ext_refs:
+            if isinstance(ref, ExternalPackageRef):
+                messages = ref.validate(messages)
+            else:
+                messages.append(
+                    "External package references must be of the type "
+                    "spdx.package.ExternalPackageRef and not " + str(type(ref))
+                )
 
         return messages
 
     def validate_files(self, messages):
         if self.are_files_analyzed:
-            if not self.files:
-                messages.append(
-                    "Package must have at least one file."
-                )
-            else:
-                for f in self.files:
-                    messages = f.validate(messages)
+            for file in self.files:
+                messages = file.validate(messages)
 
         return messages
 
@@ -278,6 +265,7 @@ class Package(object):
             "description",
             "attribution_text",
             "comment",
+            "cr_text"
         ]
         self.validate_str_fields(FIELDS, True, messages)
 
@@ -287,7 +275,7 @@ class Package(object):
         """Fields marked as Mandatory and of type string in class
         docstring must be of a type that provides __str__ method.
         """
-        FIELDS = ["name", "spdx_id", "download_location", "cr_text"]
+        FIELDS = ["name", "spdx_id", "download_location"]
         self.validate_str_fields(FIELDS, False, messages)
 
         return messages
diff --git a/spdx/parsers/jsonyamlxml.py b/spdx/parsers/jsonyamlxml.py
index 9ac9dee..eca4727 100644
--- a/spdx/parsers/jsonyamlxml.py
+++ b/spdx/parsers/jsonyamlxml.py
@@ -584,7 +584,7 @@ class SnippetParser(BaseParser):
                 return self.builder.set_snippet_copyright(self.document, copyright_text)
             except CardinalityError:
                 self.more_than_one_error("SNIPPET_COPYRIGHT")
-        else:
+        elif copyright_text is not None:
             self.value_error("SNIPPET_COPYRIGHT", copyright_text)
 
     def parse_snippet_license_comment(self, license_comment):
@@ -633,11 +633,11 @@ class SnippetParser(BaseParser):
                     self.document, license_object
                 )
             except SPDXValueError:
-                self.value_error("SNIPPET_SINGLE_LICS", concluded_license)
+                self.value_error("SNIPPET_CONCLUDED_LICENSE", concluded_license)
             except CardinalityError:
-                self.more_than_one_error("SNIPPET_SINGLE_LICS")
-        else:
-            self.value_error("SNIPPET_SINGLE_LICS", concluded_license)
+                self.more_than_one_error("SNIPPET_CONCLUDED_LICENSE")
+        elif concluded_license is not None:
+            self.value_error("SNIPPET_CONCLUDED_LICENSE", concluded_license)
 
     def parse_snippet_license_info_from_snippet(self, license_info_from_snippet):
         """
@@ -660,7 +660,7 @@ class SnippetParser(BaseParser):
                         self.value_error("SNIPPET_LIC_INFO", lic_in_snippet)
                 else:
                     self.value_error("SNIPPET_LIC_INFO", lic_in_snippet)
-        else:
+        elif license_info_from_snippet is not None:
             self.value_error("SNIPPET_LIC_INFO_FIELD", license_info_from_snippet)
 
 
@@ -836,34 +836,34 @@ class FileParser(BaseParser):
                 self.more_than_one_error("FILE_SINGLE_LICS")
             except OrderError:
                 self.order_error("FILE_SINGLE_LICS", "FILE_NAME")
-        else:
+        elif concluded_license is not None:
             self.value_error("FILE_SINGLE_LICS", concluded_license)
 
-    def parse_file_license_info_in_files(self, license_info_from_files):
+    def parse_file_license_info_in_files(self, license_info_in_files):
         """
         Parse File license information from files
         - license_info_from_files: Python list of licenses information from files (str/unicode)
         """
-        if isinstance(license_info_from_files, list):
-            for license_info_from_file in license_info_from_files:
-                if isinstance(license_info_from_file, str):
+        if isinstance(license_info_in_files, list):
+            for license_info_in_file in license_info_in_files:
+                if isinstance(license_info_in_file, str):
                     lic_parser = utils.LicenseListParser()
                     lic_parser.build(write_tables=0, debug=0)
                     license_object = self.replace_license(
-                        lic_parser.parse(license_info_from_file)
+                        lic_parser.parse(license_info_in_file)
                     )
                     try:
                         self.builder.set_file_license_in_file(
                             self.document, license_object
                         )
                     except SPDXValueError:
-                        self.value_error("FILE_LIC_FRM_FILES", license_info_from_file)
+                        self.value_error("FILE_LIC_IN_FILES", license_info_in_file)
                     except OrderError:
-                        self.order_error("FILE_LIC_FRM_FILES", "FILE_NAME")
+                        self.order_error("FILE_LIC_IN_FILES", "FILE_NAME")
                 else:
-                    self.value_error("FILE_LIC_FRM_FILES", license_info_from_file)
-        else:
-            self.value_error("FILE_LIC_FRM_FILES_FIELD", license_info_from_files)
+                    self.value_error("FILE_LIC_IN_FILES", license_info_in_file)
+        elif license_info_in_files is not None:
+            self.value_error("FILE_LIC_IN_FILES_FIELD", license_info_in_files)
 
     def parse_file_license_comments(self, license_comments):
         """
@@ -914,7 +914,7 @@ class FileParser(BaseParser):
                 self.more_than_one_error("FILE_COPYRIGHT_TEXT")
             except OrderError:
                 self.order_error("FILE_COPYRIGHT_TEXT", "FILE_NAME")
-        else:
+        elif copyright_text is not None:
             self.value_error("FILE_COPYRIGHT_TEXT", copyright_text)
 
     def parse_file_artifacts(self, file_artifacts):
@@ -1234,7 +1234,7 @@ class PackageParser(BaseParser):
                 pkg_verif_code_field.get("packageVerificationCodeExcludedFiles")
             )
             return self.parse_pkg_verif_code(pkg_verif_code_field.get("packageVerificationCodeValue"))
-        else:
+        elif pkg_verif_code_field is not None:
             self.value_error("PKG_VERIF_CODE_FIELD", pkg_verif_code_field)
 
     def parse_pkg_verif_code(self, pkg_verif_code):
@@ -1329,7 +1329,7 @@ class PackageParser(BaseParser):
                 self.more_than_one_error("PKG_SINGLE_LICS")
             except OrderError:
                 self.order_error("PKG_SINGLE_LICS", "PKG_NAME")
-        else:
+        elif pkg_concluded_license is not None:
             self.value_error("PKG_SINGLE_LICS", pkg_concluded_license)
 
     def parse_pkg_license_info_from_files(self, license_info_from_files):
@@ -1359,7 +1359,7 @@ class PackageParser(BaseParser):
                         self.order_error("PKG_LIC_FRM_FILES", "PKG_NAME")
                 else:
                     self.value_error("PKG_LIC_FRM_FILES", license_info_from_file)
-        else:
+        elif license_info_from_files is not None:
             self.value_error("PKG_LIC_FRM_FILES_FIELD", license_info_from_files)
 
     def parse_pkg_attribution_text(self, pkg_attribution_texts):
@@ -1403,7 +1403,7 @@ class PackageParser(BaseParser):
                 self.more_than_one_error("PKG_DECL_LIC")
             except OrderError:
                 self.order_error("PKG_DECL_LIC", "PKG_NAME")
-        else:
+        elif pkg_declared_license is not None:
             self.value_error("PKG_DECL_LIC", pkg_declared_license)
 
     def parse_pkg_license_comment(self, pkg_license_comment):
@@ -1435,7 +1435,7 @@ class PackageParser(BaseParser):
                 self.more_than_one_error("PKG_COPYRIGHT_TEXT")
             except OrderError:
                 self.order_error("PKG_COPYRIGHT_TEXT", "PKG_NAME")
-        else:
+        elif pkg_copyright_text is not None:
             self.value_error("PKG_COPYRIGHT_TEXT", pkg_copyright_text)
 
     def parse_pkg_summary(self, pkg_summary):
@@ -1484,7 +1484,7 @@ class PackageParser(BaseParser):
                     self.parse_file(pkg_file.get("File"))
                 else:
                     self.value_error("PKG_FILE", pkg_file)
-        else:
+        elif pkg_files is not None:
             self.value_error("PKG_FILES", pkg_files)
 
     def parse_pkg_chksum(self, pkg_chksum):
diff --git a/spdx/parsers/rdf.py b/spdx/parsers/rdf.py
index 8e4b14d..289ba8d 100644
--- a/spdx/parsers/rdf.py
+++ b/spdx/parsers/rdf.py
@@ -896,7 +896,7 @@ class SnippetParser(LicenseParser):
                         lics = self.handle_lics(licenses)
                         self.builder.set_snip_concluded_license(self.doc, lics)
                     except SPDXValueError:
-                        self.value_error("SNIPPET_SINGLE_LICS", licenses)
+                        self.value_error("SNIPPET_CONCLUDED_LICENSE", licenses)
         except CardinalityError:
             self.more_than_one_error(
                 "package {0}".format(self.spdx_namespace["licenseConcluded"])
diff --git a/spdx/parsers/tagvaluebuilders.py b/spdx/parsers/tagvaluebuilders.py
index 3a4b7cf..27b4180 100644
--- a/spdx/parsers/tagvaluebuilders.py
+++ b/spdx/parsers/tagvaluebuilders.py
@@ -815,7 +815,7 @@ class PackageBuilder(object):
         self.assert_package_exists()
         if not self.package_conc_lics_set:
             self.package_conc_lics_set = True
-            if validations.validate_lics_conc(licenses):
+            if validations.validate_lics_conc(licenses, optional=True):
                 doc.packages[-1].conc_lics = licenses
                 return True
             else:
@@ -830,7 +830,7 @@ class PackageBuilder(object):
         Raise OrderError if no package previously defined.
         """
         self.assert_package_exists()
-        if validations.validate_lics_from_file(lic):
+        if validations.validate_lics_from_file(lic, optional=True):
             doc.packages[-1].licenses_from_files.append(lic)
             return True
         else:
@@ -846,7 +846,7 @@ class PackageBuilder(object):
         self.assert_package_exists()
         if not self.package_license_declared_set:
             self.package_license_declared_set = True
-            if validations.validate_lics_conc(lic):
+            if validations.validate_lics_conc(lic, optional=True):
                 doc.packages[-1].license_declared = lic
                 return True
             else:
@@ -894,7 +894,7 @@ class PackageBuilder(object):
         self.assert_package_exists()
         if not self.package_cr_text_set:
             self.package_cr_text_set = True
-            if validations.validate_pkg_cr_text(text):
+            if validations.validate_pkg_cr_text(text, optional=True):
                 if isinstance(text, str):
                     doc.packages[-1].cr_text = str_from_text(text)
                 else:
@@ -1146,7 +1146,7 @@ class FileBuilder(object):
         if self.has_package(doc) and self.has_file(doc):
             if not self.file_conc_lics_set:
                 self.file_conc_lics_set = True
-                if validations.validate_lics_conc(lic):
+                if validations.validate_lics_conc(lic, optional=True):
                     self.file(doc).conc_lics = lic
                     return True
                 else:
@@ -1197,7 +1197,7 @@ class FileBuilder(object):
         if self.has_package(doc) and self.has_file(doc):
             if not self.file_copytext_set:
                 self.file_copytext_set = True
-                if validations.validate_file_cpyright(text):
+                if validations.validate_file_cpyright(text, optional=True):
                     if isinstance(text, str):
                         self.file(doc).copyright = str_from_text(text)
                     else:
@@ -1466,7 +1466,7 @@ class SnippetBuilder(object):
         self.assert_snippet_exists()
         if not self.snippet_copyright_set:
             self.snippet_copyright_set = True
-            if validations.validate_snippet_copyright(text):
+            if validations.validate_snippet_copyright(text, optional=True):
                 if isinstance(text, str):
                     doc.snippet[-1].copyright = str_from_text(text)
                 else:
@@ -1522,7 +1522,7 @@ class SnippetBuilder(object):
         self.assert_snippet_exists()
         if not self.snippet_conc_lics_set:
             self.snippet_conc_lics_set = True
-            if validations.validate_lics_conc(conc_lics):
+            if validations.validate_lics_conc(conc_lics, optional=True):
                 doc.snippet[-1].conc_lics = conc_lics
                 return True
             else:
@@ -1536,7 +1536,7 @@ class SnippetBuilder(object):
         Raise SPDXValueError if the data is a malformed value.
         """
         self.assert_snippet_exists()
-        if validations.validate_snip_lics_info(lics_info):
+        if validations.validate_snip_lics_info(lics_info, optional=True):
             doc.snippet[-1].add_lics(lics_info)
             return True
         else:
diff --git a/spdx/snippet.py b/spdx/snippet.py
index cb250e6..166008d 100644
--- a/spdx/snippet.py
+++ b/spdx/snippet.py
@@ -71,7 +71,7 @@ class Snippet(object):
             messages.append("Snippet has no SPDX Identifier.")
 
     def validate_copyright_text(self, messages):
-        if not isinstance(
+        if self.copyright and not isinstance(
             self.copyright,
             (str, utils.NoAssert, utils.SPDXNone),
         ):
@@ -84,26 +84,25 @@ class Snippet(object):
             messages.append("Snippet has no Snippet from File SPDX Identifier.")
 
     def validate_concluded_license(self, messages):
-        if not isinstance(
-            self.conc_lics, (document.License, utils.NoAssert, utils.SPDXNone)
+        if self.conc_lics and not isinstance(
+            self.conc_lics, (utils.SPDXNone, utils.NoAssert, document.License)
         ):
             messages.append(
-                "Snippet Concluded License must be one of "
-                "document.License, utils.NoAssert or utils.SPDXNone"
+                "Snippet concluded license must be instance of "
+                "spdx.utils.SPDXNone or spdx.utils.NoAssert or "
+                "spdx.document.License"
             )
 
     def validate_licenses_in_snippet(self, messages):
-        if len(self.licenses_in_snippet) == 0:
-            messages.append("Snippet must have at least one license in file.")
-        else:
-            for lic in self.licenses_in_snippet:
-                if not isinstance(
-                    lic, (document.License, utils.NoAssert, utils.SPDXNone)
-                ):
-                    messages.append(
-                        "Licenses in Snippet must be one of "
-                        "document.License, utils.NoAssert or utils.SPDXNone"
-                    )
+        for lic in self.licenses_in_snippet:
+            if not isinstance(
+                lic, (document.License, utils.NoAssert, utils.SPDXNone)
+            ):
+                messages.append(
+                    "License in snippet must be instance of "
+                    "spdx.utils.SPDXNone or spdx.utils.NoAssert or "
+                    "spdx.document.License"
+                )
 
     def has_optional_field(self, field):
         return bool (getattr(self, field, None))
diff --git a/spdx/writers/jsonyamlxml.py b/spdx/writers/jsonyamlxml.py
index 1012e23..a78d797 100644
--- a/spdx/writers/jsonyamlxml.py
+++ b/spdx/writers/jsonyamlxml.py
@@ -134,12 +134,18 @@ class PackageWriter(BaseWriter):
                 package_object["packageVerificationCode"] = self.package_verification_code(
                     package
                 )
-            package_object["licenseInfoFromFiles"] = list(
-                map(self.license, package.licenses_from_files)
-            )
-        package_object["licenseConcluded"] = self.license(package.conc_lics)
-        package_object["licenseDeclared"] = self.license(package.license_declared)
-        package_object["copyrightText"] = package.cr_text.__str__()
+            if package.has_optional_field("licenses_from_files"):
+                package_object["licenseInfoFromFiles"] = list(
+                    map(self.license, package.licenses_from_files)
+                )
+        if package.has_optional_field("conc_lics"):
+            package_object["licenseConcluded"] = self.license(package.conc_lics)
+
+        if package.has_optional_field("license_declared"):
+            package_object["licenseDeclared"] = self.license(package.license_declared)
+
+        if package.has_optional_field("cr_text"):
+            package_object["copyrightText"] = package.cr_text.__str__()
 
         if package.has_optional_field("version"):
             package_object["versionInfo"] = package.version
@@ -236,11 +242,17 @@ class FileWriter(BaseWriter):
         file_object["fileName"] = file.name
         file_object["SPDXID"] = self.spdx_id(file.spdx_id)
         file_object["checksums"] = [self.checksum(file.chksum)]
-        file_object["licenseConcluded"] = self.license(file.conc_lics)
-        file_object["licenseInfoInFiles"] = list(
-            map(self.license, file.licenses_in_file)
-        )
-        file_object["copyrightText"] = file.copyright.__str__()
+
+        if file.has_optional_field("conc_lics"):
+            file_object["licenseConcluded"] = self.license(file.conc_lics)
+
+        if file.has_optional_field("licenses_in_file"):
+            file_object["licenseInfoInFiles"] = list(
+                map(self.license, file.licenses_in_file)
+            )
+
+        if file.has_optional_field("copyright"):
+            file_object["copyrightText"] = file.copyright.__str__()
 
         if file.has_optional_field("comment"):
             file_object["comment"] = file.comment
@@ -376,12 +388,18 @@ class SnippetWriter(BaseWriter):
         for snippet in snippets:
             snippet_object = dict()
             snippet_object["SPDXID"] = self.spdx_id(snippet.spdx_id)
-            snippet_object["copyrightText"] = snippet.copyright
             snippet_object["fileId"] = self.spdx_id(snippet.snip_from_file_spdxid)
-            snippet_object["licenseConcluded"] = self.license(snippet.conc_lics)
-            snippet_object["licenseInfoFromSnippet"] = list(
-                map(self.license, snippet.licenses_in_snippet)
-            )
+
+            if snippet.has_optional_field("copyright"):
+                snippet_object["copyrightText"] = snippet.copyright
+
+            if snippet.has_optional_field("conc_lics"):
+                snippet_object["licenseConcluded"] = self.license(snippet.conc_lics)
+
+            if snippet.has_optional_field("licenses_in_snippet"):
+                snippet_object["licenseInfoFromSnippet"] = list(
+                    map(self.license, snippet.licenses_in_snippet)
+                )
 
             if snippet.has_optional_field("name"):
                 snippet_object["name"] = snippet.name


Test patch:
diff --git a/tests/test_document.py b/tests/test_document.py
index 448baca..c06e593 100644
--- a/tests/test_document.py
+++ b/tests/test_document.py
@@ -89,19 +89,11 @@ class TestDocument(TestCase):
         pack.add_lics_from_file(lic1)
         messages = ErrorMessages()
         messages = doc.validate(messages)
-        expected = [
-            'Sample_Document-V2.1: Creation info missing created date.',
-            'Sample_Document-V2.1: No creators defined, must have at least one.',
-            'Sample_Document-V2.1: some/path: Package checksum must be instance of '
-            'spdx.checksum.Algorithm',
-            'Sample_Document-V2.1: some/path: Package concluded license must be instance '
-            'of spdx.utils.SPDXNone or spdx.utils.NoAssert or spdx.document.License',
-            'Sample_Document-V2.1: some/path: Package cr_text can not be None.',
-            'Sample_Document-V2.1: some/path: Package declared license must be instance '
-            'of spdx.utils.SPDXNone or spdx.utils.NoAssert or spdx.document.License',
-            'Sample_Document-V2.1: some/path: Package download_location can not be None.',
-            'Sample_Document-V2.1: some/path: Package must have at least one file.',
-        ]
+        expected = ['Sample_Document-V2.1: Creation info missing created date.',
+                    'Sample_Document-V2.1: No creators defined, must have at least one.',
+                    'Sample_Document-V2.1: some/path: Package checksum must be instance of '
+                    'spdx.checksum.Algorithm',
+                    'Sample_Document-V2.1: some/path: Package download_location can not be None.']
         assert sorted(expected) == sorted(messages)
 
     def test_document_is_valid_when_using_or_later_licenses(self):