update behaviour for undefined multi property based on joern's behaviour

joernio/joern#4382
joernio · Mar 22, 2024 · 293d717 · 293d717
1 parent 72c2b09
commit 293d717
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 14 deletions.
diff --git a/core/src/main/scala/flatgraph/Accessors.scala b/core/src/main/scala/flatgraph/Accessors.scala
@@ -147,9 +147,7 @@ object Accessors {
     schema.getNodePropertyFormalQuantity(nodeKind, propertyKind) match {
       case FormalQtyType.QtyNone                          => None
       case FormalQtyType.QtyOne | FormalQtyType.QtyOption => getNodePropertyOption[Object](graph, nodeKind, propertyKind, seq)
-      case FormalQtyType.QtyMulti =>
-        val multiPropertyValue = getNodePropertyMulti[Object](graph, nodeKind, propertyKind, seq)
-        Option(multiPropertyValue).filter(_.nonEmpty)
+      case FormalQtyType.QtyMulti                         => Option(getNodePropertyMulti[Object](graph, nodeKind, propertyKind, seq))
     }
   }
 

diff --git a/core/src/main/scala/flatgraph/traversal/Language.scala b/core/src/main/scala/flatgraph/traversal/Language.scala
@@ -455,8 +455,10 @@ class NodeMethods(node: GNode) extends AnyVal {
   def propertyOption[ValueType](propertyKey: OptionalPropertyKey[ValueType]): Option[ValueType] =
     Accessors.getNodePropertyOption(node.graph, node.nodeKind, propertyKey.kind, node.seq())
 
-  def propertyOption[ValueType](propertyKey: MultiPropertyKey[ValueType]): IndexedSeq[ValueType] =
-    Accessors.getNodePropertyMulti(node.graph, node.nodeKind, propertyKey.kind, node.seq())
+  // TODO this should rather return `None` for an undefined property, rather than `Some(Seq.empty)`, but we only want
+  //  to make that change after joern's transition to flatgraph - see https://github.com/joernio/joern/pull/4382
+  def propertyOption[ValueType](propertyKey: MultiPropertyKey[ValueType]): Option[IndexedSeq[ValueType]] =
+    Option(Accessors.getNodePropertyMulti(node.graph, node.nodeKind, propertyKey.kind, node.seq()))
 
   def propertyOption[ValueType](name: String): Option[ValueType] = {
     node.graph.schema.getPropertyKindByName(name) match {

diff --git a/core/src/test/scala/flatgraph/GraphTests.scala b/core/src/test/scala/flatgraph/GraphTests.scala
@@ -752,17 +752,20 @@ class GraphTests extends AnyWordSpec with Matchers {
 
     // test cases where the property is not defined
     v0.property(propertySingle) shouldBe "propertySingleDefault"
-    v0.property(propertyOptional) shouldBe None
-    v0.property(propertyMulti) shouldBe Seq.empty
     v0.propertyOption(propertySingle) shouldBe Some("propertySingleDefault")
+    v0.property(propertyOptional) shouldBe None
     v0.propertyOption(propertyOptional) shouldBe None
-    v0.propertyOption(propertyMulti) shouldBe Seq.empty
+    v0.property(propertyMulti) shouldBe Seq.empty
+    // TODO this should rather return `None` for an undefined property, rather than `Some(Seq.empty)`, but we only want
+    //  to make that change after joern's transition to flatgraph - see https://github.com/joernio/joern/pull/4382
+//    v0.propertyOption(propertyMulti) shouldBe None
+    v0.propertyOption(propertyMulti) shouldBe Some(Seq.empty[String])
 
     // ensure odb compat accessors work just like before
-    // see https://github.com/ShiftLeftSecurity/overflowdb/blob/1b5df058569a3e87192f2dce4e019e3b4a0c1d91/core-tests/src/test/java/overflowdb/ElementTest.java#L54-L67
+    // see https://github.com/joernio/joern/pull/4382
     Accessors.getNodePropertyOptionCompat(v0, 0) shouldBe None
     Accessors.getNodePropertyOptionCompat(v0, 1) shouldBe None
-    Accessors.getNodePropertyOptionCompat(v0, 2) shouldBe None
+    Accessors.getNodePropertyOptionCompat(v0, 2) shouldBe Some(Seq.empty)
 
     DiffGraphApplier.applyDiff(
       g,
@@ -780,14 +783,14 @@ class GraphTests extends AnyWordSpec with Matchers {
 
     // test cases where the property is defined
     v0.property(propertySingle) shouldBe "a"
-    v0.property(propertyOptional) shouldBe Some("b")
-    v0.property(propertyMulti) shouldBe Seq("c", "d")
     v0.propertyOption(propertySingle) shouldBe Some("a")
+    v0.property(propertyOptional) shouldBe Some("b")
     v0.propertyOption(propertyOptional) shouldBe Some("b")
-    v0.propertyOption(propertyMulti) shouldBe Seq("c", "d")
+    v0.property(propertyMulti) shouldBe Seq("c", "d")
+    v0.propertyOption(propertyMulti) shouldBe Some(Seq("c", "d"))
 
     // ensure odb compat accessors work just like before
-    // see https://github.com/ShiftLeftSecurity/overflowdb/blob/1b5df058569a3e87192f2dce4e019e3b4a0c1d91/core-tests/src/test/java/overflowdb/ElementTest.java#L54-L67
+    // see https://github.com/joernio/joern/pull/4382
     Accessors.getNodePropertyOptionCompat(v0, 0) shouldBe Some("a")
     Accessors.getNodePropertyOptionCompat(v0, 1) shouldBe Some("b")
     Accessors.getNodePropertyOptionCompat(v0, 2) shouldBe Some(Seq("c", "d"))