Skip to content

perf(ui): fetch database in parallel with permission on DatabaseDetailsPage (W1 template) - #30867

Closed
harsh-vador wants to merge 1 commit into
mainfrom
perf/w1-parallel-permission-database-template
Closed

perf(ui): fetch database in parallel with permission on DatabaseDetailsPage (W1 template)#30867
harsh-vador wants to merge 1 commit into
mainfrom
perf/w1-parallel-permission-database-template

Conversation

@harsh-vador

@harsh-vador harsh-vador commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Fixes 5445

Description

The database detail page paid two serial round-trips on every mount: a permission fetch, then a database useQuery gated on the permission resolving (enabled: hasViewBasicPermission && !permissionsLoading). This fires the database GET in parallel with the permission fetch instead.

This is the template for the W1 request-waterfall fix — the audit lists ~7 primary-render pages (SearchIndexDetailsPage, Database/DatabaseSchema/Version pages, StoredProcedurePage, APICollectionPage) with the identical permission-gates-fetch shape. Reference parallel pattern already in the tree: PlatformLineage.tsx.

Preserving the no-permission UX (the subtle part)

Because the GET now runs speculatively, a no-permission user's GET returns 403. Three coordinated changes keep the prior behaviour exactly:

  1. enabled is gated only on the FQN → the query fires immediately, in parallel.
  2. Render checks !hasViewBasicPermission before isError → a no-permission 403 shows the inline PERMISSION placeholder, not the generic error placeholder.
  3. The /forbidden redirect fires only on a genuine permission desync (hasViewBasicPermission true but the backend still denied), never for a normal no-permission user.

Type of change

  • Performance (request waterfall → parallel)

Tests

  • Added a regression unit test: a no-permission user gets the PERMISSION placeholder (no generic error, no forbidden redirect) and the database GET is still fired in parallel.
  • All 5 DatabaseDetailsPage unit tests pass; eslint clean.

Verification note

The 403 / permission UX is guarded by the new unit test, but is best also confirmed in a running app / Playwright (Jest mocks permissions and does not exercise the real backend 403 path). Recommend a manual/Playwright pass on: (a) happy path, (b) no-permission user sees the inline placeholder and is not redirected to /forbidden.

Ref: open-metadata/openmetadata-collate#5442

…lsPage (W1 template)

The database GET was gated on the permission fetch resolving first
(useQuery enabled: hasViewBasicPermission && !permissionsLoading), so every
mount paid two serial round-trips. Now the GET fires in parallel with the
permission fetch.

To preserve the no-permission UX now that the GET runs speculatively:
- enabled is gated only on the FQN, so the query fires immediately.
- render checks !hasViewBasicPermission BEFORE isError, so a no-permission
  user's 403 shows the inline PERMISSION placeholder, not the generic error.
- the FORBIDDEN redirect fires only on a genuine permission desync
  (hasViewBasicPermission true but backend denied), not for a normal
  no-permission user.

Added a regression unit test asserting a no-permission user gets the PERMISSION
placeholder (no generic error, no forbidden redirect) and that the database GET
is still fired in parallel. All 5 DatabaseDetailsPage tests pass; eslint clean.

This is the template for the W1 waterfall fix (audit lists ~7 primary-render
pages with the same permission-gates-fetch shape). The remaining pages will
follow the same three-part pattern. The 403/permission UX is best confirmed in a
running app / Playwright in addition to the unit guard.

Ref: open-metadata/openmetadata-collate#5442

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@harsh-vador
harsh-vador requested a review from a team as a code owner August 3, 2026 10:22
@github-actions github-actions Bot added the UI UI specific issues label Aug 3, 2026
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@gitar-bot

gitar-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

Fetches database details in parallel with permission checks on DatabaseDetailsPage to eliminate request waterfalls, preserving the no-permission UX. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 335fe08ee9b0732dd157d77abf53c0ac4ad1d4e3 in Playwright run 30805553513, attempt 1.

✅ 549 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 3 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 51m 51s

⏱️ Max setup 3m 31s · max shard execution 17m 52s · max shard-job elapsed before upload 21m 29s · reporting 5s

🌐 201.44 requests/attempt · 2.83 app boots/UI scenario · 17.07% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 17.07% (convergence target: at most 15%).
  • Browser traffic was 201.44 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.83 per UI scenario (1620 boots / 572 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
🟡 Shard chromium-01 138 0 1 3 0 0
✅ Shard chromium-02 136 0 0 0 0 0
✅ Shard chromium-03 124 0 0 0 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 14 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard ingestion-01 1 0 0 0 0 0
✅ Shard reindex-01 2 0 0 0 0 0
✅ Shard search-01 10 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0
🟡 1 flaky test(s) (passed on retry)
  • Pages/Entity.spec.tsDomain Propagation (shard chromium-01, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@harsh-vador harsh-vador self-assigned this Aug 3, 2026
@harsh-vador harsh-vador added the safe to test Add this label to run secure Github workflows on PRs label Aug 3, 2026
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

⚠️ UI Checkstyle passed — lint findings in changed files

🔍 ESLint findings in this PR's files — ESLint report could not be read — see the job log.

Errors block the build. Warnings do not yet — they are rules whose backlog is still
being worked down, listed so this PR does not add to it. See docs/ui-code-quality-gate.md.

No machine-readable report was produced.


Fix locally (fast - only checks files changed in this branch):

make ui-checkstyle-changed

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
66.01% (77626/117596) 49.97% (46835/93724) 51.14% (14079/27526)

@sonarqubecloud

sonarqubecloud Bot commented Aug 3, 2026

Copy link
Copy Markdown

@harsh-vador

Copy link
Copy Markdown
Contributor Author

Closing after reconsidering the approach. PermissionProvider.getEntityPermissionByFqn caches results (entitiesPermission[fqn]), so the serial permission→data path is usually not a real waterfall — on in-app navigation the permission resolves instantly from cache. Firing the entity GET in parallel therefore only helps the narrow cold-cache (deep-link/refresh) case for permitted users, while adding a throwaway 403 for every no-permission user (backend log/metric noise). The narrow benefit doesn't justify the doomed calls, so backing this out. Ref: open-metadata/openmetadata-collate#5442

@harsh-vador harsh-vador closed this Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant