You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@texera.apache.org by gi...@apache.org on 2026/08/23 05:00:31 UTC
(texera) branch main updated: test(amber): cover the dataset search query builder's entry construction (#7855)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new 21be7acce7 test(amber): cover the dataset search query builder's entry construction (#7855)
21be7acce7 is described below
commit 21be7acce7b529bedefda1d008caa4501b8456b7
Author: Xinyuan Lin <xi...@uci.edu>
AuthorDate: Sun Aug 23 04:36:03 2026 +0000
test(amber): cover the dataset search query builder's entry construction (#7855)
### What changes were proposed in this PR?
`DatasetSearchQueryBuilderSpec` goes from 9 tests to 18. The existing
spec asserted on rendered SQL only, so `toEntryImpl` — the half that
builds what the dashboard actually displays — was entirely unexercised.
Measured with one fresh sbt JVM per run, `rm -rf` on the jacoco dir
between them, and the same suite-name filter for before and after. Two
metrics, because they differ: Codecov's (fully-covered lines, so a line
with any missed branch arm counts against you) and JaCoCo's line-hit.
| File | Codecov | JaCoCo line-hit | Branches |
|---|---|---|---|
| `DatasetSearchQueryBuilder.scala` | 34/58 = 58.6% → **53/58 = 91.4%**
| 35/58 → **56/58** | 6/18 → **13/18** |
Incidental, same module and same measurement, all reproduced from a HEAD
baseline:
| File | Codecov |
|---|---|
| `FulltextSearchQueryUtils.scala` | 15/33 = 45.5% → **20/33 = 60.6%** |
| `UnifiedResourceSchema.scala` | 61/76 = 80.3% → **65/76 = 85.5%** |
| `SearchQueryBuilder.scala` | 14/20 = 70.0% → **16/20 = 80.0%** |
| `DashboardResource.scala` | 14/89 = 15.7% → **18/89 = 20.2%** |
What the new tests pin: the size that reaches the dashboard, `isOwner`,
the `NONE` privilege fallback, the resource-type literal that
`DashboardResource` `MatchError`s on, the null that becomes the
user-visible `hasMismatch` flag, every dataset column under the alias
its schema slot names, the owner join predicate, the AND-composition of
the date/id/keyword filters, and one keyword per reserved character.
### Two mechanical notes a reviewer will want
**`toEntryImpl` is `override protected` here**, while the `Project` and
`Workflow` siblings widen theirs to `override def`. So the
`ProjectSearchQueryBuilderSpec` pattern does not transfer, and widening
the modifier would be a production edit. The route in is the trait's
public `toEntry`, which runs `UnifiedResourceSchema.translateRecord`
first — so the record must carry the aliased select fields. The spec
fetches the builder's own query to get them.
**The LakeFS loopback stub binds at the host and port the config already
names**, rather than repointing `StorageConfig.lakefsEndpoint`. That is
deliberate and load-bearing: `LakeFSStorageClient`'s
`apiClient`/`refsApi`/`objectsApi` are private `lazy val`s that capture
the endpoint once per JVM, and amber has `Tags.limit(Tags.Test, 1)` but
no `Test / fork`, so the module shares one JVM. `workflow-core` gets
away with mutating that global only because it forks its
`@NonParallelTest` suites; in amber the same move is order-dependent and
would leave the client pointed at a dead port for every later suite. The
stub itself is ~90 duplicated lines, because `amber/build.sbt` declares
only `DAO % "test->test"` and `Auth % "test->test"`, so
`workflow-core`'s stub is not on amber's test classpath.
### Verification
29 mutations. Every survivor is stated below rather than dropped.
A selection, weighted toward the ones that pin something a reader would
otherwise assume:
| Mutation | Killed by |
|---|---|
| **exchange** the `name` and `description` schema slots | projects
every dataset column under the alias its schema slot names |
| the same for `is_public`/`is_downloadable`, `did`/`ownerId`,
`repository`/… | same test, one assertion per pair |
| drop the `creation_time` slot | the projection test, plus a non-null
check on the column |
| alter the owner join predicate | joins the owner row on the dataset's
owner |
| drop any one of the three where-clause connectives | ANDs the date, id
and keyword filters together |
| render only one half of the full-text field list | searches the
dataset's name *and* its description |
| drop either date bound | the same where-clause test, which pins the
rendered range in order |
| break the split for any one of `+ - ( ) < > ~ * @ "` | one assertion
per reserved character, each in a `withClue` naming it |
| construct the entry with the wrong resource type | tags the entry as a
dataset and fills the dataset payload slot |
**Survivors, stated plainly:**
- Replacing `record.into(USER).into(classOf[User])` with a fresh `User`
survives. It is an equivalent mutant *given* a production defect —
`ownerEmail` is always null — so killing it would need either a
production change or an assertion that cements the bug. Neither was
done.
- Deleting `.filter(_.nonEmpty)` at line 110 survives:
`getFullTextSearchFilter` re-filters downstream in
`FulltextSearchQueryUtils`. Equivalent.
- **Environmental, and worth knowing:** with port 8000 held by another
process, the eight mutants behind the stub all become survivors, because
their killer tests `cancel`. This was measured, not assumed. CI is
deterministic here — the amber unit job that runs
`WorkflowExecutionService/jacoco` has no LakeFS, and the
`treeverse/lakefs` container at `build.yml:1011` belongs to a different
job. But a local run with `bin/local-dev.sh up` will silently disarm
those assertions and still go green, so the spec documents that a
`canceled N` line is a disarmed suite rather than a skipped nicety.
### Five claims corrected during review
Adversarial review found 11 defects in the first draft; three of its
suggested fixes were refused as not actually discriminating the mutant
they targeted, or as adding a permanently-dead test-only branch.
Corrections worth recording because they were stated confidently and
wrongly:
- "The frontend renders `resourceName`/`resourceDescription` straight
from the search response" — **false**. `searchAllResources` consumes
every record into a `DashboardClickableFileEntry`; the only grep hit is
unrelated.
- "pgroonga is never reached, so one fewer unrestored global is needed"
— **false**. Dumping the render of the exact call the keyword test makes
shows the pgroonga operator present.
- "Two of the six branch arms of `dataset.getOwnerUid == uid`" — the
jacoco XML says three unreached arms, not two.
- An in-test comment claimed an entry-level assertion would catch a
schema field wired to the wrong column. It cannot, when the wrong column
belongs to another slot — `translateRecord` hides it.
- The first draft presented a name/description swap as a hole it had
found and fixed. True but incomplete: it closed exactly one pair and
left three others, which is why the projection test now carries ten
alias assertions instead of two.
### Reported, not pinned
`ownerEmail` on the constructed entry is always null. Pinning current
behaviour would cement it, so the spec asserts nothing about it.
No production file is touched.
### Any related issues, documentation, discussions?
Closes #7854
### How was this PR tested?
```
sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.dashboard.DatasetSearchQueryBuilderSpec"
```
```
[info] Total number of tests run: 18
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 18, failed 0, canceled 0, ignored 0, pending 0
```
`canceled 0` means port 8000 was free and the suite ran fully armed.
`Test/scalafmtCheck` and `Test/scalafix --check` both pass.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
---
.../dashboard/DatasetSearchQueryBuilderSpec.scala | 471 ++++++++++++++++++++-
1 file changed, 449 insertions(+), 22 deletions(-)
diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala
index 6e760e119c..338a2b3651 100644
--- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala
+++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala
@@ -19,24 +19,37 @@
package org.apache.texera.web.resource.dashboard
+import com.sun.net.httpserver.{HttpExchange, HttpServer}
+import org.apache.texera.common.config.StorageConfig
import org.apache.texera.dao.MockTexeraDB
-import org.apache.texera.web.resource.dashboard.DashboardResource.SearchQueryParams
-import org.scalatest.BeforeAndAfterAll
+import org.apache.texera.dao.jooq.generated.enums.{PrivilegeEnum, UserRoleEnum}
+import org.apache.texera.dao.jooq.generated.tables.daos.{DatasetDao, DatasetUserAccessDao, UserDao}
+import org.apache.texera.dao.jooq.generated.tables.pojos.{Dataset, DatasetUserAccess, User}
+import org.apache.texera.web.resource.dashboard.DashboardResource.{
+ DashboardClickableFileEntry,
+ SearchQueryParams
+}
+import org.jooq.Record
+import org.scalatest.{BeforeAndAfterAll, OptionValues}
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers
+import java.io.IOException
+import java.net.{InetSocketAddress, URI}
+import java.nio.charset.StandardCharsets.UTF_8
+import java.util.concurrent.{ExecutorService, Executors}
import scala.jdk.CollectionConverters._
/**
- * Covers the access-control shape of the SQL `DatasetSearchQueryBuilder` produces.
+ * Covers `DatasetSearchQueryBuilder` from both ends: the shape of the SQL it builds, and the
+ * `DashboardClickableFileEntry` it turns a fetched row into.
*
- * Every member of the builder is `override protected`, so unlike the workflow and project arms
- * nothing here is directly callable. The one public route in is the trait's `final constructQuery`,
- * and what it returns can be rendered to SQL and inspected without ever executing it — so these are
- * assertions about the query's *shape*, not about rows.
+ * == Part 1: which datasets a caller may see ==
*
- * What that buys is the piece of this file with real consequences: which datasets a caller is
- * allowed to see. Three branches decide it and none is otherwise tested —
+ * Every member of the builder is `override protected`, so the query side is reachable only through
+ * the trait's `final constructQuery`, and what it returns can be rendered to SQL and inspected
+ * without executing it — those are assertions about the query's *shape*. Three branches decide
+ * visibility and nothing else tests them:
*
* - the join predicate `.and(if (uid == null) DSL.falseCondition() else DATASET_USER_ACCESS.UID.eq(uid))`,
* which scopes the grant rows to the caller. Without the `eq(uid)` the `UID.isNotNull` check
@@ -45,27 +58,251 @@ import scala.jdk.CollectionConverters._
* - `includePublic == false`, which must return only explicitly-granted datasets and must not
* leak public ones in.
*
- * `initializeDBAndReplaceDSLContext` is needed only because `SearchQueryBuilder.context` reads
- * `SqlServer.getInstance()`; no query is run against the database.
+ * == Part 2: what a row becomes ==
+ *
+ * `toEntryImpl` is `override protected` here (the workflow and project siblings widen theirs to
+ * `override def`), so it is not directly callable. The route in is the trait's public `toEntry`,
+ * which first runs `UnifiedResourceSchema.translateRecord` — so the record handed to it must carry
+ * the *aliased* select fields, not raw `DATASET.*` fields. Rather than hand-build that record,
+ * these tests fetch the builder's own query and pass a real row, which is exactly what
+ * `DashboardResource.searchAllResources` does.
+ *
+ * A record permutation is invisible from this end. `translateRecord` writes each aliased value back
+ * to the very column the schema paired it with (`UnifiedResourceSchema:174`), so ANY swap of two
+ * same-typed schema assignments — `name`/`description`, `is_public`/`is_downloadable`,
+ * `did`/`ownerId` — cancels out exactly and produces a byte-identical entry. What the entry-level
+ * column assertions below really pin is that each slot still references *some* `DATASET` column
+ * (wire a slot to a literal and `translateRecord` drops the field, so the value reads back null).
+ * The projections are pinned where a permutation is actually visible: the SQL-shape test
+ * "project every dataset column under the alias its schema slot names".
+ *
+ * `toEntryImpl` also calls `LakeFSStorageClient.retrieveRepositorySize`, which is a live HTTP call
+ * with no injectable seam. Two non-obvious things make it testable anyway:
+ *
+ * - **The stub binds AT the configured endpoint** (`StorageConfig.lakefsEndpoint`, i.e.
+ * `localhost:8000`) instead of repointing that endpoint at an ephemeral port. This is
+ * deliberate. `LakeFSStorageClient.apiClient` is a `lazy val` that captures the endpoint once
+ * per JVM, and `amber/build.sbt` runs every suite in ONE unforked JVM
+ * (`Tags.limit(Tags.Test, 1)` serializes them but does not isolate them). Mutating the global
+ * `StorageConfig.lakefsEndpoint` here would therefore be order-dependent — any earlier suite
+ * that forced `apiClient` wins — and would leave the client bound to a dead port for every
+ * later suite. Binding at the address the config already names is order-independent and
+ * memoizes nothing wrongly. The price is that the port is machine-global: if something else
+ * holds it (a local `bin/local-dev.sh up` lakeFS, or a sibling worktree running these tests),
+ * the four tests that need a *successful* size `cancel` rather than fail. CI has no lakeFS in
+ * this job, so it is deterministic there.
+ *
+ * A cancel is louder than it looks, and the sbt log shows it only as `canceled 4` while the
+ * build stays green. It disarms the whole `toEntry` half of this suite: not just its coverage
+ * (which falls back toward the ~59% this file had before), but every assertion protecting
+ * `toEntryImpl`. Measured: with the port held, mutating `size` to `0L` in the entry leaves
+ * `succeeded 14, failed 0, canceled 4` and sbt printing "All tests passed" at exit 0. The
+ * assertions in the SQL-shape tests are unaffected, because they render rather than fetch and
+ * never call lakeFS — that is the half that stays armed everywhere (verified: with the port
+ * held, breaking a projection or a where-clause connective still fails the build).
+ * - **The `null`-return test needs no stub at all.** Connection-refused, 404 and 501 all surface
+ * as `io.lakefs.clients.sdk.ApiException` (`ApiClient` wraps `IOException` in one, and non-2xx
+ * throws one directly), so that test behaves identically whether the stub bound or not.
+ *
+ * The stub is ~50 lines duplicated in spirit from
+ * `common/workflow-core/.../LakeFSStorageClientSpec`. It cannot be shared: `build.sbt` gives
+ * `WorkflowExecutionService` a test->test dependency on `DAO` and `Auth` only, so workflow-core's
+ * test tree is not on this module's test classpath.
+ *
+ * The keyword tests RENDER a full-text predicate (they do not fetch one), which does read the
+ * JVM-global `FulltextSearchQueryUtils.usePgroonga` and emits whichever arm it currently selects:
+ * `pgroonga_condition(...)` when this suite runs alone, the `to_tsvector`/`to_tsquery` arm if
+ * `DatasetResourceSpec` or `WorkflowResourceSpec` ran earlier in this JVM and left the global
+ * `false` (both set it and neither restores it; amber has no `Test / fork`). This suite therefore
+ * neither touches nor restores that global, and every keyword assertion here is deliberately
+ * branch-independent: the tokens themselves and the `coalesce(...) || ' ' || coalesce(...)`
+ * expression are built at `FulltextSearchQueryUtils:49-51`, *before* the `if (usePgroonga)`.
+ * Anything added here must keep that property — an assertion on `pgroonga_condition` would pass
+ * solo and fail in a full-module run.
+ *
+ * Two lines are executed but unobservable, on purpose. `val owner = record.into(USER)...` and
+ * `owner.getEmail` run on every entry, yet replacing them with a bare `new User` changes nothing:
+ * the dataset schema leaves `UnifiedResourceSchema`'s `userEmail` at its `DSL.inline("")` default,
+ * so the translated record carries no `USER` column and `DashboardDataset.ownerEmail` is ALWAYS
+ * null for dataset search results — which also makes the `leftJoin(USER)` a join that is selected
+ * from and never read. That is a production defect, reported separately; asserting the null here
+ * would cement it, so these tests pin the join's shape and leave the value alone.
*
- * Deliberately not covered: `toEntryImpl` (lines 128-164) is ~80% of this file's uncovered lines
- * and sits behind a live LakeFS `retrieveRepositorySize` call with no mockable seam. Reaching it
- * would need a source change (injecting a repository-size provider), not a test.
+ * Not covered, and not coverable from a test:
+ * - `constructFromClause`'s `includePublic: Boolean = false` default. `scalac` emits
+ * `constructFromClause$default$3`, but `constructQuery` always passes all three arguments, so
+ * it has zero call sites and the method is `protected`.
+ * - the `LazyLogging` `logger` lazy-val bitmap on the `object` line, and scala-logging's
+ * `if (underlying.isErrorEnabled)` guard around `logger.error`. Only the enabled arm ever runs.
+ * - three of the six branch arms of `dataset.getOwnerUid == uid` (JaCoCo reports cb=3, mb=3).
+ * They are the null-safe boxed-`Integer` equality's null checks, and they describe a null
+ * `owner_uid` — a state `INT NOT NULL` plus an FK to `"user"(uid)` forbids.
+ * - `class DatasetSearchQueryBuilder {}` at the bottom of the file: a vestigial empty class with
+ * zero references repo-wide. It should be deleted, not instantiated by a test.
+ *
+ * Covered but unconstrainable, so no reviewer should count it as pinned behaviour:
+ * - the `.filter(_.nonEmpty)` after the keyword split. `getFullTextSearchFilter` re-applies
+ * `keywords.filter(_.nonEmpty)` itself (`FulltextSearchQueryUtils:43`), so dropping it here is
+ * an equivalent mutation — no observable differs.
*/
class DatasetSearchQueryBuilderSpec
extends AnyFlatSpec
with Matchers
+ with OptionValues
with BeforeAndAfterAll
with MockTexeraDB {
private val uid: Integer = Integer.valueOf(42)
+ // Owner and a second signed-in caller. Both are outside java.lang.Integer's cache (-128..127) so
+ // that `dataset.getOwnerUid == uid` cannot pass by reference identity.
+ private val ownerUid: Integer = Integer.valueOf(9101)
+ private val otherUid: Integer = Integer.valueOf(9102)
+
+ private val sizedDid: Integer = Integer.valueOf(9001)
+ private val goneDid: Integer = Integer.valueOf(9002)
+
+ private val sizedRepo = "texera-ds-sized"
+ private val goneRepo = "texera-ds-gone"
+
+ // ---------------------------------------------------------------------------------------------
+ // lakeFS loopback stub, bound at the endpoint the config already names (see the class comment).
+ // ---------------------------------------------------------------------------------------------
+
+ private val lakefsUri = new URI(StorageConfig.lakefsEndpoint)
+
+ // Normalised, because the handler paths below are matched by exact equality and
+ // StorageConfig.lakefsEndpoint is a `var` that tests are expected to override. A configured
+ // ".../api/v1/" would otherwise yield "/api/v1//repositories/..." here, match nothing, and make
+ // the requireStub()-guarded tests FAIL on a 404 rather than cancel -- the one failure mode the
+ // class comment promises they will not have.
+ private val apiPrefix = lakefsUri.getPath.stripSuffix("/")
+
+ private val commitId = "c0ffee"
+
+ private val commitListJson =
+ s"""{"pagination":{"has_more":false,"next_offset":"","results":1,"max_per_page":1000},
+ |"results":[{"id":"$commitId","parents":[],"committer":"tester","message":"m",
+ |"creation_date":1700000000,"meta_range_id":"mr"}]}""".stripMargin
+
+ private def objectStats(path: String, sizeBytes: Long): String =
+ s"""{"path":"$path","path_type":"object","physical_address":"s3://bucket/$path",
+ |"checksum":"chk","mtime":1700000000,"size_bytes":$sizeBytes}""".stripMargin
+
+ /** Two objects, 10 + 32 bytes, so the size the entry carries is a value nothing else produces. */
+ private val objectPageJson = {
+ val objects = Seq(objectStats("small.csv", 10L), objectStats("large.csv", 32L)).mkString(",")
+ s"""{"pagination":{"has_more":false,"next_offset":"","results":2,"max_per_page":1000},
+ |"results":[$objects]}""".stripMargin
+ }
+
+ private var server: HttpServer = _
+ private var serverPool: ExecutorService = _
+ private var stubBound: Boolean = false
+
+ private def respond(exchange: HttpExchange, status: Int, body: String): Unit = {
+ val bytes = body.getBytes(UTF_8)
+ exchange.getResponseHeaders.set("Content-Type", "application/json")
+ exchange.sendResponseHeaders(status, bytes.length.toLong)
+ exchange.getResponseBody.write(bytes)
+ }
+
+ private def handle(exchange: HttpExchange): Unit =
+ try {
+ exchange.getRequestBody.readAllBytes()
+ exchange.getRequestURI.getPath match {
+ case p if p == s"$apiPrefix/repositories/$sizedRepo/refs/main/commits" =>
+ respond(exchange, 200, commitListJson)
+ case p if p == s"$apiPrefix/repositories/$sizedRepo/refs/$commitId/objects/ls" =>
+ respond(exchange, 200, objectPageJson)
+ case _ =>
+ // Everything else — including `goneRepo` — is a repository lakeFS does not have.
+ respond(exchange, 404, """{"message":"repository not found"}""")
+ }
+ } finally exchange.close()
+
+ /**
+ * Tests that need `retrieveRepositorySize` to *succeed* can only run if the stub owns the
+ * configured port. Cancelling is the documented environmental skip; it is not a failure.
+ */
+ private def requireStub(): Unit =
+ if (!stubBound) {
+ cancel(
+ s"another process holds ${lakefsUri.getHost}:${lakefsUri.getPort}, " +
+ s"the endpoint StorageConfig.lakefsEndpoint names; cannot serve a lakeFS size here"
+ )
+ }
+
+ // ---------------------------------------------------------------------------------------------
+ // Fixture
+ // ---------------------------------------------------------------------------------------------
+
+ private def user(u: Integer, name: String): User = {
+ val user = new User
+ user.setUid(u)
+ user.setName(name)
+ user.setEmail(s"$name@mail.com")
+ user.setRole(UserRoleEnum.REGULAR)
+ user
+ }
+
+ private def dataset(
+ did: Integer,
+ name: String,
+ repositoryName: String,
+ isDownloadable: Boolean
+ ): Dataset = {
+ val d = new Dataset
+ d.setDid(did)
+ d.setOwnerUid(ownerUid)
+ d.setName(name)
+ d.setRepositoryName(repositoryName)
+ d.setIsPublic(true)
+ d.setIsDownloadable(java.lang.Boolean.valueOf(isDownloadable))
+ d.setDescription(s"description of $name")
+ d.setCoverImage(s"$name.png")
+ d
+ }
+
+ private def grant(did: Integer, u: Integer, privilege: PrivilegeEnum): DatasetUserAccess = {
+ val access = new DatasetUserAccess
+ access.setDid(did)
+ access.setUid(u)
+ access.setPrivilege(privilege)
+ access
+ }
+
override protected def beforeAll(): Unit = {
initializeDBAndReplaceDSLContext()
+
+ val cfg = getDSLContext.configuration()
+ new UserDao(cfg)
+ .insert(user(ownerUid, "dataset_search_owner"), user(otherUid, "dataset_search_other"))
+ new DatasetDao(cfg).insert(
+ dataset(sizedDid, "sized_dataset", sizedRepo, isDownloadable = false),
+ dataset(goneDid, "gone_dataset", goneRepo, isDownloadable = true)
+ )
+ // Only the owner holds an explicit grant; `otherUid` sees the dataset purely because it is
+ // public, which is what drives the PrivilegeEnum.NONE fallback.
+ new DatasetUserAccessDao(cfg).insert(grant(sizedDid, ownerUid, PrivilegeEnum.WRITE))
+
+ try {
+ server = HttpServer.create(new InetSocketAddress(lakefsUri.getHost, lakefsUri.getPort), 0)
+ server.createContext("/", (exchange: HttpExchange) => handle(exchange))
+ serverPool = Executors.newFixedThreadPool(2)
+ server.setExecutor(serverPool)
+ server.start()
+ stubBound = true
+ } catch {
+ case _: IOException => stubBound = false
+ }
}
override protected def afterAll(): Unit = {
- shutdownDB()
+ try {
+ if (server != null) server.stop(0)
+ if (serverPool != null) serverPool.shutdownNow()
+ } finally shutdownDB()
}
private def params(keywords: String*): SearchQueryParams =
@@ -85,6 +322,21 @@ class DatasetSearchQueryBuilderSpec
.toLowerCase
.replace("\"", "")
+ /** One row of the builder's own query, narrowed to a single dataset id. */
+ private def rowFor(callerUid: Integer, did: Integer): Record = {
+ val p = SearchQueryParams(datasetIds = List(did).asJava)
+ val rows = DatasetSearchQueryBuilder.constructQuery(callerUid, p, includePublic = true).fetch()
+ rows.size() shouldBe 1
+ rows.get(0)
+ }
+
+ private def entryFor(callerUid: Integer, did: Integer): DashboardClickableFileEntry =
+ DatasetSearchQueryBuilder.toEntry(callerUid, rowFor(callerUid, did))
+
+ // ---------------------------------------------------------------------------------------------
+ // constructQuery: access control
+ // ---------------------------------------------------------------------------------------------
+
"an anonymous search" should "be restricted to public datasets" in {
val sql = sqlFor(null, includePublic = true)
@@ -128,14 +380,72 @@ class DatasetSearchQueryBuilderSpec
sql should include(" or ")
}
- "the keyword filter" should "split on the punctuation the full-text engine reserves" in {
- // A user pasting `a+b` means two terms, not a literal. The split set is this file's own and is
- // not shared with the other builders.
- val sql = sqlFor(uid, includePublic = true, params("alpha+beta"))
+ "the keyword filter" should "split on every character the full-text engine reserves" in {
+ // A user pasting `a+b` means two terms, not a literal, and the split set is this file's own —
+ // not shared with the other builders. Every character in it needs its own keyword: whatever
+ // survives the split is interpolated straight into a plain-SQL condition string
+ // (FulltextSearchQueryUtils:56), so a character dropped from the class reaches the rendered
+ // predicate intact. `"` is the one that matters most, being a quote inside a raw SQL literal.
+ val reserved = Seq('+', '-', '(', ')', '<', '>', '~', '*', '@', '"')
+
+ reserved.zipWithIndex.foreach {
+ case (reservedChar, i) =>
+ val left = s"lhs$i"
+ val right = s"rhs$i"
+ val keyword = s"$left$reservedChar$right"
+ // Rendered WITHOUT `sqlFor`'s quote-stripping: for `"` that helper would erase the very
+ // character under test. Both arms of `usePgroonga` carry the surviving tokens verbatim.
+ val sql = getDSLContext
+ .renderInlined(
+ DatasetSearchQueryBuilder.constructQuery(uid, params(keyword), includePublic = true)
+ )
+ .toLowerCase
+
+ withClue(s"reserved character '$reservedChar' -> ") {
+ sql should include(left)
+ sql should include(right)
+ sql should not include keyword
+ }
+ }
+ }
+
+ it should "search the dataset's name and its description, not just one of them" in {
+ // The keyword tokens land in the predicate's search argument whichever columns are searched, so
+ // only this concatenated expression can see the field list shrink to one column — dataset search
+ // silently stopping to match descriptions, or names. The expression is built at
+ // FulltextSearchQueryUtils:49-51, before the `if (usePgroonga)`, so both arms render it.
+ val sql = sqlFor(uid, includePublic = true, params("alpha"))
- sql should include("alpha")
- sql should include("beta")
- sql should not include "alpha+beta"
+ sql should include("coalesce(texera_db.dataset.name, '')")
+ sql should include("coalesce(texera_db.dataset.description, '')")
+ }
+
+ "the where clause" should "AND its date, id and keyword filters together" in {
+ // The only test that renders more than one non-empty filter at once, and therefore the only one
+ // that can see the connectives: jOOQ drops `noCondition()` out of an AND chain AND out of an OR
+ // chain, so with a single filter present `.and` and `.or` render identically. Swapping either
+ // link turns a dashboard search into a UNION of the filters and hands back rows the caller's
+ // filter excluded.
+ val p = SearchQueryParams(
+ keywords = List("alpha").asJava,
+ datasetIds = List(sizedDid).asJava,
+ creationStartDate = "2020-01-01",
+ creationEndDate = "2020-12-31"
+ )
+ val sql = sqlFor(uid, includePublic = true, p)
+
+ // The create-date range, on the create-date column, with the bounds in that order. Nothing else
+ // in the suite passes a date, so without this the whole `getDateFilter` call is inert: it is
+ // satisfied by the modified-date params, or by the two arguments swapped.
+ sql should include(
+ "texera_db.dataset.creation_time between timestamp '2020-01-01 00:00:00.0' " +
+ "and timestamp '2020-12-31 23:59:59.999'"
+ )
+ // ... AND the dataset-id filter ...
+ sql should include(s"and texera_db.dataset.did = $sizedDid")
+ // ... AND the full-text filter. Matching only the opening paren keeps this independent of which
+ // `usePgroonga` arm rendered the predicate itself.
+ sql should include(s"texera_db.dataset.did = $sizedDid and (")
}
"the query" should "dedupe with selectDistinct rather than a group by" in {
@@ -152,4 +462,121 @@ class DatasetSearchQueryBuilderSpec
// MatchError at fetch time rather than a compile error.
sqlFor(uid, includePublic = true) should include("'dataset'")
}
+
+ it should "project every dataset column under the alias its schema slot names" in {
+ // `toEntry` cannot see a mistake here at all. `UnifiedResourceSchema.translateRecord` writes
+ // every alias back to the column the schema paired it with, so swapping ANY two same-typed
+ // assignments — name/description, is_public/is_downloadable, did/ownerId — cancels out by the
+ // time `toEntryImpl` reads the record and yields a byte-identical entry. The rendered SELECT is
+ // the only place a permutation is visible.
+ //
+ // A permutation is not harmless downstream. `DashboardResource.getOrderFields` sorts on
+ // `resourceName` (its "Name" case, DashboardResource:168) and on `resourceCreationTime` (its
+ // "CreateTime" case), both by alias, so a swap silently re-points dashboard sorting. It is NOT
+ // the frontend that reads these aliases: `searchAllResources` consumes each record into a
+ // `DashboardClickableFileEntry` and returns those, so no alias survives into the HTTP response.
+ // `resourceDescription` in fact has no production reader at all today —
+ // `UnifiedResourceSchema.resourceDescriptionField` is referenced nowhere and `getColumnField`
+ // has no "Description" case — which is worth an issue of its own; it is pinned here anyway
+ // because the schema slot is what a copy-paste between builders gets wrong.
+ val sql = sqlFor(uid, includePublic = true)
+
+ sql should include("dataset.name as resourcename")
+ sql should include("dataset.description as resourcedescription")
+ sql should include("dataset.creation_time as resourcecreationtime")
+ sql should include("dataset.owner_uid as resourceownerid")
+ sql should include("dataset.did as did")
+ sql should include("dataset.repository_name as repository_name")
+ sql should include("dataset.is_public as is_dataset_public")
+ sql should include("dataset.is_downloadable as is_dataset_downloadable")
+ sql should include("dataset_user_access.privilege as user_dataset_access")
+ sql should include("dataset.cover_image as cover_image")
+ }
+
+ it should "join the owner row on the dataset's owner" in {
+ // `toEntryImpl` reads `owner.getEmail` out of this join, so the predicate is load-bearing on
+ // paper; in practice the value is always null (see the class comment) and asserting it would
+ // cement that bug. The join's shape is safe to pin and is otherwise unconstrained: nothing else
+ // in the suite can tell `USER.UID.eq(DATASET.OWNER_UID)` from any other predicate, or from the
+ // join being absent altogether.
+ val sql = sqlFor(uid, includePublic = true)
+
+ sql should include("left outer join texera_db.user on texera_db.user.uid = ")
+ sql should include("texera_db.user.uid = texera_db.dataset.owner_uid")
+ }
+
+ // ---------------------------------------------------------------------------------------------
+ // toEntry
+ // ---------------------------------------------------------------------------------------------
+
+ "toEntry" should "carry every dataset column and the lakeFS repository size into the entry" in {
+ requireStub()
+
+ val entry = entryFor(ownerUid, sizedDid)
+ entry should not be null
+
+ val dd = entry.dataset.value
+ dd.dataset.getDid shouldBe sizedDid
+ dd.dataset.getOwnerUid shouldBe ownerUid
+ dd.dataset.getName shouldBe "sized_dataset"
+ dd.dataset.getDescription shouldBe "description of sized_dataset"
+ dd.dataset.getRepositoryName shouldBe sizedRepo
+ dd.dataset.getIsPublic shouldBe true
+ // The fixture flips this away from the column default, so a slot wired to a literal (rather
+ // than to a DATASET column) reads back as null here rather than accidentally matching. What it
+ // cannot see is a swap with `is_public`: see the projection test for why, and for where that is
+ // pinned instead.
+ dd.dataset.getIsDownloadable shouldBe false
+ dd.dataset.getCoverImage shouldBe "sized_dataset.png"
+ // DDL-defaulted, so this is null only if the schema stops projecting the column at all.
+ dd.dataset.getCreationTime should not be null
+ // 10 + 32 across the newest commit's two objects. This is the number the dashboard shows.
+ dd.size shouldBe 42L
+ }
+
+ it should "set isOwner only for the dataset's own owner" in {
+ requireStub()
+
+ entryFor(ownerUid, sizedDid).dataset.value.isOwner shouldBe true
+ entryFor(otherUid, sizedDid).dataset.value.isOwner shouldBe false
+ // An anonymous caller reaches the dataset because it is public, and owns nothing.
+ entryFor(null, sizedDid).dataset.value.isOwner shouldBe false
+ }
+
+ it should "report the granted privilege, and fall back to NONE with no grant row" in {
+ requireStub()
+
+ entryFor(ownerUid, sizedDid).dataset.value.accessPrivilege shouldBe PrivilegeEnum.WRITE
+ // `otherUid` has no dataset_user_access row: the left join yields null, and null must read as
+ // NONE rather than propagate to the client as a missing privilege.
+ entryFor(otherUid, sizedDid).dataset.value.accessPrivilege shouldBe PrivilegeEnum.NONE
+ }
+
+ it should "tag the entry as a dataset and fill the dataset payload slot" in {
+ // `entry.workflow shouldBe None` / `entry.project shouldBe None` used to sit here and were
+ // vacuous: `DashboardClickableFileEntry` declares both `= None` (DashboardResource:40-41) and
+ // this file passes neither, so no mutation of `toEntryImpl` can falsify them — they assert
+ // another file's case-class defaults. Only `resourceType` and `dataset` are this file's to set.
+ //
+ // `resourceType` here has the production constant on the right-hand side, so retargeting the
+ // constant's *value* moves both sides together; the inlined-literal assertion in
+ // "tag its rows as datasets" is what actually pins the value. The `getDid` check below is an
+ // identity check on the payload — that the entry carries the row it was built from — and is
+ // redundant with the column test rather than newly discriminating.
+ requireStub()
+
+ val entry = entryFor(ownerUid, sizedDid)
+ entry.resourceType shouldBe SearchQueryBuilder.DATASET_RESOURCE_TYPE
+ entry.dataset should be(defined)
+ entry.dataset.value.dataset.getDid shouldBe sizedDid
+ }
+
+ it should "drop the row entirely when lakeFS rejects the repository" in {
+ // No `requireStub()`: a 404 from the stub, a 404 from a real lakeFS and a refused connection all
+ // arrive as io.lakefs.clients.sdk.ApiException, so this holds however the port is occupied.
+ //
+ // The null is load-bearing. DashboardResource filters these rows out and raises `hasMismatch`
+ // from them; without it a dataset whose repository is gone is shown to the user as 0 bytes.
+ DatasetSearchQueryBuilder.toEntry(ownerUid, rowFor(ownerUid, goneDid)) shouldBe null
+ }
}