Skip to content

Fixes #33474: remove RdfTagUpdater's invalid inline SPARQL tag writer - #33510

Queued
fmcardoso wants to merge 7 commits into
mainfrom
fmcardoso/rdf-rdftagupdater-builds-invalid-sparql-for-tag
Queued

fmcardoso wants to merge 7 commits into
mainfrom
fmcardoso/rdf-rdftagupdater-builds-invalid-sparql-for-tag

Conversation

@fmcardoso

@fmcardoso fmcardoso commented Sep 17, 2026

Copy link
Copy Markdown
Member

Describe your changes:

Fixes #33474

Removing a tag or glossary term from an entity with RDF enabled logged org.apache.jena.query.QueryParseException from RdfTagUpdater.removeTagInline (Sentry: https://collate-3b.sentry.io/issues/7735060699/). The PATCH still succeeded because the exception was caught and logged, but the RDF sync was wrong in more ways than the parse error:

  1. Invalid IRI. RdfTagUpdater built SPARQL with String.format, concatenating the tag FQN into an IRI unescaped. A tag name with a space (or any char the SPARQL IRIREF grammar forbids) produced an invalid IRI and crashed the parser.
  2. Wrong URIs even when it parsed. It wrote tag/<fqn-as-path> and a hash-based entity/<guessed-type>/<fqn.hashCode()>, never the canonical entity/tag/{uuid} / entity/{type}/{uuid} URIs RdfPropertyMapper actually reads/writes.
  3. Wrong graph. It wrote to Fuseki's default graph instead of the named graph/knowledge graph. With tdb2:unionDefaultGraph true, these orphan triples leaked into every query that doesn't scope to GRAPH ?g.
  4. A fourth bug found while fixing this (not in the original issue): column-level orphans. JenaFusekiStorage's reconciliation DELETE (RdfOwnedResources.ownedTriplesPattern()) never wiped stale triples on column URIs, so removing a column's tag left it orphaned in RDF forever — independent of RdfTagUpdater.

None of RdfTagUpdater's inline writing was necessary: EntityRepository.postUpdate/postCreate already call RdfUpdater.updateEntity, the async snapshot writer that reloads the entity and reconciles its tag triples with canonical URIs in the right graph, for the entity and its columns. This PR removes the inline writer entirely and makes the snapshot writer the single source of truth for tag triples, then closes the few paths that had no snapshot write after them.

Type of change:

  • Bug fix

High-level design:

  • RdfTagUpdater: deleted applyTag/removeTag, applyTagInline/removeTagInline, and all their SPARQL-building helpers (fqnToUri, toTargetUri, resolveTagInfo, resolveGlossaryTermId, inferEntityType, escapeSparqlString, TagInfo). Kept the deferral scope (beginDeferral/checkpoint/drainDeferredToList/runDeferredClosures/clearDeferred) in place — EntityRepository's create-flush collector still opens/drains it, and it's now a harmless no-op (always drains an empty list). Left as a documented follow-up rather than torn out here, to keep this diff reviewable.
  • EntityRepository: removed the RDF branches from applyTags/applyTagsAdd/applyTagsDelete/applyTagsBatch (renamed from applyTagsBatchWithRdf) and the inner EntityUpdater's applyTags*InFlushAndDeferRdf methods (renamed to applyTags*InFlush, dropping the now-inaccurate suffix). Collapsed the targetType/targetId overloads, which existed only to feed the old writer and had no other callers once it's gone.
  • TagRepository / GlossaryTermRepository: added RdfUpdater.updateEntity(...) next to every existing searchRepository.updateEntity(...) in the bulk add/remove-to-assets paths (entity, column, and glossary-term-self-tag cases) — these never triggered a snapshot write at all, so RDF silently went stale on every bulk operation, most notably every bulk remove.
  • TaskWorkflowHandler: added the same call after its three tag-write fallbacks (merge/tier/suggestion) that bypass the normal patch path.
  • TableRepository.addDataModel: also bypasses EntityRepository.update() entirely (documented in its own existing comment) and never ran postUpdate. Not in the original call-site list — found because it used to rely on RdfTagUpdater and would have silently lost RDF sync once that writer was removed.
  • RdfOwnedResources: added a third ownedTriplesPattern() branch walking om:hasColumn then zero-or-more om:hasChildColumn hops (for nested/struct columns), mirroring the existing hasCustomProperty/hasExtension branches, so the reconciliation DELETE actually wipes stale column-level triples.

Alternative considered and rejected: fixing RdfTagUpdater's SPARQL in place (proper escaping, GRAPH clause, canonical URIs shared with RdfPropertyMapper). Rejected because that leaves two independent writers for the same predicates racing the async snapshot writer, with no ordering guarantee — worse to maintain than removing one of them.

Existing orphan data: deployments that already hit this bug have orphan default-graph triples. Not shipping a cleanup script in this PR — an RDF rebuild (rdf/rebuild/RdfDatasetManager) already produces a clean dataset from scratch, which is the documented remediation for already-affected deployments.

Collate: grepped openmetadata-collate (outside the nested OpenMetadata/ai-platform submodules) for RdfTagUpdater and every renamed method (applyTagsAddInFlushAndDeferRdf, applyTagsDeleteInFlushAndDeferRdf, applyTagsReplaceInFlushAndDeferRdf, applyTagsBatchWithRdf, and the collapsed 4-arg applyTags/applyTagsAdd/applyTagsDelete overloads). No references found — no coordination needed.

Tests:

Use cases covered

  • Adding/removing a classification tag or glossary term whose name has a space round-trips through the canonical entity/tag/{uuid} / entity/glossaryTerm/{uuid} URI and disappears from the union graph on removal.
  • Removing a column's tag clears the canonical column URI's hasTag triple (previously orphaned forever).
  • Bulk add/remove of a tag or glossary term to/from an asset via PUT /v1/tags/{id}/assets/{add,remove} and PUT /v1/glossaryTerms/{id}/assets/{add,remove} now syncs RDF (previously no sync at all, most notably on remove).
  • No orphan tag/<fqn> synthetic-URI triples appear in the union graph for entity or column tags.

Unit tests

  • No new unit tests added. The change is almost entirely deletion plus one-line RdfUpdater.updateEntity(...) calls wired into already-tested repository methods; per this repo's testing philosophy, the regression protection for RDF-sync behavior belongs in integration tests against the real Fuseki store, not mocks around RdfUpdater/RdfRepository.
  • Ran the full existing Rdf*Test unit suite (mvn test -pl openmetadata-service -Dtest='Rdf*Test') to confirm no regressions: 0 failures, 0 errors across ~400 tests (RdfPropertyMapperTest, RdfUpdaterTest, RdfUtilsTest, RdfOntologyContractTest, etc.).
  • Ran TaskWorkflowHandlerTest (the one unit test touching a changed file): 11/11 pass.

Backend integration tests

  • Added openmetadata-integration-tests/.../RdfTagsTierCertificationIT: 8 new tests covering the use cases above. Confirmed RED on main (special-char-tag round trip crash, bulk add/remove never syncing RDF, column-tag-removal orphan) before this fix, and GREEN after — both runs live against a Docker-backed Fuseki/Postgres/Elasticsearch stack (-DenableRdf=true).
  • Regression-checked TagResourceIT (29 tests), ColumnResourceIT (280 tests, 18 skipped), GlossaryTermResourceIT (59 tests, 1 skipped) in the same run: 0 failures, 0 errors.
  • Files added/updated: openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/RdfTagsTierCertificationIT.java

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • Not applicable (no UI changes).

Manual testing performed

  • No manual UI testing performed — this is a backend-only fix with no UI surface. Verification is the automated unit + integration test evidence above, run against a live Docker-backed stack rather than mocks.

UI screen recording / screenshots:

Not applicable — no UI changes.

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. (N/A — no schema changes)

  • For UI changes: I attached a screen recording and/or screenshots above. (N/A — no UI changes)

  • I have added tests (unit / integration / Playwright as applicable) and listed them above.

  • I have added a test that covers the exact scenario we are fixing (RdfTagsTierCertificationIT, referencing RDF: RdfTagUpdater builds invalid SPARQL for tag FQNs with special characters and writes non-canonical tag triples to the default graph #33474).

Opened as draft — final CI run and a maintainer pass over the four commits (tests / core removal / bulk-sync-gap closures / column-orphan fix) still pending before marking ready for review.

🤖 Generated with Claude Code

RetriggerConfidence Score: 5/5

The PR appears safe to merge; no outstanding correctness, security, or repository-rule issue was identified.

Summary

This PR removes the invalid inline SPARQL tag writer and makes the existing entity-snapshot reconciliation path authoritative for RDF tag state.

  • Adds explicit RDF snapshot updates to bulk tag/glossary operations, task fallbacks, and TableRepository.addDataModel.
  • Extends owned-resource reconciliation to clear stale top-level and nested column triples.
  • Adds live Fuseki integration coverage for canonical URIs, special-character names, removals, columns, and bulk operations.
  • Retains the now-empty RDF deferral scope as a documented follow-up to keep this fix focused.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Tag or glossary mutation] --> B[Persist tag_usage changes]
  B --> C[RdfUpdater.updateEntity]
  C --> D[Capture entity type and ID]
  D --> E[Post-commit live-write queue]
  E --> F[Reload authoritative entity state]
  F --> G[Delete entity-owned RDF snapshot]
  G --> H[Write canonical entity and column triples]
  H --> I[Named knowledge graph]
Loading

Reviews (3) · Last reviewed commit: "Merge branch 'main' into fmcardoso/rdf-r..."

fmcardoso and others added 4 commits September 17, 2026 15:04
Covers the round trip for tags/glossary terms with special characters,
column tag removal, orphan default-graph triples, and bulk add/remove
to assets for both tags and glossary terms. Confirmed live against
unmodified main: the bulk add/remove and column-tag-removal cases fail
today because RdfTagUpdater's inline SPARQL never syncs those paths.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RdfTagUpdater hand-built SPARQL with String.format to write/remove
om:hasTag / om:hasGlossaryTerm inline on every tag mutation. It built
invalid IRIs for tag FQNs with special characters (the crash behind
Sentry 7735060699), used non-canonical URIs even when it parsed, and
wrote to Fuseki's default graph instead of the named knowledge graph.

None of it was necessary: EntityRepository.postUpdate/postCreate
already call RdfUpdater.updateEntity, which reloads the entity and
reconciles its tag triples with canonical URIs in the right graph.
Deleted the inline writer and its SPARQL-building helpers from
RdfTagUpdater, keeping only the deferral scope the create-flush
collector still opens/drains (now a no-op; left in place rather than
torn out, see follow-up note). Dropped the corresponding RDF branches
from EntityRepository's tag-apply methods, collapsed the targetType/
targetId overloads that existed only to feed the old writer, and
renamed applyTags*InFlushAndDeferRdf / applyTagsBatchWithRdf to drop
the now-inaccurate suffixes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
With RdfTagUpdater's inline writer gone, RdfUpdater.updateEntity (the
async snapshot writer) is now the only thing that writes tag triples.
Most tag-mutation paths already trigger it via postUpdate/postCreate,
but a few never did — they only wrote to the DB and search index, so
RDF silently went stale:

- TagRepository/GlossaryTermRepository bulk add/remove-to-assets
  (including the column and glossary-term-self-tag cases): the remove
  side had no RDF sync at all, even before this change.
- TaskWorkflowHandler's three tag-write fallbacks (merge/tier/
  suggestion) that skip the normal patch path.
- TableRepository.addDataModel, which bypasses EntityRepository.update()
  entirely (documented in its own comment) and so never ran postUpdate.
  Not in the original call-site audit; found because it used to rely on
  RdfTagUpdater and would have silently lost RDF sync once that writer
  was removed.

Added RdfUpdater.updateEntity(...) next to each existing
searchRepository.updateEntity(...) call so the two indices stay in
sync going forward.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found via the columnTagRemoval_clearsCanonicalTriple IT while
verifying the RdfTagUpdater removal: JenaFusekiStorage's predicate-
scoped DELETE (buildPredicateScopedDelete) only scopes to the entity's
own URI plus whatever RdfOwnedResources.ownedTriplesPattern() matches,
which covered hasCustomProperty and hasExtension/hasExtensionProperty
owned subresources but never column URIs. emitColumns links
tableResource om:hasColumn columnResource and writes fresh om:hasTag
triples onto each column resource, so removing a column's tag never
deleted the stale triple on that column's canonical URI — it was
orphaned permanently, independent of RdfTagUpdater's bugs. Reproduced
live via both a table-nested PATCH and the dedicated
/v1/columns/name/{fqn} endpoint, ruling out a client-diffing artifact.

Added a third UNION branch to ownedTriplesPattern() mirroring the
existing two, walking om:hasColumn then zero-or-more om:hasChildColumn
hops so nested/struct columns are covered too.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@fmcardoso fmcardoso self-assigned this Sep 17, 2026
@github-actions

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!

@fmcardoso
fmcardoso marked this pull request as ready for review September 17, 2026 14:00
@fmcardoso
fmcardoso requested a review from a team as a code owner September 17, 2026 14:00
@github-actions

github-actions Bot commented Sep 17, 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

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!

@fmcardoso fmcardoso added the safe to test Add this label to run secure Github workflows on PRs label Sep 17, 2026
@fmcardoso fmcardoso moved this to In Progress 🏗️ in Shipping Sep 17, 2026
Per PR review: awaitAsk/awaitAskFalse caught the broad Exception type
instead of the specific one, violating java.md's error-handling rule.
Awaitility.await().until(...) only ever throws
ConditionTimeoutException here (the polled condition itself never
throws — RdfTestUtils.executeSparqlAsk swallows its own exceptions and
returns false), so narrow the catch to that.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit a3da9ad823d55aebb76fd28c7bfe9c6b4528f1a2 in Playwright run 35359276746, attempt 1.

✅ 4490 passed · ❌ 0 failed · 🟡 6 flaky · ⏭️ 1 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) 59m 48s

⏱️ Max setup 4m 16s · max shard execution 21m 40s · max shard-job elapsed before upload 24m 59s · reporting 19s

🌐 221.45 requests/attempt · 2.23 app boots/UI scenario · 42.51% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 42.51% (convergence target: at most 15%).
  • Browser traffic was 221.45 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.23 per UI scenario (10675 boots / 4784 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard advanced-search-01 130 0 0 0 0 0
✅ Shard chromium-01 169 0 0 0 0 0
✅ Shard chromium-02 162 0 0 0 0 0
✅ Shard chromium-03 177 0 0 0 0 0
✅ Shard chromium-04 149 0 0 0 0 0
✅ Shard chromium-05 194 0 0 0 0 0
✅ Shard chromium-06 158 0 0 0 0 0
✅ Shard chromium-07 159 0 0 0 0 0
✅ Shard chromium-08 139 0 0 0 0 0
✅ Shard chromium-09 143 0 0 0 0 0
✅ Shard chromium-10 153 0 0 0 0 0
🟡 Shard chromium-11 155 0 1 0 0 0
🟡 Shard chromium-12 161 0 1 0 0 0
✅ Shard chromium-13 187 0 0 0 0 0
✅ Shard chromium-14 164 0 0 0 0 0
🟡 Shard chromium-15 191 0 1 0 0 0
✅ Shard chromium-16 150 0 0 0 0 0
✅ Shard chromium-17 155 0 0 1 0 0
✅ Shard chromium-18 154 0 0 0 0 0
✅ Shard chromium-19 146 0 0 0 0 0
✅ Shard chromium-20 161 0 0 0 0 0
🟡 Shard chromium-21 182 0 3 0 0 0
✅ Shard chromium-22 172 0 0 0 0 0
✅ Shard chromium-23 161 0 0 0 0 0
✅ Shard chromium-24 186 0 0 0 0 0
✅ Shard data-asset-rules-01 65 0 0 0 0 0
✅ Shard domain-isolation-01 16 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard import-export-01 114 0 0 0 0 0
✅ Shard import-export-02 36 0 0 0 0 0
✅ Shard ingestion-01 43 0 0 0 0 0
✅ Shard ingestion-02 55 0 0 0 0 0
✅ Shard reindex-01 28 0 0 0 0 0
✅ Shard search-01 12 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0
🟡 6 flaky test(s) (passed on retry)
  • Features/CustomizeDetailPage.spec.tsAPI Collection - customization should work (shard chromium-11, 1 retry)
  • Pages/ExplorePageRightPanel.spec.tsShould display and verify schema fields for pipeline (shard chromium-12, 1 retry)
  • Pages/DataContractsSemanticRules.spec.tsValidate Description Rule Is_Not_Set (shard chromium-15, 1 retry)
  • Features/UserProfileOnlineStatus.spec.tsShould show online status badge on user profile for active users (shard chromium-21, 1 retry)
  • Pages/DataProductCertificationFilter.spec.tsglossary term option keeps its original casing while the filter value stays lowercased (shard chromium-21, 1 retry)
  • Pages/ExplorePageRightPanel_KnowledgeCenter.spec.tsShould remove user owner for knowledgeCenter (shard chromium-21, 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

@gitar-bot

gitar-bot Bot commented Sep 18, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

🟡 Medium risk

Removes the invalid inline SPARQL tag writer from RdfTagUpdater and makes the queued entity-snapshot reconciliation path the single source of truth for RDF tag state, fixing crashes on special-character tag names, wrong URIs, wrong graph placement, and orphaned column-level triples. Adds explicit RDF snapshot updates to bulk tag/glossary operations, task fallbacks, and the table data-model path; extends owned-resource reconciliation to clear stale column triples; and includes Fuseki-backed integration tests covering canonical tag URIs, removals, bulk operations, and column cleanup. No issues found.

Review coverage

Rules No rules evaluated

Functional validation Not enabled · Set up

Auto-approval Not enabled · Set up

Options

Display: compact → Counting what did not apply, without listing it.

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

Compact
gitar display:verbose         

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

@fmcardoso
fmcardoso added this pull request to the merge queue Sep 18, 2026
Any commits made after this event will not be merged.
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

Projects

Status: In Progress 🏗️

Development

Successfully merging this pull request may close these issues.

RDF: RdfTagUpdater builds invalid SPARQL for tag FQNs with special characters and writes non-canonical tag triples to the default graph

2 participants