Skip to content

Ontology position#27200

Merged
anuj-kumary merged 7 commits intomainfrom
ontology-position
Apr 9, 2026
Merged

Ontology position#27200
anuj-kumary merged 7 commits intomainfrom
ontology-position

Conversation

@anuj-kumary
Copy link
Copy Markdown
Member

Describe your changes:

Screen.Recording.2026-04-09.at.2.24.45.PM.mov
Screenshot 2026-04-09 at 2 24 32 PM

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

@anuj-kumary anuj-kumary self-assigned this Apr 9, 2026
@anuj-kumary anuj-kumary requested a review from a team as a code owner April 9, 2026 09:05
@anuj-kumary anuj-kumary added UI UI specific issues safe to test Add this label to run secure Github workflows on PRs labels Apr 9, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
64.19% (59559/92778) 43.72% (31031/70962) 46.89% (9363/19967)

siddhant1
siddhant1 previously approved these changes Apr 9, 2026
@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Apr 9, 2026

Code Review ⚠️ Changes requested 4 resolved / 5 findings

Refactors ontology position handling with pagination fixes and API concurrency limits, but the missing-related-terms fetch lacks an upper bound on API calls—every missing ID triggers a getGlossaryTermsById call with no MAX_SAFE_PAGES guard like other pagination loops.

⚠️ Edge Case: Missing related-terms fetch has no upper bound on API calls

📄 openmetadata-ui/src/main/resources/ui/src/components/OntologyExplorer/OntologyExplorer.tsx:1071-1085

Every other pagination loop in this PR was capped with a MAX_SAFE_PAGES guard, but the new missing-related-terms fetch (lines 1071-1092) issues one getGlossaryTermsById call per missing ID with no cap on missingIds.size. If a glossary has thousands of terms each referencing unique cross-glossary terms, this could fire thousands of individual HTTP requests, overwhelming the API — the exact scenario the other guards were added to prevent.

Suggested fix
const MAX_MISSING_TERMS = 200; // or another sensible cap
if (missingIds.size > 0) {
  const missingIdList = Array.from(missingIds).slice(0, MAX_MISSING_TERMS);
  for (let i = 0; i < missingIdList.length; i += CONCURRENCY) {
✅ 4 resolved
Edge Case: runLayout can hang forever if AFTER_LAYOUT event never fires

📄 openmetadata-ui/src/main/resources/ui/src/components/OntologyExplorer/hooks/useOntologyGraph.ts:84-89
The runLayout helper registers a graph.once(GraphEvent.AFTER_LAYOUT) listener and then calls graph.layout(). If the layout worker crashes, the graph is destroyed mid-layout, or the event simply never fires, the returned Promise will never settle. This silently hangs the render pipeline — the user sees an incomplete graph with no error feedback.

Consider adding a timeout (e.g., Promise.race with a 10-15 second deadline) so the caller can fall back to the bare graph.draw() path that already exists in the catch block.

Edge Case: All pagination loops are unbounded — risk of infinite loop

📄 openmetadata-ui/src/main/resources/ui/src/components/OntologyExplorer/OntologyExplorer.tsx:757-765 📄 openmetadata-ui/src/main/resources/ui/src/components/OntologyExplorer/OntologyExplorer.tsx:950-964 📄 openmetadata-ui/src/main/resources/ui/src/components/OntologyExplorer/OntologyExplorer.tsx:1060-1074
Three pagination loops had their safety bounds removed (the old DEFAULT_LIMIT = 500 cap):

  1. fetchAllMetrics (do/while on after) — previously capped at 500
  2. fetchGraphDataFromRdf (while-true with offset) — no max iterations
  3. fetchAllGlossaryData inline IIFE (do/while on afterCursor) — no max iterations

If any API returns a non-empty after cursor indefinitely (e.g., a backend bug or stale cursor), the UI thread will loop forever, making the page unresponsive.

Add a maximum iteration/item count as a safety valve (e.g., MAX_PAGES = 200 or MAX_ITEMS = 50_000).

Performance: 20-concurrent glossary fetches may overwhelm the API

📄 openmetadata-ui/src/main/resources/ui/src/components/OntologyExplorer/OntologyExplorer.tsx:1005 📄 openmetadata-ui/src/main/resources/ui/src/components/OntologyExplorer/OntologyExplorer.tsx:1035-1045
The fetchGraphDataFromDatabase function fires up to 20 parallel fetchTermsForGlossary calls (CONCURRENCY = 20), and each one internally paginates with sequential requests. There is no global rate limiting or request throttling in the API client. For organizations with many glossaries, this creates bursts of 20+ simultaneous requests that could trigger server-side rate limits or degrade performance for other users.

Consider lowering the concurrency (e.g., 5-8) or adding an AbortController so in-flight requests are cancelled if the component unmounts.

Quality: fitViewWithMinZoom logic duplicated in catch block

📄 openmetadata-ui/src/main/resources/ui/src/components/OntologyExplorer/hooks/useOntologyGraph.ts:886-890 📄 openmetadata-ui/src/main/resources/ui/src/components/OntologyExplorer/hooks/useOntologyGraph.ts:896-900
In the runRender function, the zoom-clamping logic (fitViewWithMinZoom + PRACTICAL_MIN_ZOOM check) appears identically in both the try and catch blocks (lines 886-890 and 896-900). This makes maintenance error-prone — a change in one path could be missed in the other. Consider extracting it into a small helper or using a finally-like pattern.

🤖 Prompt for agents
Code Review: Refactors ontology position handling with pagination fixes and API concurrency limits, but the missing-related-terms fetch lacks an upper bound on API calls—every missing ID triggers a `getGlossaryTermsById` call with no `MAX_SAFE_PAGES` guard like other pagination loops.

1. ⚠️ Edge Case: Missing related-terms fetch has no upper bound on API calls
   Files: openmetadata-ui/src/main/resources/ui/src/components/OntologyExplorer/OntologyExplorer.tsx:1071-1085

   Every other pagination loop in this PR was capped with a `MAX_SAFE_PAGES` guard, but the new missing-related-terms fetch (lines 1071-1092) issues one `getGlossaryTermsById` call per missing ID with no cap on `missingIds.size`. If a glossary has thousands of terms each referencing unique cross-glossary terms, this could fire thousands of individual HTTP requests, overwhelming the API — the exact scenario the other guards were added to prevent.

   Suggested fix:
   const MAX_MISSING_TERMS = 200; // or another sensible cap
   if (missingIds.size > 0) {
     const missingIdList = Array.from(missingIds).slice(0, MAX_MISSING_TERMS);
     for (let i = 0; i < missingIdList.length; i += CONCURRENCY) {

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 9, 2026

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

🟡 Playwright Results — all passed (24 flaky)

✅ 3594 passed · ❌ 0 failed · 🟡 24 flaky · ⏭️ 207 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 454 0 3 2
🟡 Shard 2 638 0 4 32
🟡 Shard 3 647 0 4 26
🟡 Shard 4 616 0 6 47
🟡 Shard 5 606 0 1 67
🟡 Shard 6 633 0 6 33
🟡 24 flaky test(s) (passed on retry)
  • Features/CustomizeDetailPage.spec.ts › Table - customization should work (shard 1, 1 retry)
  • Pages/AuditLogs.spec.ts › should apply both User and EntityType filters simultaneously (shard 1, 1 retry)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Database Schema (shard 2, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/BulkImport.spec.ts › Table (shard 2, 1 retry)
  • Features/ColumnBulkOperations.spec.ts › should select column, open drawer, and verify form fields (shard 2, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
  • Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3, 1 retry)
  • Flow/ExploreDiscovery.spec.ts › Should display deleted assets when showDeleted is checked and deleted is not present in queryFilter (shard 3, 1 retry)
  • Flow/NotificationAlerts.spec.ts › Conversation source alert (shard 3, 1 retry)
  • Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Directory (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Database (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate Owner Rule Is_Not (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate Owner Rule Any_In (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Rename domain with tags and glossary terms preserves associations (shard 4, 1 retry)
  • Pages/ExploreTree.spec.ts › Verify Database and Database Schema available in explore tree (shard 5, 1 retry)
  • Pages/Glossary.spec.ts › Column dropdown drag-and-drop functionality for Glossary Terms table (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)
  • Pages/ODCSImportExport.spec.ts › Multi-object ODCS contract - object selector shows all schema objects (shard 6, 1 retry)
  • Pages/ProfilerConfigurationPage.spec.ts › Non admin user (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Admin user can edit teams from the user profile (shard 6, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 6, 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

@anuj-kumary anuj-kumary merged commit 9fde3f1 into main Apr 9, 2026
45 checks passed
@anuj-kumary anuj-kumary deleted the ontology-position branch April 9, 2026 13:00
SaaiAravindhRaja pushed a commit to SaaiAravindhRaja/OpenMetadata that referenced this pull request Apr 12, 2026
* Fix glossary terms position for large data

* nit

* fix lint issue

* Refactor performance issue

* fix lint issue
SaaiAravindhRaja pushed a commit to SaaiAravindhRaja/OpenMetadata that referenced this pull request Apr 12, 2026
* Fix glossary terms position for large data

* nit

* fix lint issue

* Refactor performance issue

* fix lint issue
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.

2 participants