Repository: alexklibisz/elastiknn (language: python)

Issue:
Bug: decoders do not tolerate extra keys, fails with "Unexpected name [blah]"
**Describe the bug**

Given a vector JSON with some additional irrelevant keys:

`{ "values": [1,2,3], "extra": "junk" }`

The `XContentCodec` will parse the JSON, but will fail to decode the vector:

```
  org.elasticsearch.xcontent.XContentParseException: Unexpected name [extra]
```

This is also true for other JSON, e.g., mappings.

**Expected behavior**

The decoder should ignore the extra keys and proceed to decode the JSON.

**Environment (please complete the following information):**

 - Elastiknn version: [e.g. 7.17.0.0]

**To Reproduce**

This test case can be added to the XContentCodecSuite and will fail:

```scala
"fails to decode vector with extra keys in it" in {
    val vec = Vec.DenseFloat.random(10)
    val string = s"""{ "values": ${vec.values.asJson.noSpaces}, "extra": "junk" """
    val result = decodeUnsafeFromString[Vec](string)
    result shouldBe vec
  }
```

**Additional context**
Add any other context about the problem here.



Discussion / hints:
Just noting this for others. I'll try to fix sometime next week. Such is life with hand-coded JSON parsing.

Fix patch:
diff --git a/Taskfile.jvm.yml b/Taskfile.jvm.yml
index 5199e3d67..450c1414f 100644
--- a/Taskfile.jvm.yml
+++ b/Taskfile.jvm.yml
@@ -65,11 +65,8 @@ tasks:
     desc: Publish a snapshot release to Github
     cmds:
       - "{{ .CMD_GRADLE }} assemble"
-      - echo {{ .VERSION }} > release.md
-      - echo "" >> release.md
-      - "git log $(git tag | grep -v SNAPSHOT | tail -n1)...HEAD --format='- %h: %s' >> release.md"
       - hub release delete {{ .VERSION }} || true
-      - hub release create -p -F release.md -a elastiknn-plugin/build/distributions/elastiknn-*.zip {{ .VERSION }}
+      - hub release create -p -a elastiknn-plugin/build/distributions/elastiknn-*.zip {{ .VERSION }}
 
   plugin:publish:release:
     desc: Publish an official release to Github
@@ -77,10 +74,7 @@ tasks:
       - curl -f -s -o /dev/null {{ .GITHUB_URL }}/releases/tag/{{ .VERSION }}
     cmds:
       - "{{ .CMD_GRADLE }} assemble"
-      - echo {{ .VERSION }} > release.md
-      - echo "" >> release.md
-      - "git log $(git tag | grep -v SNAPSHOT | tail -n1)...HEAD --format='- %h: %s' >> release.md"
-      - hub release create -p -F release.md -a elastiknn-plugin/build/distributions/elastiknn-*.zip {{ .VERSION }}
+      - hub release create -p -a elastiknn-plugin/build/distributions/elastiknn-*.zip {{ .VERSION }}
 
   run:gradle:
     desc: Run Elasticsearch using gradle
diff --git a/elastiknn-api4s/src/main/scala/com/klibisz/elastiknn/api/XContentCodec.scala b/elastiknn-api4s/src/main/scala/com/klibisz/elastiknn/api/XContentCodec.scala
index 4b9976198..2ef930900 100644
--- a/elastiknn-api4s/src/main/scala/com/klibisz/elastiknn/api/XContentCodec.scala
+++ b/elastiknn-api4s/src/main/scala/com/klibisz/elastiknn/api/XContentCodec.scala
@@ -349,9 +349,6 @@ object XContentCodec {
       override def compare(x: Token, y: Token): Int = x.name().compareTo(y.name())
     }
 
-    private def unexpectedName(name: String): String =
-      s"Unexpected name [$name]"
-
     private def unexpectedValue(text: String, expected: SortedSet[String]): String =
       s"Expected token to be one of [${expected.mkString(",")}] but found [$text]"
 
@@ -426,6 +423,7 @@ object XContentCodec {
     }
 
     implicit val vec: Decoder[Vec] = new Decoder[Vec] {
+
       override def decodeUnsafe(p: XContentParser): Vec = {
         var field: Option[String] = None
         var id: Option[String] = None
@@ -460,7 +458,7 @@ object XContentCodec {
                 case n @ Names.VALUES =>
                   assertToken(n, p.nextToken(), START_ARRAY)
                   values = Some(parseFloatArray(p, 42))
-                case n => throw new XContentParseException(unexpectedName(n))
+                case _ => p.nextToken()
               }
             }
           case START_ARRAY =>
@@ -489,7 +487,8 @@ object XContentCodec {
               Vec.SparseBool(trueIndices, totalIndices)
             case (_, _, _, _, _, Some(values)) =>
               Vec.DenseFloat(values)
-            case _ => throw new XContentParseException(unableToConstruct("vector"))
+            case _ =>
+              throw new XContentParseException(unableToConstruct("vector"))
           }
       }
     }
@@ -561,10 +560,10 @@ object XContentCodec {
                   assertToken(n, p.nextToken(), VALUE_BOOLEAN)
                   repeating = Some(p.booleanValue())
                 case Names.SIMILARITY => similarity = Some(Decoder.similarity.decodeUnsafe(p))
-                case n                => throw new XContentParseException(unexpectedName(n))
+                case _                => p.nextToken()
               }
             }
-          case n => throw new XContentParseException(unexpectedName(n))
+          case _ => p.nextToken()
         }
       }
       (typ, model, dims, similarity, l, k, w, repeating) match {
@@ -614,7 +613,7 @@ object XContentCodec {
               probes = Some(p.intValue())
             case Names.SIMILARITY => similarity = Some(decodeUnsafe[Similarity](p))
             case Names.VEC        => vec = Some(decodeUnsafe[Vec](p))
-            case n                => throw new XContentParseException(unexpectedName(n))
+            case _                => p.nextToken()
           }
         }
         (candidates, field, model, probes, similarity, vec) match {


Test patch:
diff --git a/elastiknn-testing/src/test/scala/com/klibisz/elastiknn/api/XContentCodecSuite.scala b/elastiknn-testing/src/test/scala/com/klibisz/elastiknn/api/XContentCodecSuite.scala
index 8c7e875a9..75600d542 100644
--- a/elastiknn-testing/src/test/scala/com/klibisz/elastiknn/api/XContentCodecSuite.scala
+++ b/elastiknn-testing/src/test/scala/com/klibisz/elastiknn/api/XContentCodecSuite.scala
@@ -18,16 +18,27 @@ class XContentCodecSuite extends AnyFreeSpec with Matchers {
 
   private implicit val rng: Random = new Random(0)
 
-  private def shuffle(j: Json): Json = j.asObject match {
+  private def shuffle(original: Json): Json = original.asObject match {
     case Some(obj) =>
       val entries = obj.toIterable
-      val shuffledValues = entries.map { case (k, v) => (k, shuffle(v)) }
-      val shuffledKeys = rng.shuffle(shuffledValues).toList
-      Json.obj(shuffledKeys: _*)
-    case None => j
+      val shuffledChildren = entries.map { case (k, v) => (k, shuffle(v)) }
+      val shuffledThisLevel = rng.shuffle(shuffledChildren).toList
+      val shuffled = Json.obj(shuffledThisLevel: _*)
+      shuffled
+    case None => original
   }
 
-  private def randomize(j: Json): String = {
+  private def addExtraKeys(original: Json): Json = original.asObject match {
+    case Some(_) =>
+      val junk = Json.obj(
+        "extra_int" -> Json.fromInt(rng.nextInt()),
+        "extra_string" -> Json.fromString(rng.nextString(rng.nextInt(10)))
+      )
+      original.deepMerge(junk)
+    case None => original
+  }
+
+  private def toString(j: Json): String = {
     val d = rng.nextFloat()
     if (d < 0.2) shuffle(j).noSpaces
     else if (d < 0.4) shuffle(j).spaces2
@@ -36,39 +47,66 @@ class XContentCodecSuite extends AnyFreeSpec with Matchers {
     else shuffle(j).spaces4SortKeys
   }
 
-  private def roundtrip[T: XContentCodec.Encoder: XContentCodec.Decoder: ElasticsearchCodec](expected: Json, t: T): Assertion = {
+  private def randomize(j: Json, addExtraKeys: Boolean): String =
+    if (addExtraKeys) toString(shuffle(this.addExtraKeys(j))) else toString(shuffle(j))
+
+  private def roundtrip[T: XContentCodec.Encoder: XContentCodec.Decoder: ElasticsearchCodec](
+      expectedJson: Json,
+      expectedObject: T,
+      addExtraKeys: Boolean = true
+  ): Assertion = {
     // Encode with XContent and check that it matches the expected JSON w/ sorted keys and no spaces.
-    val encodedXContent: String = XContentCodec.encodeUnsafeToString(t)
-    encodedXContent shouldBe expected.noSpacesSortKeys
+    val encodedXContent: String = XContentCodec.encodeUnsafeToString(expectedObject)
+    encodedXContent shouldBe expectedJson.noSpacesSortKeys
     // Encode with Circe.
-    val encodedCirce: Json = ElasticsearchCodec.encode(t)
+    val encodedCirce: Json = ElasticsearchCodec.encode(expectedObject)
     // Test roundtrip between the two codecs.
     // Encode -> Decode
     // Circe -> Circe
     val decodedCirceCirce = ElasticsearchCodec.decodeJsonGet[T](encodedCirce)
-    decodedCirceCirce shouldBe t
+    decodedCirceCirce shouldBe expectedObject
     // Circe -> XContent
     val decodedCirceXContent = XContentCodec.decodeUnsafeFromString(encodedCirce.noSpaces)
-    decodedCirceXContent shouldBe t
+    decodedCirceXContent shouldBe expectedObject
     // XContent -> Circe
     val decodedXContentCirce = ElasticsearchCodec.decodeJsonGet[T](ElasticsearchCodec.parseGet(encodedXContent))
-    decodedXContentCirce shouldBe t
+    decodedXContentCirce shouldBe expectedObject
     // XContent -> XContent
-    val decodedXContentXContent = XContentCodec.decodeUnsafeFromString(randomize(expected))
-    decodedXContentXContent shouldBe t
+    val randomized = randomize(expectedJson, addExtraKeys)
+    val decodedXContentXContent = XContentCodec.decodeUnsafeFromString(randomized)
+    decodedXContentXContent shouldBe expectedObject
   }
 
-  private def decode[T: XContentCodec.Decoder](expected: Json, t: T): Assertion = {
-    val decoded = decodeUnsafeFromString[T](randomize(expected))
-    decoded shouldBe t
+  private def decode[T: XContentCodec.Decoder](expectedJson: Json, expectedObject: T, addExtraKeys: Boolean = false): Assertion = {
+    val decoded = decodeUnsafeFromString[T](randomize(expectedJson, addExtraKeys))
+    decoded shouldBe expectedObject
   }
 
   "test utilities" - {
-    "shuffled" in {
-      val j = Json.obj("foo" -> "1".asJson, "nest" -> Json.obj("foo" -> "1".asJson, "bar" -> "2".asJson)).asJson
-      val shuffled = (0 to 100).map(_ => randomize(j))
+    "randomize" in {
+      import io.circe.syntax._
+      import io.circe.parser
+      val originalObject = Json.obj("foo" -> "1".asJson, "nest" -> Json.obj("foo" -> "1".asJson, "bar" -> "2".asJson)).asJson
+      val shuffled = (0 to 100).map(_ => randomize(originalObject, false))
+      // If you just look at the strings, there should be several variations.
       shuffled.distinct.length shouldBe >=(5)
+      // If you parse the strings back to Json objects, there should only be one, the original object.
+      shuffled.map(parser.parse).distinct shouldBe List(Right(originalObject))
     }
+    "randomize with extra keys" in {
+      val originalObject = Json.obj("foo" -> "1".asJson, "nest" -> Json.obj("foo" -> "1".asJson, "bar" -> "2".asJson)).asJson
+      val shuffled = (0 to 100).map(_ => randomize(originalObject, true))
+      // Both strings and Json objects should vary, as we've added extra keys.
+      shuffled.distinct.length shouldBe >=(5)
+      shuffled.map(parser.parse(_).fold(fail(_), identity)).distinct.length shouldBe >=(5)
+    }
+  }
+
+  "fails to decode vector with extra keys in it" in {
+    val vec = Vec.DenseFloat.random(10)
+    val string = s"""{ "extra": "junk", "values": ${vec.values.asJson.noSpaces}, "more": "junk" }"""
+    val result = decodeUnsafeFromString[Vec](string)
+    result shouldBe vec
   }
 
   "Similarity" - {
@@ -178,11 +216,11 @@ class XContentCodecSuite extends AnyFreeSpec with Matchers {
 
   "Vec.Empty" - {
     "roundtrip" in {
-      roundtrip(Json.obj(), Vec.Empty())
+      roundtrip(Json.obj(), Vec.Empty(), addExtraKeys = false)
     }
     "errors" in {
       val ex1 = intercept[XContentParseException](decodeUnsafeFromString[Vec]("""{"...":"..."}"""))
-      ex1.getMessage shouldBe "Unexpected name [...]"
+      ex1.getMessage shouldBe "Unable to construct [vector] from parsed JSON"
     }
   }
 
diff --git a/elastiknn-testing/src/test/scala/com/klibisz/elastiknn/query/RecallSuite.scala b/elastiknn-testing/src/test/scala/com/klibisz/elastiknn/query/RecallSuite.scala
index f4e5b5ba6..7ed60cf5e 100644
--- a/elastiknn-testing/src/test/scala/com/klibisz/elastiknn/query/RecallSuite.scala
+++ b/elastiknn-testing/src/test/scala/com/klibisz/elastiknn/query/RecallSuite.scala
@@ -7,6 +7,7 @@ import com.sksamuel.elastic4s.ElasticDsl._
 import com.sksamuel.elastic4s.Response
 import com.sksamuel.elastic4s.requests.searches.SearchResponse
 import futil.Futil
+import org.scalatest.concurrent.AsyncCancelAfterFailure
 import org.scalatest.funsuite.AsyncFunSuite
 import org.scalatest.matchers.should.Matchers
 
@@ -24,7 +25,7 @@ import scala.util.hashing.MurmurHash3.orderedHash
   *   the same query, I have seen different results at times. This seems to be an effect at the Elasticsearch level.
   *   I've tested at the Lucene (sans ES) level and that seems to be reliably deterministic.
   */
-class RecallSuite extends AsyncFunSuite with Matchers with ElasticAsyncClient {
+class RecallSuite extends AsyncFunSuite with Matchers with ElasticAsyncClient with AsyncCancelAfterFailure {
 
   // Each test case consists of setting up one Mapping and then running several queries against that mapping.
   // Each query has an expected recall that will be checked.