This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7875-50321e403c82df299a13deb50a7f9849dd93bdba in repository https://gitbox.apache.org/repos/asf/texera.git
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()
