Skip to content

Backoffice Search: Batch the ancestors lookup for search results to avoid exceeding the maximum URL length (closes #23032)#23048

Open
AndyButland wants to merge 4 commits into
v17/devfrom
v17/bugfix/23032-batch-ancestor-calls
Open

Backoffice Search: Batch the ancestors lookup for search results to avoid exceeding the maximum URL length (closes #23032)#23048
AndyButland wants to merge 4 commits into
v17/devfrom
v17/bugfix/23032-batch-ancestor-calls

Conversation

@AndyButland
Copy link
Copy Markdown
Contributor

@AndyButland AndyButland commented Jun 3, 2026

Description

As reported in #23032, backoffice global search (documents and media) hangs with a never-ending spinner when a query returns many results. Searching a common term that returns 100+ hits shows the loading indicator forever, while a narrow term displays normally.

Fixes #23032,

Root cause

After the search query returns, the backoffice fetches the ancestors of every result so each hit can be rendered within its tree hierarchy. That ancestor lookup was sent as a single GET request with one id=<guid> query parameter per result:

/umbraco/management/api/v1/item/document/ancestors?id=<guid>&id=<guid>&…(100+)

With ~40 characters per id= segment, 100+ GUIDs push the query string past default limits, which rejects the request with HTTP 404. The failed call halts the data source, so the results never render.

Regular multi-item ("items") lookups already avoid this: they go through UmbItemDataApiGetRequestController, which splits the ids into chunks of 40, runs them in parallel, and amalgamates the results. The two search data sources had their own bespoke #fetchAncestors that bypassed this controller and sent every id in one request, so they never inherited the batching protection.

Fix

Route #fetchAncestors in both search data sources through the existing UmbItemDataApiGetRequestController so the ancestors lookup is chunked exactly like every other multi-item call.

Testing

Automated

I've added unit tests for both search data sources. They stub the search to return 95 results and assert the ancestors request is split into batches of at most 40 ids, that every result is mapped to its ancestors from the amalgamated batches, and that a small result set still uses a single request. Verified the tests fail against the pre-fix single-request code and pass after batching.

Manual

To test with a smaller result set I've temporarily lowered the #batchSize settings in item-data-api-get-request.controller.ts to a small number, and verified network calls are as expected (i.e. a search result that exceeds the batch size in number of results leads to more than one request to the ancestors, and the search results are correctly rendered.

…e batched to avoid a single query exceeding the maximum URL length.
Copilot AI review requested due to automatic review settings June 3, 2026 04:38
@claude
Copy link
Copy Markdown

claude Bot commented Jun 3, 2026

Claude finished @AndyButland's task in 5m 38s —— View job


PR Review

Target: origin/v17/dev · Based on commit: d159566f
· Skipped: 0 noise files out of 4 total

Routes #fetchAncestors in both search data sources through UmbItemDataApiGetRequestController, giving them the same ID-batching (≤ 40 per request) that every other multi-item lookup already uses. Adds unit tests verifying the split-and-merge behaviour.

  • Other changes: Backoffice global search no longer hangs with a spinner when a query returns 40+ results (previously a single over-long ancestors URL was rejected by IIS as HTTP 414/404).

Suggestions

  • document-search.server.data-source.test.ts:76 / media-search.server.data-source.test.ts:74: expect(ancestorRequestBatches.length).to.equal(3) is derived from UmbItemDataApiGetRequestController's private #batchSize = 40. If that constant is ever tuned, this assertion will start failing for a test that hasn't changed. The max-size assertion on the line below already proves batching is happening; consider replacing the equality check with to.be.greaterThan(1) to decouple the test from the controller's internals.

  • document-search.server.data-source.test.ts:57 / media-search.server.data-source.test.ts:55: The tests monkey-patch DocumentService/MediaService with (Service as any) casts, bypassing TypeScript type checking on the stub. The project docs recommend MSW for API mocking (it catches signature drift automatically). That said, MSW operates at the network level and would be less natural for asserting how many batched calls are made, so this tradeoff is understandable. If you want type safety without changing the approach, a narrowly typed wrapper (e.g. const stub = DocumentService as { getItemDocumentAncestors: (...) => ... }) would at least keep the stub's parameter shape in sync with the API.

  • document-search.server.data-source.test.ts / media-search.server.data-source.test.ts: No test covers the error path (when requestController.request() returns an error). Adding a test that stubs the ancestors call to reject and asserting the search method propagates the error would round out coverage.


Approved with Suggestions for improvement

Good to go, but please carefully consider the importance of the suggestions.

The production fix itself is minimal, correct, and consistent with how every other multi-item lookup is handled. The tests fail on the pre-fix code and pass on the fix (as described) — the batching/merge logic is well-exercised.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a backoffice global search hang for large result sets by batching the “ancestors” lookup (to avoid exceeding IIS URL/query-string length limits) and adds unit tests to verify the batching behavior for both document and media search data sources.

Changes:

  • Route document + media search ancestors lookups through UmbItemDataApiGetRequestController (batch size 40) instead of issuing a single long GET URL.
  • Add new unit tests for both search data sources verifying batching behavior and ancestor mapping.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
src/Umbraco.Web.UI.Client/src/packages/media/media/search/media-search.server.data-source.ts Uses UmbItemDataApiGetRequestController to batch ancestors requests for media search results.
src/Umbraco.Web.UI.Client/src/packages/media/media/search/media-search.server.data-source.test.ts Adds unit tests asserting media ancestors requests are batched and results are mapped.
src/Umbraco.Web.UI.Client/src/packages/documents/documents/search/document-search.server.data-source.ts Uses UmbItemDataApiGetRequestController to batch ancestors requests for document search results.
src/Umbraco.Web.UI.Client/src/packages/documents/documents/search/document-search.server.data-source.test.ts Adds unit tests asserting document ancestors requests are batched and results are mapped.

@claude claude Bot added category/front-end category/ux User experience labels Jun 3, 2026
AndyButland and others added 2 commits June 3, 2026 06:51
batchTryExecute resolves each chunk via tryExecute, which never rejects, so
a per-chunk failure comes back as a fulfilled result carrying an error and
leaves an undefined hole in the amalgamated data without surfacing an error.
Detect that before mapping and return an explicit error instead of throwing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Strengthen the batching tests to assert every search-result id is requested
exactly once (Set size), not just that the total count matches. Add the
no-direct-api-import disable on the controller's api callback, matching the
existing url data sources, since the call is wrapped by the controller.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@AndyButland AndyButland changed the title Search: Batch the ancestors lookup for search results to avoid exceeding the maximum URL length (closes #23032) Backoffice Search: Batch the ancestors lookup for search results to avoid exceeding the maximum URL length (closes #23032) Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants