fix(spdx-utils): Do not test for sub-expressions based on strings

The string-approach fails for compound expressions whose second operand ends with an exception (the `WITH` operator binds stronger than `AND` / `OR`). As the string comparison was meant as a performance optimization any anyway, just remove it. In exchange, a special case for the `AND` operator is required, for which the `containsAll()` check with valid choices is be too strict: All valid choices for an `AND` expression would contain the `AND` itself. However, sub-expressions should be allowed to also match only either side of the `AND`. Signed-off-by: Sebastian Schuberth <[email protected]>
oss-review-toolkit · Feb 14, 2024 · a1cb37e · a1cb37e
1 parent 56cfa0d
commit a1cb37e
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 3 deletions.
diff --git a/utils/spdx/src/main/kotlin/SpdxExpression.kt b/utils/spdx/src/main/kotlin/SpdxExpression.kt
@@ -318,10 +318,11 @@ class SpdxCompoundExpression(
     override fun isSubExpression(subExpression: SpdxExpression?): Boolean {
         if (subExpression == null) return false
 
-        val expressionString = toString()
-        val subExpressionString = subExpression.toString()
+        if (operator == SpdxOperator.AND) {
+            if (left.isSubExpression(subExpression) || right.isSubExpression(subExpression)) return true
+        }
 
-        return subExpressionString in expressionString || validChoices().containsAll(subExpression.validChoices())
+        return validChoices().containsAll(subExpression.validChoices())
     }
 
     override fun equals(other: Any?): Boolean {

diff --git a/utils/spdx/src/test/kotlin/SpdxExpressionTest.kt b/utils/spdx/src/test/kotlin/SpdxExpressionTest.kt
@@ -657,6 +657,21 @@ class SpdxExpressionTest : WordSpec({
 
             mit.isSubExpression(mit) shouldBe true
         }
+
+        "work correctly for compound expressions with exceptions" {
+            val gplWithException = "CDDL-1.1 OR GPL-2.0-only WITH Classpath-exception-2.0".toSpdx()
+            val gpl = "CDDL-1.1 OR GPL-2.0-only".toSpdx()
+
+            gplWithException.isSubExpression(gpl) shouldBe false
+        }
+
+        "work correctly for nested compound expressions" {
+            val expression = "(CDDL-1.1 OR GPL-2.0-only) AND (CDDL-1.1 OR GPL-2.0-only WITH Classpath-exception-2.0)"
+                .toSpdx()
+            val subExpression = "CDDL-1.1 OR GPL-2.0-only".toSpdx()
+
+            expression.isSubExpression(subExpression) shouldBe true
+        }
     }
 
     "applyChoice()" should {