fix(glossary): include cardinality on default term relation types - #28376
sonika-shah merged 6 commits into
Conversation
github-actions
Bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes an inconsistency in the System Settings API where system-seeded glossary term relation types were returned without a cardinality field (while custom relation types included it). The change ensures defaults always include cardinality and backfills it on reads for existing installations seeded before this fix.
Changes:
- Seed default glossary term relation types with an explicit
RelationCardinality(currentlyMANY_TO_MANY) during initial settings creation. - Backfill missing
cardinalityon GET/v1/system/settings/glossaryTermRelationSettingswithout mutatingsourceMax/targetMax. - Extend the integration test to assert
cardinalityis present and non-null for all returned relation types.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java | Backfills missing cardinality in glossary term relation settings on read via getSettingByName. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/settings/SettingsCache.java | Ensures system-seeded default relation types are created with cardinality populated. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermRelationSettingsIT.java | Adds assertions that cardinality exists and is non-null in the settings GET response. |
The system-seeded glossary term relation types (relatedTo, synonym, broader, narrower, partOf, hasPart, etc.) were created without a cardinality value, so the GET /v1/system/settings/glossaryTermRelationSettings response omitted the field for defaults while returning it for custom relations. The UI fell back to MANY_TO_MANY for every default and, on the next PUT, wrote that fallback back into the system relations - overriding any intended value. Set the expected cardinality on each default in SettingsCache and add a 2.0.2 data migration that backfills the field for installs that were already seeded without it. broader/narrower follow strict SKOS semantics (ONE_TO_MANY / MANY_TO_ONE - a term has at most one broader parent); all other system relations stay MANY_TO_MANY.
7446c7e to
60f2219
Compare
|
🟡 Playwright Results — all passed (14 flaky)✅ 4241 passed · ❌ 0 failed · 🟡 14 flaky · ⏭️ 87 skipped
🟡 14 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
This PR has had no activity for 30 days and will be closed in 7 days if no further activity occurs. |
|
Closing due to inactivity. Reopen anytime to continue. |
The glossary cardinality backfill bound the settings payload as a string into openmetadata_settings.json, which is jsonb on Postgres - Postgres refuses the implicit varchar->jsonb cast and the whole migration (and therefore TestSuiteBootstrap / every IT in the lane) aborted. Select the DB-specific UPDATE the way PiiRecognizerMigrationUtil / v1126 do: plain :json for MySQL, :json::jsonb for Postgres.
github-actions
Bot
added
safe to test
…MANY Resolves the enum/bounds inversion flagged in review by not imposing any per-relation cardinality on the system defaults - the original bug was only that GET returned a null cardinality, not that hierarchy needed enforcing. - SettingsCache: broader/narrower now MANY_TO_MANY with null bounds, matching every other system default. GET returns a non-null cardinality and the UI no longer falls back to (and persists) a guessed value - with zero new enforcement, so existing terms with multiple broader parents keep working. - v203 migration: backfill null cardinality on system-defined relations to MANY_TO_MANY and derive bounds via the canonical normalize(), deleting the hand-written (inverted) bounds switch and per-relation map. - IT: assert exact cardinality (MANY_TO_MANY) and null sourceMax/targetMax for all 10 system relations, so an inverted or bounded value fails the test.
Code Review ✅ Approved 4 closed / 4 findingsAdds explicit cardinality metadata to system-defined glossary relation types and introduces a 2.0.3 migration to backfill missing cardinalities on existing installations. Resolved cardinality bounds inversions for broader/narrower relations, applied backfill to the list endpoint, and improved code quality with final fields and immutable collections. ✅ 4 closed✅ Bug: backfillMissingCardinality not applied in list endpoint
✅ Quality: MigrationUtil.handle field should be final
✅ Quality: Use Map.of() for immutable SYSTEM_DEFAULT_CARDINALITIES
✅ Bug: Migration inverts ONE_TO_MANY/MANY_TO_ONE cardinality bounds
OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
|
🤖 Backport to |
|
🤖 Backport to |
|
🤖 Backport to |
|
🤖 Backport to |
Summary
Fixes collate#4212:
GET /v1/system/settings/glossaryTermRelationSettingsreturned the system-seeded relation types withcardinality: null. The UI fell back toMANY_TO_MANYand, on the next PUT, persisted that fallback onto the system relations.Root cause
SettingsCache.createRelationTypeseeded each system default without a cardinality, so between server boot and the first settings PUT the defaults shipped withcardinality == null. The PUT path runsGlossaryTermRelationSettingsUtil.normalize(...), but GET had no equivalent, so the null leaked to the client.Fix
Seed and backfill every system-defined relation with an explicit
cardinalityso GET always returns a non-null value and the UI never has to guess. All ten system relations are set toMANY_TO_MANYwith nosourceMax/targetMaxbounds — unbounded, exactly matching the behavior installs have today. This intentionally adds no cardinality enforcement, so existing terms that already have (for example) multiplebroaderparents keep working and there is no upgrade risk.On hierarchy semantics: conceptually
broaderisMANY_TO_ONE(a term has one broader parent; a parent has many children) andnarroweris itsONE_TO_MANYinverse. We deliberately do not encode those as enforced cardinalities here — turning on asourceMax=1cap in a patch release would start rejecting existing terms that have multiple parents on their next edit. Enforcing single-parent hierarchy is a separate, opt-in change that needs a data-cleanup migration, and is out of scope for this fix.SettingsCache.java— every system default is now created withMANY_TO_MANYand null bounds.2.0.3data migration (migration/utils/v203/MigrationUtil+mysql|postgres/v203/Migration) — backfillsMANY_TO_MANYon system-defined relations whosecardinalityis still null on existing installs, deriving bounds through the canonicalGlossaryTermRelationSettingsUtil.normalize()(single source of truth, so enum and bounds cannot disagree). Custom, user-defined relations are left untouched.GlossaryTermRelationSettingsIT.test_systemRelationTypesAreUnboundedManyToMany— asserts the exact cardinality (MANY_TO_MANY) and nullsourceMax/targetMaxfor all ten system relations, so a wrong or bounded value fails the test.Test plan
mvn clean install -pl openmetadata-service,openmetadata-integration-tests -am -DskipTests— BUILD SUCCESS.maven-collate-cipass, including the new IT.MANY_TO_MANY,sourceMax=null,targetMax=null.MANY_TO_MANYwith null bounds.