Fix: bypass shouldCompare for Topic schema field updates#26735
Fix: bypass shouldCompare for Topic schema field updates#26735Khairajani wants to merge 9 commits intomainfrom
Conversation
Khairajani
added
the
safe to test
There was a problem hiding this comment.
Pull request overview
This PR addresses a PATCH change-tracking gap where updates to a Topic’s nested messageSchema.schemaFields were being skipped because EntityUpdater.shouldCompare only considers top-level patched fields.
Changes:
- Temporarily bypasses
shouldComparefiltering while updating TopicmessageSchemaschema fields so nested field updates are not silently ignored. - Exposes
patchedFieldsvia a generated getter onEntityRepository.EntityUpdaterto allow saving/restoring the patched-field set.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TopicRepository.java | Temporarily disables patched-field filtering around updateSchemaFields for messageSchema updates. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java | Adds a Lombok getter for patchedFields in EntityUpdater (previously setter-only). |
| private ChangeSource changeSource; | ||
| private final boolean useOptimisticLocking; | ||
| @Setter private Set<String> patchedFields; | ||
| @Getter @Setter private Set<String> patchedFields; |
There was a problem hiding this comment.
EntityUpdater is a public inner class, so adding a Lombok @Getter makes getPatchedFields() a new public API and exposes the mutable Set directly. To avoid widening the surface area and accidental external mutation, consider generating a protected (or package-private) getter, or returning an unmodifiable copy, and keep the setter visibility as narrow as possible.
There was a problem hiding this comment.
All three concerns from the original review have been addressed in commits 7006505 and d871673:
EntityRepository.java–@Getterremoved; replaced with a hand-writtenprotectedgetter that returnsSet.copyOf(patchedFields), preventing external mutation and keeping the accessor non-public.TopicRepository.java(logic) – Instead of nullingpatchedFields, the fix now temporarily extends the set with"schemaFields", so only schema field tracking is unblocked without disablingshouldComparefor all other fields.- Comment – Updated to name
EntityUtil.getSchemaField(...)as the precise source of the path mismatch, with aTODOpointing to the long-term proper fix. - Bonus – Fixed a stray fully-qualified
new java.util.HashSet<>()to use the already-importedHashSet(d871673).
| // Temporarily disable shouldCompare for nested schema field updates. | ||
| // The field names from getSchemaField() start with "schemaFields." which | ||
| // doesn't match the patchedField "messageSchema", causing recordChange | ||
| // to silently skip the update. | ||
| Set<String> saved = getPatchedFields(); | ||
| setPatchedFields(null); | ||
| try { | ||
| updateSchemaFields( | ||
| "messageSchema.schemaFields", | ||
| original.getMessageSchema() == null | ||
| ? new ArrayList<>() | ||
| : listOrEmpty(original.getMessageSchema().getSchemaFields()), | ||
| listOrEmpty(updated.getMessageSchema().getSchemaFields()), | ||
| EntityUtil.schemaFieldMatch); | ||
| } finally { | ||
| setPatchedFields(saved); | ||
| } |
There was a problem hiding this comment.
Setting patchedFields to null temporarily disables all shouldCompare filtering inside updateSchemaFields, not just the nested schema-field change tracking. This can cause PATCH requests that touch messageSchema (e.g., schemaText/schemaType) to also record/propagate changes from schema fields that were not intended to be part of the patch. Consider a narrower approach: temporarily extend the patched field set to include the schemaFields prefix (or make EntityUtil.getSchemaField(...) return paths under messageSchema.schemaFields) so only schema-field tracking is unblocked without disabling filtering globally.
| // The field names from getSchemaField() start with "schemaFields." which | ||
| // doesn't match the patchedField "messageSchema", causing recordChange | ||
| // to silently skip the update. |
There was a problem hiding this comment.
The new comment says the field names come from getSchemaField(), but the mismatch described here is specifically from EntityUtil.getSchemaField(...) producing paths starting with schemaFields. while the patched top-level field is messageSchema. Please update the comment to point to the correct helper/mismatch (and ideally add a TODO/link to a tracking issue since this is labeled “temporary”).
| // The field names from getSchemaField() start with "schemaFields." which | |
| // doesn't match the patchedField "messageSchema", causing recordChange | |
| // to silently skip the update. | |
| // EntityUtil.getSchemaField(...) (used by schemaFieldMatch) produces field | |
| // paths starting with "schemaFields.", which do not match the patchedField | |
| // "messageSchema", causing recordChange() to silently skip the update. | |
| // TODO: Remove this workaround once schema field paths are aligned with | |
| // messageSchema patching. |
OpenMetadata Service New-Code Coverage✅ PASS. Required changed-line coverage:
Only changed executable lines under |
🟡 Playwright Results — all passed (14 flaky)✅ 3417 passed · ❌ 0 failed · 🟡 14 flaky · ⏭️ 209 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 |
- hand-written protected getter - extend with "schemaFields". shouldCompare still filters everything else - Updated comment
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TopicRepository.java
Outdated
Show resolved
Hide resolved
|
@copilot Review this PR again |
Co-authored-by: yan-3005 <81347100+yan-3005@users.noreply.github.com> Agent-Logs-Url: https://github.com/open-metadata/OpenMetadata/sessions/5a135efc-82c7-4e78-8a52-032f0216eec6
Re-reviewed — all three concerns from the automated reviewer have been addressed:
Automated code review and security scan both pass cleanly. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Code Review ✅ Approved 1 resolved / 1 findingsBypasses shouldCompare filtering for Topic schema field updates to prevent silent data loss during PUT operations, addressing the regression where schema field list and tag changes were dropped. ✅ 1 resolved✅ Bug: PUT operations regress: schema field list/tag changes silently dropped
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
This pull request introduces regression tests and a targeted fix to ensure that PATCH requests correctly update nested schema fields in a
Topic's message schema. Previously, an optimization in theEntityUpdatercaused updates to nested fields to be silently ignored when using JSON Patch, due to a mismatch between the patched field name and the schema field prefix. The changes include both new regression tests and a workaround in the update logic to guarantee correct behavior.Testing improvements:
TopicUpdaterPatchTest.javato verify PATCH operations on both top-level and nested schema fields, ensuring that descriptions and display names are updated and persisted correctly, that patchedFields are correctly restored after the update, and that PUT operations (patchedFields=null) continue to work without filtering.Bug fix for schema field PATCH operations:
TopicRepository.java, temporarily extends thepatchedFieldsset to include"schemaFields"before callingupdateSchemaFieldsformessageSchema, then restores the original set afterwards. This is a narrower fix than nullingpatchedFields: it unblocks only schema field change tracking without disablingshouldComparefiltering for all other fields. ATODOcomment documents the proper long-term fix (aligningEntityUtil.getSchemaField(...)paths undermessageSchema.schemaFields).Supporting code changes:
@Getterannotation onpatchedFieldsinEntityUpdaterwith a hand-writtenprotectedgetter that returnsSet.copyOf(patchedFields), preventing exposure of a mutableSetthrough a public API while still allowing subclass access.Type of change:
Checklist:
Fixes <issue-number>: <short explanation>💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.