aglinxinyuan commented on code in PR #4956: URL: https://github.com/apache/texera/pull/4956#discussion_r3231137956
########## amber/src/test/scala/org/apache/texera/workflow/LogicalLinkSpec.scala: ########## @@ -0,0 +1,240 @@ +/* + * 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.workflow + +import com.fasterxml.jackson.databind.JsonNode +import com.fasterxml.jackson.databind.exc.MismatchedInputException +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 + +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) + } + + it should "consider a self-loop link well-formed (same fromOpId / toOpId, distinct ports)" in { + // Self-loops aren't structurally invalid at the LogicalLink level — + // higher layers reject cycles, but the data type allows fromOpId == + // toOpId. Pin so a future == check on construction breaks this on + // purpose. + val selfLoop = LogicalLink( + OperatorIdentity("op-A"), + PortIdentity(0), + OperatorIdentity("op-A"), + PortIdentity(1) + ) + assert(selfLoop.fromOpId == selfLoop.toOpId) Review Comment: Done in cd5bb17138 — the primary constructor now `require`s `fromOpId != toOpId` and throws `IllegalArgumentException("LogicalLink self-loop not allowed: …")`. Covered by `should reject a self-loop link (fromOpId == toOpId) regardless of port` (primary-constructor path) and `should reject a self-loop via the @JsonCreator constructor (same string op id)` (string-overload path). ########## amber/src/test/scala/org/apache/texera/workflow/LogicalLinkSpec.scala: ########## @@ -0,0 +1,240 @@ +/* + * 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.workflow + +import com.fasterxml.jackson.databind.JsonNode +import com.fasterxml.jackson.databind.exc.MismatchedInputException +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 + +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) + } + + it should "consider a self-loop link well-formed (same fromOpId / toOpId, distinct ports)" in { + // Self-loops aren't structurally invalid at the LogicalLink level — + // higher layers reject cycles, but the data type allows fromOpId == + // toOpId. Pin so a future == check on construction breaks this on + // purpose. + val selfLoop = LogicalLink( + OperatorIdentity("op-A"), + PortIdentity(0), + OperatorIdentity("op-A"), + PortIdentity(1) + ) + assert(selfLoop.fromOpId == selfLoop.toOpId) + } + + // --------------------------------------------------------------------------- + // Secondary @JsonCreator constructor (string opId variant) + // --------------------------------------------------------------------------- + + "LogicalLink secondary @JsonCreator 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")) + // Equal to a link built via the primary constructor. + assert( + link == LogicalLink( + OperatorIdentity("op-A"), + PortIdentity(0), + OperatorIdentity("op-B"), + PortIdentity(1) + ) + ) + } + + it should "accept identifiers containing dashes / dots / digits (no normalization)" 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")) + } + + it should "accept the empty string as an op id (no validation in the data type)" in { + // Pin: the secondary constructor does not validate; an empty string + // wraps into `OperatorIdentity("")`. A future change adding non-empty + // validation should fail this test on purpose. + val link = new LogicalLink("", PortIdentity(0), "", PortIdentity(1)) + assert(link.fromOpId == OperatorIdentity("")) + assert(link.toOpId == OperatorIdentity("")) Review Comment: Done in cd5bb17138 — `OperatorIdentity("")` is now rejected by the primary constructor's `require(fromOpId.id.nonEmpty, …)`. The `@JsonCreator` string overload routes through the primary constructor, so empty strings are rejected there too. Covered by `should reject an OperatorIdentity wrapping an empty id in the primary constructor` and `should reject the empty string as an op id via the @JsonCreator constructor`. -- 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]
