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 0298a3bbe4 feat(amber): report the owner's email in dataset search
(#7875)
0298a3bbe4 is described below
commit 0298a3bbe49b644d062a5e3b3dd56dd0e399cc84
Author: Xinyuan Lin <[email protected]>
AuthorDate: Mon Aug 31 15:14:02 2026 +0000
feat(amber): report the owner's email in dataset search (#7875)
### What changes were proposed in this PR?
**Why it was never populated.** Every LakeFS-backed resource hydrates
its dashboard entry through `VersionedResourceTables.hydrate`, which
reads the owner out of the joined `USER` row:
```scala
record.into(USER).into(classOf[User]).getEmail
```
`joinWithAccessAndOwner` writes that join for every versioned resource.
But the select list comes entirely from each subclass's
`mappedResourceSchema`, and `UnifiedResourceSchema.apply` defaults
`userEmail` to `DSL.inline("")` — a slot `DatasetSearchQueryBuilder`
never named. So `hydrate` reads the owner out of a record that has no
owner in it.
Two steps turn the missing projection into a `null`:
```
schema slot userEmail = DSL.inline("") <- default, never overridden
|
SQL '' as email <- USER is joined, never read
|
translateRecord dedupes by ORIGINAL field, and jOOQ compares fields by
rendered SQL, so projectsOfWorkflow / userName / userEmail
/
projectColor -- all DSL.inline("") -- collapse to ONE entry
keyed on the first of them
|
record no `email` column at all -> record.into(USER) = empty User
|
entry DashboardDataset.ownerEmail = null (not "")
```
**Change.** Name the slot — in the base class, so every versioned
resource gets it (see below). `hydrate` is untouched.
Before → after:
| | before | after |
|---|---|---|
| projection | `'' as email` | `texera_db.user.email as email` |
| `DashboardDataset.ownerEmail` | `null`, every row | the dataset
owner's address |
| owner `leftJoin(USER)` | joined, selected from, never read | read |
`WorkflowSearchQueryBuilder` is the sibling that shows the step that was
missed: it opts into the USER column it reads (`userName = USER.NAME`),
and `VersionedResourceSearchQueryBuilder` already filters on
`USER.EMAIL` for the `owners` query param — the email was reachable
through the join all along, only the projection was absent.
`HubResource` and file-service's `/dataset/list` both populate the same
field correctly; dataset search was the one producer that did not.
Scope of the impact, stated precisely because it is narrower than it
looks: `DashboardEntry.ownerEmail` (`dashboard-entry.ts:132`) receives
the null, and no frontend code reads that field for datasets today, so
no screen was visibly wrong. It was a trap rather than a broken page —
`dataset-selection-modal.component.ts:127` builds the storage logical
path `/${ResourceType.Dataset}/${ownerEmail}/${name}/${version}` out of
a `DashboardDataset`, and only escapes `/dataset/null/...` because it
lists via file-service.
**The slot lives in the base class, per @mengw15's review.** `hydrate`
is `final` on `VersionedResourceTables` and reads `USER.EMAIL` for
*every* versioned resource, so leaving the projection to each subclass
meant the next resource type would inherit the same trap.
`VersionedResourceSearchQueryBuilder` now builds the whole
`UnifiedResourceSchema` as a `final lazy val` a subclass cannot replace:
```scala
final override protected lazy val mappedResourceSchema:
UnifiedResourceSchema =
UnifiedResourceSchema(
resourceType = DSL.inline(tables.resourceType),
name = tables.nameColumn,
...
userEmail = USER.EMAIL,
repositoryName = repositoryNameColumn,
...
)
```
Eight of the twelve fields come off the `tables` descriptor, `userEmail`
is fixed here, and a subclass supplies only the three columns the
descriptor does not name:
```scala
object DatasetSearchQueryBuilder
extends
VersionedResourceSearchQueryBuilder(VersionedResourceTables.DatasetTables) {
override protected val repositoryNameColumn: Field[String] =
DATASET.REPOSITORY_NAME
override protected val isDownloadableColumn: Field[java.lang.Boolean] =
DATASET.IS_DOWNLOADABLE
override protected val coverImageColumn: Field[String] =
DATASET.COVER_IMAGE
}
```
A new versioned resource now gets the owner email whether or not its
author thinks about it. It also removes the duplication where the
descriptor and the projection each spelled out `DATASET.NAME` /
`DATASET.DESCRIPTION` / `DATASET.DID`, so the FROM clause and the select
list can no longer disagree about which column they mean.
Two things worth a reviewer's eye:
- **`lazy` is load-bearing, not decoration.** The abstract projection
members are subclass `val`s, so a plain `val` here reads them before
they are initialised. Verified: it dies with `NullPointerException:
Cannot invoke "org.jooq.Field.as(String)" because "repositoryName" is
null`.
- **The rendered projection is otherwise unchanged.** Every pre-existing
per-alias assertion in the projection test passes untouched; only the
`email` column is added. That is what confirms `tables.nameColumn` and
friends are the same fields the subclass used to name by hand.
### Any related issues, documentation, discussions?
Closes #7874
### How was this PR tested?
`DatasetSearchQueryBuilderSpec` goes from 21 tests to 24, reusing the
fixture and lakeFS loopback stub #7855 built.
| test | what it pins |
|---|---|
| `carry the owner's email address` | the value on the entry — the
assertion that kills the mutant below |
| `take the email from the dataset's owner, not from the caller` |
fetched as `otherUid`, who reaches the dataset only because it is
public, so a lookup that echoed the signed-in caller back would fail |
| `project every dataset column under the alias its schema slot names`
(extended) | `user.email as email` in the SELECT. Now that the slot is
the base class's, this guards every versioned resource: dropping it from
`VersionedResourceSearchQueryBuilder` fails 3 tests here |
| `stay union-compatible with the workflow and project branches` | new —
see below |
The mutation #7855 recorded as surviving now dies. Replacing the
`record.into(USER).into(classOf[User])` in `hydrate` with a fresh
`User`:
| | before this PR | after |
|---|---|---|
| `new User` mutant | survives the whole suite (equivalent mutant, given
the defect) | `succeeded 22, failed 2` — both owner-email tests |
Verified in order, one sbt JVM each, all after merging main:
| run | result |
|---|---|
| with the change | `succeeded 24, failed 0, canceled 0` |
| `new User` mutant in `hydrate` | `succeeded 22, failed 2, canceled 0`
|
| `userEmail` slot dropped from the base class | `succeeded 21, failed
3, canceled 0` |
| base schema as a plain `val` instead of `lazy val` |
`NullPointerException` on `repositoryName` — why the `lazy` is there |
| the 7 dashboard suites (adds `UnifiedResourceSchemaSpec`,
`WorkflowSearchQueryBuilderSpec`, `ProjectSearchQueryBuilderSpec`) |
`Suites: completed 7, Tests: succeeded 113, failed 0, canceled 0` |
| `scalafmtCheckAll` + `Test/scalafixAll --check` | clean |
```bash
sbt "WorkflowExecutionService/testOnly
org.apache.texera.web.resource.dashboard.DatasetSearchQueryBuilderSpec"
```
Two notes for a reviewer:
**What the union test is and is not for.** @mengw15 is right that the
hazard is not new: `userName` already has exactly this shape — only the
workflow branch projects a real column, dataset and project both project
`DSL.inline("")` — and that union runs in production today, so a
`varchar`-vs-`''` mix in one slot is not something this PR introduces. I
have corrected the test's comment and dropped the overstated claim that
was in this description. The test is still worth keeping:
`DashboardResource.searchAllResources` stacks the three builders with
`unionAll` for the dashboard's default view, nothing else in the suite
executes that union, and the contract is invisible from inside a single
builder.
**`canceled` is the failure mode to watch, and it moved.** The stub
binds the configured port (`localhost:8000`) rather than an ephemeral
one, because `LakeFSStorageClient.apiClient` is a `lazy val` capturing
`StorageConfig.lakefsEndpoint` once per JVM and amber runs every suite
in one unforked JVM. If something else holds that port — a local
`bin/local-dev.sh up` — the stub-dependent tests cancel, and that count
goes from 4 to 6 with the owner-email pair added. A cancel is quiet: sbt
prints `All tests passed` at exit 0. Measured on the merged suite, with
the port held and `size` mutated to `0L` in `hydrate`, the run reports
`succeeded 18, failed 0, canceled 6` and still exits green — so the
owner-email pair is disarmed alongside the rest of the `toEntry` half.
The class comment carries these numbers and I re-measured them after the
merge. CI has no lakeFS in this job, so it is deterministic there.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-5)
---
.../dashboard/DatasetSearchQueryBuilder.scala | 27 +++---
.../VersionedResourceSearchQueryBuilder.scala | 40 ++++++++-
.../dashboard/DatasetSearchQueryBuilderSpec.scala | 97 ++++++++++++++++++----
3 files changed, 129 insertions(+), 35 deletions(-)
diff --git
a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilder.scala
b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilder.scala
index ae8b89c371..8fea491156 100644
---
a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilder.scala
+++
b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilder.scala
@@ -19,26 +19,21 @@
package org.apache.texera.web.resource.dashboard
-import org.apache.texera.dao.jooq.generated.Tables.{DATASET,
DATASET_USER_ACCESS}
-import org.jooq.impl.DSL
+import org.apache.texera.dao.jooq.generated.Tables.DATASET
+import org.jooq.Field
-/** Query logic lives in [[VersionedResourceSearchQueryBuilder]]; only the
projection is here. */
+/**
+ * Query logic and projection live in
[[VersionedResourceSearchQueryBuilder]]; only the columns
+ * the [[VersionedResourceTables]] descriptor does not already name are here.
+ */
object DatasetSearchQueryBuilder
extends
VersionedResourceSearchQueryBuilder(VersionedResourceTables.DatasetTables) {
- override protected val mappedResourceSchema: UnifiedResourceSchema =
UnifiedResourceSchema(
- resourceType = DSL.inline(SearchQueryBuilder.DATASET_RESOURCE_TYPE),
- name = DATASET.NAME,
- description = DATASET.DESCRIPTION,
- creationTime = DATASET.CREATION_TIME,
- ownerId = DATASET.OWNER_UID,
- versionedResourceId = DATASET.DID,
- repositoryName = DATASET.REPOSITORY_NAME,
- isVersionedResourcePublic = DATASET.IS_PUBLIC,
- isVersionedResourceDownloadable = DATASET.IS_DOWNLOADABLE,
- versionedResourceUserAccess = DATASET_USER_ACCESS.PRIVILEGE,
- versionedResourceCoverImage = DATASET.COVER_IMAGE
- )
+ override protected val repositoryNameColumn: Field[String] =
DATASET.REPOSITORY_NAME
+
+ override protected val isDownloadableColumn: Field[java.lang.Boolean] =
DATASET.IS_DOWNLOADABLE
+
+ override protected val coverImageColumn: Field[String] = DATASET.COVER_IMAGE
}
class DatasetSearchQueryBuilder {}
diff --git
a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/VersionedResourceSearchQueryBuilder.scala
b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/VersionedResourceSearchQueryBuilder.scala
index 8893c20395..4dd17c9221 100644
---
a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/VersionedResourceSearchQueryBuilder.scala
+++
b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/VersionedResourceSearchQueryBuilder.scala
@@ -26,18 +26,52 @@ import
org.apache.texera.web.resource.dashboard.FulltextSearchQueryUtils.{
}
import org.apache.texera.dao.jooq.generated.tables.User.USER
import org.jooq.impl.DSL
-import org.jooq.{Condition, GroupField, Record, TableLike}
+import org.jooq.{Condition, Field, GroupField, Record, TableLike}
import scala.jdk.CollectionConverters.CollectionHasAsScala
/**
- * The one copy of FROM / WHERE / hydration for every LakeFS-backed resource.
A concrete
- * builder supplies only its [[VersionedResourceTables]] descriptor and its
projection.
+ * The one copy of FROM / WHERE / projection / hydration for every
LakeFS-backed resource. A
+ * concrete builder supplies only its [[VersionedResourceTables]] descriptor
and the three
+ * projected columns the descriptor does not already name.
*/
abstract class VersionedResourceSearchQueryBuilder[Rec <: Record, P](
tables: VersionedResourceTables[Rec, P]
) extends SearchQueryBuilder {
+ /** The resource's LakeFS repository name, which
[[VersionedResourceTables.hydrate]] sizes. */
+ protected val repositoryNameColumn: Field[String]
+
+ protected val isDownloadableColumn: Field[java.lang.Boolean]
+
+ protected val coverImageColumn: Field[String]
+
+ /**
+ * Built here rather than per-subclass, and `final` so a subclass cannot
replace it, because
+ * `hydrate` reads columns the subclass would otherwise have to remember to
project. `userEmail`
+ * is the one that bit: it defaults to `DSL.inline("")`, so omitting it
cost nothing at compile
+ * time and silently made `ownerEmail` null on every row. Everything else
comes off the
+ * descriptor, so the projection and the FROM clause cannot disagree about
which columns they mean.
+ *
+ * `lazy` matters: the abstract members above are subclass `val`s, still
null while this class's
+ * constructor runs.
+ */
+ final override protected lazy val mappedResourceSchema:
UnifiedResourceSchema =
+ UnifiedResourceSchema(
+ resourceType = DSL.inline(tables.resourceType),
+ name = tables.nameColumn,
+ description = tables.descriptionColumn,
+ creationTime = tables.creationTimeColumn,
+ ownerId = tables.ownerUidColumn,
+ userEmail = USER.EMAIL,
+ versionedResourceId = tables.idColumn,
+ repositoryName = repositoryNameColumn,
+ isVersionedResourcePublic = tables.isPublicColumn,
+ isVersionedResourceDownloadable = isDownloadableColumn,
+ versionedResourceUserAccess = tables.access.privilegeColumn,
+ versionedResourceCoverImage = coverImageColumn
+ )
+
/**
* `uid` is null for anonymous callers. Visibility: public only when `uid`
is null;
* explicitly-granted only when `includePublic` is false; both when it is
true.
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 e585848d3c..4594dd9b1a 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
@@ -89,14 +89,16 @@ import scala.jdk.CollectionConverters._
* 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
+ * the six 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
+ * A cancel is louder than it looks, and the sbt log shows it only as
`canceled 6` 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
+ * `toEntryImpl` — the owner-email pair included, so the `new User`
mutant this suite otherwise
+ * kills goes unnoticed too. Measured: with the port held, mutating
`size` to `0L` in
+ * `VersionedResourceTables.hydrate` leaves `succeeded 18, failed 0,
canceled 6` 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).
@@ -120,13 +122,22 @@ import scala.jdk.CollectionConverters._
* 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.
+ * The `record.into(USER).into(classOf[User]).getEmail` in
`VersionedResourceTables.hydrate` used to
+ * be executed but unobservable: the dataset schema left
`UnifiedResourceSchema`'s `userEmail` at its
+ * `DSL.inline("")` default, so the translated record carried no `USER`
column,
+ * `DashboardDataset.ownerEmail` was ALWAYS null for dataset search results,
and the owner
+ * `leftJoin(USER)` was a join that was selected from and never read.
Replacing that read with a bare
+ * `new User` therefore changed nothing. The schema now names `userEmail =
USER.EMAIL` and the value
+ * is asserted below, which kills that mutant; the owner-email tests are the
ones that hold it dead,
+ * so a schema slot silently dropped again fails them.
+ *
+ * The slot now lives in the base rather than here, which is what stops the
next versioned resource
+ * from repeating the bug: `hydrate` is `final` on `VersionedResourceTables`
and reads `USER.EMAIL`
+ * for EVERY versioned resource, so `VersionedResourceSearchQueryBuilder`
builds the whole projection
+ * — `userEmail` included — as a `final lazy val` a subclass cannot replace,
and takes only the three
+ * columns its descriptor does not already name. A new resource type
therefore gets the owner email
+ * whether or not its author thinks about it, and the projection assertion
below guards the base for
+ * every subclass instead of just this one.
*
* Not covered, and not coverable from a test:
* - `constructFromClause`'s `includePublic: Boolean = false` default.
`scalac` emits
@@ -162,6 +173,9 @@ class DatasetSearchQueryBuilderSpec
private val sizedDid: Integer = Integer.valueOf(9001)
private val goneDid: Integer = Integer.valueOf(9002)
+ /** Every dataset `beforeAll` seeds, so row-count assertions do not
hard-code the fixture size. */
+ private val seededDids: Seq[Integer] = Seq(sizedDid, goneDid)
+
private val sizedRepo = "texera-ds-sized"
private val goneRepo = "texera-ds-gone"
@@ -520,14 +534,42 @@ class DatasetSearchQueryBuilderSpec
sql should include("dataset.is_downloadable as
is_versioned_resource_downloadable")
sql should include("dataset_user_access.privilege as
user_versioned_resource_access")
sql should include("dataset.cover_image as versioned_resource_cover_image")
+ // The one projected column that is not a DATASET column, and the one this
schema used to leave
+ // at its `DSL.inline("")` default. It gets an assertion of its own rather
than trusting the
+ // entry-level test because it is now the base class's slot, not this
builder's: dropping it from
+ // `VersionedResourceSearchQueryBuilder` renders `'' as email` for every
versioned resource, and
+ // this names the missing slot instead of surfacing as a null three layers
downstream.
+ sql should include("user.email as email")
+ }
+
+ it should "stay union-compatible with the workflow and project branches" in {
+ // `DashboardResource.searchAllResources` stacks the three builders with
`unionAll` for a
+ // resourceType of "" — the dashboard's default view — so every branch
must project the same
+ // aliases in the same order with types Postgres will unify. A
`varchar`-vs-`''` mix in one slot
+ // is not itself new: `userName` already has exactly this shape (only the
workflow branch
+ // projects a real column; dataset and project both project
`DSL.inline("")`) and that union
+ // runs in production today. What the test buys is that the contract is
invisible from inside a
+ // single builder — nothing fails to compile, and a genuine mismatch would
surface only as a
+ // failed query at runtime. This is the only test that executes the union;
every other test here
+ // renders one branch, or fetches from one.
+ val union = WorkflowSearchQueryBuilder
+ .constructQuery(uid, params(), includePublic = true)
+ .unionAll(ProjectSearchQueryBuilder.constructQuery(uid, params(),
includePublic = true))
+ .unionAll(DatasetSearchQueryBuilder.constructQuery(uid, params(),
includePublic = true))
+
+ // Both seeded datasets are public, so both reach `uid`; no workflow or
project rows are seeded.
+ // Derived from the fixture rather than hard-coded, since the count is
incidental — that Postgres
+ // accepts the union at all is what is under test.
+ getDSLContext.fetch(union).size() shouldBe seededDids.size
}
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.
+ // `VersionedResourceTables.hydrate` reads the owner email out of this
join. The owner-email
+ // tests below now see the
+ // value, so they would fail on a join dropped altogether — but not on a
join RE-AIMED at a
+ // same-typed column, because every seeded dataset shares one owner.
Pinning the predicate is
+ // what separates `USER.UID.eq(DATASET.OWNER_UID)` from
`eq(DATASET_USER_ACCESS.UID)`, which
+ // would report the *caller's* email as the owner's on every shared
dataset.
val sql = sqlFor(uid, includePublic = true)
sql should include("left outer join texera_db.user on texera_db.user.uid =
")
@@ -563,6 +605,29 @@ class DatasetSearchQueryBuilderSpec
dd.size shouldBe 42L
}
+ it should "carry the owner's email address" in {
+ requireStub()
+
+ // Was null for every dataset in the dashboard until the schema named
`userEmail = USER.EMAIL`:
+ // the USER join was selected from and never read. This is also the
assertion that kills the
+ // `record.into(USER).into(classOf[User])` -> `new User` mutant in
+ // `VersionedResourceTables.hydrate`, which survived the whole suite while
the value was null.
+ entryFor(ownerUid, sizedDid).dataset.value.ownerEmail shouldBe
"[email protected]"
+ }
+
+ it should "take the email from the dataset's owner, not from the caller" in {
+ requireStub()
+
+ // `otherUid` reaches this dataset only because it is public, and has its
own address seeded. The
+ // entry must still name the owner — this is what the dashboard labels the
dataset with, and it
+ // is the only assertion here that separates a real owner lookup from one
that echoes the
+ // signed-in caller back.
+ val dd = entryFor(otherUid, sizedDid).dataset.value
+
+ dd.ownerEmail shouldBe "[email protected]"
+ dd.ownerEmail should not be "[email protected]"
+ }
+
it should "set isOwner only for the dataset's own owner" in {
requireStub()
