Skip to content

fix: honor Option contract on tryGetExistingExecution miss#5221

Merged
Yicong-Huang merged 3 commits into
apache:mainfrom
Ma77Ball:fix/5211-trygetexisting-some-null
May 26, 2026
Merged

fix: honor Option contract on tryGetExistingExecution miss#5221
Yicong-Huang merged 3 commits into
apache:mainfrom
Ma77Ball:fix/5211-trygetexisting-some-null

Conversation

@Ma77Ball
Copy link
Copy Markdown
Contributor

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 test(amber): DB-backed unit tests for ExecutionsMetadataPersistService and updateWorkflowState #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

Ma77Ball added 2 commits May 26, 2026 00:27
…xisting-some-null

# Conflicts:
#	amber/src/test/scala/org/apache/texera/web/service/ExecutionsMetadataPersistServiceSpec.scala
@Ma77Ball
Copy link
Copy Markdown
Contributor Author

/request-review @aglinxinyuan

@github-actions github-actions Bot requested a review from aglinxinyuan May 26, 2026 07:41
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.54%. Comparing base (24f7702) to head (9079dbe).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5221      +/-   ##
============================================
- Coverage     48.55%   48.54%   -0.02%     
+ Complexity     2372     2370       -2     
============================================
  Files          1042     1042              
  Lines         39971    39971              
  Branches       4252     4252              
============================================
- Hits          19408    19403       -5     
- Misses        19414    19418       +4     
- Partials       1149     1150       +1     
Flag Coverage Δ *Carryforward flag
access-control-service 39.53% <ø> (ø) Carriedforward from 24f7702
agent-service 33.76% <ø> (ø) Carriedforward from 24f7702
amber 51.48% <100.00%> (-0.04%) ⬇️
computing-unit-managing-service 0.00% <ø> (ø) Carriedforward from 24f7702
config-service 0.00% <ø> (ø) Carriedforward from 24f7702
file-service 37.99% <ø> (ø) Carriedforward from 24f7702
frontend 40.02% <ø> (ø) Carriedforward from 24f7702
python 90.51% <ø> (ø) Carriedforward from 24f7702
workflow-compiling-service 56.81% <ø> (ø) Carriedforward from 24f7702

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Yicong-Huang Yicong-Huang enabled auto-merge May 26, 2026 07:59
@Yicong-Huang Yicong-Huang added this pull request to the merge queue May 26, 2026
Merged via the queue into apache:main with commit 8a7366f May 26, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tryGetExistingExecution returns Some(null) instead of None for missing eid

3 participants