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 <[email protected]>
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"[email protected]")
+ 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
+ }
}
