Skip to content

fix(workflow): return 404 for missing workflow type - #8398

Open
VuMartin wants to merge 3 commits into
apache:mainfrom
VuMartin:fix/8300-workflow-type-not-found
Open

fix(workflow): return 404 for missing workflow type#8398
VuMartin wants to merge 3 commits into
apache:mainfrom
VuMartin:fix/8300-workflow-type-not-found

Conversation

@VuMartin

@VuMartin VuMartin commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Return 404 Not Found when the workflow type endpoint is requested for a workflow that does not exist. Previously, getWorkflowType dereferenced a null DAO result and returned 500 Internal Server Error.

Before:
image

After:
image

Any related issues, documentation, discussions?

Closes #8300

How was this PR tested?

  • Ran WorkflowResourceSpec
  • Verified GET /api/workflow/type/2147483647 returns 404 instead of 500
  • Added regression coverage for a missing workflow type

Was this PR authored or co-authored using generative AI tooling?

Generated-by: ChatGPT (5.5 mini)

@Yicong-Huang Yicong-Huang added release/v1.3 back porting to release/v1.3 release/v1.2 back porting to release/v1.2 labels Sep 4, 2026
@github-actions
github-actions Bot requested review from mengw15 and xuang7 September 4, 2026 07:16
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. release/* labels drive the post-merge backport, so add or remove one to change where this fix lands.

Release branch Analysis
release/v1.3 Change detected on this branch — label added; this fix is queued to backport here. Requested review from @mengw15.
release/v1.2 Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7.

Auto-label run.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @yangzhang75, @aglinxinyuan
    You can notify them by mentioning @yangzhang75, @aglinxinyuan in a comment.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 0 better · 🔴 5 worse · ⚪ 10 noise (<±5%) · 0 without baseline

Compared against main 1cbe857 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 400 0.244 23,883/30,589/30,589 us 🔴 +14.2% / 🔴 +87.3%
🔴 bs=100 sw=10 sl=64 915 0.559 106,659/153,905/153,905 us 🔴 +11.5% / 🔴 +39.5%
bs=1000 sw=10 sl=64 1,106 0.675 895,289/987,773/987,773 us ⚪ within ±5% / 🟢 -11.3%
Baseline details

Latest main 1cbe857 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 400 tuples/sec 449 tuples/sec 749.39 tuples/sec -10.9% -46.6%
bs=10 sw=10 sl=64 MB/s 0.244 MB/s 0.274 MB/s 0.457 MB/s -10.9% -46.7%
bs=10 sw=10 sl=64 p50 23,883 us 20,914 us 13,045 us +14.2% +83.1%
bs=10 sw=10 sl=64 p95 30,589 us 31,021 us 16,328 us -1.4% +87.3%
bs=10 sw=10 sl=64 p99 30,589 us 31,021 us 19,518 us -1.4% +56.7%
bs=100 sw=10 sl=64 throughput 915 tuples/sec 943 tuples/sec 968.07 tuples/sec -3.0% -5.5%
bs=100 sw=10 sl=64 MB/s 0.559 MB/s 0.576 MB/s 0.591 MB/s -3.0% -5.4%
bs=100 sw=10 sl=64 p50 106,659 us 103,789 us 103,330 us +2.8% +3.2%
bs=100 sw=10 sl=64 p95 153,905 us 138,069 us 110,305 us +11.5% +39.5%
bs=100 sw=10 sl=64 p99 153,905 us 138,069 us 121,264 us +11.5% +26.9%
bs=1000 sw=10 sl=64 throughput 1,106 tuples/sec 1,103 tuples/sec 998 tuples/sec +0.3% +10.8%
bs=1000 sw=10 sl=64 MB/s 0.675 MB/s 0.673 MB/s 0.609 MB/s +0.3% +10.8%
bs=1000 sw=10 sl=64 p50 895,289 us 906,264 us 1,009,354 us -1.2% -11.3%
bs=1000 sw=10 sl=64 p95 987,773 us 967,213 us 1,052,748 us +2.1% -6.2%
bs=1000 sw=10 sl=64 p99 987,773 us 967,213 us 1,081,982 us +2.1% -8.7%
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,500.25,200,128000,400,0.244,23883.48,30589.19,30589.19
1,100,10,64,20,2185.03,2000,1280000,915,0.559,106658.97,153905.31,153905.31
2,1000,10,64,20,18078.44,20000,12800000,1106,0.675,895288.64,987772.82,987772.82

@codecov-commenter

codecov-commenter commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.06%. Comparing base (1cbe857) to head (f725f38).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8398      +/-   ##
============================================
+ Coverage     94.03%   94.06%   +0.03%     
- Complexity     4821     4926     +105     
============================================
  Files          1204     1208       +4     
  Lines         48991    49796     +805     
  Branches       5956     6067     +111     
============================================
+ Hits          46067    46840     +773     
- Misses         1458     1479      +21     
- Partials       1466     1477      +11     
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø) Carriedforward from 3608088
agent-service 99.32% <ø> (ø) Carriedforward from 3608088
amber 90.18% <100.00%> (+0.26%) ⬆️
computing-unit-managing-service 73.67% <ø> (ø) Carriedforward from 3608088
config-service 87.12% <ø> (ø) Carriedforward from 3608088
file-service 87.91% <ø> (ø) Carriedforward from 3608088
frontend 96.79% <ø> (ø) Carriedforward from 3608088
notebook-migration-service 83.57% <ø> (ø) Carriedforward from 3608088
pyamber 98.47% <ø> (ø) Carriedforward from 3608088
workflow-compiling-service 77.19% <ø> (ø) Carriedforward from 3608088

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

☔ View full report in Codecov by Harness.
📢 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 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 1 must-fix · 0 advisory · 1 polish: the guard is correct and complete, and the identical null dereference sits two methods below it.

Correctness (1)

  • WorkflowResource.scala:852: retrievePublicWorkflow takes the same unguarded path (must-fix, see inline)

Polish: 1 quick touch-up (see inline comments).

Verification trace

Checked every other fetchOneByWid site in the file: 181, 428, 536, 571 and 610 sit behind an access check or an insert in the same method, so none can reach the dereference. The three fetchOneInto(classOf[String]) lookups (871, 883, 918) return null instead of dereferencing, which Jersey maps to 204 rather than 500. retrievePublicWorkflow at 891 to 897 is the only remaining site with this shape.

@carloea2

carloea2 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

I am working on this one
#8140

@Yicong-Huang Yicong-Huang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 2 resolved · 0 open · 0 new (0 new = 0 newly introduced · 0 late catches)

The test name is fixed, and the retrievePublicWorkflow must-fix is withdrawn: you were right to keep it scoped to #8140. Nothing open on this PR.

Verification trace

Both carry-over items were checked against the tree at f725f38, not taken on report. WorkflowResourceSpec.scala:950 now reads it should "throw NotFoundException for a missing workflow type", matching the sibling at 1059, so that thread is resolved. For the scope question I checked #8140 before answering: it is open, changes these same two files, and closes #8139, so the guard is tracked on its own branch. retrievePublicWorkflow is untouched by this PR and already in main, so requiring it here would merge two independently reviewable fixes.

Re-ran the fix-completeness sweep on the unchanged production hunk: every other fetchOneByWid site in this file (181, 428, 536, 571, 610) sits behind an access check or an insert in the same method, and the three fetchOneInto(classOf[String]) lookups (871, 883, 918) return null rather than dereferencing. retrievePublicWorkflow remains the only other site with this shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine fix release/v1.2 back porting to release/v1.2 release/v1.3 back porting to release/v1.3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing workflow returns 500 from workflow type endpoint

4 participants