Skip to content

Fixes 30200: Paginate glossary term relation settings#30201

Open
harshach wants to merge 4 commits into
mainfrom
harshach/paginate-term-relations
Open

Fixes 30200: Paginate glossary term relation settings#30201
harshach wants to merge 4 commits into
mainfrom
harshach/paginate-term-relations

Conversation

@harshach

@harshach harshach commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Describe your changes:

Fixes #30200

I paginated glossary relation type settings and added compact CRUD endpoints because the page previously fetched the complete settings payload and all relation usage counts before rendering. Relation type creation now rejects case-insensitive duplicate names inside the atomic settings update, while deletion checks usage only when requested.

Type of change:

  • Improvement

High-level design:

  • The UI requests an offset-based page of relation types and renders page-size controls.
  • New backend list and per-relation CRUD endpoints return compact payloads while preserving the existing full-settings API.
  • Shared normalization and repository-level uniqueness validation protect both new and legacy update paths, including concurrent creates.
  • Usage aggregation is deferred until deletion so initial page load no longer waits for it.

Tests:

Use cases covered

  • Admin can page through relation types without loading the complete settings payload.
  • Creating a relation type rejects duplicate names regardless of case.
  • Create, update, and delete calls operate on a single relation type; in-use and system-defined types remain protected from deletion.

Unit tests

  • Added backend unit tests for normalization and duplicate rejection.
  • Added UI tests for pagination and compact REST calls.
  • Files: GlossaryTermRelationSettingsUtilTest.java, SystemRepositoryPatchSettingTest.java, GlossaryTermRelationSettings.test.tsx, and glossaryRelationTypesAPI.test.ts.
  • Result: 9 Java tests and 3 Jest tests passed; coverage was not measured.

Backend integration tests

  • Added integration coverage for pagination and case-insensitive duplicate rejection.
  • File: GlossaryTermRelationSettingsIT.java.
  • All 434 integration-test sources compile; the live-stack integration test was not run locally.

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • Not added; the pagination behavior is covered by focused Jest tests.

Manual testing performed

  • Not performed against a local stack.
  • Java Spotless, UI checkstyle, focused Java/Jest tests, integration-test compilation, and diff checks passed.
  • Repository-wide UI type-check remains blocked by pre-existing generated-schema errors; none reference changed files.

UI screen recording / screenshots:

Not attached yet; required before marking this draft ready for review.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • 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.
  • For UI changes: I attached a screen recording and/or screenshots above.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.
  • I have added tests around the new logic.
  • For connector/ingestion changes: Not applicable.

Greptile Summary

This PR adds paginated glossary relation-type settings and compact CRUD operations. The main changes are:

  • Adds paginated relation-type loading and page-size controls.
  • Adds backend endpoints for listing, creating, updating, and deleting one relation type.
  • Enforces case-insensitive name uniqueness during atomic settings updates.
  • Defers relation usage checks until deletion.
  • Adds focused backend, integration, and UI tests.

Confidence Score: 5/5

This looks safe to merge after the existing review findings are resolved.

  • No additional blocking issues were found in the latest changes.
  • The remaining predicate-uniqueness behavior is already covered by an existing review finding.

Important Files Changed

Filename Overview
openmetadata-ui/src/main/resources/ui/src/pages/GlossaryTermRelationSettings/GlossaryTermRelationSettings.tsx Adds paginated loading, compact CRUD calls, page navigation, and conflict handling.
openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java Adds admin-only paginated list and per-relation CRUD endpoints.
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/SystemRepository.java Adds atomic normalization, uniqueness validation, and stale-update handling.
openmetadata-service/src/main/java/org/openmetadata/service/util/GlossaryTermRelationSettingsUtil.java Centralizes normalization, name uniqueness, and system-defined relation protection.
openmetadata-ui/src/main/resources/ui/src/rest/glossaryAPI.ts Adds typed API helpers for paginated listing and per-relation CRUD.

Comments Outside Diff (1)

  1. openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java, line 457-466 (link)

    P1 Usage Check Races Deletion

    A relation can be created after the usage query returns zero but before the settings patch removes the type. The settings update then succeeds and leaves that relation referring to a relation type that no longer exists because its compare-and-set does not cover relation writes.

    Context Used: CLAUDE.md (source)

Reviews (4): Last reviewed commit: "Merge branch 'main' into harshach/pagina..." | Re-trigger Greptile

Context used (3)

  • Context used - CLAUDE.md (source)
  • Context used - openmetadata-ui-core-components/CLAUDE.md (source)
  • Context used - AGENTS.md (source)

@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Jul 18, 2026
@harshach
harshach marked this pull request as ready for review July 18, 2026 15:55
@harshach
harshach requested a review from a team as a code owner July 18, 2026 15:55
Copilot AI review requested due to automatic review settings July 18, 2026 15:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

✅ PR checks passed

The linked issue has a description and all required Shipping project fields set. Thanks!

@github-actions

github-actions Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.21% (75791/116226) 49.01% (45183/92177) 49.83% (13681/27451)

Copilot AI review requested due to automatic review settings July 18, 2026 16:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gitar-bot

gitar-bot Bot commented Jul 18, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 2 resolved / 3 findings

Implements paginated glossary term relation settings and introduces compact CRUD endpoints to optimize performance. Note that the deletion usage check is susceptible to race conditions with concurrent relation creation.

💡 Quality: Duplicate-name UX depends on matching English error substring

📄 openmetadata-ui/src/main/resources/ui/src/pages/GlossaryTermRelationSettings/GlossaryTermRelationSettings.tsx:434-448

The catch block decides whether to surface a duplicate-name error on the name field by matching the raw backend string with errorMessage?.includes('already exists') and then displays that untranslated server message. If the backend message ever changes wording (or is localized), the inline field error silently reverts to the generic toast, and the shown text bypasses i18n. Prefer keying off a stable signal (e.g. the SYSTEM_SETTINGS_EXCEPTION error type / HTTP 400 on create) and rendering a translated message via useTranslation rather than substring-matching English prose.

✅ 2 resolved
Edge Case: Client-side duplicate-name check only scans current page

📄 openmetadata-ui/src/main/resources/ui/src/pages/GlossaryTermRelationSettings/GlossaryTermRelationSettings.tsx:295-305
After pagination, relationTypes state holds only the current page instead of the full settings payload, so the validateForm duplicate check (relationTypes.some(... name.toLowerCase() === formValues.name?.toLowerCase())) can no longer detect a name that collides with a relation type on another page. The user only learns about the conflict after the create request fails server-side (the new validateUniqueNames catches it), producing a delayed/confusing error toast instead of inline validation. This is a UX regression; consider surfacing the backend duplicate error message inline on the name field, or note that inline validation is now best-effort only.

Bug: Update/delete index computed outside atomic patch snapshot

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:349-363 📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:380-394
updateGlossaryTermRelationType/deleteGlossaryTermRelationType compute relationTypeIndex from a separate getGlossaryTermRelationSettings() read, then build a JsonPatch against /relationTypes/{index} while patchGlossaryTermRelationSettings re-reads its own snapshot for the compare-and-set. If another update reorders/removes earlier elements between the two reads, the index is stale. The .test(/relationTypes/{index}/name, existing.getName()) guard (plus unique names) prevents modifying the wrong element, but a shifted or out-of-range index causes JsonUtils.applyPatch to throw an unhandled exception (HTTP 500) rather than a clean 404/409 conflict. Consider translating patch/test failures into a PreconditionFailed (409) response for a clearer client contract.

🤖 Prompt for agents
Code Review: Implements paginated glossary term relation settings and introduces compact CRUD endpoints to optimize performance. Note that the deletion usage check is susceptible to race conditions with concurrent relation creation.

1. 💡 Quality: Duplicate-name UX depends on matching English error substring
   Files: openmetadata-ui/src/main/resources/ui/src/pages/GlossaryTermRelationSettings/GlossaryTermRelationSettings.tsx:434-448

   The catch block decides whether to surface a duplicate-name error on the name field by matching the raw backend string with `errorMessage?.includes('already exists')` and then displays that untranslated server message. If the backend message ever changes wording (or is localized), the inline field error silently reverts to the generic toast, and the shown text bypasses i18n. Prefer keying off a stable signal (e.g. the `SYSTEM_SETTINGS_EXCEPTION` error type / HTTP 400 on create) and rendering a translated message via `useTranslation` rather than substring-matching English prose.

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

Copilot AI review requested due to automatic review settings July 18, 2026 17:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gitar-bot

gitar-bot Bot commented Jul 18, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 3 resolved / 3 findings

Implements paginated glossary relation settings and compact CRUD endpoints to improve performance, while resolving duplicate-name validation and atomicity issues. All identified findings regarding name-clash handling and update race conditions have been addressed.

✅ 3 resolved
Edge Case: Client-side duplicate-name check only scans current page

📄 openmetadata-ui/src/main/resources/ui/src/pages/GlossaryTermRelationSettings/GlossaryTermRelationSettings.tsx:295-305
After pagination, relationTypes state holds only the current page instead of the full settings payload, so the validateForm duplicate check (relationTypes.some(... name.toLowerCase() === formValues.name?.toLowerCase())) can no longer detect a name that collides with a relation type on another page. The user only learns about the conflict after the create request fails server-side (the new validateUniqueNames catches it), producing a delayed/confusing error toast instead of inline validation. This is a UX regression; consider surfacing the backend duplicate error message inline on the name field, or note that inline validation is now best-effort only.

Bug: Update/delete index computed outside atomic patch snapshot

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:349-363 📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:380-394
updateGlossaryTermRelationType/deleteGlossaryTermRelationType compute relationTypeIndex from a separate getGlossaryTermRelationSettings() read, then build a JsonPatch against /relationTypes/{index} while patchGlossaryTermRelationSettings re-reads its own snapshot for the compare-and-set. If another update reorders/removes earlier elements between the two reads, the index is stale. The .test(/relationTypes/{index}/name, existing.getName()) guard (plus unique names) prevents modifying the wrong element, but a shifted or out-of-range index causes JsonUtils.applyPatch to throw an unhandled exception (HTTP 500) rather than a clean 404/409 conflict. Consider translating patch/test failures into a PreconditionFailed (409) response for a clearer client contract.

Quality: Duplicate-name UX depends on matching English error substring

📄 openmetadata-ui/src/main/resources/ui/src/pages/GlossaryTermRelationSettings/GlossaryTermRelationSettings.tsx:434-448
The catch block decides whether to surface a duplicate-name error on the name field by matching the raw backend string with errorMessage?.includes('already exists') and then displays that untranslated server message. If the backend message ever changes wording (or is localized), the inline field error silently reverts to the generic toast, and the shown text bypasses i18n. Prefer keying off a stable signal (e.g. the SYSTEM_SETTINGS_EXCEPTION error type / HTTP 400 on create) and rendering a translated message via useTranslation rather than substring-matching English prose.

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

@github-actions

github-actions Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

🔴 Playwright Results — 1 failure(s), 23 flaky

✅ 4547 passed · ❌ 1 failed · 🟡 23 flaky · ⏭️ 95 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 438 0 2 16
✅ Shard 2 11 0 0 0
🔴 Shard 3 823 1 8 8
🟡 Shard 4 820 0 3 18
🟡 Shard 5 839 0 2 5
🟡 Shard 6 787 0 1 46
🟡 Shard 7 829 0 7 2

Genuine Failures (failed on all attempts)

Features/ContextCenterArticles.spec.ts › Article edit persistence and unsaved title behavior are correct (shard 3)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoHaveValue�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator:  getByTestId('entity-header-display-name')
Expected: �[32m"CC Edit Article 20e3993f Updated"�[39m
Received: �[31m"CC Edit Article 20e3993f Updated�[7m Unsaved�[27m"�[39m
Timeout:  15000ms

Call log:
�[2m  - Expect "toHaveValue" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('entity-header-display-name')�[22m
�[2m    18 × locator resolved to <textarea rows="1" id="title-input" spellcheck="false" class="knowledge-page-title-input" data-testid="entity-header-display-name" placeholder="CC Edit Article 20e3993f Updated Unsaved">CC Edit Article 20e3993f Updated Unsaved</textarea>�[22m
�[2m       - unexpected value "CC Edit Article 20e3993f Updated Unsaved"�[22m

🟡 23 flaky test(s) (passed on retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab IS visible for supported type: metric (shard 1, 1 retry)
  • Flow/SearchRBAC.spec.ts › the browse tree only shows the asset-type categories a user can access (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Database (shard 3, 2 retries)
  • Features/BulkEditEntity.spec.ts › Table (shard 3, 2 retries)
  • Features/ContextCenterArchive.spec.ts › archive page lazy-loads more rows on scroll within its own scroll container (shard 3, 1 retry)
  • Features/ContextCenterArticles.spec.ts › Article listing search filters, clears, and shows empty state (shard 3, 1 retry)
  • Features/ContextCenterArticles.spec.ts › description: switching articles does not bleed unsaved content into next article (shard 3, 1 retry)
  • Features/ContextCenterMemories.spec.ts › adding a linked asset in edit mode shows entity badge on the row (shard 3, 1 retry)
  • Features/ContextCenterPermission.spec.ts › a document archived by a different user can be permanently deleted, via the UI, by a user with Delete permission (shard 3, 1 retry)
  • Features/DataQuality/ProfilerIngestionForm.spec.ts › switching DYNAMIC → STATIC does not leak smartSampling or thresholds (shard 3, 1 retry)
  • Features/Glossary/GlossaryP3Tests.spec.ts › should validate reference URL requires http/https prefix when creating term (shard 4, 1 retry)
  • Features/Glossary/GlossaryTermRelationsGraphNested.spec.ts › viewing a child term: parent appears as a 1-hop neighbour via parentOf edge (shard 4, 1 retry)
  • Features/Glossary/LargeGlossaryPerformance.spec.ts › should search and filter glossary terms (shard 4, 1 retry)
  • Flow/PersonaFlow.spec.ts › Set default persona for team should work properly (shard 5, 1 retry)
  • Pages/CustomProperties.spec.ts › Date (shard 5, 1 retry)
  • Pages/ExplorePageRightPanel_KnowledgeCenter.spec.ts › Should remove user owner for knowledgeCenter (shard 6, 1 retry)
  • Pages/ExplorePageRightPanel.spec.ts › Should allow Data Steward to edit glossary terms for dashboardDataModel (shard 7, 1 retry)
  • Pages/Glossary.spec.ts › Approve and reject glossary term from Glossary Listing (shard 7, 1 retry)
  • Pages/Glossary.spec.ts › Create glossary with all optional fields (tags, owners, reviewers, domain) (shard 7, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify Impact Analysis service filter selection (shard 7, 1 retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab is NOT visible for pipelineService in platform lineage (shard 7, 1 retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab is NOT visible for apiService in platform lineage (shard 7, 1 retry)
  • Pages/TestSuite.spec.ts › Logical TestSuite (shard 7, 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

@harshach harshach added the To release Will cherry-pick this PR into the release branch label Jul 18, 2026
Copilot AI review requested due to automatic review settings July 18, 2026 19:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gitar-bot

gitar-bot Bot commented Jul 18, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 3 resolved / 3 findings

Implements paginated glossary relation settings and compact CRUD endpoints to improve performance, while resolving duplicate-name validation and atomicity issues. All identified findings regarding name-clash handling and update race conditions have been addressed.

✅ 3 resolved
Edge Case: Client-side duplicate-name check only scans current page

📄 openmetadata-ui/src/main/resources/ui/src/pages/GlossaryTermRelationSettings/GlossaryTermRelationSettings.tsx:295-305
After pagination, relationTypes state holds only the current page instead of the full settings payload, so the validateForm duplicate check (relationTypes.some(... name.toLowerCase() === formValues.name?.toLowerCase())) can no longer detect a name that collides with a relation type on another page. The user only learns about the conflict after the create request fails server-side (the new validateUniqueNames catches it), producing a delayed/confusing error toast instead of inline validation. This is a UX regression; consider surfacing the backend duplicate error message inline on the name field, or note that inline validation is now best-effort only.

Bug: Update/delete index computed outside atomic patch snapshot

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:349-363 📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java:380-394
updateGlossaryTermRelationType/deleteGlossaryTermRelationType compute relationTypeIndex from a separate getGlossaryTermRelationSettings() read, then build a JsonPatch against /relationTypes/{index} while patchGlossaryTermRelationSettings re-reads its own snapshot for the compare-and-set. If another update reorders/removes earlier elements between the two reads, the index is stale. The .test(/relationTypes/{index}/name, existing.getName()) guard (plus unique names) prevents modifying the wrong element, but a shifted or out-of-range index causes JsonUtils.applyPatch to throw an unhandled exception (HTTP 500) rather than a clean 404/409 conflict. Consider translating patch/test failures into a PreconditionFailed (409) response for a clearer client contract.

Quality: Duplicate-name UX depends on matching English error substring

📄 openmetadata-ui/src/main/resources/ui/src/pages/GlossaryTermRelationSettings/GlossaryTermRelationSettings.tsx:434-448
The catch block decides whether to surface a duplicate-name error on the name field by matching the raw backend string with errorMessage?.includes('already exists') and then displays that untranslated server message. If the backend message ever changes wording (or is localized), the inline field error silently reverts to the generic toast, and the shown text bypasses i18n. Prefer keying off a stable signal (e.g. the SYSTEM_SETTINGS_EXCEPTION error type / HTTP 400 on create) and rendering a translated message via useTranslation rather than substring-matching English prose.

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

@sonarqubecloud

Copy link
Copy Markdown

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve glossary relation settings performance with pagination

2 participants