feat(amber): return 404 for unavailable public workflow - #8140
Conversation
Backport auto-label reportThis
|
Automated Reviewer SuggestionsBased on the
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 395 | 0.241 | 25,367/32,632/32,632 us | 🟢 -27.7% / 🔴 +109.5% |
| 🔴 | bs=100 sw=10 sl=64 | 801 | 0.489 | 125,236/148,134/148,134 us | 🔴 +17.8% / 🔴 +38.1% |
| 🔴 | bs=1000 sw=10 sl=64 | 919 | 0.561 | 1,081,112/1,275,166/1,275,166 us | 🔴 +14.5% / 🔴 +24.6% |
Baseline details
Latest main 7d57cd6 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 395 tuples/sec | 386 tuples/sec | 787.98 tuples/sec | +2.3% | -49.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.241 MB/s | 0.236 MB/s | 0.481 MB/s | +2.1% | -49.9% |
| bs=10 sw=10 sl=64 | p50 | 25,367 us | 20,662 us | 12,593 us | +22.8% | +101.4% |
| bs=10 sw=10 sl=64 | p95 | 32,632 us | 45,118 us | 15,579 us | -27.7% | +109.5% |
| bs=10 sw=10 sl=64 | p99 | 32,632 us | 45,118 us | 18,786 us | -27.7% | +73.7% |
| bs=100 sw=10 sl=64 | throughput | 801 tuples/sec | 855 tuples/sec | 1,008 tuples/sec | -6.3% | -20.5% |
| bs=100 sw=10 sl=64 | MB/s | 0.489 MB/s | 0.522 MB/s | 0.615 MB/s | -6.3% | -20.5% |
| bs=100 sw=10 sl=64 | p50 | 125,236 us | 116,708 us | 100,701 us | +7.3% | +24.4% |
| bs=100 sw=10 sl=64 | p95 | 148,134 us | 125,709 us | 107,244 us | +17.8% | +38.1% |
| bs=100 sw=10 sl=64 | p99 | 148,134 us | 125,709 us | 116,122 us | +17.8% | +27.6% |
| bs=1000 sw=10 sl=64 | throughput | 919 tuples/sec | 933 tuples/sec | 1,042 tuples/sec | -1.5% | -11.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.561 MB/s | 0.57 MB/s | 0.636 MB/s | -1.6% | -11.8% |
| bs=1000 sw=10 sl=64 | p50 | 1,081,112 us | 1,068,142 us | 981,959 us | +1.2% | +10.1% |
| bs=1000 sw=10 sl=64 | p95 | 1,275,166 us | 1,113,284 us | 1,023,080 us | +14.5% | +24.6% |
| bs=1000 sw=10 sl=64 | p99 | 1,275,166 us | 1,113,284 us | 1,051,697 us | +14.5% | +21.2% |
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,506.74,200,128000,395,0.241,25367.14,32632.37,32632.37
1,100,10,64,20,2495.97,2000,1280000,801,0.489,125235.61,148134.44,148134.44
2,1000,10,64,20,21754.63,20000,12800000,919,0.561,1081111.92,1275166.37,1275166.37
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8140 +/- ##
============================================
+ Coverage 94.03% 94.04% +0.01%
- Complexity 4762 4806 +44
============================================
Files 1181 1183 +2
Lines 47877 48003 +126
Branches 5324 5343 +19
============================================
+ Hits 45020 45146 +126
+ Misses 1433 1430 -3
- Partials 1424 1427 +3
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟡 0 must-fix · 2 advisory · 1 polish — the fix is correct, minimal, and matches the guard already in this file; nothing here blocks it.
Correctness (1)
WorkflowResource.scala:844—getWorkflowTypehas the identical unguarded dereference and no access-object guard either, so/api/workflow/type/999999still 500s for any REGULAR user (advisory, see inline)
Simplifications (1)
WorkflowResourceCoverSpec.scala:155— the tests landed in the cover-image-scoped spec, and the positive case duplicates one that already exists (advisory, see inline)
Polish: 1 quick touch-up (see inline comment).
Verification trace
Checked the fix against the file's own precedent rather than in isolation: it is the same guard already at :89-91, placed before any use of the fetched row, and the frontend consumer treats every non-2xx identically, so no client change is owed.
Then enumerated every single-row fetch in the file — lines 88, 160, 411, 469, 501, 551, 592, 712, 724, 800, 842 — to see whether the defect class was fully closed. getWorkflowType at :800-801 is the only remaining instance. It differs from makePublic/makePrivate, which are shielded by hasWriteAccess (:709, :721) since getPrivilege returns non-WRITE when no access row exists; getWorkflowType has no access-object call at all.
That finding is advisory by deliberate downgrade, recorded in the working notes: the defect is wholly in the merge target, this diff neither touches nor relies on it, and the PR fixes exactly what its linked issue scopes, with tests.
|
Feedback is addressed and CI is green. Ready for another review. |
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 2 resolved · 1 open · 2 new (2 new = 2 newly introduced · 0 late catches)
Still open: WorkflowResource.scala:800 — the getWorkflowType deref you've scoped out of this PR. That call is yours; please file the follow-up so the 500 doesn't disappear with the thread.
Conventions (1)
- Description: it claims positive coverage this PR no longer adds, and names
WorkflowResourceCoverSpecfor the repro, which now holds neither case (must-fix — the body becomes the squash commit message)
Polish: 1 quick touch-up (see inline comment).
Verification trace
Checked all three of last round's items against the tree rather than against the replies. The guard at :843-845 is now braced like its siblings at :89-91 and :742-744; both cases left WorkflowResourceCoverSpec and the rejection case landed at WorkflowResourceSpec.scala:1025, netting that file to zero in the diff; and the fixture still observes the defect, since without the guard workflow.getName at :846 dereferences null, and is_public defaults to false (sql/texera_ddl.sql:169) with the neighbouring assertion at :1015 pinning that independently.
The description entry is graded must-fix on one checked fact: this repo squash-merges with the PR body as the commit body — git log %b on 98588bf, 14a574f and 50dd7d5 all carry the full template — so the stale text lands in history rather than staying an editable PR field.
365d1d4 to
f3b17b8
Compare
|
Filed #8300 after reproducing the 500 locally. GitHub did not allow carloea2 to set the Bug type. |
|
The description now matches the moved regression test and its focused test command. |
carloea2
changed the title
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 3 resolved · 0 open · 1 new (1 new = 1 newly introduced · 0 late catches)
The code is finished: braces, the regression case under its own subject, a description that matches the diff, and #8300 recording the deferred getWorkflowType 500. Nothing in the diff drew a finding this round. What is open was not in the diff.
Conventions (1)
- Retitle →
fix(amber): return 404 for unavailable public workflow— a null guard is not a new capability, andrelease/v1.2still routes this to the release branch, where afeatdoes not belong (must-fix)
Verification trace
All three of last round's items were checked against the tree rather than against the replies. The guard at :843-845 is braced like its siblings; the regression case at WorkflowResourceSpec.scala:1030-1034 opens its own "WorkflowResource.retrievePublicWorkflow" should subject below the makePublic block, and the next test at :1036 opens another, so nothing inherits it; and both description claims now resolve — the positive case to :1019, the regression to :1030, and the test command to the package declared at :20.
The title entry is must-fix on one traced fact rather than on taste. backport-auto-label.yml:107-112 only auto-labels titles matching ^fix(...)?: — its own comment says feat/chore/docs are not auto-backported. But direct-backport-push.yml:295-308 picks targets purely from release/* labels and never reads the title, and release/v1.2 has been on this PR since 30 Aug. So the rename cancelled nothing; it only changed what backportTitle (:885-895) will render on the release branch — feat(amber, v1.2): return 404 for unavailable public workflow.
|
I kept this as feat under the definition you shared. Unavailable public workflows have returned 500 since this endpoint was introduced, so this adds 404 handling rather than restoring previous behavior. CI is green. Could you take another look? |
5 participants
What changes were proposed in this PR?
Return 404 when the public-workflow query finds no record instead of dereferencing a null result. The existing makePublic test supplies the positive case. This PR adds private and missing regression coverage in WorkflowResourceSpec.
Before: private and missing workflows returned 500.
After: private and missing workflows return 404, while a public workflow still returns 200.
Any related issues, documentation, discussions?
Closes #8139
How was this PR tested?
sbt "WorkflowExecutionService / Test / testOnly org.apache.texera.web.resource.dashboard.file.WorkflowResourceSpec"sbt scalafmtCheckAllsbt "scalafixAll --check"texera-webfrom this worktree. Missing and private workflow requests returned 404. A temporarily public local workflow returned 200 and was restored to private afterward.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Codex