fix(spdx-utils): Correctly determine choices for AND expressions

The original code was clearly wrong in that it recursively decomposed an expression and composed the contained expressions always with `AND` although there could be nested `OR` expressions. Instead, if there is a top-level `AND` in an expressions, create all combinations of choices on the left and choices on the right to get the total valid choices. Reword affected tests according to the fact that returned choices are not simplified, but explicit, though still deduplicated. Commonly introduce a `choice` variable along the way to make tests for long license expressions more readable. Fixes #8082. Signed-off-by: Sebastian Schuberth <[email protected]>
oss-review-toolkit · Jan 17, 2024 · 6ad4d0a · 6ad4d0a
1 parent ec58923
commit 6ad4d0a
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 13 deletions.
diff --git a/utils/spdx/src/main/kotlin/SpdxExpression.kt b/utils/spdx/src/main/kotlin/SpdxExpression.kt
@@ -314,7 +314,18 @@ class SpdxCompoundExpression(
 
     override fun validChoicesForDnf(): Set<SpdxExpression> =
         when (operator) {
-            SpdxOperator.AND -> setOf(decompose().reduce(SpdxExpression::and))
+            SpdxOperator.AND -> {
+                val leftChoices = left.validChoicesForDnf()
+                val rightChoices = right.validChoicesForDnf()
+
+                // Cartesian product of choices on the left and right.
+                leftChoices.flatMapTo(mutableSetOf()) { leftChoice ->
+                    rightChoices.map { rightChoice ->
+                        leftChoice and rightChoice
+                    }
+                }
+            }
+
             SpdxOperator.OR -> left.validChoicesForDnf() + right.validChoicesForDnf()
         }
 

diff --git a/utils/spdx/src/test/kotlin/SpdxExpressionTest.kt b/utils/spdx/src/test/kotlin/SpdxExpressionTest.kt
@@ -422,29 +422,38 @@ class SpdxExpressionTest : WordSpec({
     }
 
     "validChoices()" should {
-        "list the valid choices for a complex expression" {
-            "(a OR b) AND c AND (d OR e)".toSpdx().validChoices().map { it.toString() } should containExactlyInAnyOrder(
+        "return the valid choices for a complex expression" {
+            val choices = "(a OR b) AND c AND (d OR e)".toSpdx().validChoices()
+
+            choices.map { it.toString() } should containExactlyInAnyOrder(
                 "a AND c AND d",
                 "a AND c AND e",
                 "b AND c AND d",
                 "b AND c AND e"
             )
         }
 
-        "not contain a duplicate valid choice for a simple expression" {
-            "a AND a".toSpdx().validChoices().map { it.toString() } should containExactly("a")
-        }
+        "return the valid choices for a nested expression" {
+            val choices = "(a OR (b AND (c OR d))) AND (e OR f)".toSpdx().validChoices()
 
-        "not contain duplicate valid choice for a complex expression" {
-            "(a OR b) AND (a OR b)".toSpdx().validChoices().map { it.toString() } should containExactlyInAnyOrder(
-                "a",
-                "b",
-                "a AND b"
+            choices.map { it.toString() } should containExactlyInAnyOrder(
+                "a AND e",
+                "a AND f",
+                "b AND c AND e",
+                "b AND c AND f",
+                "b AND d AND e",
+                "b AND d AND f"
             )
         }
 
-        "not contain duplicate valid choice different left and right expressions" {
-            "a AND a AND b".toSpdx().validChoices().map { it.toString() } should containExactly("a AND b")
+        "be explicit about the choices even if they could be simplified" {
+            val choices = "(a OR b) AND (a OR b)".toSpdx().validChoices()
+
+            choices.map { it.toString() } should containExactlyInAnyOrder(
+                "a AND a",
+                "a AND b",
+                "b AND b"
+            )
         }
     }