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-5729-8583b2bbab6f7d5007a1030ad1c996506ea6607e in repository https://gitbox.apache.org/repos/asf/texera.git
commit 4ba0234eda021cf2f9dcfb1f853f65fe8a7f3e2e Author: William Wong <[email protected]> AuthorDate: Thu Aug 13 05:58:39 2026 +0000 feat(workflow-compiling-service): LogicalLink OperatorIdentity round-trip (#5729) <!-- Thanks for sending a pull request (PR)! Here are some tips for you: 1. If this is your first time, please read our contributor guidelines: [Contributing to Texera](https://github.com/apache/texera/blob/main/CONTRIBUTING.md) 2. Ensure you have added or run the appropriate tests for your PR 3. If the PR is work in progress, mark it a draft on GitHub. 4. Please write your PR title to summarize what this PR proposes, we are following Conventional Commits style for PR titles as well. 5. Be sure to keep the PR description updated to reflect all changes. --> ### What changes were proposed in this PR? <!-- Please clarify what changes you are proposing. The purpose of this section is to outline the changes. Here are some tips for you: 1. If you propose a new API, clarify the use case for a new API. 2. If you fix a bug, you can clarify why it is a bug. 3. If it is a refactoring, clarify what has been changed. 3. It would be helpful to include a before-and-after comparison using screenshots or GIFs. 4. Please consider writing useful notes for better and faster reviews. --> Bug fix: `LogicalLink` `@JsonCreator` constructor (`amber` and `workflow-compiling-service`) `@JsonCreator` was previously placed on the `String` convenience constructor of `LogicalLink` in both modules. `OperatorIdentity` is a case class that Jackson serializes as an object (`{"id":"op-A"}`), not a plain string. When reading back a serialized `LogicalLink`, Jackson dispatched to the `@JsonCreator` String constructor but could not coerce the `{"id":"op-A"}` object node to a `String`, causing any `writeValueAsString` → `readValue` round-trip to fail with `MismatchedInputException`. The fix introduces a private `readOperatorIdentity(node: JsonNode, fieldName: String)` helper in the companion object and moves `@JsonCreator` to a new `JsonNode` constructor that delegates to it. The helper accepts both the plain-string shape (front-end input) and the object shape (serialized form), maps null/absent ids to `OperatorIdentity(null)`, and rejects malformed nodes. The `String` convenience constructor is retained but no longer carries `@JsonCreator`. ### Any related issues, documentation, discussions? <!-- Please use this section to link other resources if not mentioned already. 1. If this PR fixes an issue, please include `Fixes #1234`, `Resolves #1234` or `Closes #1234`. If it is only related, simply mention the issue number. 2. If there is design documentation, please add the link. 3. If there is a discussion in the mailing list, please add the link. --> Closes #5042 This PR continues and adopts work from #5175 ### How was this PR tested? <!-- If tests were added, say they were added here. Or simply mention that if the PR is tested with existing test cases. Make sure to include/update test cases that check the changes thoroughly including negative and positive cases if possible. If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future. If tests were not added, please describe why they were not added and/or why it was difficult to add. --> The new `workflow-compiling-service` `LogicalLinkSpec` adds unit coverage of `LogicalLink` and `readOperatorIdentity` model-level logic that existing tests do not cover and pins the leniency contract (no `require` guards in the compiler-service variant). The existing `amber` `LogicalLinkSpec` was updated to match the renamed constructor section, drop the now-invalid `MismatchedInputException` expectation, and add a passing round-trip test. Run with: ``` sbt "WorkflowExecutionService/testOnly *LogicalLinkSpec" ``` Result: 18/18 tests pass ``` sbt "WorkflowCompilingService/testOnly *LogicalLinkSpec" ``` Result: 15/15 tests pass ### Was this PR authored or co-authored using generative AI tooling? <!-- If generative AI tooling has been used in the process of authoring this PR, please include the phrase: 'Generated-by: ' followed by the name of the tool and its version. If no, write 'No'. Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details. --> Generated-by: Claude Sonnet 4.6 --------- Signed-off-by: William Wong <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> Co-authored-by: Yicong Huang <[email protected]> Co-authored-by: Xinyuan Lin <[email protected]> --- .../texera/common/compiler/model/LogicalLink.scala | 51 ++++++++- .../common/compiler/model/LogicalLinkSpec.scala | 121 ++++++++++++++++----- 2 files changed, 142 insertions(+), 30 deletions(-) diff --git a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/model/LogicalLink.scala b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/model/LogicalLink.scala index 30d5d3f3ab..62d899061e 100644 --- a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/model/LogicalLink.scala +++ b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/model/LogicalLink.scala @@ -20,9 +20,40 @@ package org.apache.texera.common.compiler.model import com.fasterxml.jackson.annotation.{JsonCreator, JsonProperty} +import com.fasterxml.jackson.databind.JsonNode import org.apache.texera.amber.core.virtualidentity.OperatorIdentity import org.apache.texera.amber.core.workflow.PortIdentity +object LogicalLink { + + // Reads an OperatorIdentity from either the plain-string shape the + // frontend emits (`"op-A"`) or the object shape Jackson emits for an + // OperatorIdentity (`{"id":"op-A"}`), so a writeValueAsString -> + // readValue round-trip of a LogicalLink succeeds. + private def readOperatorIdentity(node: JsonNode, fieldName: String): OperatorIdentity = { + if (node == null || node.isNull) { + OperatorIdentity(null) + } else if (node.isTextual) { + OperatorIdentity(node.asText()) + } else if (node.isObject) { + val idNode = node.get("id") + if (idNode == null || idNode.isNull) { + OperatorIdentity(null) + } else if (idNode.isTextual) { + OperatorIdentity(idNode.asText()) + } else { + throw new IllegalArgumentException( + s"LogicalLink $fieldName.id must be a string or null, but was ${idNode.getNodeType}" + ) + } + } else { + throw new IllegalArgumentException( + s"LogicalLink $fieldName must be a string or an object, but was ${node.getNodeType}" + ) + } + } +} + case class LogicalLink( @JsonProperty("fromOpId") fromOpId: OperatorIdentity, fromPortId: PortIdentity, @@ -42,13 +73,27 @@ case class LogicalLink( s"LogicalLink self-loop not allowed: fromOpId == toOpId == ${fromOpId.id}" ) - @JsonCreator def this( - @JsonProperty("fromOpId") fromOpId: String, + fromOpId: String, fromPortId: PortIdentity, - @JsonProperty("toOpId") toOpId: String, + toOpId: String, toPortId: PortIdentity ) = { this(OperatorIdentity(fromOpId), fromPortId, OperatorIdentity(toOpId), toPortId) } + + @JsonCreator + def this( + @JsonProperty("fromOpId") fromOpId: JsonNode, + fromPortId: PortIdentity, + @JsonProperty("toOpId") toOpId: JsonNode, + toPortId: PortIdentity + ) = { + this( + LogicalLink.readOperatorIdentity(fromOpId, "fromOpId"), + fromPortId, + LogicalLink.readOperatorIdentity(toOpId, "toOpId"), + toPortId + ) + } } diff --git a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/model/LogicalLinkSpec.scala b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/model/LogicalLinkSpec.scala index 6ffb9d497d..2428bf53b1 100644 --- a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/model/LogicalLinkSpec.scala +++ b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/model/LogicalLinkSpec.scala @@ -20,7 +20,7 @@ package org.apache.texera.common.compiler.model import com.fasterxml.jackson.databind.JsonNode -import com.fasterxml.jackson.databind.exc.{MismatchedInputException, ValueInstantiationException} +import com.fasterxml.jackson.databind.exc.ValueInstantiationException import org.apache.texera.amber.core.virtualidentity.OperatorIdentity import org.apache.texera.amber.core.workflow.PortIdentity import org.apache.texera.amber.util.JSONUtils.objectMapper @@ -131,10 +131,10 @@ class LogicalLinkSpec extends AnyFlatSpec { } // --------------------------------------------------------------------------- - // Secondary @JsonCreator constructor (string opId variant) + // Secondary string opId constructor // --------------------------------------------------------------------------- - "LogicalLink secondary @JsonCreator constructor" should "wrap raw String op ids in OperatorIdentity" in { + "LogicalLink secondary String constructor" should "wrap raw String op ids in OperatorIdentity" in { val link = new LogicalLink( fromOpId = "op-A", fromPortId = PortIdentity(0), @@ -160,7 +160,7 @@ class LogicalLinkSpec extends AnyFlatSpec { assert(link.toOpId == OperatorIdentity("my.op-2")) } - it should "reject the empty string as an op id via the @JsonCreator constructor" in { + it should "reject the empty string as an op id via the secondary String constructor" in { intercept[IllegalArgumentException] { new LogicalLink("", PortIdentity(0), "op-B", PortIdentity(1)) } @@ -169,7 +169,7 @@ class LogicalLinkSpec extends AnyFlatSpec { } } - it should "reject a null string op id via the @JsonCreator constructor" in { + it should "reject a null string op id via the secondary String constructor" in { intercept[IllegalArgumentException] { new LogicalLink(null: String, PortIdentity(0), "op-B", PortIdentity(1)) } @@ -178,7 +178,7 @@ class LogicalLinkSpec extends AnyFlatSpec { } } - it should "reject a self-loop via the @JsonCreator constructor (same string op id)" in { + it should "reject a self-loop via the secondary String constructor (same string op id)" in { val ex = intercept[IllegalArgumentException] { new LogicalLink("op-A", PortIdentity(0), "op-A", PortIdentity(1)) } @@ -194,12 +194,11 @@ class LogicalLinkSpec extends AnyFlatSpec { // wiring (annotations, default-Scala-module config) surfaces here. "LogicalLink Jackson deserialization" should - "deserialize fromOpId / toOpId from raw String values via the secondary @JsonCreator constructor" in { + "deserialize fromOpId / toOpId from raw String values via the Jackson creator" in { // Build the JSON by hand to mimic a user-saved workflow file where // `fromOpId` and `toOpId` are written as plain strings (the only shape // production actually receives, since the frontend emits them as - // strings). Jackson dispatches to the @JsonCreator string-overload - // constructor. + // strings). Jackson dispatches to the @JsonCreator constructor. val node = objectMapper.createObjectNode() node.put("fromOpId", "op-A") node.set("fromPortId", objectMapper.valueToTree[JsonNode](PortIdentity(0))) @@ -245,17 +244,7 @@ class LogicalLinkSpec extends AnyFlatSpec { assert(tree.has("toPortId")) } - it should "NOT round-trip through writeValueAsString (the @JsonCreator string overload is incompatible with the object-shape OperatorIdentity that writeValueAsString emits)" in { - // Characterization of a real asymmetry tracked by - // https://github.com/apache/texera/issues/5042. Production reads - // user-saved workflow JSON where `fromOpId`/`toOpId` are plain - // strings, but `objectMapper.writeValueAsString` writes - // OperatorIdentity as `{"id":"op-A"}` (the case-class object form). - // Re-reading the emitted JSON fails because Jackson dispatches on the - // @JsonCreator string overload, which can't accept an object for - // fromOpId. When the issue is fixed (additional @JsonCreator object - // overload or a custom @JsonDeserialize), this test must flip to a - // passing round-trip assertion alongside the fix. + it should "round-trip through writeValueAsString when OperatorIdentity fields use object shape" in { val original = LogicalLink( OperatorIdentity("op-A"), PortIdentity(0), @@ -269,16 +258,14 @@ class LogicalLinkSpec extends AnyFlatSpec { val tree = objectMapper.readTree(json) assert(tree.path("fromOpId").isObject, s"expected fromOpId to be an object: $json") assert(tree.path("fromOpId").path("id").asText() == "op-A") - // Re-reading the just-emitted JSON fails because the @JsonCreator - // String overload can't accept the object-shape fromOpId. - intercept[MismatchedInputException] { - objectMapper.readValue(json, classOf[LogicalLink]) - } + + val roundTripped = objectMapper.readValue(json, classOf[LogicalLink]) + assert(roundTripped == original) } - it should "reject missing string op-id fields when deserializing via Jackson" in { + it should "reject missing op-id fields when deserializing via Jackson" in { // When `fromOpId` / `toOpId` are omitted, Jackson invokes the - // @JsonCreator with `null` for the missing String args. The primary + // @JsonCreator with `null` for the missing args. The primary // constructor's `require` on non-null/non-empty ids then throws, and // Jackson wraps it in `ValueInstantiationException` with the original // `IllegalArgumentException` as the cause. @@ -288,4 +275,84 @@ class LogicalLinkSpec extends AnyFlatSpec { } assert(ex.getCause.isInstanceOf[IllegalArgumentException]) } + + it should "reject an object-shape op id with no `id` field (null id fails the require guard)" in { + // An object-shape fromOpId lacking an `id` field resolves to + // OperatorIdentity(null), which the primary constructor's `require` + // rejects — surfaced as ValueInstantiationException. + val node = objectMapper.createObjectNode() + node.set("fromOpId", objectMapper.createObjectNode()) // {} — no "id" + node.set("fromPortId", objectMapper.valueToTree[JsonNode](PortIdentity(0))) + node.put("toOpId", "op-B") + node.set("toPortId", objectMapper.valueToTree[JsonNode](PortIdentity(1))) + val ex = intercept[ValueInstantiationException] { + objectMapper.treeToValue(node, classOf[LogicalLink]) + } + assert(ex.getCause.isInstanceOf[IllegalArgumentException]) + assert(ex.getCause.getMessage.contains("fromOpId must be non-null")) + } + + it should "reject an object-shape op id whose `id` field is non-textual" in { + // `{"id": 123}` is malformed: `id` must be a string (or null). The + // helper throws rather than silently coercing 123 -> "123"; Jackson + // wraps the IllegalArgumentException in a ValueInstantiationException. + val node = objectMapper.createObjectNode() + val badOpId = objectMapper.createObjectNode() + badOpId.put("id", 123) + node.set("fromOpId", badOpId) + node.set("fromPortId", objectMapper.valueToTree[JsonNode](PortIdentity(0))) + node.put("toOpId", "op-B") + node.set("toPortId", objectMapper.valueToTree[JsonNode](PortIdentity(1))) + val ex = intercept[ValueInstantiationException] { + objectMapper.treeToValue(node, classOf[LogicalLink]) + } + assert(ex.getCause.isInstanceOf[IllegalArgumentException]) + assert(ex.getCause.getMessage.contains("fromOpId.id must be a string")) + } + + it should "reject an op id that is neither a string nor an object (e.g. a number)" in { + // A top-level numeric fromOpId hits the final else branch of + // readOperatorIdentity, which throws; Jackson wraps it. + val node = objectMapper.createObjectNode() + node.put("fromOpId", 12345) + node.set("fromPortId", objectMapper.valueToTree[JsonNode](PortIdentity(0))) + node.put("toOpId", "op-B") + node.set("toPortId", objectMapper.valueToTree[JsonNode](PortIdentity(1))) + val ex = intercept[ValueInstantiationException] { + objectMapper.treeToValue(node, classOf[LogicalLink]) + } + assert(ex.getCause.isInstanceOf[IllegalArgumentException]) + assert(ex.getCause.getMessage.contains("fromOpId must be a string or an object")) + } + + it should "reject an explicit JSON null op id (exercises the node.isNull branch)" in { + // An explicit `"fromOpId": null` arrives as a NullNode (an absent field + // arrives as Java null), exercising the `node.isNull` branch; require + // then rejects the null id. + val node = objectMapper.createObjectNode() + node.set("fromOpId", objectMapper.nullNode()) + node.set("fromPortId", objectMapper.valueToTree[JsonNode](PortIdentity(0))) + node.put("toOpId", "op-B") + node.set("toPortId", objectMapper.valueToTree[JsonNode](PortIdentity(1))) + val ex = intercept[ValueInstantiationException] { + objectMapper.treeToValue(node, classOf[LogicalLink]) + } + assert(ex.getCause.isInstanceOf[IllegalArgumentException]) + } + + it should "reject an object-shape op id with an explicit null `id` (exercises the idNode.isNull branch)" in { + // `{"id": null}` makes idNode a NullNode, exercising the `idNode.isNull` + // branch; require then rejects the resulting OperatorIdentity(null). + val opId = objectMapper.createObjectNode() + opId.set("id", objectMapper.nullNode()) + val node = objectMapper.createObjectNode() + node.set("fromOpId", opId) + node.set("fromPortId", objectMapper.valueToTree[JsonNode](PortIdentity(0))) + node.put("toOpId", "op-B") + node.set("toPortId", objectMapper.valueToTree[JsonNode](PortIdentity(1))) + val ex = intercept[ValueInstantiationException] { + objectMapper.treeToValue(node, classOf[LogicalLink]) + } + assert(ex.getCause.isInstanceOf[IllegalArgumentException]) + } }
