The GitHub Actions job "Required Checks" on texera.git/main has failed. Run started by GitHub user github-merge-queue[bot] (triggered by github-merge-queue[bot]).
Head commit for run: 0298a3bbe49b644d062a5e3b3dd56dd0e399cc84 / Xinyuan Lin <[email protected]> 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) Report URL: https://github.com/apache/texera/actions/runs/33409301112 With regards, GitHub Actions via GitBox
