propertyOption for single and multi properties (#161)

* propertyOption for MultiPropertyKeys

* propertyOption for single and multi properties

also: fix nodePropertyOptionCompat - there were slight differences to
odb, see https://github.com/ShiftLeftSecurity/overflowdb/blob/1b5df058569a3e87192f2dce4e019e3b4a0c1d91/core-tests/src/test/java/overflowdb/ElementTest.java#L54-L67

* fmt

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

joernio/joern#4382

core/src/main/scala/flatgraph/Accessors.scala

@@ -144,11 +144,10 @@ object Accessors {
     val schema   = graph.schema
     val seq      = node.seq()
     val nodeKind = node.nodeKind.toInt
-    val res      = getNodePropertyMulti[Object](graph, nodeKind, propertyKind, seq)
     schema.getNodePropertyFormalQuantity(nodeKind, propertyKind) match {
       case FormalQtyType.QtyNone                          => None
       case FormalQtyType.QtyOne | FormalQtyType.QtyOption => getNodePropertyOption[Object](graph, nodeKind, propertyKind, seq)
-      case FormalQtyType.QtyMulti                         => Some(getNodePropertyMulti[Object](graph, nodeKind, propertyKind, seq))
+      case FormalQtyType.QtyMulti                         => Option(getNodePropertyMulti[Object](graph, nodeKind, propertyKind, seq))
     }
   }
 

core/src/main/scala/flatgraph/traversal/Language.scala

@@ -435,14 +435,31 @@ class NodeMethods(node: GNode) extends AnyVal {
   // the "property" accessors have somewhat special behavior. They don't throw if the property is not present,
   // and they distinguish whether the property formally exists on the node-type as multi-valued thing.
   // the static info from the propertyKey is ignored.
-  // this semantics may or may not be desireable -- but it is what odbv1 does, and these are compat anyway.
+  // this semantics may or may not be desirable -- but it is what odbv1 does, and these are compat anyway.
   // we don't really want to specialize here -- otherwise we can get nonsense like `null.asInstanceOf[Double]`
-  def property[ValueType, CompleteType](propertyKey: PropertyKey[ValueType, CompleteType]): CompleteType = {
-    Accessors.getNodePropertyOptionCompat(node, propertyKey.kind).orNull.asInstanceOf[CompleteType]
-  }
+
+  def property[ValueType](propertyKey: SinglePropertyKey[ValueType]): ValueType =
+    Accessors.getNodePropertySingle(node.graph, node.nodeKind, propertyKey.kind, node.seq(), propertyKey.default)
+
+  def property[ValueType](propertyKey: OptionalPropertyKey[ValueType]): Option[ValueType] =
+    Accessors.getNodePropertyOption(node.graph, node.nodeKind, propertyKey.kind, node.seq())
+
+  def property[ValueType](propertyKey: MultiPropertyKey[ValueType]): IndexedSeq[ValueType] =
+    Accessors.getNodePropertyMulti(node.graph, node.nodeKind, propertyKey.kind, node.seq())
+
   def propertyOption[ValueType](propertyKey: SinglePropertyKey[ValueType]): Option[ValueType] =
+    Accessors
+      .getNodePropertyOption(node.graph, node.nodeKind, propertyKey.kind, node.seq())
+      .orElse(Option(propertyKey.default))
+
+  def propertyOption[ValueType](propertyKey: OptionalPropertyKey[ValueType]): Option[ValueType] =
     Accessors.getNodePropertyOption(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 {
       case Schema.UndefinedKind =>
@@ -554,11 +571,11 @@ class NodeSteps[A <: GNode](traversal: Iterator[A]) extends AnyVal {
     }
 
   def property[@specialized ValueType](propertyKey: SinglePropertyKey[ValueType]): Iterator[ValueType] =
-    traversal.map(_.property[ValueType, ValueType](propertyKey))
+    traversal.map(_.property(propertyKey))
 
   def property[@specialized ValueType](propertyKey: OptionalPropertyKey[ValueType]): Iterator[ValueType] =
-    traversal.flatMap(_.property[ValueType, Option[ValueType]](propertyKey))
+    traversal.flatMap(_.property(propertyKey))
 
   def property[@specialized ValueType](propertyKey: MultiPropertyKey[ValueType]): Iterator[ValueType] =
-    traversal.flatMap(_.property[ValueType, IndexedSeq[ValueType]](propertyKey))
+    traversal.flatMap(_.property(propertyKey))
 }

core/src/test/scala/flatgraph/GraphTests.scala

@@ -655,6 +655,7 @@ class GraphTests extends AnyWordSpec with Matchers {
         |Node kind 0. (eid, nEdgesOut, nEdgesIn):
         |Node kind 1. (eid, nEdgesOut, nEdgesIn):
         |""".stripMargin
+
     DiffGraphApplier.applyDiff(
       g,
       new DiffGraphBuilder(schema)
@@ -711,7 +712,6 @@ class GraphTests extends AnyWordSpec with Matchers {
         ._setNodeProperty(V1_1.storedRef.get, 0, 6.toShort :: Nil)
         ._setNodeProperty(V0_1.storedRef.get, 0, 6.toShort :: Nil)
     )
-    println(debugDump(g))
     debugDump(g) shouldBe
       """#Node numbers (kindId, nnodes) (0: 3), (1: 2), total 5
         |Node kind 0. (eid, nEdgesOut, nEdgesIn):
@@ -720,14 +720,80 @@ class GraphTests extends AnyWordSpec with Matchers {
         |Node kind 1. (eid, nEdgesOut, nEdgesIn):
         |   V1_1       : 0: [6]
         |""".stripMargin
-    // lets check the legacy accessors. Optional property
-    Accessors.getNodePropertyOptionCompat(V0_0.storedRef.get, 0) shouldBe None
-    Accessors.getNodePropertyOptionCompat(V0_1.storedRef.get, 0) shouldBe Some(6.toShort)
-    // lets check the legacy accessors. Multi property
-    Accessors.getNodePropertyOptionCompat(V0_1.storedRef.get, 1) shouldBe Some(IndexedSeq(null))
-    Accessors.getNodePropertyOptionCompat(V1_1.storedRef.get, 0) shouldBe Some(IndexedSeq(6.toShort))
-    Accessors.getNodePropertyOptionCompat(V1_1.storedRef.get, 1) shouldBe Some(IndexedSeq())
+  }
+
+  "node property accessors" in {
+    val schema = TestSchema.make(
+      nodeKinds = 1,
+      edgeKinds = 0,
+      properties = 3,
+      nodePropertyPrototypes = Array(new Array[String](0), new Array[String](0), new Array[String](0)),
+      formalQtys = Array(FormalQtyType.QtyOne, null, FormalQtyType.QtyOption, null, FormalQtyType.QtyMulti)
+    )
+
+    // just to verify the free schema setup
+    schema.getNodePropertyFormalQuantity(0, 0) shouldBe FormalQtyType.QtyOne
+    schema.getNodePropertyFormalQuantity(0, 1) shouldBe FormalQtyType.QtyOption
+    schema.getNodePropertyFormalQuantity(0, 2) shouldBe FormalQtyType.QtyMulti
+
+    val g = new Graph(schema)
+    val v0 = {
+      val diffGraph = new DiffGraphBuilder(schema)
+      val dnode     = new GenericDNode(0)
+      diffGraph.addNode(dnode)
+      DiffGraphApplier.applyDiff(g, diffGraph)
+      dnode.storedRef.get
+    }
 
+    // PropertyKey accessors - these are normally generated by the DomainClassesGenerator based on the schema
+    val propertySingle   = new SinglePropertyKey[String](kind = 0, name = "propertySingle", default = "propertySingleDefault")
+    val propertyOptional = new OptionalPropertyKey[String](kind = 1, name = "propertyOptional")
+    val propertyMulti    = new MultiPropertyKey[String](kind = 2, name = "propertyMulti")
+
+    // test cases where the property is not defined
+    v0.property(propertySingle) shouldBe "propertySingleDefault"
+    v0.propertyOption(propertySingle) shouldBe Some("propertySingleDefault")
+    v0.property(propertyOptional) shouldBe None
+    v0.propertyOption(propertyOptional) shouldBe None
+    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/joernio/joern/pull/4382
+    Accessors.getNodePropertyOptionCompat(v0, 0) shouldBe None
+    Accessors.getNodePropertyOptionCompat(v0, 1) shouldBe None
+    Accessors.getNodePropertyOptionCompat(v0, 2) shouldBe Some(Seq.empty)
+
+    DiffGraphApplier.applyDiff(
+      g,
+      new DiffGraphBuilder(schema)
+        ._setNodeProperty(v0, 0, "a")           // qty.one
+        ._setNodeProperty(v0, 1, "b")           // qty.option
+        ._setNodeProperty(v0, 2, Seq("c", "d")) // qty.multi
+    )
+
+    debugDump(g) shouldBe
+      """#Node numbers (kindId, nnodes) (0: 1), total 1
+        |Node kind 0. (eid, nEdgesOut, nEdgesIn):
+        |   V0_0       : 0: [a], 1: [b], 2: [c, d]
+        |""".stripMargin
+
+    // test cases where the property is defined
+    v0.property(propertySingle) shouldBe "a"
+    v0.propertyOption(propertySingle) shouldBe Some("a")
+    v0.property(propertyOptional) shouldBe Some("b")
+    v0.propertyOption(propertyOptional) shouldBe Some("b")
+    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/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"))
   }
 
   "Support custom domain classes for detached nodes" in {
@@ -817,7 +883,6 @@ class GraphTests extends AnyWordSpec with Matchers {
         ._setNodeProperty(v2.storedRef.get, 0, "p2")
     )
     g.inverseIndices.get(0) shouldBe null
-    // println(debugDump.debugDump(g))
   }
 
   "write to storage on close and read from storage on open" in {