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-5300-d26bff669e0e253da451be3e64567a353036e00d in repository https://gitbox.apache.org/repos/asf/texera.git
commit 531d79ff226acba9d5f8d1d13d57564359ba0cec Author: Matthew B. <[email protected]> AuthorDate: Mon Jun 1 16:32:17 2026 -0700 fix: require non-null uid in insertNewExecution (#5300) ### What changes were proposed in this PR? - `ExecutionsMetadataPersistService.insertNewExecution` kept its `uid: Option[Integer]` signature but replaced `setUid(uid.orNull)` with `setUid(uid.getOrElse(throw new IllegalArgumentException(...)))`, so a missing uid is rejected with a clear error instead of writing null into the NOT NULL `workflow_executions.uid` column (which produced a cryptic jOOQ DataAccessException). - No `.orNull` is used; the caller (`WorkflowService.initExecutionService`) passes the `Option` through unchanged. - Updated `ExecutionsMetadataPersistServiceSpec`: the missing-uid case now passes `None` and asserts an `IllegalArgumentException` is raised. ### Any related issues, documentation, or discussions? Closes: #5212 ### How was this PR tested? - Compiled amber main + test sources under JDK 17 (clean). - Ran `WorkflowExecutionService/testOnly ...ExecutionsMetadataPersistServiceSpec`: all 9 tests pass. - Ran `sbt scalafmtAll` (clean). ### Was this PR authored or co-authored using generative AI tooling? Co-authored with Claude Opus 4.8 in compliance with ASF --------- Signed-off-by: Matthew B. <[email protected]> --- .../service/ExecutionsMetadataPersistService.scala | 18 +++++++++++--- .../texera/web/service/WorkflowService.scala | 9 ++++++- .../ExecutionsMetadataPersistServiceSpec.scala | 28 +++++++++++++--------- 3 files changed, 40 insertions(+), 15 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/service/ExecutionsMetadataPersistService.scala b/amber/src/main/scala/org/apache/texera/web/service/ExecutionsMetadataPersistService.scala index b9b29dff72..bafe7a178a 100644 --- a/amber/src/main/scala/org/apache/texera/web/service/ExecutionsMetadataPersistService.scala +++ b/amber/src/main/scala/org/apache/texera/web/service/ExecutionsMetadataPersistService.scala @@ -22,6 +22,7 @@ package org.apache.texera.web.service import com.typesafe.scalalogging.LazyLogging import org.apache.texera.amber.core.virtualidentity.{ExecutionIdentity, WorkflowIdentity} import org.apache.texera.dao.SqlServer +import org.jooq.exception.DataAccessException import org.apache.texera.dao.jooq.generated.tables.daos.WorkflowExecutionsDao import org.apache.texera.dao.jooq.generated.tables.pojos.WorkflowExecutions import org.apache.texera.web.resource.dashboard.user.workflow.WorkflowVersionResource._ @@ -52,7 +53,7 @@ object ExecutionsMetadataPersistService extends LazyLogging { def insertNewExecution( workflowId: WorkflowIdentity, - uid: Option[Integer], + uid: Integer, executionName: String, environmentVersion: String, computingUnitId: Integer @@ -64,14 +65,25 @@ object ExecutionsMetadataPersistService extends LazyLogging { newExecution.setName(executionName) } newExecution.setVid(vid) - newExecution.setUid(uid.orNull) + newExecution.setUid(uid) newExecution.setStartingTime(new Timestamp(System.currentTimeMillis())) newExecution.setEnvironmentVersion(environmentVersion) // Set computing unit ID if provided newExecution.setCuid(computingUnitId) - workflowExecutionsDao.insert(newExecution) + try { + workflowExecutionsDao.insert(newExecution) + } catch { + // A NOT NULL column (e.g. uid, vid, cuid) was null. Postgres reports this + // as SQLState 23502; surface a readable message instead of the raw + // jOOQ/JDBC stack trace, while preserving the original as the cause. + case e: DataAccessException if e.sqlState() == "23502" => + throw new IllegalArgumentException( + "Cannot persist workflow execution: a required field (uid, vid, or cuid) was null.", + e + ) + } ExecutionIdentity(newExecution.getEid.longValue()) } diff --git a/amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala b/amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala index 809faf6a52..0044c95967 100644 --- a/amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala +++ b/amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala @@ -192,6 +192,13 @@ class WorkflowService( val (uidOpt, userEmailOpt) = userOpt.map(user => (user.getUid, user.getEmail)).unzip + // uid is NOT NULL in the DB; fail early here rather than letting the insert fail downstream. + val uid = uidOpt.getOrElse( + throw new IllegalArgumentException( + "Cannot start execution: a user id (uid) is required but none was provided." + ) + ) + val workflowContext: WorkflowContext = createWorkflowContext() var controllerConf = ControllerConfig.default @@ -204,7 +211,7 @@ class WorkflowService( workflowContext.executionId = ExecutionsMetadataPersistService.insertNewExecution( workflowContext.workflowId, - uidOpt, + uid, req.executionName, convertToJson(req.engineVersion), req.computingUnitId diff --git a/amber/src/test/scala/org/apache/texera/web/service/ExecutionsMetadataPersistServiceSpec.scala b/amber/src/test/scala/org/apache/texera/web/service/ExecutionsMetadataPersistServiceSpec.scala index 5c7a687728..1d67fa0331 100644 --- a/amber/src/test/scala/org/apache/texera/web/service/ExecutionsMetadataPersistServiceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/service/ExecutionsMetadataPersistServiceSpec.scala @@ -166,7 +166,7 @@ class ExecutionsMetadataPersistServiceSpec "insertNewExecution" should "insert a row tied to the latest workflow version" in { val id = ExecutionsMetadataPersistService.insertNewExecution( WorkflowIdentity(testWid.toLong), - Some(testUid), + testUid, executionName = "named-execution", environmentVersion = "env-1", computingUnitId = seededCuid @@ -185,7 +185,7 @@ class ExecutionsMetadataPersistServiceSpec it should "skip setName when executionName is the empty string" in { val id = ExecutionsMetadataPersistService.insertNewExecution( WorkflowIdentity(testWid.toLong), - Some(testUid), + testUid, executionName = "", environmentVersion = "env-2", computingUnitId = seededCuid @@ -198,23 +198,29 @@ class ExecutionsMetadataPersistServiceSpec stored.getName shouldBe "Untitled Execution" } - it should "throw a DB constraint violation when uid is None" in { - // The method signature accepts Option[Integer] for uid and calls - // `newExecution.setUid(uid.orNull)`, but workflow_executions.uid is - // NOT NULL per texera_ddl.sql, so passing None propagates a jOOQ - // DataAccessException. Pinning the current behavior so a future fix — - // either tightening the signature to a required Integer or making the - // column nullable — breaks the spec deliberately. See follow-up bug. - val ex = intercept[org.jooq.exception.DataAccessException] { + it should "let the DB NOT NULL constraint reject a null uid (surfaced as a readable error)" in { + // No more pre-insert require: a null uid is passed straight to jOOQ and the + // DB's NOT NULL constraint rejects it. insertNewExecution catches the + // resulting DataAccessException (SQLState 23502) and rethrows a readable + // IllegalArgumentException. Assert both the readable message and that the + // original DB exception is preserved as the cause, and that nothing was + // written. + val before = workflowExecutionsDao.fetchByVid(seededVid).size() + + val ex = intercept[IllegalArgumentException] { ExecutionsMetadataPersistService.insertNewExecution( WorkflowIdentity(testWid.toLong), - None, + null, // uid is NOT NULL in the DB executionName = "anonymous", environmentVersion = "env-3", computingUnitId = seededCuid ) } ex.getMessage should include("uid") + ex.getCause shouldBe a[org.jooq.exception.DataAccessException] + + // The failed insert must not have persisted a row. + workflowExecutionsDao.fetchByVid(seededVid).size() shouldBe before } // -- tryGetExistingExecution ------------------------------------------------
