Copilot commented on code in PR #5729:
URL: https://github.com/apache/texera/pull/5729#discussion_r3464113762
##########
workflow-compiling-service/src/main/scala/org/apache/texera/amber/compiler/model/LogicalLink.scala:
##########
@@ -20,22 +20,68 @@
package org.apache.texera.amber.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"}`). Keep in sync with the amber
+ // workflow LogicalLink.readOperatorIdentity, which is intentionally
+ // identical.
+ 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 with an id
field, but was ${node.getNodeType}"
+ )
Review Comment:
The thrown error message says the value must be an object "with an id
field", but the parser actually accepts object-shaped opIds without an `id`
field (mapping them to `OperatorIdentity(null)`). This makes the message
misleading for malformed inputs like numbers/arrays; consider removing the
"with an id field" requirement from the message.
##########
workflow-compiling-service/src/test/scala/org/apache/texera/amber/compiler/model/LogicalLinkSpec.scala:
##########
@@ -0,0 +1,298 @@
+/*
+ * 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.compiler.model
+
+import com.fasterxml.jackson.databind.JsonNode
+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
+import org.scalatest.flatspec.AnyFlatSpec
+
+/**
+ * Unit tests for the workflow-compiling-service [[LogicalLink]].
+ *
+ * Unlike the amber engine's LogicalLink, this version is intentionally
+ * lenient: it carries no `require` guards so that the compiler can
+ * represent partially-built or invalid workflows during editing without
+ * throwing. Tests here pin that contract and verify the Jackson wiring
+ * that lets the service round-trip saved workflow JSON.
+ */
+class LogicalLinkSpec extends AnyFlatSpec {
+
+ //
---------------------------------------------------------------------------
+ // Primary constructor + case-class semantics
+ //
---------------------------------------------------------------------------
+
+ "LogicalLink primary constructor" should "expose the four fields it was
constructed with" in {
+ val link = LogicalLink(
+ fromOpId = OperatorIdentity("op-A"),
+ fromPortId = PortIdentity(0),
+ toOpId = OperatorIdentity("op-B"),
+ toPortId = PortIdentity(1, internal = true)
+ )
+ assert(link.fromOpId == OperatorIdentity("op-A"))
+ assert(link.fromPortId == PortIdentity(0))
+ assert(link.toOpId == OperatorIdentity("op-B"))
+ assert(link.toPortId == PortIdentity(1, internal = true))
+ }
+
+ "LogicalLink case-class equality" should "use structural equality across all
four fields" in {
+ val a =
+ LogicalLink(OperatorIdentity("x"), PortIdentity(0),
OperatorIdentity("y"), PortIdentity(1))
+ val b =
+ LogicalLink(OperatorIdentity("x"), PortIdentity(0),
OperatorIdentity("y"), PortIdentity(1))
+ assert(a == b)
+ assert(a.hashCode == b.hashCode)
+ }
+
+ it should "distinguish links that differ only in fromOpId" in {
+ val a =
+ LogicalLink(OperatorIdentity("x"), PortIdentity(0),
OperatorIdentity("y"), PortIdentity(1))
+ val b =
+ LogicalLink(OperatorIdentity("z"), PortIdentity(0),
OperatorIdentity("y"), PortIdentity(1))
+ assert(a != b)
+ }
+
+ it should "distinguish links that differ only in toPortId.internal" in {
+ val a = LogicalLink(
+ OperatorIdentity("x"),
+ PortIdentity(0),
+ OperatorIdentity("y"),
+ PortIdentity(1, internal = false)
+ )
+ val b = LogicalLink(
+ OperatorIdentity("x"),
+ PortIdentity(0),
+ OperatorIdentity("y"),
+ PortIdentity(1, internal = true)
+ )
+ assert(a != b)
+ }
+
+ //
---------------------------------------------------------------------------
+ // Secondary String constructor
+ //
---------------------------------------------------------------------------
+
+ "LogicalLink secondary String constructor" should "wrap raw String op ids in
OperatorIdentity" in {
+ val link = new LogicalLink(
+ fromOpId = "op-A",
+ fromPortId = PortIdentity(0),
+ toOpId = "op-B",
+ toPortId = PortIdentity(1)
+ )
+ assert(link.fromOpId == OperatorIdentity("op-A"))
+ assert(link.toOpId == OperatorIdentity("op-B"))
+ assert(
+ link == LogicalLink(
+ OperatorIdentity("op-A"),
+ PortIdentity(0),
+ OperatorIdentity("op-B"),
+ PortIdentity(1)
+ )
+ )
+ }
+
+ it should "accept identifiers containing dashes, dots, and digits" in {
+ val link = new LogicalLink("my.op-1", PortIdentity(0), "my.op-2",
PortIdentity(1))
+ assert(link.fromOpId == OperatorIdentity("my.op-1"))
+ assert(link.toOpId == OperatorIdentity("my.op-2"))
+ }
+
+ //
---------------------------------------------------------------------------
+ // Leniency contract: no require guards in the compiler-service variant
+ //
---------------------------------------------------------------------------
+ //
+ // The compiler-service LogicalLink is intentionally lenient so that a
+ // mid-edit, partially-built workflow (e.g. one where an operator id has
+ // not yet been assigned) can be represented without throwing. The amber
+ // engine's LogicalLink enforces strict validation; tests for that live in
+ // amber/src/test.
+
+ "LogicalLink (compiler-service)" should "accept a null OperatorIdentity id
without throwing" in {
+ val link = LogicalLink(
+ OperatorIdentity(null),
+ PortIdentity(0),
+ OperatorIdentity("op-B"),
+ PortIdentity(1)
+ )
+ assert(link.fromOpId == OperatorIdentity(null))
+ }
+
+ it should "accept a self-loop link (fromOpId == toOpId) without throwing" in
{
+ val link = LogicalLink(
+ OperatorIdentity("op-A"),
+ PortIdentity(0),
+ OperatorIdentity("op-A"),
+ PortIdentity(1)
+ )
+ assert(link.fromOpId == link.toOpId)
+ }
+
+ //
---------------------------------------------------------------------------
+ // Jackson round-trip (production objectMapper)
+ //
---------------------------------------------------------------------------
+ //
+ // These tests use the same `JSONUtils.objectMapper` that production uses
+ // to read user-saved workflow JSON, so a regression in the Jackson wiring
+ // (annotations, default-Scala-module config) surfaces here.
+
+ "LogicalLink Jackson deserialization" should
+ "deserialize fromOpId / toOpId from raw String values via the Jackson
creator" in {
+ val node = objectMapper.createObjectNode()
+ node.put("fromOpId", "op-A")
+ node.set("fromPortId", objectMapper.valueToTree[JsonNode](PortIdentity(0)))
+ node.put("toOpId", "op-B")
+ node.set("toPortId", objectMapper.valueToTree[JsonNode](PortIdentity(1)))
+ val link = objectMapper.treeToValue(node, classOf[LogicalLink])
+ assert(link.fromOpId == OperatorIdentity("op-A"))
+ assert(link.toOpId == OperatorIdentity("op-B"))
+ assert(link.fromPortId == PortIdentity(0))
+ assert(link.toPortId == PortIdentity(1))
+ }
+
+ it should "round-trip through writeValueAsString when OperatorIdentity
fields use object shape" in {
+ val original = LogicalLink(
+ OperatorIdentity("op-A"),
+ PortIdentity(0),
+ OperatorIdentity("op-B"),
+ PortIdentity(1)
+ )
+ val json = objectMapper.writeValueAsString(original)
+ 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")
+
+ val roundTripped = objectMapper.readValue(json, classOf[LogicalLink])
+ assert(roundTripped == original)
+ }
+
+ it should "emit `fromOpId` / `toOpId` JSON keys pinned by @JsonProperty
annotations" in {
+ // @JsonProperty pins the key name — a Scala parameter rename keeps the
+ // JSON key stable, which is required for saved-workflow compatibility.
+ val link = LogicalLink(
+ OperatorIdentity("op-A"),
+ PortIdentity(0),
+ OperatorIdentity("op-B"),
+ PortIdentity(1)
+ )
+ val tree = objectMapper.valueToTree[JsonNode](link)
+ assert(tree.has("fromOpId"))
+ assert(tree.has("toOpId"))
+ }
+
+ it should "emit `fromPortId` / `toPortId` JSON keys derived from Scala
parameter names (no @JsonProperty)" in {
+ // No @JsonProperty on these fields — the JSON key comes from the Scala
+ // parameter name. A future rename WOULD silently break saved-workflow
+ // compatibility; this test exists so that rename breaks here on purpose.
+ val link = LogicalLink(
+ OperatorIdentity("op-A"),
+ PortIdentity(0),
+ OperatorIdentity("op-B"),
+ PortIdentity(1)
+ )
+ val tree = objectMapper.valueToTree[JsonNode](link)
+ assert(tree.has("fromPortId"))
+ assert(tree.has("toPortId"))
+ }
+
+ it should "produce OperatorIdentity(null) when fromOpId is absent from JSON
entirely (lenient)" in {
+ // When fromOpId / toOpId are omitted entirely, Jackson passes null to the
+ // @JsonCreator parameter. readOperatorIdentity treats null as
OperatorIdentity(null)
+ // rather than throwing — lenient by design for partial workflows mid-edit.
+ val empty = objectMapper.createObjectNode()
+ val link = objectMapper.treeToValue(empty, classOf[LogicalLink])
+ assert(link.fromOpId == OperatorIdentity(null))
+ assert(link.toOpId == OperatorIdentity(null))
+ }
+
+ it should "produce OperatorIdentity(null) for an object-shape id with no id
field (lenient)" in {
+ // WCS LogicalLink is lenient: an object-shape fromOpId with no "id" field
+ // produces OperatorIdentity(null) rather than throwing. This lets the
+ // compiler represent partially-built workflows mid-edit.
+ val node = objectMapper.createObjectNode()
+ node.set("fromOpId", objectMapper.createObjectNode()) // {} — no "id" field
+ node.set("fromPortId", objectMapper.valueToTree[JsonNode](PortIdentity(0)))
+ node.put("toOpId", "op-B")
+ node.set("toPortId", objectMapper.valueToTree[JsonNode](PortIdentity(1)))
+
+ val link = objectMapper.treeToValue(node, classOf[LogicalLink])
+ assert(link.fromOpId == OperatorIdentity(null))
+ }
+
+ it should "throw IllegalArgumentException when an opId is a numeric value
instead of text or object" in {
Review Comment:
This test description says it "throw[s] IllegalArgumentException", but
Jackson actually throws `ValueInstantiationException` (wrapping the
`IllegalArgumentException` as the cause). Renaming the test makes the expected
behavior clearer.
##########
amber/src/main/scala/org/apache/texera/workflow/LogicalLink.scala:
##########
@@ -20,9 +20,41 @@
package org.apache.texera.workflow
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"}`). Keep in sync with the
+ // workflow-compiling-service LogicalLink.readOperatorIdentity, which is
+ // intentionally identical.
+ 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 with an id
field, but was ${node.getNodeType}"
+ )
Review Comment:
The error message says the value must be an object "with an id field", but
`readOperatorIdentity` also accepts object-shaped opIds without an `id` field
(returning `OperatorIdentity(null)` and letting the primary constructor's
`require` decide). Consider removing the "with an id field" wording so the
message matches the helper's actual contract.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
