[issue-386] Finish Package validation

src/spdx/validation/package_validator.py

@@ -13,7 +13,8 @@
 
 from spdx.model.document import Document
 from spdx.model.package import Package
-from spdx.model.relationship import RelationshipType
+from spdx.model.relationship import RelationshipType, Relationship
+from spdx.model.relationship_filters import filter_by_type_and_origin, filter_by_type_and_target
 from spdx.validation.checksum_validator import validate_checksums
 from spdx.validation.external_package_ref_validator import validate_external_package_refs
 from spdx.validation.license_expression_validator import validate_license_expression, validate_license_expressions
@@ -43,23 +44,18 @@ def validate_package_within_document(package: Package, document: Document) -> Li
     for message in validate_spdx_id(package.spdx_id, document):
         validation_messages.append(ValidationMessage(message, context))
 
-    # TODO: make test for this (https://github.com/spdx/tools-python/issues/386)
     if not package.files_analyzed:
-        package_contains_relationships = [relationship for relationship in document.relationships if
-                                          relationship.relationship_type == RelationshipType.CONTAINS and relationship.spdx_element_id == package.spdx_id]
-        if package_contains_relationships:
-            validation_messages.append(
-                ValidationMessage(
-                    f"package must contain no elements if files_analyzed is False, but found {package_contains_relationships}",
-                    context)
-            )
+        package_contains_relationships = filter_by_type_and_origin(document.relationships, RelationshipType.CONTAINS,
+                                                                   package.spdx_id)
+        contained_in_package_relationships = filter_by_type_and_target(document.relationships,
+                                                                       RelationshipType.CONTAINED_BY, package.spdx_id)
+
+        combined_relationships: List[Relationship] = package_contains_relationships + contained_in_package_relationships
 
-        contained_in_package_relationships = [relationship for relationship in document.relationships if
-                                              relationship.relationship_type == RelationshipType.CONTAINED_BY and relationship.related_spdx_element_id == package.spdx_id]
-        if contained_in_package_relationships:
+        if combined_relationships:
             validation_messages.append(
                 ValidationMessage(
-                    f"package must contain no elements if files_analyzed is False, but found {contained_in_package_relationships}",
+                    f"package must contain no elements if files_analyzed is False, but found {combined_relationships}",
                     context)
             )
 
@@ -83,7 +79,6 @@ def validate_package(package: Package, context: Optional[ValidationContext] = No
         for message in validate_url(homepage):
             validation_messages.append(ValidationMessage("homepage " + message, context))
 
-    # TODO: is verification_code required if files_analyzed=True? (https://github.com/spdx/tools-python/issues/386)
     verification_code = package.verification_code
     if verification_code:
         if not package.files_analyzed:

tests/spdx/validation/test_package_validator.py

@@ -14,11 +14,12 @@
 import pytest
 from license_expression import Licensing
 
+from spdx.model.relationship import Relationship, RelationshipType
 from spdx.model.spdx_no_assertion import SpdxNoAssertion
 from spdx.model.spdx_none import SpdxNone
 from spdx.validation.package_validator import validate_package_within_document
 from spdx.validation.validation_message import ValidationMessage, ValidationContext, SpdxElementType
-from tests.spdx.fixtures import package_fixture, package_verification_code_fixture, document_fixture
+from tests.spdx.fixtures import package_fixture, package_verification_code_fixture, document_fixture, file_fixture
 
 
 def test_valid_package():
@@ -42,7 +43,7 @@ def test_valid_package():
                                            license_info_from_files=[Licensing().parse("some_license")],
                                            verification_code=None),
                            "license_info_from_files must be None if files_analyzed is False, but is: [LicenseSymbol('some_license', "
-                                      "is_exception=False)]")
+                           "is_exception=False)]")
                           ])
 def test_invalid_package(package_input, expected_message):
     validation_messages: List[ValidationMessage] = validate_package_within_document(package_input,
@@ -54,3 +55,25 @@ def test_invalid_package(package_input, expected_message):
                                                    full_element=package_input))
 
     assert validation_messages == [expected]
+
+
+@pytest.mark.parametrize("relationships",
+                         [[Relationship("SPDXRef-Package", RelationshipType.CONTAINS, "SPDXRef-File1")],
+                          [Relationship("SPDXRef-Package", RelationshipType.CONTAINS, "DocumentRef-external:SPDXRef-File")],
+                          [Relationship("SPDXRef-File2", RelationshipType.CONTAINED_BY, "SPDXRef-Package")],
+                          [Relationship("DocumentRef-external:SPDXRef-File", RelationshipType.CONTAINED_BY, "SPDXRef-Package")],
+                          [Relationship("SPDXRef-Package", RelationshipType.CONTAINS, "SPDXRef-File2"),
+                           Relationship("SPDXRef-File1", RelationshipType.CONTAINED_BY, "SPDXRef-Package")]])
+def test_invalid_package_with_contains(relationships):
+    document = document_fixture(relationships=relationships,
+                                files=[file_fixture(spdx_id="SPDXRef-File1"), file_fixture(spdx_id="SPDXRef-File2")])
+    package = package_fixture(files_analyzed=False, verification_code=None, license_info_from_files=[])
+    context = ValidationContext(spdx_id=package.spdx_id, parent_id=document.creation_info.spdx_id,
+                                element_type=SpdxElementType.PACKAGE,
+                                full_element=package)
+
+    validation_messages: List[ValidationMessage] = validate_package_within_document(package, document)
+
+    assert validation_messages == [
+        ValidationMessage(f"package must contain no elements if files_analyzed is False, but found {relationships}",
+                          context)]