Copilot commented on code in PR #7099: URL: https://github.com/apache/texera/pull/7099#discussion_r3679629829
########## amber/src/test/scala/org/apache/texera/web/model/websocket/request/TexeraWebSocketRequestSpec.scala: ########## @@ -0,0 +1,281 @@ +/* + * 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.web.model.websocket.request + +import com.fasterxml.jackson.annotation.JsonSubTypes +import com.fasterxml.jackson.databind.exc.InvalidTypeIdException +import org.apache.texera.amber.operator.limit.LimitOpDesc +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.apache.texera.web.model.websocket.request.python.{ + DebugCommandRequest, + PythonExpressionEvaluateRequest +} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Pins the client -> server half of the websocket wire contract. + * + * `WorkflowWebsocketResource.myOnMsg` deserializes every inbound frame with + * `objectMapper.readValue(message, classOf[TexeraWebSocketRequest])`, so this spec + * uses the very same `JSONUtils.objectMapper` (DefaultScalaModule + NoCtorDeserModule + * + Include.NON_ABSENT). A fresh `new ObjectMapper()` would test fiction: without + * DefaultScalaModule none of the Scala case classes below bind at all. + * + * Why the discriminator strings are asserted literally: `TexeraWebSocketRequest` + * carries `@JsonTypeInfo(use = Id.NAME, include = As.PROPERTY, property = "type")` + * and every `@JsonSubTypes.Type` entry omits `name =`, so Jackson's + * `TypeNameIdResolver` falls back to the BARE SIMPLE CLASS NAME as the wire id. + * The Angular client hard-codes those same strings; from + * `frontend/src/app/workspace/types/workflow-websocket.interface.ts`: + * + * "Each type definition MUST follow the following rules: + * in either TexeraWebsocketRequestTypeMap or TexeraWebsocketEventTypeMap + * add a map entry: + * 1. key is the 'type' string, it must be the same as corresponding backend class name + * 2. value is the payload this request/event needs" + * + * The keys of `TexeraWebsocketRequestTypeMap` there are `EditingTimeCompilationRequest`, + * `HeartBeatRequest`, `ModifyLogicRequest`, `ResultExportRequest`, + * `ResultPaginationRequest`, `RetryRequest`, `SkipTupleRequest`, + * `WorkflowExecuteRequest`, `WorkflowKillRequest`, `WorkflowPauseRequest`, + * `WorkflowCheckpointRequest`, `WorkflowResumeRequest`, + * `PythonExpressionEvaluateRequest` and `DebugCommandRequest`. Renaming a Scala class + * in this package compiles cleanly on both sides and silently breaks the UI at + * runtime -- that is the breakage this spec exists to catch. + * + * `ResultPaginationRequest`'s default arguments are the highest-value pin here and sit + * on the live pagination path: the TS `PaginationRequest` declares `columnOffset?`, + * `columnLimit?` and `columnSearch?` as OPTIONAL, so real frames omit them and the + * server must fill in 0 / Int.MaxValue / None. Those defaults only materialize because + * DefaultScalaModule calls the synthetic `$lessinit$greater$default$N` methods -- drop + * that module and `columnLimit` binds to 0, so result pagination would silently return + * zero columns for every request instead of failing loudly. + */ +class TexeraWebSocketRequestSpec extends AnyFlatSpec with Matchers { + + private def read(json: String): TexeraWebSocketRequest = + objectMapper.readValue(json, classOf[TexeraWebSocketRequest]) + + private def frame(typeId: String, fields: String): String = + if (fields.isEmpty) s"""{"type":"$typeId"}""" else s"""{"type":"$typeId",$fields}""" + + // A LogicalPlanPojo / EditingTimeCompilationRequest payload with all four lists empty. + private val emptyPlanFields = + """"operators":[],"links":[],"opsToViewResult":[],"opsToReuseResult":[]""" + + // LogicalOp is itself polymorphic on a *different* property ("operatorType"), so a + // nested op exercises both discriminators in one frame. + private val limitOpJson = """{"operatorType":"Limit","limit":7}""" + + private val executeFields = + s""""executionName":"exec-alpha","engineVersion":"engine-beta",""" + + s""""logicalPlan":{$emptyPlanFields},"workflowSettings":{},""" + + s""""emailNotificationEnabled":false,"computingUnitId":3""" + + /** + * (wire type id, extra JSON fields, expected concrete class). The payloads are + * built from each subtype's own declared field names, so the table pins what the + * *server* accepts; the expected class proves the id resolved to the right + * subtype rather than to a same-shaped sibling. + * + * Note this is deliberately not a claim that every payload here matches what the + * shipped Angular client sends. `SkipTupleRequest` is a known divergence β the + * client sends `workers` (execute-workflow.service.ts) while the case class + * declares `workerIds`, and the shared mapper never disables + * FAIL_ON_UNKNOWN_PROPERTIES, so the real client frame would be rejected. That + * is a production naming bug, not something to encode as an expectation here; + * the feature is disabled anyway (ExecutionRuntimeService throws + * "skipping tuple is temporarily disabled" before reading the field). + */ + private val registeredRequests: List[(String, String, Class[_ <: TexeraWebSocketRequest])] = + List( + ("EditingTimeCompilationRequest", emptyPlanFields, classOf[EditingTimeCompilationRequest]), + ("HeartBeatRequest", "", classOf[HeartBeatRequest]), + ("ModifyLogicRequest", s""""operator":$limitOpJson""", classOf[ModifyLogicRequest]), + ( + "ResultPaginationRequest", + """"requestID":"req-1","operatorID":"op-2","pageIndex":3,"pageSize":25""", + classOf[ResultPaginationRequest] + ), + ("RetryRequest", """"workers":["worker-1"]""", classOf[RetryRequest]), + ("SkipTupleRequest", """"workerIds":["worker-1"]""", classOf[SkipTupleRequest]), + ("WorkflowExecuteRequest", executeFields, classOf[WorkflowExecuteRequest]), + ("WorkflowKillRequest", "", classOf[WorkflowKillRequest]), + ("WorkflowPauseRequest", "", classOf[WorkflowPauseRequest]), + ("WorkflowResumeRequest", "", classOf[WorkflowResumeRequest]), + ("WorkflowCheckpointRequest", "", classOf[WorkflowCheckpointRequest]), + ( + "PythonExpressionEvaluateRequest", + """"expression":"1 + 1","operatorId":"op-eval"""", + classOf[PythonExpressionEvaluateRequest] + ), + ( + "DebugCommandRequest", + """"operatorId":"op-dbg","workerId":"worker-dbg","cmd":"break 12"""", + classOf[DebugCommandRequest] + ) + ) + + // The 13 strings the Angular client is allowed to put in "type". Spelled out rather + // than derived so a rename shows up as a set diff instead of quietly re-deriving. + private val expectedTypeIds: Set[String] = Set( + "EditingTimeCompilationRequest", + "HeartBeatRequest", + "ModifyLogicRequest", + "ResultPaginationRequest", + "RetryRequest", + "SkipTupleRequest", + "WorkflowExecuteRequest", + "WorkflowKillRequest", + "WorkflowPauseRequest", + "WorkflowResumeRequest", + "WorkflowCheckpointRequest", + "PythonExpressionEvaluateRequest", + "DebugCommandRequest" + ) + + "TexeraWebSocketRequest @JsonSubTypes" should + "register exactly the wire type ids the Angular client sends" in { + val subTypes = classOf[TexeraWebSocketRequest].getAnnotation(classOf[JsonSubTypes]) + subTypes should not be null + subTypes.value().map(_.value().getSimpleName).toSet shouldBe expectedTypeIds + } + + it should "leave every subtype unnamed so the wire id stays the simple class name" in { + // Adding `name = "..."` to any entry would change that subtype's wire id without + // touching the class name, which the Angular map keys on. + val named = classOf[TexeraWebSocketRequest] + .getAnnotation(classOf[JsonSubTypes]) + .value() + .filter(_.name().nonEmpty) + .map(t => s"${t.value().getSimpleName}=${t.name()}") + named.toList shouldBe empty + } + + "every registered request type" should "deserialize through the polymorphic base" in { + registeredRequests.map(_._1).toSet shouldBe expectedTypeIds + registeredRequests.foreach { + case (typeId, fields, expected) => + withClue(s"""type id "$typeId": """) { + read(frame(typeId, fields)).getClass shouldBe expected + } + } + } + + "an unknown type id" should "be rejected instead of silently ignored" in { + // A stale/typo'd client is a real path; it must fail loudly at the mapper. + val ex = intercept[InvalidTypeIdException](read("""{"type":"ResultExportRequest"}""")) + ex.getMessage should include("ResultExportRequest") + } Review Comment: The negative test for an unknown type id currently uses `ResultExportRequest`, which is *defined* in the frontend websocket type map. Using a clearly nonexistent discriminator makes the scenario unambiguous and avoids suggesting `ResultExportRequest` is a websocket subtype the server should accept. ########## amber/src/test/scala/org/apache/texera/web/model/websocket/request/TexeraWebSocketRequestSpec.scala: ########## @@ -0,0 +1,281 @@ +/* + * 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.web.model.websocket.request + +import com.fasterxml.jackson.annotation.JsonSubTypes +import com.fasterxml.jackson.databind.exc.InvalidTypeIdException +import org.apache.texera.amber.operator.limit.LimitOpDesc +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.apache.texera.web.model.websocket.request.python.{ + DebugCommandRequest, + PythonExpressionEvaluateRequest +} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Pins the client -> server half of the websocket wire contract. + * + * `WorkflowWebsocketResource.myOnMsg` deserializes every inbound frame with + * `objectMapper.readValue(message, classOf[TexeraWebSocketRequest])`, so this spec + * uses the very same `JSONUtils.objectMapper` (DefaultScalaModule + NoCtorDeserModule + * + Include.NON_ABSENT). A fresh `new ObjectMapper()` would test fiction: without + * DefaultScalaModule none of the Scala case classes below bind at all. + * + * Why the discriminator strings are asserted literally: `TexeraWebSocketRequest` + * carries `@JsonTypeInfo(use = Id.NAME, include = As.PROPERTY, property = "type")` + * and every `@JsonSubTypes.Type` entry omits `name =`, so Jackson's + * `TypeNameIdResolver` falls back to the BARE SIMPLE CLASS NAME as the wire id. + * The Angular client hard-codes those same strings; from + * `frontend/src/app/workspace/types/workflow-websocket.interface.ts`: + * + * "Each type definition MUST follow the following rules: + * in either TexeraWebsocketRequestTypeMap or TexeraWebsocketEventTypeMap + * add a map entry: + * 1. key is the 'type' string, it must be the same as corresponding backend class name + * 2. value is the payload this request/event needs" + * + * The keys of `TexeraWebsocketRequestTypeMap` there are `EditingTimeCompilationRequest`, + * `HeartBeatRequest`, `ModifyLogicRequest`, `ResultExportRequest`, + * `ResultPaginationRequest`, `RetryRequest`, `SkipTupleRequest`, + * `WorkflowExecuteRequest`, `WorkflowKillRequest`, `WorkflowPauseRequest`, + * `WorkflowCheckpointRequest`, `WorkflowResumeRequest`, + * `PythonExpressionEvaluateRequest` and `DebugCommandRequest`. Renaming a Scala class + * in this package compiles cleanly on both sides and silently breaks the UI at + * runtime -- that is the breakage this spec exists to catch. + * + * `ResultPaginationRequest`'s default arguments are the highest-value pin here and sit + * on the live pagination path: the TS `PaginationRequest` declares `columnOffset?`, + * `columnLimit?` and `columnSearch?` as OPTIONAL, so real frames omit them and the + * server must fill in 0 / Int.MaxValue / None. Those defaults only materialize because + * DefaultScalaModule calls the synthetic `$lessinit$greater$default$N` methods -- drop + * that module and `columnLimit` binds to 0, so result pagination would silently return + * zero columns for every request instead of failing loudly. + */ +class TexeraWebSocketRequestSpec extends AnyFlatSpec with Matchers { + + private def read(json: String): TexeraWebSocketRequest = + objectMapper.readValue(json, classOf[TexeraWebSocketRequest]) + + private def frame(typeId: String, fields: String): String = + if (fields.isEmpty) s"""{"type":"$typeId"}""" else s"""{"type":"$typeId",$fields}""" + + // A LogicalPlanPojo / EditingTimeCompilationRequest payload with all four lists empty. + private val emptyPlanFields = + """"operators":[],"links":[],"opsToViewResult":[],"opsToReuseResult":[]""" + + // LogicalOp is itself polymorphic on a *different* property ("operatorType"), so a + // nested op exercises both discriminators in one frame. + private val limitOpJson = """{"operatorType":"Limit","limit":7}""" + + private val executeFields = + s""""executionName":"exec-alpha","engineVersion":"engine-beta",""" + + s""""logicalPlan":{$emptyPlanFields},"workflowSettings":{},""" + + s""""emailNotificationEnabled":false,"computingUnitId":3""" + + /** + * (wire type id, extra JSON fields, expected concrete class). The payloads are + * built from each subtype's own declared field names, so the table pins what the + * *server* accepts; the expected class proves the id resolved to the right + * subtype rather than to a same-shaped sibling. + * + * Note this is deliberately not a claim that every payload here matches what the + * shipped Angular client sends. `SkipTupleRequest` is a known divergence β the + * client sends `workers` (execute-workflow.service.ts) while the case class + * declares `workerIds`, and the shared mapper never disables + * FAIL_ON_UNKNOWN_PROPERTIES, so the real client frame would be rejected. That + * is a production naming bug, not something to encode as an expectation here; + * the feature is disabled anyway (ExecutionRuntimeService throws + * "skipping tuple is temporarily disabled" before reading the field). + */ + private val registeredRequests: List[(String, String, Class[_ <: TexeraWebSocketRequest])] = + List( + ("EditingTimeCompilationRequest", emptyPlanFields, classOf[EditingTimeCompilationRequest]), + ("HeartBeatRequest", "", classOf[HeartBeatRequest]), + ("ModifyLogicRequest", s""""operator":$limitOpJson""", classOf[ModifyLogicRequest]), + ( + "ResultPaginationRequest", + """"requestID":"req-1","operatorID":"op-2","pageIndex":3,"pageSize":25""", + classOf[ResultPaginationRequest] + ), + ("RetryRequest", """"workers":["worker-1"]""", classOf[RetryRequest]), + ("SkipTupleRequest", """"workerIds":["worker-1"]""", classOf[SkipTupleRequest]), + ("WorkflowExecuteRequest", executeFields, classOf[WorkflowExecuteRequest]), + ("WorkflowKillRequest", "", classOf[WorkflowKillRequest]), + ("WorkflowPauseRequest", "", classOf[WorkflowPauseRequest]), + ("WorkflowResumeRequest", "", classOf[WorkflowResumeRequest]), + ("WorkflowCheckpointRequest", "", classOf[WorkflowCheckpointRequest]), + ( + "PythonExpressionEvaluateRequest", + """"expression":"1 + 1","operatorId":"op-eval"""", + classOf[PythonExpressionEvaluateRequest] + ), + ( + "DebugCommandRequest", + """"operatorId":"op-dbg","workerId":"worker-dbg","cmd":"break 12"""", + classOf[DebugCommandRequest] + ) + ) + + // The 13 strings the Angular client is allowed to put in "type". Spelled out rather + // than derived so a rename shows up as a set diff instead of quietly re-deriving. Review Comment: The comment above `expectedTypeIds` says these are the strings the Angular client is allowed to send, but the frontend type map also defines `ResultExportRequest` while the backend `TexeraWebSocketRequest` registry does not include it. Consider rewording this comment to reflect that this set is the backend-accepted websocket request ids (and call out the extra frontend-only type separately) to avoid confusion. This issue also appears on line 155 of the same file. ########## amber/src/test/scala/org/apache/texera/web/model/websocket/request/TexeraWebSocketRequestSpec.scala: ########## @@ -0,0 +1,281 @@ +/* + * 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.web.model.websocket.request + +import com.fasterxml.jackson.annotation.JsonSubTypes +import com.fasterxml.jackson.databind.exc.InvalidTypeIdException +import org.apache.texera.amber.operator.limit.LimitOpDesc +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.apache.texera.web.model.websocket.request.python.{ + DebugCommandRequest, + PythonExpressionEvaluateRequest +} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Pins the client -> server half of the websocket wire contract. + * + * `WorkflowWebsocketResource.myOnMsg` deserializes every inbound frame with + * `objectMapper.readValue(message, classOf[TexeraWebSocketRequest])`, so this spec + * uses the very same `JSONUtils.objectMapper` (DefaultScalaModule + NoCtorDeserModule + * + Include.NON_ABSENT). A fresh `new ObjectMapper()` would test fiction: without + * DefaultScalaModule none of the Scala case classes below bind at all. + * + * Why the discriminator strings are asserted literally: `TexeraWebSocketRequest` + * carries `@JsonTypeInfo(use = Id.NAME, include = As.PROPERTY, property = "type")` + * and every `@JsonSubTypes.Type` entry omits `name =`, so Jackson's + * `TypeNameIdResolver` falls back to the BARE SIMPLE CLASS NAME as the wire id. + * The Angular client hard-codes those same strings; from + * `frontend/src/app/workspace/types/workflow-websocket.interface.ts`: + * + * "Each type definition MUST follow the following rules: + * in either TexeraWebsocketRequestTypeMap or TexeraWebsocketEventTypeMap + * add a map entry: + * 1. key is the 'type' string, it must be the same as corresponding backend class name + * 2. value is the payload this request/event needs" + * + * The keys of `TexeraWebsocketRequestTypeMap` there are `EditingTimeCompilationRequest`, + * `HeartBeatRequest`, `ModifyLogicRequest`, `ResultExportRequest`, + * `ResultPaginationRequest`, `RetryRequest`, `SkipTupleRequest`, + * `WorkflowExecuteRequest`, `WorkflowKillRequest`, `WorkflowPauseRequest`, + * `WorkflowCheckpointRequest`, `WorkflowResumeRequest`, + * `PythonExpressionEvaluateRequest` and `DebugCommandRequest`. Renaming a Scala class + * in this package compiles cleanly on both sides and silently breaks the UI at + * runtime -- that is the breakage this spec exists to catch. + * + * `ResultPaginationRequest`'s default arguments are the highest-value pin here and sit + * on the live pagination path: the TS `PaginationRequest` declares `columnOffset?`, + * `columnLimit?` and `columnSearch?` as OPTIONAL, so real frames omit them and the + * server must fill in 0 / Int.MaxValue / None. Those defaults only materialize because + * DefaultScalaModule calls the synthetic `$lessinit$greater$default$N` methods -- drop + * that module and `columnLimit` binds to 0, so result pagination would silently return + * zero columns for every request instead of failing loudly. + */ +class TexeraWebSocketRequestSpec extends AnyFlatSpec with Matchers { + + private def read(json: String): TexeraWebSocketRequest = + objectMapper.readValue(json, classOf[TexeraWebSocketRequest]) + + private def frame(typeId: String, fields: String): String = + if (fields.isEmpty) s"""{"type":"$typeId"}""" else s"""{"type":"$typeId",$fields}""" + + // A LogicalPlanPojo / EditingTimeCompilationRequest payload with all four lists empty. + private val emptyPlanFields = + """"operators":[],"links":[],"opsToViewResult":[],"opsToReuseResult":[]""" + + // LogicalOp is itself polymorphic on a *different* property ("operatorType"), so a + // nested op exercises both discriminators in one frame. + private val limitOpJson = """{"operatorType":"Limit","limit":7}""" + + private val executeFields = + s""""executionName":"exec-alpha","engineVersion":"engine-beta",""" + + s""""logicalPlan":{$emptyPlanFields},"workflowSettings":{},""" + + s""""emailNotificationEnabled":false,"computingUnitId":3""" + + /** + * (wire type id, extra JSON fields, expected concrete class). The payloads are + * built from each subtype's own declared field names, so the table pins what the + * *server* accepts; the expected class proves the id resolved to the right + * subtype rather than to a same-shaped sibling. + * + * Note this is deliberately not a claim that every payload here matches what the + * shipped Angular client sends. `SkipTupleRequest` is a known divergence β the + * client sends `workers` (execute-workflow.service.ts) while the case class + * declares `workerIds`, and the shared mapper never disables + * FAIL_ON_UNKNOWN_PROPERTIES, so the real client frame would be rejected. That + * is a production naming bug, not something to encode as an expectation here; + * the feature is disabled anyway (ExecutionRuntimeService throws + * "skipping tuple is temporarily disabled" before reading the field). + */ + private val registeredRequests: List[(String, String, Class[_ <: TexeraWebSocketRequest])] = + List( + ("EditingTimeCompilationRequest", emptyPlanFields, classOf[EditingTimeCompilationRequest]), + ("HeartBeatRequest", "", classOf[HeartBeatRequest]), + ("ModifyLogicRequest", s""""operator":$limitOpJson""", classOf[ModifyLogicRequest]), + ( + "ResultPaginationRequest", + """"requestID":"req-1","operatorID":"op-2","pageIndex":3,"pageSize":25""", + classOf[ResultPaginationRequest] + ), + ("RetryRequest", """"workers":["worker-1"]""", classOf[RetryRequest]), + ("SkipTupleRequest", """"workerIds":["worker-1"]""", classOf[SkipTupleRequest]), + ("WorkflowExecuteRequest", executeFields, classOf[WorkflowExecuteRequest]), + ("WorkflowKillRequest", "", classOf[WorkflowKillRequest]), + ("WorkflowPauseRequest", "", classOf[WorkflowPauseRequest]), + ("WorkflowResumeRequest", "", classOf[WorkflowResumeRequest]), + ("WorkflowCheckpointRequest", "", classOf[WorkflowCheckpointRequest]), + ( + "PythonExpressionEvaluateRequest", + """"expression":"1 + 1","operatorId":"op-eval"""", + classOf[PythonExpressionEvaluateRequest] + ), + ( + "DebugCommandRequest", + """"operatorId":"op-dbg","workerId":"worker-dbg","cmd":"break 12"""", + classOf[DebugCommandRequest] + ) + ) + + // The 13 strings the Angular client is allowed to put in "type". Spelled out rather + // than derived so a rename shows up as a set diff instead of quietly re-deriving. + private val expectedTypeIds: Set[String] = Set( + "EditingTimeCompilationRequest", + "HeartBeatRequest", + "ModifyLogicRequest", + "ResultPaginationRequest", + "RetryRequest", + "SkipTupleRequest", + "WorkflowExecuteRequest", + "WorkflowKillRequest", + "WorkflowPauseRequest", + "WorkflowResumeRequest", + "WorkflowCheckpointRequest", + "PythonExpressionEvaluateRequest", + "DebugCommandRequest" + ) + + "TexeraWebSocketRequest @JsonSubTypes" should + "register exactly the wire type ids the Angular client sends" in { + val subTypes = classOf[TexeraWebSocketRequest].getAnnotation(classOf[JsonSubTypes]) + subTypes should not be null + subTypes.value().map(_.value().getSimpleName).toSet shouldBe expectedTypeIds + } + + it should "leave every subtype unnamed so the wire id stays the simple class name" in { + // Adding `name = "..."` to any entry would change that subtype's wire id without + // touching the class name, which the Angular map keys on. + val named = classOf[TexeraWebSocketRequest] + .getAnnotation(classOf[JsonSubTypes]) + .value() + .filter(_.name().nonEmpty) + .map(t => s"${t.value().getSimpleName}=${t.name()}") + named.toList shouldBe empty + } + + "every registered request type" should "deserialize through the polymorphic base" in { + registeredRequests.map(_._1).toSet shouldBe expectedTypeIds + registeredRequests.foreach { + case (typeId, fields, expected) => + withClue(s"""type id "$typeId": """) { + read(frame(typeId, fields)).getClass shouldBe expected + } + } + } + + "an unknown type id" should "be rejected instead of silently ignored" in { + // A stale/typo'd client is a real path; it must fail loudly at the mapper. + val ex = intercept[InvalidTypeIdException](read("""{"type":"ResultExportRequest"}""")) + ex.getMessage should include("ResultExportRequest") + } + + "a frame with no type property" should "be rejected" in { + val ex = intercept[InvalidTypeIdException]( + read("""{"requestID":"req-1","operatorID":"op-2","pageIndex":3,"pageSize":25}""") + ) + ex.getMessage should include("missing type id property 'type'") + } Review Comment: Asserting on the full Jackson exception message text is brittle across Jackson versions. Itβs safer to assert only that the message mentions the missing discriminator property (e.g., contains "type"). ########## amber/src/test/scala/org/apache/texera/web/model/websocket/event/TexeraWebSocketEventSpec.scala: ########## @@ -0,0 +1,347 @@ +/* + * 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.web.model.websocket.event + +import com.fasterxml.jackson.annotation.JsonSubTypes +import com.google.protobuf.timestamp.Timestamp +import org.apache.texera.amber.core.tuple.{Attribute, AttributeType} +import org.apache.texera.amber.core.workflowruntimestate.{FatalErrorType, WorkflowFatalError} +import org.apache.texera.amber.engine.architecture.rpc.controlcommands.{ + ConsoleMessage, + ConsoleMessageType +} +import org.apache.texera.amber.engine.architecture.rpc.controlreturns.{EvaluatedValue, TypedValue} +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.apache.texera.web.model.websocket.event.python.ConsoleUpdateEvent +import org.apache.texera.web.model.websocket.response.python.PythonExpressionEvaluateResponse +import org.apache.texera.web.model.websocket.response.{ + ClusterStatusUpdateEvent, + HeartBeatResponse, + ModifyLogicCompletedEvent, + ModifyLogicResponse, + RegionUpdateEvent +} +import org.apache.texera.web.service.ExecutionResultService.{ + PaginationMode, + SetDeltaMode, + WebDataUpdate, + WebPaginationUpdate +} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.time.Instant + +/** + * Pins the server -> client half of the websocket wire contract. + * + * `SessionState.send` ships every event with + * `session.getAsyncRemote.sendText(objectMapper.writeValueAsString(msg))`, where + * `msg: TexeraWebSocketEvent` and the mapper is `JSONUtils.objectMapper`. This spec + * uses that exact mapper and that exact call so the emitted bytes are the real ones. + * + * `TexeraWebSocketEvent` carries + * `@JsonTypeInfo(use = Id.NAME, include = As.PROPERTY, property = "type")` and none of + * its `@JsonSubTypes.Type` entries supply `name =`, so the wire id is Jackson's + * `TypeNameIdResolver` default: the BARE SIMPLE CLASS NAME. The Angular client + * switches on those literals; `frontend/src/app/workspace/types/workflow-websocket.interface.ts` + * spells the rule out: + * + * "Each type definition MUST follow the following rules: + * in either TexeraWebsocketRequestTypeMap or TexeraWebsocketEventTypeMap + * add a map entry: + * 1. key is the 'type' string, it must be the same as corresponding backend class name + * 2. value is the payload this request/event needs" + * + * Its `TexeraWebsocketEventTypeMap` keys include `HeartBeatResponse`, + * `WorkflowStateEvent`, `OperatorStatisticsUpdateEvent`, `WebResultUpdateEvent`, + * `WorkflowErrorEvent`, `ConsoleUpdateEvent`, `PaginatedResultEvent`, + * `CacheStatusUpdateEvent`, `PythonExpressionEvaluateResponse`, + * `WorkerAssignmentUpdateEvent`, `ModifyLogicResponse`, `ModifyLogicCompletedEvent`, + * `ExecutionDurationUpdateEvent`, `ClusterStatusUpdateEvent`, `RegionUpdateEvent` and + * `RegionStateEvent`. Renaming a Scala event class compiles fine on both sides and + * silently drops the corresponding UI update -- that is what this spec guards. + * + * Deliberate asymmetry: events are tested SERIALIZE-first. Five live event classes + * (`ExecutionDurationUpdateEvent`, `RegionStateEvent`, `ModifyLogicCompletedEvent`, + * `ClusterStatusUpdateEvent`, `RegionUpdateEvent`) are NOT listed in `@JsonSubTypes`. + * They serialize correctly through the simple-name fallback, but `typeFromId` cannot + * resolve them, so a blanket read-back loop would throw `InvalidTypeIdException`. + * That is harmless today because those events are outbound only -- and it is pinned + * below so nobody assumes the tier is symmetric. + */ +class TexeraWebSocketEventSpec extends AnyFlatSpec with Matchers { + + private def write(e: TexeraWebSocketEvent): String = objectMapper.writeValueAsString(e) + + private def typeIdOf(e: TexeraWebSocketEvent): String = + objectMapper.readTree(write(e)).get("type").asText() + + private val fatalError = WorkflowFatalError( + FatalErrorType.EXECUTION_FAILURE, + Timestamp(Instant.ofEpochSecond(1_700_000_000L)), + "message-1", + "details-2", + "op-3", + "worker-4" + ) + + private val consoleMessage = ConsoleMessage( + "worker-5", + Timestamp(Instant.ofEpochSecond(1_700_000_001L)), + ConsoleMessageType.PRINT, + "source-6", + "title-7", + "message-8" + ) + + private val metrics = OperatorAggregatedMetrics( + operatorState = "COMPLETED", + aggregatedInputRowCount = 11L, + aggregatedInputSize = 12L, + inputPortMetrics = Map("in-0" -> 13L), + aggregatedOutputRowCount = 14L, + aggregatedOutputSize = 15L, + outputPortMetrics = Map("out-0" -> 16L), + numWorkers = 17L, + aggregatedDataProcessingTime = 18L, + aggregatedControlProcessingTime = 19L, + aggregatedIdleTime = 20L + ) + + private val resultRow = objectMapper.createObjectNode().put("city", "Irvine") + + /** The 11 events that ARE in `@JsonSubTypes`, one instance each. */ + private val registeredEvents: List[(String, TexeraWebSocketEvent)] = List( + "HeartBeatResponse" -> HeartBeatResponse(), + "WorkflowErrorEvent" -> WorkflowErrorEvent(Seq(fatalError)), + "WorkflowStateEvent" -> WorkflowStateEvent("Running"), + "OperatorStatisticsUpdateEvent" -> OperatorStatisticsUpdateEvent(Map("op-stats" -> metrics)), + "WebResultUpdateEvent" -> WebResultUpdateEvent( + Map("op-page" -> WebPaginationUpdate(PaginationMode(), 7L, List(1, 3))), + Map("op-page" -> Map("city" -> Map[String, Any]("distinct" -> 2))) + ), + "ConsoleUpdateEvent" -> ConsoleUpdateEvent("op-console", Seq(consoleMessage)), + "CacheStatusUpdateEvent" -> CacheStatusUpdateEvent(Map("op-cache" -> "cache valid")), + "PaginatedResultEvent" -> PaginatedResultEvent( + "req-1", + "op-2", + 3, + List(resultRow), + List(new Attribute("city", AttributeType.STRING)) + ), + "PythonExpressionEvaluateResponse" -> PythonExpressionEvaluateResponse( + "len(tuple_)", + Seq(EvaluatedValue(Some(TypedValue("expr-1", "ref-2", "str-3", "type-4", true)), Seq.empty)) + ), + "WorkerAssignmentUpdateEvent" -> WorkerAssignmentUpdateEvent( + "op-assign", + Seq("worker-9", "worker-10") + ), + "ModifyLogicResponse" -> ModifyLogicResponse("op-modify", isValid = false, "error-11") + ) + + /** + * Live events with a real producer that are NOT in `@JsonSubTypes`. The producers are + * `ExecutionStatsService` (duration), `RegionExecutionManager` (region state), + * `ExecutionReconfigurationService` (modify-logic completed), `ClusterListener` / + * `WorkflowWebsocketResource` (cluster status) and `Coordinator` (region update). + * + * `WorkflowAvailableResultEvent` is the sixth unregistered subtype but is deliberately + * absent from this list: nothing in main constructs it, so pinning its wire shape would + * only cement dead code. + */ + private val outboundOnlyEvents: List[(String, TexeraWebSocketEvent)] = List( + "ExecutionDurationUpdateEvent" -> ExecutionDurationUpdateEvent(1234L, isRunning = true), + "RegionStateEvent" -> RegionStateEvent(21L, "RUNNING"), + "ModifyLogicCompletedEvent" -> ModifyLogicCompletedEvent(List("op-22")), + "ClusterStatusUpdateEvent" -> ClusterStatusUpdateEvent(23), + "RegionUpdateEvent" -> RegionUpdateEvent(List((24L, List("op-25")))) + ) + + // Spelled out rather than derived, so a class rename surfaces as a set diff. + private val expectedTypeIds: Set[String] = Set( + "HeartBeatResponse", + "WorkflowErrorEvent", + "WorkflowStateEvent", + "OperatorStatisticsUpdateEvent", + "WebResultUpdateEvent", + "ConsoleUpdateEvent", + "CacheStatusUpdateEvent", + "PaginatedResultEvent", + "PythonExpressionEvaluateResponse", + "WorkerAssignmentUpdateEvent", + "ModifyLogicResponse" + ) + + "TexeraWebSocketEvent @JsonSubTypes" should + "register exactly the wire type ids the Angular client resolves" in { + val subTypes = classOf[TexeraWebSocketEvent].getAnnotation(classOf[JsonSubTypes]) + subTypes should not be null + subTypes.value().map(_.value().getSimpleName).toSet shouldBe expectedTypeIds + } Review Comment: This test description says the registered ids are exactly what the Angular client resolves, but the frontend `TexeraWebsocketEventTypeMap` contains additional event discriminators beyond the backend `@JsonSubTypes` list. Rewording this to refer to the backend registry will make the contract being asserted clearer. -- 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]
