Yicong-Huang commented on code in PR #7464:
URL: https://github.com/apache/texera/pull/7464#discussion_r3916372907
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala:
##########
@@ -190,10 +182,10 @@ object DashboardResource {
class DashboardResource {
/**
- * This method performs a full-text search across all resources -
workflows, projects, and files -
+ * This method performs a full-text search across all resources - workflows
and datasets -
Review Comment:
Same gap in the method Scaladoc: the all-types branch at `:104-107` unions
the model query too. The filter list on `:188` ("workflow IDs and operators")
has drifted the same way — `modelIds` is a documented param at `:106`.
```suggestion
* This method performs a full-text search across all resources -
workflows, datasets and models -
```
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/DashboardResource.scala:
##########
@@ -56,15 +55,14 @@ object DashboardResource {
The following class describe the available params from the frontend for
full text search.
* @param user The authenticated user performing the search.
* @param keywords A list of search keywords. The API will return
resources that match any of these keywords.
- * @param resourceType The type of the resources to include in the
search results. Acceptable values are "workflow", "project", "file" and "" (for
all types).
+ * @param resourceType The type of the resources to include in the
search results. Acceptable values are "workflow", "dataset" and "" (for all
types).
Review Comment:
This enumeration reads as exhaustive — `:108` throws on anything not listed
— but the dispatch at `:101` also accepts `MODEL_RESOURCE_TYPE`, which arrived
with the merge after this line was rewritten. A caller trusting the doc would
think `resourceType=model` is invalid.
```suggestion
* @param resourceType The type of the resources to include in the
search results. Acceptable values are "workflow", "dataset", "model" and ""
(for all types).
```
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala:
##########
@@ -542,23 +542,22 @@ class DatasetSearchQueryBuilderSpec
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
+ it should "stay union-compatible with the workflow branch" in {
+ // `DashboardResource.searchAllResources` stacks both builders with
`unionAll` for a
Review Comment:
`searchAllResources` stacks three builders, not two — the rewrite dropped
the project branch but counted the model branch out as well. Worth fixing the
parenthetical on `:551` too: dataset *and model* both project `DSL.inline("")`
there.
The test itself is fine as a two-branch check; the three-branch union is
exercised by `WorkflowResourceSpec` against real Postgres.
```suggestion
// `DashboardResource.searchAllResources` stacks the three builders with
`unionAll` for a
```
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala:
##########
@@ -681,99 +634,104 @@ class WorkflowResourceSpec
)
}
- "/search API" should "be able to search for resources in different tables"
in {
+ "/search API" should "be able to search for resources by keyword" in {
- // create different types of resources, project, workflow, and file
- projectResource.createProject(sessionUser1, "test project1")
workflowResource.persistWorkflow(testWorkflow1, sessionUser1)
// search
val DashboardClickableFileEntryList =
dashboardResource.searchAllResourcesCall(
sessionUser1,
SearchQueryParams(getKeywordsArray("test"))
)
- assert(DashboardClickableFileEntryList.results.length == 2)
+ assert(DashboardClickableFileEntryList.results.length == 1)
}
it should "return all resources when no keyword provided" in {
- projectResource.createProject(sessionUser1, "test project1")
workflowResource.persistWorkflow(testWorkflow1, sessionUser1)
val DashboardClickableFileEntryList =
dashboardResource.searchAllResourcesCall(
sessionUser1,
SearchQueryParams(getKeywordsArray(""))
)
- assert(DashboardClickableFileEntryList.results.length == 2)
+ assert(DashboardClickableFileEntryList.results.length == 1)
}
it should "return multiple matching resources from a single resource type"
in {
workflowResource.persistWorkflow(testWorkflow1, sessionUser1)
- projectResource.createProject(sessionUser1, "common project1")
- projectResource.createProject(sessionUser1, "common project2")
+ workflowResource.persistWorkflow(testWorkflow2, sessionUser1)
val DashboardClickableFileEntryList =
dashboardResource.searchAllResourcesCall(
sessionUser1,
- SearchQueryParams(getKeywordsArray("common"))
+ SearchQueryParams(getKeywordsArray("test"))
)
assert(DashboardClickableFileEntryList.results.length == 2)
}
it should "handle multiple keywords correctly" in {
- projectResource.createProject(sessionUser1, "test project1")
workflowResource.persistWorkflow(testWorkflow1, sessionUser1)
+ workflowResource.persistWorkflow(testWorkflow2, sessionUser1)
val DashboardClickableFileEntryList =
dashboardResource.searchAllResourcesCall(
sessionUser1,
- SearchQueryParams(getKeywordsArray("test", "project1"))
+ SearchQueryParams(getKeywordsArray("test", "workflow1"))
)
assert(
DashboardClickableFileEntryList.results.length == 1
- ) // should only return the project
+ ) // should only return test_workflow1
}
it should "filter results by different resourceType" in {
- // create different types of resources
- // 3 projects, 2 file, and 1 workflow,
- projectResource.createProject(sessionUser1, "test project1")
- projectResource.createProject(sessionUser1, "test project2")
- projectResource.createProject(sessionUser1, "test project3")
+ // create 3 workflows
workflowResource.persistWorkflow(testWorkflow1, sessionUser1)
+ workflowResource.persistWorkflow(testWorkflow2, sessionUser1)
+ workflowResource.persistWorkflow(testWorkflow3, sessionUser1)
// search resources with all resourceType
var DashboardClickableFileEntryList =
dashboardResource.searchAllResourcesCall(
sessionUser1,
SearchQueryParams(getKeywordsArray("test"))
)
- assert(DashboardClickableFileEntryList.results.length == 4)
+ assert(DashboardClickableFileEntryList.results.length == 3)
// filter resources by workflow
DashboardClickableFileEntryList = dashboardResource.searchAllResourcesCall(
sessionUser1,
SearchQueryParams(resourceType = "workflow", keywords =
getKeywordsArray("test"))
)
- assert(DashboardClickableFileEntryList.results.length == 1)
+ assert(DashboardClickableFileEntryList.results.length == 3)
- // filter resources by project
+ // filter resources by dataset
DashboardClickableFileEntryList = dashboardResource.searchAllResourcesCall(
sessionUser1,
- SearchQueryParams(resourceType = "project", keywords =
getKeywordsArray("test"))
+ SearchQueryParams(resourceType = "dataset", keywords =
getKeywordsArray("test"))
)
- assert(DashboardClickableFileEntryList.results.length == 3)
+ assert(DashboardClickableFileEntryList.results.isEmpty)
+
+ // The counts above cannot distinguish a working filter from an ignored
one, because every
+ // seeded row is a workflow and the only other searchable type is
LakeFS-backed (a seeded
Review Comment:
My last round asked you to make this singular, and that was right at the
time — `dataset` really was the only other type. The merge added `model`, so
there are two again and the plural is correct. Sorry for the round trip.
```suggestion
// seeded row is a workflow and the only other searchable types are
LakeFS-backed (a seeded
```
--
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]
