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-5221-24f7702714800152004b98f7b01ba53244cd2eb7 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 8a7366f3b1ec18fc118a92b76326c06b92be1ff3 Author: Matthew B. <[email protected]> AuthorDate: Tue May 26 01:06:23 2026 -0700 fix: honor Option contract on tryGetExistingExecution miss (#5221) ### What changes were proposed in this PR? - Change `tryGetExistingExecution` in `ExecutionsMetadataPersistService` to wrap the DAO call with `Option(...)` instead of `Some(...)`, so a missing row (jOOQ returns `null`) collapses to `None` rather than `Some(null)`. - Keep the existing `catch Throwable` block, which still earns its keep for hard DB errors (e.g. closed connection); the `Option` wrap only addresses the no-row miss path. - Update `ExecutionsMetadataPersistServiceSpec` (added in #5213): replace the two paired pin tests (the `Some(null)` pin and the inverted `intercept[TestFailedException]` xfail-strict) with a single positive `shouldBe None` test. ### Any related issues, documentation, or discussions? Closes: #5211. Depends on #5213 (test scaffolding); rebase after it lands. ### How was this PR tested? - Ran `sbt scalafmtAll` (no rewrites needed). - `ExecutionsMetadataPersistServiceSpec.tryGetExistingExecution` now has positive tests for both the hit (`Some(row)`) and the miss (`None`) cases. ### Was this PR authored or co-authored using generative AI tooling? Co-authored with Claude Opus 4.7 in compliance with ASF --- .../service/ExecutionsMetadataPersistService.scala | 2 +- .../ExecutionsMetadataPersistServiceSpec.scala | 24 ++-------------------- 2 files changed, 3 insertions(+), 23 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 833c443332..b9b29dff72 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 @@ -77,7 +77,7 @@ object ExecutionsMetadataPersistService extends LazyLogging { def tryGetExistingExecution(executionId: ExecutionIdentity): Option[WorkflowExecutions] = { try { - Some(workflowExecutionsDao.fetchOneByEid(executionId.id.toInt)) + Option(workflowExecutionsDao.fetchOneByEid(executionId.id.toInt)) } catch { case t: Throwable => logger.info("Unable to get execution. Error = " + t.getMessage) 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 f0f583ecc4..5c7a687728 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 @@ -228,31 +228,11 @@ class ExecutionsMetadataPersistServiceSpec fetched.get.getEid shouldBe eid } - it should "currently return Some(null) for an unknown eid (latent bug)" in { - // Pin actual behavior: `fetchOneByEid` returns null for a miss (no throw), - // and the production code wraps the result in `Some(...)` before the - // try/catch can convert it to None. So the Option contract is broken for - // misses — callers that destructure with `getOrElse` get the null through. - // The catch only fires on hard errors (e.g. a closed connection), which - // is why the method name says "tryGet". Filed as a follow-up bug. + it should "return None for an unknown eid" in { val fetched = ExecutionsMetadataPersistService.tryGetExistingExecution( ExecutionIdentity(-1L) ) - fetched shouldBe Some(null) - } - - it should "(intended contract) return None for an unknown eid" in { - // xfail-strict equivalent in ScalaTest: invert via intercept. When the - // bug above is fixed (e.g. `Option(workflowExecutionsDao.fetchOneByEid(...))` - // instead of `Some(...)`), the inner assertion will pass, intercept will - // not catch a TestFailedException, and this test will flip red — forcing - // the pinned-behavior test above to be updated in the same PR. - intercept[org.scalatest.exceptions.TestFailedException] { - val fetched = ExecutionsMetadataPersistService.tryGetExistingExecution( - ExecutionIdentity(-1L) - ) - fetched shouldBe None - } + fetched shouldBe None } // -- tryUpdateExistingExecution ---------------------------------------------
