Apply hooks search filter before pagination#8368
Apply hooks search filter before pagination#8368nitishagar wants to merge 1 commit intotaskcluster:mainfrom
Conversation
nitishagar
requested review from
lotas,
matt-boris and
petemoore
and removed request for
a team
nitishagar
force-pushed
the
fix/7972-hooks-search
branch
from
52be92b to
aef4cd2
Compare
| query HookGroups { | ||
| hookGroups { | ||
| hookGroupId | ||
| hooks { |
There was a problem hiding this comment.
This feels like the right way to fix the given search issue, but the reason why we have separate HookGroups query is because it is fast.
We had to create a new HookGroups view specifically to make first page load fast - original query takes <300ms to respond with all groups.
Now, when we include hooks inside that is suddenly becomes much slower - takes ~6s to load
lotas
left a comment
lotas
left a comment
There was a problem hiding this comment.
We cannot accept current changes as it would be a regression.
Plus having client-side only search doesn't help here.
We should probably think about implementing a server-side filtering, or only include nested hooks when search is present.
Another idea on top of my head would be to use different view for the search purposes
nitishagar
force-pushed
the
fix/7972-hooks-search
branch
5 times, most recently
from
4191d78 to
d6f5b5e
Compare
|
Thanks for the review @lotas. Reworked this with server-side filtering:
|
|
hey @nitishagar thanks for the refactor, I think this a step in the right direction, since we remove the expensive nested hook loading. That said, I think we are still overloading listHookGroups with search behavior that does not match its natural ocntract. We filter for both hook group and hook id, but return only hook group. That shape existed mostly because current UI was optimized that way. I think a cleaner approach would be to introduce a dedicated hook search endpoint and add new UI element that would display filtered hooks.
|
nitishagar
force-pushed
the
fix/7972-hooks-search
branch
from
d6f5b5e to
b0fb28a
Compare
Previously the hooks search only matched hook group names visible on the
current page, and couldn't search within hook IDs. The fix added
hooks { hookId } to the hookGroups GraphQL query which caused a ~6s
initial page load regression due to N+1 requests (one per group).
This commit reworks the fix with proper server-side filtering:
- db/versions/0123.yml: add get_hook_groups_2(search_term_in text) that
searches hook_group_id and hook_id via ILIKE in a single DB query;
deprecate get_hook_groups() per the _2 convention
- services/hooks/src/api.js: add optional `search` query param to
listHookGroups; call get_hook_groups_2 to filter at the DB layer
- services/web-server/src/loaders/hooks.js: pass search to the TC client
- services/web-server/src/resolvers/Hooks.js: extract search from args
- services/web-server/src/graphql/Hooks.graphql: add search: String arg
- ui/src/views/Hooks/ListHookGroups/hookGroups.graphql: use $search
variable, remove hooks { hookId } (no longer needed)
- ui/src/views/Hooks/ListHookGroups/index.jsx: pass search as a GraphQL
variable so the query re-runs on URL change; remove client-side filter
- db/test/fns/hooks_test.js: tests for get_hook_groups_2
Initial page load remains fast (<300ms, group IDs only). Searches trigger
a single efficient DB query matching group or hook IDs case-insensitively.
Closes taskcluster#7972
nitishagar
force-pushed
the
fix/7972-hooks-search
branch
from
b0fb28a to
c844c3f
Compare
2 participants
Summary
Fixes #7972.
Refactors the hooks search to use a dedicated
searchHooksAPI endpoint instead ofoverloading
listHookGroups. This was done per reviewer feedback in#8368 (comment).
Root cause: The previous approach added a
searchquery param tolistHookGroups,which didn't match its natural contract (it returns groups but was filtering by hook IDs).
The initial page load was also slow due to N+1 requests when fetching hook IDs per group.
Fix:
db/versions/0123.yml: addssearch_hooks(search_term_in, page_size, page_offset)—a single
ILIKEquery acrosshook_group_idandhook_id, returns full hook rowsGET /hooks/search?q=<term>API endpoint (searchHooks) — returns matchinghook definitions across all groups (same shape as
listHooks)listHookGroupsreverted to its clean contract (no search param, usesget_hook_groups())hookGroupsno longer takessearch; newsearchHooks(query: String!): [Hook]added?search=URL param is present, fetches fromsearchHooksand renders a flatHooksListTable; otherwise renders the normalHookGroupsTableInitial page load is fast (group IDs only). Searches trigger a single efficient DB query.
Changes
DB:
db/versions/0123.yml— newsearch_hooksfunction (replacesget_hook_groups_2)db/test/fns/hooks_test.js— tests forsearch_hooksdb/test/versions/0123_test.js— version smoke testAPI (
services/hooks):services/hooks/schemas/v1/search-hooks-response.yml— new schema (same shape aslistHooks)services/hooks/src/api.js— revertedlistHookGroups, addedsearchHooksat/hooks/searchservices/hooks/test/api_test.js— tests forsearchHooksGraphQL/Web-server:
services/web-server/src/graphql/Hooks.graphql— removedsearchfromhookGroups, addedsearchHooksservices/web-server/src/loaders/hooks.js— newhookSearchDataLoaderservices/web-server/src/resolvers/Hooks.js— newsearchHooksresolverUI:
ui/src/views/Hooks/ListHookGroups/hookGroups.graphql— simplified (no$search)ui/src/views/Hooks/ListHookGroups/searchHooks.graphql— new queryui/src/views/Hooks/ListHookGroups/index.jsx— two-mode renderingGenerated (from
yarn generate):generated/references.json,generated/db-schema.jsonlibraries/postgres/@types/fns.d.ts,db/fns.md,db/schema.md,db/versions/README.mdsearchHooksmethodTest Plan
cd db && yarn test— DB function tests passcd services/hooks && yarn test— API tests pass including newsearchHookssuite/hooks— groups table loads normally (no regression)PROJECTmatchesproject-releng)