You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "carloea2 (via GitHub)" <gi...@apache.org> on 2026/08/30 21:13:13 UTC
[PR] fix(amber): reject invalid dashboard sort [texera]
carloea2 opened a new pull request, #8151:
URL: https://github.com/apache/texera/pull/8151
### What changes were proposed in this PR?
Reject dashboard sort values outside the documented `orderBy` grammar instead of silently removing query ordering. Keep all existing valid sort mappings unchanged.
Before: an unknown sort value returned 200 with unordered pagination.
After: an unknown sort value returns 400 with the rejected value.
### Any related issues, documentation, discussions?
Closes #8147
### How was this PR tested?
- `sbt "WorkflowExecutionService / Test / testOnly org.apache.texera.web.resource.dashboard.DashboardResourceSpec"`
- `sbt scalafmtCheckAll`
- `sbt "scalafixAll --check"`
- Built with `bin/local-dev.sh up texera-web --build --json` and ran the generated distribution on localhost.
- Verified `orderBy=Bogus` returns 400 and `orderBy=NameAsc` still returns 200.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Codex
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): reject invalid dashboard sort [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8151:
URL: https://github.com/apache/texera/pull/8151#issuecomment-5471292367
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@Neilk1021`, `@aglinxinyuan`
You can notify them by mentioning `@Neilk1021`, `@aglinxinyuan` in a comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): reject invalid dashboard sort [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #8151:
URL: https://github.com/apache/texera/pull/8151#discussion_r3936598169
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala:
##########
@@ -158,9 +158,10 @@ object DashboardResource {
case "Asc" => value.asc()
case "Desc" => value.desc().nullsLast()
})
- case None => List()
+ case None =>
+ throw new BadRequestException(s"Unknown orderBy: ${searchQueryParams.orderBy}")
Review Comment:
These two arms are now the same throw, but only one is the caller's fault. `case _` fires on a value outside the grammar — a genuine 400. This one fires when a value the regex at :150 accepts has no `getColumnField` mapping. That is a server-side gap, and `Unknown orderBy: <value>` sends whoever debugs it after client input.
Unreachable today: :171-174 maps all four regex column names. Rather than pick a second exception type for a branch that should not exist, make `getColumnField` return `Field[_]` and throw `IllegalStateException` from its own `case _`. `getOrderFields` then has one failure arm, and the 400 belongs to it.
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala:
##########
@@ -101,17 +102,12 @@ class DashboardResourceSpec extends AnyFlatSpec with Matchers {
// -- getOrderFields: everything the grammar rejects -------------------------
- it should "fall back to no ordering at all when orderBy does not match the pattern" in {
- // Consequence of the empty list: the query runs unordered while
- // offset/limit still apply, so pagination silently becomes
- // non-deterministic. Pin the inputs that take this branch.
- orderSql("") shouldBe empty // the frontend can send an empty query param
- orderSql("Bogus") shouldBe empty
- orderSql("NameSideways") shouldBe empty // known column, junk direction
- orderSql("Name") shouldBe empty // direction missing entirely
- orderSql("nameAsc") shouldBe empty // the regex is case-sensitive
- orderSql("SortByNameAsc") shouldBe empty // Regex.unapplySeq anchors the
- orderSql("NameAscending") shouldBe empty // whole string, both ends
+ it should "reject values outside the orderBy grammar" in {
+ Seq("", "Bogus", "NameSideways", "Name", "nameAsc", "SortByNameAsc", "NameAscending")
+ .foreach { orderBy =>
+ val thrown = the[BadRequestException] thrownBy orderSql(orderBy)
+ thrown.getMessage shouldBe s"Unknown orderBy: $orderBy"
+ }
}
// Two branches in this area are unreachable and are therefore deliberately
Review Comment:
The note starting here still quotes `case None => List()`, which this round replaced with the throw at `DashboardResource.scala:161-162`. The reasoning holds and the `case _ => null` half is accurate — only the quote is stale.
While you are here: the seven fixtures at :106 lost their per-case comments. `nameAsc` pinned case-sensitivity, `SortByNameAsc` and `NameAscending` the regex's anchoring, `""` the empty query param. Without those they read as spellings of one idea, and the next person pruning duplicates has nothing to stop them. My miss from last round, not yours.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): reject invalid dashboard sort [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8151:
URL: https://github.com/apache/texera/pull/8151#issuecomment-5471323658
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 0 better · 🔴 7 worse · ⚪ 8 noise (<±5%) · 0 without baseline
> Compared against main `98588bf` 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](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/33335818807)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| 🔴 | bs=10 sw=10 sl=64 | 352 | 0.215 | 27,327/42,233/42,233 us | 🔴 +28.2% / 🔴 +185.6% |
| ⚪ | bs=100 sw=10 sl=64 | 776 | 0.473 | 125,723/178,886/178,886 us | ⚪ within ±5% / 🔴 +77.2% |
| 🔴 | bs=1000 sw=10 sl=64 | 909 | 0.555 | 1,098,994/1,207,371/1,207,371 us | 🔴 +8.2% / 🔴 +24.8% |
<details><summary>Baseline details</summary>
Latest main `98588bf` from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 352 tuples/sec | 413 tuples/sec | 844.57 tuples/sec | -14.8% | -58.3% |
| bs=10 sw=10 sl=64 | MB/s | 0.215 MB/s | 0.252 MB/s | 0.515 MB/s | -14.7% | -58.3% |
| bs=10 sw=10 sl=64 | p50 | 27,327 us | 21,315 us | 11,837 us | +28.2% | +130.9% |
| bs=10 sw=10 sl=64 | p95 | 42,233 us | 34,243 us | 14,788 us | +23.3% | +185.6% |
| bs=10 sw=10 sl=64 | p99 | 42,233 us | 34,243 us | 17,651 us | +23.3% | +139.3% |
| bs=100 sw=10 sl=64 | throughput | 776 tuples/sec | 792 tuples/sec | 1,084 tuples/sec | -2.0% | -28.4% |
| bs=100 sw=10 sl=64 | MB/s | 0.473 MB/s | 0.483 MB/s | 0.662 MB/s | -2.1% | -28.5% |
| bs=100 sw=10 sl=64 | p50 | 125,723 us | 121,586 us | 94,505 us | +3.4% | +33.0% |
| bs=100 sw=10 sl=64 | p95 | 178,886 us | 174,124 us | 100,941 us | +2.7% | +77.2% |
| bs=100 sw=10 sl=64 | p99 | 178,886 us | 174,124 us | 108,642 us | +2.7% | +64.7% |
| bs=1000 sw=10 sl=64 | throughput | 909 tuples/sec | 925 tuples/sec | 1,119 tuples/sec | -1.7% | -18.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.555 MB/s | 0.565 MB/s | 0.683 MB/s | -1.8% | -18.8% |
| bs=1000 sw=10 sl=64 | p50 | 1,098,994 us | 1,074,212 us | 924,404 us | +2.3% | +18.9% |
| bs=1000 sw=10 sl=64 | p95 | 1,207,371 us | 1,115,360 us | 967,785 us | +8.2% | +24.8% |
| bs=1000 sw=10 sl=64 | p99 | 1,207,371 us | 1,115,360 us | 995,630 us | +8.2% | +21.3% |
</details>
<details><summary>Raw CSV</summary>
```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,568.05,200,128000,352,0.215,27326.76,42232.83,42232.83
1,100,10,64,20,2578.12,2000,1280000,776,0.473,125722.95,178885.71,178885.71
2,1000,10,64,20,21990.95,20000,12800000,909,0.555,1098994.29,1207370.84,1207370.84
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): reject invalid dashboard sort [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #8151:
URL: https://github.com/apache/texera/pull/8151#issuecomment-5471356008
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/8151?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 94.02%. Comparing base ([`98588bf`](https://app.codecov.io/gh/apache/texera/commit/98588bf2ab49aac65ebf002496eb057670e6fc07?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5f3c9d1`](https://app.codecov.io/gh/apache/texera/commit/5f3c9d16acd852cb68d155a043a2e2fd8a68ad7f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:white_check_mark: All tests successful. No failed tests found.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #8151 +/- ##
============================================
- Coverage 94.03% 94.02% -0.01%
+ Complexity 4762 4760 -2
============================================
Files 1181 1181
Lines 47877 47877
Branches 5324 5324
============================================
- Hits 45020 45017 -3
- Misses 1433 1434 +1
- Partials 1424 1426 +2
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.00% <ø> (ø)` | | Carriedforward from [98588bf](https://app.codecov.io/gh/apache/texera/commit/98588bf2ab49aac65ebf002496eb057670e6fc07?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `99.32% <ø> (ø)` | | Carriedforward from [98588bf](https://app.codecov.io/gh/apache/texera/commit/98588bf2ab49aac65ebf002496eb057670e6fc07?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.01% <100.00%> (-0.02%)` | :arrow_down: | |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.67% <ø> (ø)` | | Carriedforward from [98588bf](https://app.codecov.io/gh/apache/texera/commit/98588bf2ab49aac65ebf002496eb057670e6fc07?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `86.86% <ø> (ø)` | | Carriedforward from [98588bf](https://app.codecov.io/gh/apache/texera/commit/98588bf2ab49aac65ebf002496eb057670e6fc07?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.91% <ø> (ø)` | | Carriedforward from [98588bf](https://app.codecov.io/gh/apache/texera/commit/98588bf2ab49aac65ebf002496eb057670e6fc07?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `96.68% <ø> (ø)` | | Carriedforward from [98588bf](https://app.codecov.io/gh/apache/texera/commit/98588bf2ab49aac65ebf002496eb057670e6fc07?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.31% <ø> (ø)` | | Carriedforward from [98588bf](https://app.codecov.io/gh/apache/texera/commit/98588bf2ab49aac65ebf002496eb057670e6fc07?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `98.87% <ø> (ø)` | | Carriedforward from [98588bf](https://app.codecov.io/gh/apache/texera/commit/98588bf2ab49aac65ebf002496eb057670e6fc07?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/8151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.19% <ø> (ø)` | | Carriedforward from [98588bf](https://app.codecov.io/gh/apache/texera/commit/98588bf2ab49aac65ebf002496eb057670e6fc07?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/8151?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): reject invalid dashboard sort [texera]
Posted by "carloea2 (via GitHub)" <gi...@apache.org>.
carloea2 commented on PR #8151:
URL: https://github.com/apache/texera/pull/8151#issuecomment-5545679697
@Yicong-Huang All review comments are addressed and checks are green. Could you take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): reject invalid dashboard sort [texera]
Posted by "carloea2 (via GitHub)" <gi...@apache.org>.
carloea2 commented on code in PR #8151:
URL: https://github.com/apache/texera/pull/8151#discussion_r3936634219
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala:
##########
@@ -158,9 +158,10 @@ object DashboardResource {
case "Asc" => value.asc()
case "Desc" => value.desc().nullsLast()
})
- case None => List()
+ case None =>
+ throw new BadRequestException(s"Unknown orderBy: ${searchQueryParams.orderBy}")
Review Comment:
Made the mapping total and moved the impossible default to an IllegalStateException inside getColumnField.
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala:
##########
@@ -101,17 +102,12 @@ class DashboardResourceSpec extends AnyFlatSpec with Matchers {
// -- getOrderFields: everything the grammar rejects -------------------------
- it should "fall back to no ordering at all when orderBy does not match the pattern" in {
- // Consequence of the empty list: the query runs unordered while
- // offset/limit still apply, so pagination silently becomes
- // non-deterministic. Pin the inputs that take this branch.
- orderSql("") shouldBe empty // the frontend can send an empty query param
- orderSql("Bogus") shouldBe empty
- orderSql("NameSideways") shouldBe empty // known column, junk direction
- orderSql("Name") shouldBe empty // direction missing entirely
- orderSql("nameAsc") shouldBe empty // the regex is case-sensitive
- orderSql("SortByNameAsc") shouldBe empty // Regex.unapplySeq anchors the
- orderSql("NameAscending") shouldBe empty // whole string, both ends
+ it should "reject values outside the orderBy grammar" in {
+ Seq("", "Bogus", "NameSideways", "Name", "nameAsc", "SortByNameAsc", "NameAscending")
+ .foreach { orderBy =>
+ val thrown = the[BadRequestException] thrownBy orderSql(orderBy)
+ thrown.getMessage shouldBe s"Unknown orderBy: $orderBy"
+ }
}
// Two branches in this area are unreachable and are therefore deliberately
Review Comment:
Updated the stale note and restored the purpose of each invalid fixture.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): reject invalid dashboard sort [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #8151:
URL: https://github.com/apache/texera/pull/8151#discussion_r3937451626
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala:
##########
@@ -152,28 +152,23 @@ object DashboardResource {
searchQueryParams.orderBy match {
case pattern(column, order) =>
val field = getColumnField(column)
- field match {
- case Some(value) =>
- List(order match {
- case "Asc" => value.asc()
- case "Desc" => value.desc().nullsLast()
- })
- case None => List()
- }
- case _ => List() // Default case if the orderBy string doesn't match the pattern
+ List(order match {
+ case "Asc" => field.asc()
+ case "Desc" => field.desc().nullsLast()
+ })
+ case _ => throw new BadRequestException(s"Unknown orderBy: ${searchQueryParams.orderBy}")
}
}
// Helper method to map column names to actual database fields based on resource type
Review Comment:
`getColumnField` never keyed on resource type — it maps the order column, and this PR just rewrote its signature on the line below.
```suggestion
// Maps an order-column name from the orderBy grammar to its unified-schema field.
```
While you are here: the `@param orderBy` doc at :69 is now the grammar this endpoint enforces with a 400, and it lists four of the eight values :150 accepts. `ExecutionTimeDesc` is missing, which is the value the dashboard itself sends (`sort-method.ts:25`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): reject invalid dashboard sort [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8151:
URL: https://github.com/apache/texera/pull/8151#issuecomment-5471291839
<!-- backport-auto-label-report -->
### 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](https://github.com/apache/texera/actions/runs/33335809738)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): reject invalid dashboard sort [texera]
Posted by "carloea2 (via GitHub)" <gi...@apache.org>.
carloea2 commented on PR #8151:
URL: https://github.com/apache/texera/pull/8151#issuecomment-5547453672
Checks are green and the review found no remaining items. Ready to merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): reject invalid dashboard sort [texera]
Posted by "carloea2 (via GitHub)" <gi...@apache.org>.
carloea2 commented on code in PR #8151:
URL: https://github.com/apache/texera/pull/8151#discussion_r3935988304
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala:
##########
@@ -158,7 +158,7 @@ object DashboardResource {
})
case None => List()
}
- case _ => List() // Default case if the orderBy string doesn't match the pattern
+ case _ => throw new BadRequestException(s"Unknown orderBy: ${searchQueryParams.orderBy}")
Review Comment:
Both fallback paths now return 400. I also documented the empty value behavior and fixed the stale spec comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): reject invalid dashboard sort [texera]
Posted by "carloea2 (via GitHub)" <gi...@apache.org>.
carloea2 commented on PR #8151:
URL: https://github.com/apache/texera/pull/8151#issuecomment-5544476215
The separate resourceType 500 path is covered by #8144, so I kept this PR scoped to orderBy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): reject invalid dashboard sort [texera]
Posted by "carloea2 (via GitHub)" <gi...@apache.org>.
carloea2 commented on code in PR #8151:
URL: https://github.com/apache/texera/pull/8151#discussion_r3937733894
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala:
##########
@@ -152,28 +152,23 @@ object DashboardResource {
searchQueryParams.orderBy match {
case pattern(column, order) =>
val field = getColumnField(column)
- field match {
- case Some(value) =>
- List(order match {
- case "Asc" => value.asc()
- case "Desc" => value.desc().nullsLast()
- })
- case None => List()
- }
- case _ => List() // Default case if the orderBy string doesn't match the pattern
+ List(order match {
+ case "Asc" => field.asc()
+ case "Desc" => field.desc().nullsLast()
+ })
+ case _ => throw new BadRequestException(s"Unknown orderBy: ${searchQueryParams.orderBy}")
}
}
// Helper method to map column names to actual database fields based on resource type
Review Comment:
Corrected the helper comment and listed all accepted orderBy values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): reject invalid dashboard sort [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #8151:
URL: https://github.com/apache/texera/pull/8151#discussion_r3911018941
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala:
##########
@@ -158,7 +158,7 @@ object DashboardResource {
})
case None => List()
}
- case _ => List() // Default case if the orderBy string doesn't match the pattern
+ case _ => throw new BadRequestException(s"Unknown orderBy: ${searchQueryParams.orderBy}")
Review Comment:
The new guard is asymmetric, and the half left behind is the half that caused #8147. `case _` now throws, but `case None => List()` at :159 still returns an empty order list — and that arm fires when a value *inside* the grammar fails to parse, which is the shape the issue reported.
Separately, `?orderBy=` present-but-empty is a real 200→400 change nothing in the PR mentions. `@DefaultValue("EditTimeDesc")` at :84 applies only when the parameter is *absent*; an empty value binds as `""` and now takes the throw arm. Worth a line in the description, since a v1.2 deployment's REST clients are not in this repo to check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org