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-7116-648d54e4e5fae0d34218e8aeb3bdc3cf561955a1 in repository https://gitbox.apache.org/repos/asf/texera.git
commit c05c0fc783088fa84aa7272820646ac0a4ca869f Author: Xinyuan Lin <[email protected]> AuthorDate: Fri Jul 31 11:44:38 2026 -0700 test(amber): add no-DB specs for the dashboard search query tier (#7116) ### What changes were proposed in this PR? `FulltextSearchQueryUtilsSpec` was the only spec in the dashboard package. It also proves the approach: a **dialect-only** `DSL.using(SQLDialect.POSTGRES)` context renders jOOQ parts with no connection, and `ctx.newRecord(...)` builds records purely in memory. These three specs extend that to the rest of the connection-free tier. | New spec | Target | Why it isn't DB-bound | |---|---|---| | `DashboardResourceSpec` | `getOrderFields`, unknown-`resourceType` guard | both public and pure; the guard throws *before* any query is built | | `WorkflowSearchQueryBuilderSpec` | `toEntryImpl`, `mappedResourceSchema` | this object drops the trait's `protected`, widening access to public | | `UnifiedResourceSchemaSpec` | `apply`, the `translatedFieldSet` de-dup | 24 default-arg `Field`s, all `DSL.inline`/`cast`/`castNull` | **`getOrderFields`** — existing coverage was only incidental (via `WorkflowResourceSpec`) and reached `NameAsc`/`Desc`, `ExecutionTimeAsc`/`Desc`, and `EditTime` through the default. This adds `CreateTime` both ways and the non-matching fall-through. It also pins the asc/desc asymmetry: ``` Asc -> "resourceCreationTime" asc Desc -> "resourceCreationTime" desc nulls last ``` That asymmetry is the point: Postgres defaults to NULLS LAST for ASC and NULLS FIRST for DESC, so the explicit `nullsLast()` on the descending branch is what makes the *observable* ordering symmetric. Dropping it looks like a harmless simplification and silently moves NULLs to the top of every descending page. **`toEntryImpl`** — covers the projects-aggregate lookup, both comma-split branches, the NULL-privilege fallback to `NONE`, and the ownership flag both ways. The first test deliberately pins the aggregate lookup itself: `toEntryImpl` builds a *fresh* `groupConcatDistinct(WORKFLOW_OF_PROJECT.PID)` rather than reusing the one in `mappedResourceSchema`, so the whole projects feature rides on jOOQ comparing two structurally identical aggregates as equal. **`UnifiedResourceSchema`** — the counts were measured, not assumed: 24 aliases, collapsing to 6 entries for the all-defaults schema and 17 for the workflow schema. Assertion strength was checked by **mutating production and confirming red**, then reverting (`git diff -- amber/src/main` is empty): | Mutation | Caught by | |---|---| | `desc().nullsLast()` → `desc()` | 2 tests | | `EditTime` → `resourceCreationTimeField` | 2 tests | | `into(WORKFLOW_OF_USER).getUid == uid` → `into(USER).getUid != uid` | 2 tests | | privilege fallback `NONE` → `READ` | 1 test | | de-dup guard → `if (true)` | 3 tests | Out of scope, and noted in-file: `translateRecord`, `resultsOwnersInfo`, `searchAllPublicResourceCall`, `constructQuery` and every fetch path go through `SqlServer.getInstance().createDSLContext()`. `DatasetSearchQueryBuilder` is untouched — all five of its members are `override protected`, and Scala `protected` grants no same-package access. Two things deliberately **not** asserted, with the reasoning left in the files: `case None => List()` in `getOrderFields` and `case _ => null` in the private `getColumnField` are unreachable (the regex yields only four column names, all mapping non-null); and the projects parser's `NumberFormatException` on a padded separator is real but unproducible in production, so pinning it would only punish someone hardening the parser. None of these specs reads or writes `FulltextSearchQueryUtils.usePgroonga`, so they neither depend on it nor mutate it — worth stating because two existing specs set that JVM-global and never restore it while amber's suites run concurrently. Fragilities found in passing, reported rather than changed: `DashboardWorkflow.ownerId` is read as `record.into(USER).getUid` although the projection never selects `USER.UID` — it works only because jOOQ matches the unqualified name `uid` against `workflow_of_user.uid`; and `searchAllResources`'s per-record `resourceType` match has no default arm, so changing the inline `'workflow'` literal becomes a runtime `MatchError`. ### Any related issues, documentation, discussions? Closes #7115 ### How was this PR tested? Three new specs, 27 tests. Run together with the existing sibling spec to confirm nothing in the package regressed — 41 tests, Java 17: ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.dashboard.DashboardResourceSpec org.apache.texera.web.resource.dashboard.WorkflowSearchQueryBuilderSpec org.apache.texera.web.resource.dashboard.UnifiedResourceSchemaSpec org.apache.texera.web.resource.dashboard.FulltextSearchQueryUtilsSpec" ``` ``` [info] Suites: completed 4, aborted 0 [info] Tests: succeeded 41, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. ``` Verified with no Postgres provisioned locally, which is the point. `Test/scalafmtCheck` and `Test/scalafix --check` both `[success]`. One brittleness to be aware of: the expected strings (`desc nulls last`, `cast(null as timestamp)`, `'workflow' as "resourceType"`) and the de-dup counts are jOOQ **3.19.36** rendering artifacts, so a jOOQ bump could require updating them together. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --- .../resource/dashboard/DashboardResourceSpec.scala | 147 ++++++++++++ .../dashboard/UnifiedResourceSchemaSpec.scala | 252 +++++++++++++++++++++ .../dashboard/WorkflowSearchQueryBuilderSpec.scala | 228 +++++++++++++++++++ 3 files changed, 627 insertions(+) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala new file mode 100644 index 0000000000..279e04ed58 --- /dev/null +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DashboardResourceSpec.scala @@ -0,0 +1,147 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.dashboard + +import org.apache.texera.auth.SessionUser +import org.apache.texera.dao.jooq.generated.tables.pojos.User +import org.apache.texera.web.resource.dashboard.DashboardResource.SearchQueryParams +import org.jooq.impl.{DSL => JDSL} +import org.jooq.{OrderField, SQLDialect} + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Covers the connection-free surface of [[DashboardResource]]: the `orderBy` + * translation and the resource-type dispatch guard. + * + * Breakage this catches: + * - a column of the `orderBy` grammar wired to the wrong unified-schema + * alias (e.g. `EditTime` sorting by creation time), which silently + * re-orders the dashboard instead of failing; + * - the `nullsLast()` on the descending branch being dropped or copied onto + * the ascending branch — under Postgres that flips where rows with a NULL + * execution time land; + * - the regex losing its implicit anchoring (`Regex.unapplySeq` requires a + * full match), which would start accepting junk like `SortByNameAsc`; + * - an unrecognised `orderBy` no longer degrading to "no ORDER BY", or the + * unknown-resourceType guard no longer throwing before the query is built. + * + * Note on the shared global `FulltextSearchQueryUtils.usePgroonga`: nothing + * exercised here reads or writes it, so this spec neither depends on its value + * nor mutates it (a "restore" would itself be a write, and amber's suites + * share one JVM). + */ +class DashboardResourceSpec extends AnyFlatSpec with Matchers { + + // jOOQ render context (Postgres dialect to match production renderers). + // Dialect-only DSLContext — it never opens a connection. + private val ctx = JDSL.using(SQLDialect.POSTGRES) + + private def sqlOf(o: OrderField[_]): String = ctx.renderInlined(o) + + private def orderSql(orderBy: String): List[String] = + DashboardResource.getOrderFields(SearchQueryParams(orderBy = orderBy)).map(sqlOf) + + // -- getOrderFields: the recognised grammar --------------------------------- + + "getOrderFields" should "map each recognised column onto its unified-schema alias, ascending" in { + // The rendered identifier is the alias of the UNION-ALL projection, not a + // physical column: sorting happens after the three sub-queries are merged. + orderSql("NameAsc") shouldBe List("\"resourceName\" asc") + orderSql("CreateTimeAsc") shouldBe List("\"resourceCreationTime\" asc") + orderSql("EditTimeAsc") shouldBe List("\"resourceLastModifiedTime\" asc") + orderSql("ExecutionTimeAsc") shouldBe List("\"resourceExecutionTime\" asc") + } + + it should "map each recognised column onto its unified-schema alias, descending" in { + orderSql("NameDesc") shouldBe List("\"resourceName\" desc nulls last") + orderSql("CreateTimeDesc") shouldBe List("\"resourceCreationTime\" desc nulls last") + orderSql("EditTimeDesc") shouldBe List("\"resourceLastModifiedTime\" desc nulls last") + orderSql("ExecutionTimeDesc") shouldBe List("\"resourceExecutionTime\" desc nulls last") + } + + it should "spell out nulls-last only on the descending branch" in { + // `Desc` goes through `value.desc().nullsLast()` while `Asc` is a bare + // `value.asc()`. The asymmetry in the code is what makes the *semantics* + // symmetric under Postgres, whose defaults are NULLS LAST for ASC and + // NULLS FIRST for DESC: workflows that never ran (NULL executionTime) + // therefore sort to the end in both directions. Removing `nullsLast()` + // would quietly float them to the top of the descending listing. + val asc = orderSql("ExecutionTimeAsc").head + val desc = orderSql("ExecutionTimeDesc").head + asc should not include "nulls" + desc should include("nulls last") + } + + it should "emit exactly one ORDER BY term" in { + // A second term would change pagination stability; the contract is one + // field per request. + DashboardResource.getOrderFields( + SearchQueryParams(orderBy = "CreateTimeDesc") + ) should have size 1 + } + + // -- getOrderFields: everything the grammar rejects ------------------------- + + it should "fall back to no ordering at all when orderBy does not match the pattern" in { + // Consequence of the empty list: the query runs unordered while + // offset/limit still apply, so pagination silently becomes + // non-deterministic. Pin the inputs that take this branch. + orderSql("") shouldBe empty // the frontend can send an empty query param + orderSql("Bogus") shouldBe empty + orderSql("NameSideways") shouldBe empty // known column, junk direction + orderSql("Name") shouldBe empty // direction missing entirely + orderSql("nameAsc") shouldBe empty // the regex is case-sensitive + orderSql("SortByNameAsc") shouldBe empty // Regex.unapplySeq anchors the + orderSql("NameAscending") shouldBe empty // whole string, both ends + } + + // Two branches in this area are unreachable and are therefore deliberately + // NOT tested: `case None => List()` inside getOrderFields and + // `case _ => null` inside the private getColumnField. The regex can only + // yield the four column names Name/CreateTime/EditTime/ExecutionTime, and + // every one of them maps to a non-null Field, so getColumnField never + // returns None. + + // -- searchAllResources: the dispatch guard -------------------------------- + + "searchAllResources" should "reject an unknown resourceType before it builds any query" in { + // The `case _ => throw` arm sits in the dispatch match, ahead of + // constructQuery / SqlServer, so this is reachable with no database. The + // exception *type* is the real assertion: if the guard were moved below the + // query construction, this would surface as a SqlServer failure instead. + val thrown = the[IllegalArgumentException] thrownBy DashboardResource.searchAllResources( + new SessionUser(new User()), // uid stays null; unused on this path + SearchQueryParams(resourceType = "notAResourceType") + ) + thrown.getMessage should include("notAResourceType") + } + + it should "reject a resourceType that only differs from a valid one by case" in { + // Guards against someone relaxing the match to be case-insensitive on one + // side only: "Workflow" is not "workflow" today. + val thrown = the[IllegalArgumentException] thrownBy DashboardResource.searchAllResources( + new SessionUser(new User()), + SearchQueryParams(resourceType = "Workflow") + ) + thrown.getMessage should include("Workflow") + } +} diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/UnifiedResourceSchemaSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/UnifiedResourceSchemaSpec.scala new file mode 100644 index 0000000000..9d5f1e0b0b --- /dev/null +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/UnifiedResourceSchemaSpec.scala @@ -0,0 +1,252 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.dashboard + +import org.apache.texera.dao.jooq.generated.Tables._ +import org.apache.texera.dao.jooq.generated.enums.PrivilegeEnum +import org.jooq.impl.{DSL => JDSL} +import org.jooq.{Field, SQLDialect} + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.sql.Timestamp + +/** + * Covers `UnifiedResourceSchema.apply` — the projection every search builder + * unions into — and the de-duplication it performs behind `allFields`. + * + * Breakage this catches: + * - a projection slot wired to the wrong alias. Note the builders cannot drift + * from each other: the class constructor is private and every builder goes + * through this one `apply`, which pairs each expression with its own alias in + * the same tuple. So the slot-order assertion here is a lock on a centrally + * fixed projection shape (which `searchAllResources` unions positionally), + * not protection against one builder reordering columns — that is impossible; + * - the de-dup in `translatedFieldSet` changing from "keep the first alias + * per distinct original Field" to keep-last (or disappearing), which + * changes which alias `translateRecord` reads values out of; + * - jOOQ no longer comparing Fields structurally, which is the sole reason + * the de-dup collapses anything at all. + * + * `translateRecord` itself is out of scope: it calls `context.newRecord`, and + * `context` is `SqlServer.getInstance().createDSLContext()`, so exercising it + * would need a live database. Nothing here reads or writes the shared global + * `FulltextSearchQueryUtils.usePgroonga`. + */ +class UnifiedResourceSchemaSpec extends AnyFlatSpec with Matchers { + + // jOOQ render context (Postgres dialect to match production renderers). + private val ctx = JDSL.using(SQLDialect.POSTGRES) + + // `translatedFieldSet` is a `private val` whose only public consumer + // (translateRecord) needs a database, so read it reflectively rather than + // standing one up. Keeping this observable is the point: the de-dup is what + // decides which aliases ever reach a translated record. + private val translatedFieldSetAccessor = { + val field = classOf[UnifiedResourceSchema].getDeclaredField("translatedFieldSet") + field.setAccessible(true) + field + } + + private def translatedPairs(schema: UnifiedResourceSchema): Seq[(Field[_], Field[_])] = + translatedFieldSetAccessor + .get(schema) + .asInstanceOf[Seq[(Field[_], Field[_])]] + + private def translatedAliases(schema: UnifiedResourceSchema): Seq[String] = + translatedPairs(schema).map(_._2.getName) + + private def translatedOriginals(schema: UnifiedResourceSchema): Seq[Field[_]] = + translatedPairs(schema).map(_._1) + + // Sentinels for the three slots that have no convenient distinct table + // column of the right type; every other slot uses a real generated column so + // that all 24 originals render differently from one another. + private val sentinelResourceType: Field[String] = JDSL.inline("s-resource-type") + private val sentinelProjects: Field[String] = JDSL.inline("s-projects") + private val sentinelStoragePath: Field[String] = JDSL.inline("s-storage-path") + + private val sentinelSchema: UnifiedResourceSchema = UnifiedResourceSchema( + resourceType = sentinelResourceType, + name = WORKFLOW.NAME, + description = DATASET.DESCRIPTION, + creationTime = WORKFLOW.CREATION_TIME, + lastModifiedTime = WORKFLOW.LAST_MODIFIED_TIME, + executionTime = WORKFLOW_EXECUTIONS.STARTING_TIME, + ownerId = WORKFLOW_OF_USER.UID, + wid = WORKFLOW.WID, + workflowUserAccess = WORKFLOW_USER_ACCESS.PRIVILEGE, + projectsOfWorkflow = sentinelProjects, + uid = USER.UID, + userName = USER.NAME, + userEmail = USER.EMAIL, + pid = PROJECT.PID, + projectOwnerId = PROJECT.OWNER_ID, + projectColor = PROJECT.COLOR, + did = DATASET.DID, + datasetStoragePath = sentinelStoragePath, + repositoryName = DATASET.REPOSITORY_NAME, + isDatasetPublic = DATASET.IS_PUBLIC, + isDatasetDownloadable = DATASET.IS_DOWNLOADABLE, + datasetUserAccess = DATASET_USER_ACCESS.PRIVILEGE, + datasetCoverImage = DATASET.COVER_IMAGE, + workflowCoverImage = WORKFLOW_COVER_IMAGE.IMAGE + ) + + // Expected projection, in order: alias -> the original it must be built from. + private val expectedProjection: Seq[(String, Field[_])] = Seq( + "resourceType" -> sentinelResourceType, + "resourceName" -> WORKFLOW.NAME, + "resourceDescription" -> DATASET.DESCRIPTION, + "resourceCreationTime" -> WORKFLOW.CREATION_TIME, + "resourceLastModifiedTime" -> WORKFLOW.LAST_MODIFIED_TIME, + "resourceExecutionTime" -> WORKFLOW_EXECUTIONS.STARTING_TIME, + "resourceOwnerId" -> WORKFLOW_OF_USER.UID, + "wid" -> WORKFLOW.WID, + "workflow_privilege" -> WORKFLOW_USER_ACCESS.PRIVILEGE, + "projects" -> sentinelProjects, + "uid" -> USER.UID, + "userName" -> USER.NAME, + "email" -> USER.EMAIL, + "pid" -> PROJECT.PID, + "owner_uid" -> PROJECT.OWNER_ID, + "color" -> PROJECT.COLOR, + "did" -> DATASET.DID, + "dataset_storage_path" -> sentinelStoragePath, + "repository_name" -> DATASET.REPOSITORY_NAME, + "is_dataset_public" -> DATASET.IS_PUBLIC, + "is_dataset_downloadable" -> DATASET.IS_DOWNLOADABLE, + "user_dataset_access" -> DATASET_USER_ACCESS.PRIVILEGE, + "cover_image" -> DATASET.COVER_IMAGE, + "workflow_cover_image" -> WORKFLOW_COVER_IMAGE.IMAGE + ) + + // -- apply(): the projection ------------------------------------------------ + + "apply" should "expose all 24 slots as aliases, in the order the UNION ALL depends on" in { + sentinelSchema.allFields should have size 24 + sentinelSchema.allFields.map(_.getName) shouldBe expectedProjection.map(_._1) + } + + it should "alias each original to its own slot" in { + // Rendered as a SELECT because that is how constructQuery consumes + // allFields; a swapped pair (`name -> description.as(resourceNameAlias)`) + // shows up here even though the alias order stays intact. + val rendered = ctx.renderInlined(JDSL.select(sentinelSchema.allFields: _*)) + expectedProjection.foreach { + case (alias, original) => + rendered should include(s"""${ctx.renderInlined(original)} as "$alias"""") + } + } + + it should "default every slot to an inline literal or a typed NULL cast" in { + // The all-defaults projection is what lets a builder that knows nothing + // about datasets still union with one that does: the column count and + // types have to line up. + val defaults = UnifiedResourceSchema() + defaults.allFields should have size 24 + val rendered = ctx.renderInlined(JDSL.select(defaults.allFields: _*)) + rendered should include("'' as \"resourceType\"") + rendered should include("cast(null as timestamp) as \"resourceCreationTime\"") + rendered should include("cast(null as int) as \"resourceOwnerId\"") + rendered should include("cast(null as boolean) as \"is_dataset_public\"") + } + + // -- the de-dup behind translatedFieldSet ----------------------------------- + + "translatedFieldSet" should "keep the first alias when the same Field feeds two slots" in { + // WorkflowSearchQueryBuilder really does this: `ownerId` and `uid` are both + // WORKFLOW_OF_USER.UID. allFields keeps both aliases (the SELECT projects the + // column twice) while the translation map keeps only the earlier one. + // + // What that does and does not mean: `translateRecord` builds its output with + // `context.newRecord(translatedFieldSet.map(_._1): _*)`, i.e. keyed by the + // ORIGINAL fields, so no `uid`-named slot exists on a translated record either + // way. And since both slots project the *same* expression, the dropped alias + // would have carried the same value — no data is lost. The only consequence is + // that the source record's `uid` alias column is never read back. So this test + // pins the keep-first rule itself, not a data loss. + val shared: Field[Integer] = JDSL.field(JDSL.name("shared_uid"), classOf[Integer]) + val schema = UnifiedResourceSchema(ownerId = shared, uid = shared) + + val aliases = translatedAliases(schema) + aliases should contain("resourceOwnerId") // first occurrence wins + aliases should not contain "uid" // second occurrence is dropped + // The de-dup rule is about the ORIGINALS: one entry per distinct original Field. + val originals = translatedOriginals(schema) + originals.distinct.size shouldBe originals.size + originals.count(_ == shared) shouldBe 1 + } + + it should "collapse the all-defaults projection down to one alias per distinct default" in { + // 24 slots, but only six structurally distinct default expressions, so the + // de-dup collapses the map to six entries. Worth pinning because it is + // surprising, and because it is what makes the keep-first rule observable at + // all: allFields stays at 24 while the translation map does not. + val defaults = UnifiedResourceSchema() + defaults.allFields should have size 24 + translatedAliases(defaults) shouldBe Seq( + "resourceType", // DSL.inline("") + "resourceCreationTime", // cast(null as timestamp) + "resourceOwnerId", // cast(null as int) + "workflow_privilege", // cast(null as privilege_enum) + "dataset_storage_path", // cast(null as varchar) + "is_dataset_public" // cast(null as boolean) + ) + } + + it should "keep every distinct original when the caller supplies 24 distinct Fields" in { + // Nothing to collapse here, which is the control case for the two tests + // above: the shrinkage they observe comes from duplicate originals only. + translatedAliases(sentinelSchema) shouldBe expectedProjection.map(_._1) + } + + it should "drop exactly the duplicated slots of the production workflow projection" in { + val workflowSchema = WorkflowSearchQueryBuilder.mappedResourceSchema + workflowSchema.allFields should have size 24 + val aliases = translatedAliases(workflowSchema) + // `uid` duplicates ownerId (WORKFLOW_OF_USER.UID); the rest are slots the + // builder left at their default, and the defaults collide by type. + workflowSchema.allFields.map(_.getName).diff(aliases) shouldBe Seq( + "uid", + "owner_uid", + "color", + "did", + "repository_name", + "is_dataset_downloadable", + "cover_image" + ) + aliases should contain("resourceOwnerId") + } + + // -- the jOOQ assumption the de-dup rests on ------------------------------- + + "jOOQ Field equality" should "be structural, which is what makes the de-dup collapse anything" in { + // If jOOQ ever switched to identity equality, translatedFieldSet would keep + // all 24 slots and translateRecord would start reading duplicated columns — + // the tests above would flip, and this one says why. + JDSL.cast(null, classOf[Integer]) shouldBe JDSL.cast(null, classOf[Integer]) + JDSL.inline("") shouldBe JDSL.inline("") + // ...but only within a type: the six default expressions stay distinct. + JDSL.cast(null, classOf[Integer]) should not be JDSL.cast(null, classOf[Timestamp]) + JDSL.cast(null, classOf[String]) should not be JDSL.castNull(classOf[PrivilegeEnum]) + } +} diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/WorkflowSearchQueryBuilderSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/WorkflowSearchQueryBuilderSpec.scala new file mode 100644 index 0000000000..59dd8d9e19 --- /dev/null +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/WorkflowSearchQueryBuilderSpec.scala @@ -0,0 +1,228 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.web.resource.dashboard + +import org.apache.texera.dao.jooq.generated.Tables._ +import org.apache.texera.dao.jooq.generated.enums.PrivilegeEnum +import org.apache.texera.web.resource.dashboard.user.workflow.WorkflowResource.DashboardWorkflow +import org.jooq.impl.{DSL => JDSL} +import org.jooq.{Record, SQLDialect} + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** + * Covers `WorkflowSearchQueryBuilder.toEntryImpl`, the pure record-to-DTO + * mapping, plus the one literal in `mappedResourceSchema` that the dashboard + * dispatch depends on. Both members widen the trait's `protected` to public, + * which is what makes them reachable from a spec at all. + * + * Breakage this catches: + * - the `projects` aggregate lookup breaking (a Field looked up by structural + * equality): `record.get(pidField)` would start returning null and every + * workflow would silently report zero projects instead of failing; + * - the comma-split branch losing/reordering ids, or the NULL branch no + * longer yielding an empty list; + * - a NULL `workflow_user_access.privilege` (a left join miss for public + * workflows) no longer degrading to NONE — an NPE or a wrong grant; + * - the ownership flag comparing the wrong uid, or owner id/name being read off + * the wrong column: two record columns are called `name` (WORKFLOW.NAME and + * USER.NAME), and `ownerId` resolves only because the unqualified name `uid` + * matches `workflow_of_user.uid` — the projection never selects USER.UID; + * - the inline `'workflow'` tag changing, which turns every "all resources" + * search into a MatchError in DashboardResource.searchAllResources. + * + * `constructFromClause`, `constructWhereClause` and `getGroupByFields` stay + * `override protected`, and Scala `protected` grants no same-package access, so + * they are not callable from here. (They are otherwise pure jOOQ DSL builders — + * the access modifier is the only thing keeping them out of scope.) + * + * Nothing here reads or writes the shared global + * `FulltextSearchQueryUtils.usePgroonga`, so this spec neither depends on it + * nor mutates it. + */ +class WorkflowSearchQueryBuilderSpec extends AnyFlatSpec with Matchers { + + // jOOQ render context (Postgres dialect to match production renderers). + private val ctx = JDSL.using(SQLDialect.POSTGRES) + + // toEntryImpl re-creates this aggregate locally and then looks it up with + // `record.get(pidField)`; the lookup only resolves because jOOQ compares + // QueryParts structurally. The first test pins that assumption. + private val pidField = JDSL.groupConcatDistinct(WORKFLOW_OF_PROJECT.PID) + private val coverField = JDSL.max(WORKFLOW_COVER_IMAGE.IMAGE).as("workflow_cover_image") + + private val ownerUid: Integer = Integer.valueOf(42) + private val viewerUid: Integer = Integer.valueOf(43) + private val wid: Integer = Integer.valueOf(7) + + /** + * Builds the subset of the *translated* record (see + * `UnifiedResourceSchema.translateRecord`, whose output is keyed by the + * original fields) that toEntryImpl actually reads. `newRecord` is pure + * in-memory jOOQ — no connection is involved. + * + * Every value is distinct so that reading the wrong column cannot pass. + */ + private def translatedRecord( + uidValue: Integer = ownerUid, + privilege: PrivilegeEnum = PrivilegeEnum.WRITE, + projects: String = "3,1,2", + cover: String = "cover-b64" + ): Record = { + val record = ctx.newRecord( + WORKFLOW.WID, + WORKFLOW.NAME, + WORKFLOW.DESCRIPTION, + WORKFLOW_OF_USER.UID, + WORKFLOW_USER_ACCESS.PRIVILEGE, + USER.NAME, + pidField, + coverField + ) + record.set(WORKFLOW.WID, wid) + record.set(WORKFLOW.NAME, "wf-name") + record.set(WORKFLOW.DESCRIPTION, "wf-description") + record.set(WORKFLOW_OF_USER.UID, uidValue) + record.set(WORKFLOW_USER_ACCESS.PRIVILEGE, privilege) + record.set(USER.NAME, "owner-name") + record.set(pidField, projects) + record.set(coverField, cover) + record + } + + private def workflowOf(record: Record, uid: Integer): DashboardWorkflow = + WorkflowSearchQueryBuilder.toEntryImpl(uid, record).workflow.get + + // -- the aggregate-field lookup -------------------------------------------- + + "toEntryImpl" should "resolve the project aggregate through structural Field equality" in { + // toEntryImpl builds a *fresh* groupConcatDistinct instance rather than + // reusing the one in mappedResourceSchema, so the whole projects feature + // rides on jOOQ treating two structurally identical aggregates as equal. + val record = translatedRecord(projects = "5,6") + JDSL.groupConcatDistinct(WORKFLOW_OF_PROJECT.PID) shouldBe pidField + record.get(JDSL.groupConcatDistinct(WORKFLOW_OF_PROJECT.PID)) shouldBe "5,6" + } + + // -- projectsOfWorkflow: both branches ------------------------------------- + + it should "split the comma-joined aggregate into Integers, preserving the aggregate's order" in { + // Deliberately unsorted so an accidental `.sorted` / `.reverse` fails. + workflowOf(translatedRecord(projects = "3,1,2"), ownerUid).projectIDs shouldBe + List(Integer.valueOf(3), Integer.valueOf(1), Integer.valueOf(2)) + } + + it should "handle a single-project aggregate (no separator present)" in { + workflowOf(translatedRecord(projects = "8"), ownerUid).projectIDs shouldBe + List(Integer.valueOf(8)) + } + + it should "return an empty project list when the aggregate is NULL" in { + // A workflow that belongs to no project left-joins to a NULL aggregate. + workflowOf(translatedRecord(projects = null), ownerUid).projectIDs shouldBe empty + } + + // Deliberately NOT asserted: that a padded separator ("1, 2") or a leading comma + // (",1") throws NumberFormatException. It does today — `Integer.valueOf` is applied + // straight to the raw `split(',')` output, so such input aborts the whole search + // request with a 500 instead of degrading. But production cannot produce it + // (Postgres' string_agg is rendered with a bare ',' separator), and pinning the + // throw would turn this suite red the moment someone hardens the parser with + // `.map(_.trim).filter(_.nonEmpty)` — i.e. it would punish an improvement. + + // -- privilege fallback ----------------------------------------------------- + + it should "fall back to NONE when the workflow privilege is NULL" in { + // Public workflows the caller has no explicit grant on left-join to a NULL + // privilege; the DTO must still carry a usable access level. + workflowOf(translatedRecord(privilege = null), ownerUid).accessLevel shouldBe + PrivilegeEnum.NONE.toString + } + + it should "pass a non-NULL privilege through unchanged" in { + // READ (not the WRITE default) so the fallback cannot be mistaken for a + // pass-through of the fixture value. + workflowOf( + translatedRecord(privilege = PrivilegeEnum.READ), + ownerUid + ).accessLevel shouldBe "READ" + } + + // -- ownership -------------------------------------------------------------- + + it should "flag ownership by comparing workflow_of_user.uid against the caller" in { + workflowOf(translatedRecord(), ownerUid).isOwner shouldBe true + workflowOf(translatedRecord(), viewerUid).isOwner shouldBe false + } + + it should "report the owner's id and name even when the caller is not the owner" in { + val dw = workflowOf(translatedRecord(), viewerUid) + dw.isOwner shouldBe false + // `ownerId` is read as `record.into(USER).getUid`, but the unified schema + // never selects USER.UID — jOOQ resolves the unqualified column name "uid" + // against workflow_of_user.uid instead. So this must be the owner's 42, + // neither the caller's 43 nor the workflow's wid of 7. + dw.ownerId shouldBe ownerUid + dw.ownerName shouldBe "owner-name" + } + + // -- workflow payload + cover image ---------------------------------------- + + it should "copy the workflow columns into the POJO without confusing them with the owner's" in { + // The record carries two columns named "name" (workflow.name and + // user.name); qualified resolution has to keep them apart. + val dw = workflowOf(translatedRecord(), ownerUid) + dw.workflow.getWid shouldBe wid + dw.workflow.getName shouldBe "wf-name" + dw.workflow.getDescription shouldBe "wf-description" + dw.ownerName shouldBe "owner-name" + } + + it should "wrap the cover image in an Option" in { + workflowOf(translatedRecord(), ownerUid).coverImage shouldBe Some("cover-b64") + workflowOf(translatedRecord(cover = null), ownerUid).coverImage shouldBe None + } + + it should "tag the entry as a workflow and leave the other payload slots empty" in { + val entry = WorkflowSearchQueryBuilder.toEntryImpl(ownerUid, translatedRecord()) + entry.resourceType shouldBe "workflow" + entry.project shouldBe None + entry.dataset shouldBe None + entry.workflow should not be None + } + + // -- the projection literal the dispatch depends on ------------------------ + + "mappedResourceSchema" should "project the literal 'workflow' as the resourceType column" in { + // searchAllResources dispatches on the *value* of this column with no + // default branch, so changing the literal turns an "all resources" search + // into a MatchError at fetch time rather than a compile error. + // Rendered as a SELECT because an aliased Field on its own renders as the + // bare alias reference, not as its underlying expression. + val rendered = ctx.renderInlined( + JDSL.select(WorkflowSearchQueryBuilder.mappedResourceSchema.allFields: _*) + ) + rendered should include("'workflow' as \"resourceType\"") + // The projects column is the aggregate toEntryImpl re-creates locally; a + // separator other than a bare ',' would break the split above. + rendered should include(s"""${ctx.renderInlined(pidField)} as "projects"""") + } +}
