This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-5799-f5cf00a8b1b7a91d18b2505b4b3de3dfcfdcbdd2 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 4722a42c019a35790134e660d9becc6ea069205f Author: Tanishq Gandhi <[email protected]> AuthorDate: Fri Jun 19 23:24:42 2026 -0700 test(workflow-core): cover PortIdentity key serde (#5799) ### What changes were proposed in this PR? Add dedicated unit specs for `PortIdentity` JSON map-key serde: | Spec | Coverage | | --- | --- | | `PortIdentityKeySerializerSpec` | Pins the exact `id_internal` key format and Jackson map-key serialization | | `PortIdentityKeyDeserializerSpec` | Pins valid key parsing, serializer/deserializer round-trips, Jackson map round-trip, and current malformed-key failure behavior | ### Any related issues, documentation, discussions? Closes #5776 ### How was this PR tested? Added unit tests and ran: ```bash sbt "project WorkflowCore" "testOnly org.apache.texera.amber.util.serde.PortIdentityKeySerializerSpec org.apache.texera.amber.util.serde.PortIdentityKeyDeserializerSpec" ``` Result: all 12 tests passed. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: OpenAI Codex (GPT-5) --- .../serde/PortIdentityKeyDeserializerSpec.scala | 129 ++++++++++++++++++++ .../util/serde/PortIdentityKeySerializerSpec.scala | 63 ++++++++++ .../amber/util/serde/PortIdentitySerdeSpec.scala | 132 --------------------- 3 files changed, 192 insertions(+), 132 deletions(-) diff --git a/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentityKeyDeserializerSpec.scala b/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentityKeyDeserializerSpec.scala new file mode 100644 index 0000000000..427239aa2c --- /dev/null +++ b/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentityKeyDeserializerSpec.scala @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.util.serde + +import org.apache.texera.amber.core.workflow.PortIdentity +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import scala.jdk.CollectionConverters._ + +class PortIdentityKeyDeserializerSpec extends AnyFlatSpec with Matchers { + + private val deserializer = new PortIdentityKeyDeserializer + + "PortIdentityKeyDeserializer.deserializeKey" should "parse id_internal keys into PortIdentity" in { + deserializer.deserializeKey("3_false", null) shouldBe PortIdentity(3, internal = false) + deserializer.deserializeKey("0_true", null) shouldBe PortIdentity(0, internal = true) + } + + it should "round-trip serializer output for representative PortIdentity values" in { + val cases = Seq( + PortIdentity(0, internal = false), + PortIdentity(0, internal = true), + PortIdentity(999999, internal = false), + PortIdentity(999999, internal = true), + PortIdentity(-1, internal = false), + PortIdentity(-1, internal = true) + ) + + cases.foreach { portIdentity => + deserializer.deserializeKey( + PortIdentityKeySerializer.portIdToString(portIdentity), + null + ) shouldBe portIdentity + } + } + + it should "round-trip Map[PortIdentity, String] keys through JSONUtils.objectMapper" in { + val original = Map( + PortIdentity(0, internal = false) -> "zero-external", + PortIdentity(3, internal = true) -> "three-internal", + PortIdentity(999999, internal = false) -> "large-external", + PortIdentity(-1, internal = true) -> "negative-internal" + ) + val json = objectMapper.writeValueAsString(original) + val mapType = objectMapper.getTypeFactory + .constructMapType(classOf[java.util.HashMap[_, _]], classOf[PortIdentity], classOf[String]) + + val restored: java.util.Map[PortIdentity, String] = objectMapper.readValue(json, mapType) + + restored.asScala.toMap shouldBe original + } + + it should "round-trip an empty Map[PortIdentity, String] through JSONUtils.objectMapper" in { + val original = Map.empty[PortIdentity, String] + val json = objectMapper.writeValueAsString(original) + val mapType = objectMapper.getTypeFactory + .constructMapType(classOf[java.util.HashMap[_, _]], classOf[PortIdentity], classOf[String]) + + val restored: java.util.Map[PortIdentity, String] = objectMapper.readValue(json, mapType) + + restored.asScala.toMap shouldBe original + } + + it should "throw NumberFormatException for a non-integer id" in { + intercept[NumberFormatException] { + deserializer.deserializeKey("notAnInt_false", null) + } + } + + it should "throw IllegalArgumentException for a non-boolean internal flag" in { + intercept[IllegalArgumentException] { + deserializer.deserializeKey("0_notABool", null) + } + } + + it should "throw NumberFormatException when a separator-less key is non-numeric" in { + intercept[NumberFormatException] { + deserializer.deserializeKey("missingSeparator", null) + } + } + + it should "throw NumberFormatException for an empty key" in { + intercept[NumberFormatException] { + deserializer.deserializeKey("", null) + } + } + + it should "throw ArrayIndexOutOfBoundsException when only the id is provided" in { + intercept[ArrayIndexOutOfBoundsException] { + deserializer.deserializeKey("5", null) + } + } + + it should "ignore extra trailing underscore-separated segments" in { + deserializer.deserializeKey("1_true_extra", null) shouldBe PortIdentity(1, internal = true) + } + + it should "eventually reject keys with extra trailing segments (pendingUntilFixed)" in pendingUntilFixed { + // Documented contract: a `PortIdentityKeySerializer` output is exactly + // `id_internal` — two underscore-separated segments. Anything else is + // corrupt JSON and should be rejected, not silently truncated. The + // current implementation is lenient (see characterization test above); + // this pendingUntilFixed flips to passing once the parser is hardened, + // then `pendingUntilFixed` inverts that into a deliberate failure forcing + // the marker to be removed. + intercept[IllegalArgumentException] { + deserializer.deserializeKey("1_true_extra", null) + } + } +} diff --git a/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentityKeySerializerSpec.scala b/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentityKeySerializerSpec.scala new file mode 100644 index 0000000000..31ba71c3c6 --- /dev/null +++ b/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentityKeySerializerSpec.scala @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.util.serde + +import org.apache.texera.amber.core.workflow.PortIdentity +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class PortIdentityKeySerializerSpec extends AnyFlatSpec with Matchers { + + "PortIdentityKeySerializer.portIdToString" should "format PortIdentity as id_internal" in { + PortIdentityKeySerializer + .portIdToString(PortIdentity(3, internal = false)) shouldBe "3_false" + PortIdentityKeySerializer + .portIdToString(PortIdentity(0, internal = true)) shouldBe "0_true" + } + + it should "preserve zero, large, negative, internal, and external ports in the key string" in { + val cases = Seq( + PortIdentity(0, internal = false) -> "0_false", + PortIdentity(0, internal = true) -> "0_true", + PortIdentity(999999, internal = false) -> "999999_false", + PortIdentity(999999, internal = true) -> "999999_true", + PortIdentity(-1, internal = false) -> "-1_false", + PortIdentity(-1, internal = true) -> "-1_true" + ) + + cases.foreach { + case (portIdentity, expected) => + PortIdentityKeySerializer.portIdToString(portIdentity) shouldBe expected + } + } + + "PortIdentityKeySerializer" should "write Jackson map keys with the id_internal format" in { + val original = Map( + PortIdentity(3, internal = false) -> "external", + PortIdentity(0, internal = true) -> "internal" + ) + + val json = objectMapper.writeValueAsString(original) + + json should include(""""3_false"""") + json should include(""""0_true"""") + } +} diff --git a/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentitySerdeSpec.scala b/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentitySerdeSpec.scala deleted file mode 100644 index e7f6ad037b..0000000000 --- a/common/workflow-core/src/test/scala/org/apache/texera/amber/util/serde/PortIdentitySerdeSpec.scala +++ /dev/null @@ -1,132 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.texera.amber.util.serde - -import org.apache.texera.amber.core.workflow.PortIdentity -import org.apache.texera.amber.util.JSONUtils.objectMapper -import org.scalatest.flatspec.AnyFlatSpec - -class PortIdentitySerdeSpec extends AnyFlatSpec { - - // --------------------------------------------------------------------------- - // PortIdentityKeySerializer.portIdToString (companion, not the Jackson class) - // --------------------------------------------------------------------------- - - "PortIdentityKeySerializer.portIdToString" should "format a PortIdentity as `id_internal`" in { - assert(PortIdentityKeySerializer.portIdToString(PortIdentity(0, internal = false)) == "0_false") - assert(PortIdentityKeySerializer.portIdToString(PortIdentity(7, internal = true)) == "7_true") - } - - // --------------------------------------------------------------------------- - // PortIdentityKeySerializer + PortIdentityKeyDeserializer (Jackson wiring) - // --------------------------------------------------------------------------- - // - // These tests use the production `JSONUtils.objectMapper` directly so a - // regression in the singleton wiring (e.g. the module that registers the - // PortIdentity key (de)serializer being removed or reordered) surfaces - // here, not just on a freshly-constructed mapper. - - "PortIdentity Jackson key (de)serialization" should "round-trip a Map[PortIdentity, String] via JSONUtils.objectMapper" in { - val original = Map( - PortIdentity(0, internal = false) -> "a", - PortIdentity(1, internal = true) -> "b" - ) - val json = objectMapper.writeValueAsString(original) - // Verify the JSON keys match the documented `id_internal` format. - assert(json.contains("\"0_false\"")) - assert(json.contains("\"1_true\"")) - val tref = objectMapper.getTypeFactory - .constructMapType(classOf[java.util.HashMap[_, _]], classOf[PortIdentity], classOf[String]) - val restored: java.util.Map[PortIdentity, String] = objectMapper.readValue(json, tref) - import scala.jdk.CollectionConverters._ - assert(restored.asScala.toMap == original) - } - - it should "round-trip an empty Map[PortIdentity, V] without invoking the (de)serializer" in { - val original = Map.empty[PortIdentity, String] - val json = objectMapper.writeValueAsString(original) - val tref = objectMapper.getTypeFactory - .constructMapType(classOf[java.util.HashMap[_, _]], classOf[PortIdentity], classOf[String]) - val restored: java.util.Map[PortIdentity, String] = objectMapper.readValue(json, tref) - assert(restored.isEmpty) - } - - "PortIdentityKeyDeserializer.deserializeKey" should "throw NumberFormatException for a non-integer id" in { - val d = new PortIdentityKeyDeserializer - intercept[NumberFormatException] { - d.deserializeKey("notAnInt_false", null) - } - } - - it should "throw IllegalArgumentException for a non-boolean internal flag" in { - val d = new PortIdentityKeyDeserializer - intercept[IllegalArgumentException] { - d.deserializeKey("0_notABool", null) - } - } - - it should "throw NumberFormatException when the underscore separator is missing and the whole string is non-numeric" in { - // `key.split("_")` on a separator-less non-numeric string yields a - // single-element array, and `parts(0).toInt` fires first → NFE. - val d = new PortIdentityKeyDeserializer - intercept[NumberFormatException] { - d.deserializeKey("missingSeparator", null) - } - } - - it should "throw ArrayIndexOutOfBoundsException when only the id is provided (no `_internal` suffix)" in { - // Different separator-missing path: `\"5\".split(\"_\")` yields - // [\"5\"], parts(0).toInt = 5 succeeds, then parts(1) reads past the - // end. Pin this failure mode explicitly so a future safer parser - // breaks the spec on purpose (and the safer error type is chosen - // consciously). - val d = new PortIdentityKeyDeserializer - intercept[ArrayIndexOutOfBoundsException] { - d.deserializeKey("5", null) - } - } - - it should "silently accept extra trailing underscore-separated segments (lenient parser, current behavior)" in { - // Pin the current lenient behavior: `parts(0).toInt` and - // `parts(1).toBoolean` ignore everything past `parts(1)`, so a key - // like `"1_true_garbage"` deserializes to `PortIdentity(1, true)` - // without complaint. The strict-rejection variant lives in a - // pendingUntilFixed test below; characterizing today's lenient - // path here means a future-tightening fix would need to update - // both tests deliberately. - val d = new PortIdentityKeyDeserializer - val pid = d.deserializeKey("1_true_garbage", null) - assert(pid == PortIdentity(1, internal = true)) - } - - it should "eventually reject keys with extra trailing segments (pendingUntilFixed)" in pendingUntilFixed { - // Documented contract: a `PortIdentityKeySerializer` output is exactly - // `id_internal` — two underscore-separated segments. Anything else is - // corrupt JSON and should be rejected, not silently truncated. The - // current implementation is lenient (see characterization test - // above); this pendingUntilFixed flips to passing once the parser - // is hardened, then `pendingUntilFixed` inverts that into a - // deliberate failure forcing the marker to be removed. - val d = new PortIdentityKeyDeserializer - intercept[IllegalArgumentException] { - d.deserializeKey("1_true_garbage", null) - } - } -}
