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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
