Skip to content

feat(amber): reject invalid dashboard resource type - #8144

Open
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/dashboard-invalid-resource-type
Open

carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/dashboard-invalid-resource-type

Conversation

@carloea2

@carloea2 carloea2 commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Return BadRequestException for an unknown dashboard resource type instead of an unhandled IllegalArgumentException.

Before: invalid resource types return 500.
After: they return 400 with a specific message.

Update the exception assertions in both dashboard test suites.

Any related issues, documentation, discussions?

Closes #8143

How was this PR tested?

Retested in a fresh detached worktree from upstream main at 7426431337, which includes #8938. The regression assertions failed before the implementation change: 3 failed and 83 passed. After the change, all 86 focused tests pass.

Used JDK 17 and a disposable PostgreSQL database initialized from sql/texera_ddl.sql. The tests use embedded PostgreSQL. Object-storage endpoints were isolated for the passing run.

$env:STORAGE_JDBC_URL='jdbc:postgresql://127.0.0.1:55489/oct15_review?currentSchema=texera_db,public'
$env:STORAGE_S3_ENDPOINT='http://127.0.0.1:1'
$env:STORAGE_LAKEFS_ENDPOINT='http://127.0.0.1:1/api/v1'
sbt -batch "WorkflowExecutionService/testOnly *DashboardResourceSpec *dashboard.file.WorkflowResourceSpec -- -oDF" "scalafixAll --check" scalafmtCheckAll

Repository lint, formatting and whitespace checks pass.

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

Generated-by: Codex

@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 30, 2026
@github-actions
github-actions Bot requested a review from xuang7 August 30, 2026 20:40
@github-actions

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 Not labeled automatically — none of the files this PR modifies exist on this branch (amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala, amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala). The fix may target code that isn't on this release, or the files were moved/renamed after the branch was cut. Please check and add release/v1.3 by hand if this fix should be backported here.
✅ 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 Aug 30, 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: @aglinxinyuan, @yangzhang75
    You can notify them by mentioning @aglinxinyuan, @yangzhang75 in a comment.

@github-actions

github-actions Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 7426431 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 388 0.237 25,175/32,029/32,029 us 🟢 -18.4% / 🔴 +107.6%
🔴 bs=100 sw=10 sl=64 786 0.48 126,162/146,877/146,877 us 🔴 +8.8% / 🔴 +41.0%
⚪ bs=1000 sw=10 sl=64 912 0.557 1,094,496/1,183,740/1,183,740 us ⚪ within ±5% / 🔴 +20.1%
Baseline details

Latest main 7426431 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 388 tuples/sec 392 tuples/sec 822.11 tuples/sec -1.0% -52.8%
bs=10 sw=10 sl=64 MB/s 0.237 MB/s 0.239 MB/s 0.502 MB/s -0.8% -52.8%
bs=10 sw=10 sl=64 p50 25,175 us 24,876 us 12,129 us +1.2% +107.6%
bs=10 sw=10 sl=64 p95 32,029 us 39,232 us 15,469 us -18.4% +107.1%
bs=10 sw=10 sl=64 p99 32,029 us 39,232 us 18,357 us -18.4% +74.5%
bs=100 sw=10 sl=64 throughput 786 tuples/sec 841 tuples/sec 1,058 tuples/sec -6.5% -25.7%
bs=100 sw=10 sl=64 MB/s 0.48 MB/s 0.513 MB/s 0.646 MB/s -6.4% -25.7%
bs=100 sw=10 sl=64 p50 126,162 us 117,485 us 97,399 us +7.4% +29.5%
bs=100 sw=10 sl=64 p95 146,877 us 134,996 us 104,184 us +8.8% +41.0%
bs=100 sw=10 sl=64 p99 146,877 us 134,996 us 117,615 us +8.8% +24.9%
bs=1000 sw=10 sl=64 throughput 912 tuples/sec 920 tuples/sec 1,099 tuples/sec -0.9% -17.0%
bs=1000 sw=10 sl=64 MB/s 0.557 MB/s 0.561 MB/s 0.671 MB/s -0.7% -17.0%
bs=1000 sw=10 sl=64 p50 1,094,496 us 1,086,993 us 946,743 us +0.7% +15.6%
bs=1000 sw=10 sl=64 p95 1,183,740 us 1,137,898 us 985,437 us +4.0% +20.1%
bs=1000 sw=10 sl=64 p99 1,183,740 us 1,137,898 us 1,012,622 us +4.0% +16.9%
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,515.07,200,128000,388,0.237,25174.99,32029.38,32029.38
1,100,10,64,20,2544.79,2000,1280000,786,0.480,126161.85,146876.57,146876.57
2,1000,10,64,20,21931.00,20000,12800000,912,0.557,1094496.44,1183740.37,1183740.37

@codecov-commenter

codecov-commenter commented Aug 30, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.31%. Comparing base (7426431) to head (ad490c9).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8144   +/-   ##
=========================================
  Coverage     92.30%   92.31%           
- Complexity     5046     5047    +1     
=========================================
  Files          1260     1260           
  Lines         53887    53887           
  Branches       6711     6711           
=========================================
+ Hits          49743    49745    +2     
  Misses         2486     2486           
+ Partials       1658     1656    -2     
Flag Coverage Δ *Carryforward flag
access-control-service 77.38% <ø> (ø) Carriedforward from 7426431
agent-service 99.16% <ø> (ø) Carriedforward from 7426431
amber 87.33% <100.00%> (+0.01%) ⬆️
computing-unit-managing-service 60.48% <ø> (ø) Carriedforward from 7426431
config-service 87.37% <ø> (ø) Carriedforward from 7426431
file-service 82.00% <ø> (ø) Carriedforward from 7426431
frontend 96.55% <ø> (ø) Carriedforward from 7426431
notebook-migration-service 83.73% <ø> (ø) Carriedforward from 7426431
pyamber 98.53% <ø> (ø) Carriedforward from 7426431
workflow-compiling-service 72.43% <ø> (ø) Carriedforward from 7426431

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

🟢 0 must-fix · 0 advisory · 2 polish — a clean one-line fix; both notes are text-level and neither blocks the merge.

Polish: 2 quick touch-ups (see inline comments).

Verification trace

Checked that the exception swap actually changes the status rather than assuming it does. amber registers no ExceptionMapper of its own, so the old IllegalArgumentException hit Dropwizard's generic Throwable handler; javax.ws.rs.BadRequestException carries its own Response.status(BAD_REQUEST) and takes the CLIENT_ERROR branch — which also stops the attacker-supplied string being logged. The approach matches an existing repo pattern (WorkflowExecutionsResource.scala:685, with the rationale spelled out at ComputingUnitAccessResource.scala:117-119).

Both endpoints that reach the guard are covered by the single line — /dashboard/search and the anonymous /dashboard/publicSearch — and the frontend's only status-sensitive interceptor gates on 401 alone, so nothing downstream changes.

Four concerns were investigated and dismissed with evidence rather than dropped. The one worth naming: @QueryParam("includePublic ") in this same file carries a trailing space, which looks like a live bug and is not — publicSearch passes a null uid and WorkflowSearchQueryBuilder.scala:83 forces IS_PUBLIC = true regardless of that flag.

@carloea2

Copy link
Copy Markdown
Contributor Author

Updates are in and CI is green. Ready to merge when you have a chance.

@carloea2 carloea2 changed the title fix(amber): reject invalid dashboard resource type feat(amber): reject invalid dashboard resource type Aug 31, 2026
@mengw15 mengw15 removed the release/v1.2 back porting to release/v1.2 label Aug 31, 2026
@xuang7 xuang7 removed the fix label Sep 1, 2026
@xuang7
xuang7 removed their request for review September 1, 2026 01:47
@carloea2
carloea2 force-pushed the fix/dashboard-invalid-resource-type branch from 0c741f9 to b3403b3 Compare September 4, 2026 18:39
@github-actions github-actions Bot added the fix label Sep 4, 2026
@carloea2

carloea2 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Amber CI failed in unrelated WorkflowResourceSpec. The dashboard tests passed. Could someone rerun the failed job?

@carloea2
carloea2 force-pushed the fix/dashboard-invalid-resource-type branch from b3403b3 to 6802e98 Compare September 24, 2026 01:00
@carloea2

Copy link
Copy Markdown
Contributor Author

I reproduced the CI failure locally. WorkflowResourceSpec still expected IllegalArgumentException after this PR changed it to BadRequestException. Updated that assertion in 6802e98. Both resource suites now pass all 87 tests, and scalafixAll --check and scalafmtCheckAll pass.

@carloea2
carloea2 force-pushed the fix/dashboard-invalid-resource-type branch from 22a02b8 to ad490c9 Compare October 15, 2026 06:45
@carloea2

Copy link
Copy Markdown
Contributor Author

@Yicong-Huang, the refreshed checks are green. Could you take a final look and merge when ready? The stale fix label should be feature.

This branch has not been deployed

No deployments
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.

Dashboard search returns server error for an unknown resource type

5 participants