Yicong-Huang commented on code in PR #5434:
URL: https://github.com/apache/texera/pull/5434#discussion_r3524397066
##########
amber/src/test/scala/org/apache/texera/web/service/ExecutionResultServiceSpec.scala:
##########
@@ -36,8 +56,110 @@ import
org.apache.texera.web.service.ExecutionResultService.{
}
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers
+import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach}
+
+import java.net.URI
+import java.sql.Timestamp
+import scala.jdk.CollectionConverters._
+
+class ExecutionResultServiceSpec
+ extends AnyFlatSpec
+ with Matchers
+ with BeforeAndAfterAll
+ with BeforeAndAfterEach
+ with MockTexeraDB {
+
+ private val testWid: Integer = 9000 + scala.util.Random.nextInt(1000)
+ private val testUid: Integer = 9000 + scala.util.Random.nextInt(1000)
+ private var executionsDao: WorkflowExecutionsDao = _
+
+ override protected def beforeAll(): Unit = {
+ initializeDBAndReplaceDSLContext()
+ }
+
+ override protected def afterAll(): Unit = {
+ shutdownDB()
+ }
+
+ override protected def beforeEach(): Unit = {
+ val user = new User
+ user.setUid(testUid)
+ user.setName("execution-result-test-user")
+ user.setEmail(s"[email protected]")
+ user.setPassword("password")
+ new UserDao(getDSLContext.configuration()).insert(user)
+
+ val workflow = new Workflow
+ workflow.setWid(testWid)
+ workflow.setName(s"execution-result-test-$testWid")
+ workflow.setContent("{}")
+ workflow.setDescription("")
+ workflow.setCreationTime(new Timestamp(System.currentTimeMillis()))
+ workflow.setLastModifiedTime(new Timestamp(System.currentTimeMillis()))
+ new WorkflowDao(getDSLContext.configuration()).insert(workflow)
+
+ val version = new WorkflowVersion
+ version.setWid(testWid)
+ version.setContent("{}")
+ version.setCreationTime(new Timestamp(System.currentTimeMillis()))
+ new WorkflowVersionDao(getDSLContext.configuration()).insert(version)
+
+ executionsDao = new WorkflowExecutionsDao(getDSLContext.configuration())
+ }
+
+ override protected def afterEach(): Unit = {
+ val ctx = getDSLContext
+ ctx
+ .deleteFrom(OPERATOR_PORT_EXECUTIONS)
+ .where(
+ OPERATOR_PORT_EXECUTIONS.WORKFLOW_EXECUTION_ID.in(
+ ctx.select(WORKFLOW_EXECUTIONS.EID).from(WORKFLOW_EXECUTIONS)
+ )
+ )
+ .execute()
+ ctx.deleteFrom(WORKFLOW_EXECUTIONS).execute()
+ import org.apache.texera.dao.jooq.generated.Tables.{USER, WORKFLOW,
WORKFLOW_VERSION}
+
ctx.deleteFrom(WORKFLOW_VERSION).where(WORKFLOW_VERSION.WID.eq(testWid)).execute()
+ ctx.deleteFrom(WORKFLOW).where(WORKFLOW.WID.eq(testWid)).execute()
+ ctx.deleteFrom(USER).where(USER.UID.eq(testUid)).execute()
+ }
-class ExecutionResultServiceSpec extends AnyFlatSpec with Matchers {
+ "persistOperatorPortResultUri" should
+ "insert the URI carried by an OperatorPortResultUriAvailable event" in {
+ val execution = new WorkflowExecutions
+ execution.setVid(1)
Review Comment:
Done — the spec now captures the generated key right after the insert
(`testVid = version.getVid`) and uses it everywhere instead of the hardcoded
`1`.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -239,15 +246,11 @@ object WorkflowExecutionsResource {
globalPortId: GlobalPortIdentity,
uri: URI
): Unit = {
- context
- .insertInto(OPERATOR_PORT_EXECUTIONS)
- .columns(
- OPERATOR_PORT_EXECUTIONS.WORKFLOW_EXECUTION_ID,
- OPERATOR_PORT_EXECUTIONS.GLOBAL_PORT_ID,
- OPERATOR_PORT_EXECUTIONS.RESULT_URI
- )
- .values(eid.id.toInt, globalPortId.serializeAsString, uri.toString)
- .execute()
+ val pojo = new OperatorPortExecutions()
+ pojo.setWorkflowExecutionId(eid.id.toInt)
+ pojo.setGlobalPortId(globalPortId.serializeAsString)
+ pojo.setResultUri(uri.toString)
+ new OperatorPortExecutionsDao(context.configuration()).insert(pojo)
Review Comment:
Updated the description — the `withTransaction` mention is gone, and the
helper is now actually deleted (inlined into
`ExecutionResultService.persistOperatorPortResultUri` per Xiao-zhen's comment),
so the description matches the diff again.
##########
amber/src/main/scala/org/apache/texera/web/service/ExecutionResultService.scala:
##########
@@ -353,6 +357,10 @@ class ExecutionResultService(
)
)
+ addSubscription(
Review Comment:
There isn't much to assert directly for the missing-registration case — if
no subscriber is attached, `ClientActor.handleClientEvent` just drops the
event, so the only observable effect is "no row appears". Two things now guard
this seam instead: the registration site carries a comment documenting the
must-register-before-`startWorkflow` invariant and its silent failure mode, and
the e2e suites cover the wiring positively — they register the same production
callback body (`TestUtils.registerResultUriPersistence`) and then read the URI
back from the DB via `getResultUriByLogicalPortId`, which is exactly the lookup
that goes empty when the registration is missing (that's how the e2e fixtures
surfaced the need for it).
--
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]
