Skip to content

fix: require non-null uid in insertNewExecution#5300

Merged
Yicong-Huang merged 9 commits into
apache:mainfrom
Ma77Ball:fix-5212-insert-execution-uid
Jun 1, 2026
Merged

fix: require non-null uid in insertNewExecution#5300
Yicong-Huang merged 9 commits into
apache:mainfrom
Ma77Ball:fix-5212-insert-execution-uid

Conversation

@Ma77Ball
Copy link
Copy Markdown
Contributor

@Ma77Ball Ma77Ball commented May 31, 2026

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

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 31, 2026

Codecov Report

❌ Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.95%. Comparing base (eb287f3) to head (6eeac99).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...rg/apache/texera/web/service/WorkflowService.scala 0.00% 3 Missing ⚠️
...web/service/ExecutionsMetadataPersistService.scala 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5300      +/-   ##
============================================
- Coverage     49.97%   49.95%   -0.02%     
+ Complexity     2421     2417       -4     
============================================
  Files          1052     1052              
  Lines         40780    40786       +6     
  Branches       4356     4357       +1     
============================================
- Hits          20378    20376       -2     
- Misses        19193    19198       +5     
- Partials       1209     1212       +3     
Flag Coverage Δ *Carryforward flag
access-control-service 41.89% <ø> (ø) Carriedforward from eb287f3
agent-service 33.76% <ø> (ø) Carriedforward from eb287f3
amber 51.98% <55.55%> (-0.04%) ⬇️
computing-unit-managing-service 1.38% <ø> (ø) Carriedforward from eb287f3
config-service 54.68% <ø> (ø) Carriedforward from eb287f3
file-service 38.42% <ø> (ø) Carriedforward from eb287f3
frontend 42.43% <ø> (ø) Carriedforward from eb287f3
pyamber 90.80% <ø> (ø) Carriedforward from eb287f3
python 90.80% <ø> (ø) Carriedforward from eb287f3
workflow-compiling-service 58.39% <ø> (ø) Carriedforward from eb287f3

*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.

Comment thread amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala Outdated
Copy link
Copy Markdown
Contributor

@Yicong-Huang Yicong-Huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left comments inline. we should reject bad frontend requests.

Comment thread amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala Outdated
@Ma77Ball Ma77Ball requested a review from Yicong-Huang June 1, 2026 02:00
Copy link
Copy Markdown
Contributor

@Yicong-Huang Yicong-Huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a nit, see inline.

Ma77Ball added 2 commits June 1, 2026 12:18
Updated documentation to clarify that uid is required.

Signed-off-by: Matthew B. <mgball@uci.edu>
@Yicong-Huang Yicong-Huang added this pull request to the merge queue Jun 1, 2026
Merged via the queue into apache:main with commit 531d79f Jun 1, 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.

insertNewExecution propagates NOT NULL violation when called with uid=None despite Option[Integer] signature

3 participants