Copilot commented on code in PR #7855:
URL: https://github.com/apache/texera/pull/7855#discussion_r3837436362
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala:
##########
@@ -45,27 +58,245 @@ import scala.jdk.CollectionConverters._
* - `includePublic == false`, which must return only explicitly-granted
datasets and must not
* leak public ones in.
*
- * `initializeDBAndReplaceDSLContext` is needed only because
`SearchQueryBuilder.context` reads
- * `SqlServer.getInstance()`; no query is run against the database.
+ * == Part 2: what a row becomes ==
+ *
+ * `toEntryImpl` is `override protected` here (the workflow and project
siblings widen theirs to
+ * `override def`), so it is not directly callable. The route in is the
trait's public `toEntry`,
+ * which first runs `UnifiedResourceSchema.translateRecord` — so the record
handed to it must carry
+ * the *aliased* select fields, not raw `DATASET.*` fields. Rather than
hand-build that record,
+ * these tests fetch the builder's own query and pass a real row, which is
exactly what
+ * `DashboardResource.searchAllResources` does.
+ *
+ * A record permutation is invisible from this end. `translateRecord` writes
each aliased value back
+ * to the very column the schema paired it with
(`UnifiedResourceSchema:174`), so ANY swap of two
+ * same-typed schema assignments — `name`/`description`,
`is_public`/`is_downloadable`,
+ * `did`/`ownerId` — cancels out exactly and produces a byte-identical entry.
What the entry-level
+ * column assertions below really pin is that each slot still references
*some* `DATASET` column
+ * (wire a slot to a literal and `translateRecord` drops the field, so the
value reads back null).
+ * The projections are pinned where a permutation is actually visible: the
SQL-shape test
+ * "project every dataset column under the alias its schema slot names".
+ *
+ * `toEntryImpl` also calls `LakeFSStorageClient.retrieveRepositorySize`,
which is a live HTTP call
+ * with no injectable seam. Two non-obvious things make it testable anyway:
+ *
+ * - **The stub binds AT the configured endpoint**
(`StorageConfig.lakefsEndpoint`, i.e.
+ * `localhost:8000`) instead of repointing that endpoint at an ephemeral
port. This is
+ * deliberate. `LakeFSStorageClient.apiClient` is a `lazy val` that
captures the endpoint once
+ * per JVM, and `amber/build.sbt` runs every suite in ONE unforked JVM
+ * (`Tags.limit(Tags.Test, 1)` serializes them but does not isolate
them). Mutating the global
+ * `StorageConfig.lakefsEndpoint` here would therefore be order-dependent
— any earlier suite
+ * that forced `apiClient` wins — and would leave the client bound to a
dead port for every
+ * later suite. Binding at the address the config already names is
order-independent and
+ * memoizes nothing wrongly. The price is that the port is
machine-global: if something else
+ * holds it (a local `bin/local-dev.sh up` lakeFS, or a sibling worktree
running these tests),
+ * the four tests that need a *successful* size `cancel` rather than
fail. CI has no lakeFS in
+ * this job, so it is deterministic there.
+ *
+ * A cancel is louder than it looks, and the sbt log shows it only as
`canceled 4` while the
+ * build stays green. It disarms the whole `toEntry` half of this suite:
not just its coverage
+ * (which falls back toward the ~59% this file had before), but every
assertion protecting
+ * `toEntryImpl`. Measured: with the port held, mutating `size` to `0L`
in the entry leaves
+ * `succeeded 14, failed 0, canceled 4` and sbt printing "All tests
passed" at exit 0. The
+ * assertions in the SQL-shape tests are unaffected, because they render
rather than fetch and
+ * never call lakeFS — that is the half that stays armed everywhere
(verified: with the port
+ * held, breaking a projection or a where-clause connective still fails
the build).
+ * - **The `null`-return test needs no stub at all.** Connection-refused,
404 and 501 all surface
+ * as `io.lakefs.clients.sdk.ApiException` (`ApiClient` wraps
`IOException` in one, and non-2xx
+ * throws one directly), so that test behaves identically whether the
stub bound or not.
+ *
+ * The stub is ~50 lines duplicated in spirit from
+ * `common/workflow-core/.../LakeFSStorageClientSpec`. It cannot be shared:
`build.sbt` gives
+ * `WorkflowExecutionService` a test->test dependency on `DAO` and `Auth`
only, so workflow-core's
+ * test tree is not on this module's test classpath.
+ *
+ * The keyword tests RENDER a full-text predicate (they do not fetch one),
which does read the
+ * JVM-global `FulltextSearchQueryUtils.usePgroonga` and emits whichever arm
it currently selects:
+ * `pgroonga_condition(...)` when this suite runs alone, the
`to_tsvector`/`to_tsquery` arm if
+ * `DatasetResourceSpec` or `WorkflowResourceSpec` ran earlier in this JVM
and left the global
+ * `false` (both set it and neither restores it; amber has no `Test / fork`).
This suite therefore
+ * neither touches nor restores that global, and every keyword assertion here
is deliberately
+ * branch-independent: the tokens themselves and the `coalesce(...) || ' ' ||
coalesce(...)`
+ * expression are built at `FulltextSearchQueryUtils:49-51`, *before* the `if
(usePgroonga)`.
+ * Anything added here must keep that property — an assertion on
`pgroonga_condition` would pass
+ * solo and fail in a full-module run.
+ *
+ * Two lines are executed but unobservable, on purpose. `val owner =
record.into(USER)...` and
+ * `owner.getEmail` run on every entry, yet replacing them with a bare `new
User` changes nothing:
+ * the dataset schema leaves `UnifiedResourceSchema`'s `userEmail` at its
`DSL.inline("")` default,
+ * so the translated record carries no `USER` column and
`DashboardDataset.ownerEmail` is ALWAYS
+ * null for dataset search results — which also makes the `leftJoin(USER)` a
join that is selected
+ * from and never read. That is a production defect, reported separately;
asserting the null here
+ * would cement it, so these tests pin the join's shape and leave the value
alone.
*
- * Deliberately not covered: `toEntryImpl` (lines 128-164) is ~80% of this
file's uncovered lines
- * and sits behind a live LakeFS `retrieveRepositorySize` call with no
mockable seam. Reaching it
- * would need a source change (injecting a repository-size provider), not a
test.
+ * Not covered, and not coverable from a test:
+ * - `constructFromClause`'s `includePublic: Boolean = false` default.
`scalac` emits
+ * `constructFromClause$default$3`, but `constructQuery` always passes
all three arguments, so
+ * it has zero call sites and the method is `protected`.
+ * - the `LazyLogging` `logger` lazy-val bitmap on the `object` line, and
scala-logging's
+ * `if (underlying.isErrorEnabled)` guard around `logger.error`. Only the
enabled arm ever runs.
+ * - three of the six branch arms of `dataset.getOwnerUid == uid` (JaCoCo
reports cb=3, mb=3).
+ * They are the null-safe boxed-`Integer` equality's null checks, and
they describe a null
+ * `owner_uid` — a state `INT NOT NULL` plus an FK to `"user"(uid)`
forbids.
+ * - `class DatasetSearchQueryBuilder {}` at the bottom of the file: a
vestigial empty class with
+ * zero references repo-wide. It should be deleted, not instantiated by a
test.
+ *
+ * Covered but unconstrainable, so no reviewer should count it as pinned
behaviour:
+ * - the `.filter(_.nonEmpty)` after the keyword split.
`getFullTextSearchFilter` re-applies
+ * `keywords.filter(_.nonEmpty)` itself (`FulltextSearchQueryUtils:43`),
so dropping it here is
+ * an equivalent mutation — no observable differs.
*/
class DatasetSearchQueryBuilderSpec
extends AnyFlatSpec
with Matchers
+ with OptionValues
with BeforeAndAfterAll
with MockTexeraDB {
private val uid: Integer = Integer.valueOf(42)
+ // Owner and a second signed-in caller. Both are outside java.lang.Integer's
cache (-128..127) so
+ // that `dataset.getOwnerUid == uid` cannot pass by reference identity.
+ private val ownerUid: Integer = Integer.valueOf(9101)
+ private val otherUid: Integer = Integer.valueOf(9102)
+
+ private val sizedDid: Integer = Integer.valueOf(9001)
+ private val goneDid: Integer = Integer.valueOf(9002)
+
+ private val sizedRepo = "texera-ds-sized"
+ private val goneRepo = "texera-ds-gone"
+
+ //
---------------------------------------------------------------------------------------------
+ // lakeFS loopback stub, bound at the endpoint the config already names (see
the class comment).
+ //
---------------------------------------------------------------------------------------------
+
+ private val lakefsUri = new URI(StorageConfig.lakefsEndpoint)
+ private val apiPrefix = lakefsUri.getPath
+
Review Comment:
`apiPrefix` is derived from `lakefsUri.getPath` without normalization. If
`StorageConfig.lakefsEndpoint` is overridden with a trailing slash (e.g.
`.../api/v1/`), the stub will match `.../api/v1//repositories/...` and return
404 for size calls even though the server successfully bound, causing the
`requireStub()`-guarded tests to fail unexpectedly. Stripping a trailing `/`
makes the stub resilient to both forms of the config value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
