Skip to content

Fix: bypass shouldCompare for Topic schema field updates #26735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Khairajani wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix: bypass shouldCompare for Topic schema field updates #26735

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e60de22
Fix: bypass shouldCompare for Topic schema field updates
Khairajani Mar 24, 2026
05ec2fe
test-case for topic children patching
Khairajani Mar 24, 2026
999a81f
topic patch UT
Khairajani Mar 25, 2026
3f47d29
topic patch UT
Khairajani Mar 25, 2026
b99710b
Merge branch 'main' into topic_patch_fix
Khairajani Mar 25, 2026
7006505
Copilot Review
Khairajani Mar 25, 2026
aad62f9
Merge branch 'main' into topic_patch_fix
Khairajani Mar 25, 2026
b0d18e2
bug fix : only non null goes to saved
Khairajani Mar 25, 2026
d871673
fix: use unqualified HashSet import in TopicRepository
Copilot Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6033,7 +6033,7 @@ public class EntityUpdater {
@Getter protected ChangeDescription incrementalChangeDescription = null;
private ChangeSource changeSource;
private final boolean useOptimisticLocking;
@Setter private Set<String> patchedFields;
@Getter @Setter private Set<String> patchedFields;
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@Khairajani Khairajani Mar 25, 2026

Choose a reason for hiding this comment

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

@copilot can you review again

Copy link
Copy Markdown
Contributor

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

All three concerns from the original review have been addressed in commits 7006505 and d871673:

  1. EntityRepository.java – @Getter removed; replaced with a hand-written protected getter that returns Set.copyOf(patchedFields), preventing external mutation and keeping the accessor non-public.
  2. TopicRepository.java (logic) – Instead of nulling patchedFields, the fix now temporarily extends the set with "schemaFields", so only schema field tracking is unblocked without disabling shouldCompare for all other fields.
  3. Comment – Updated to name EntityUtil.getSchemaField(...) as the precise source of the path mismatch, with a TODO pointing to the long-term proper fix.
  4. Bonus – Fixed a stray fully-qualified new java.util.HashSet<>() to use the already-imported HashSet (d871673).

private final List<Runnable> deferredReactOperations = new ArrayList<>();
private boolean deferredReactExecuted;

Expand Down
24 changes: 17 additions & 7 deletions openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TopicRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,13 +662,23 @@ public void entitySpecificUpdate(boolean consolidatingChanges) {
? null
: original.getMessageSchema().getSchemaType(),
updated.getMessageSchema().getSchemaType());
updateSchemaFields(
"messageSchema.schemaFields",
original.getMessageSchema() == null
? new ArrayList<>()
: listOrEmpty(original.getMessageSchema().getSchemaFields()),
listOrEmpty(updated.getMessageSchema().getSchemaFields()),
EntityUtil.schemaFieldMatch);
// 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.
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

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”).

Suggested change
// 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.

Copilot uses AI. Check for mistakes.
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);
}
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
});
compareAndUpdate(
Expand Down
Loading