Fix #26834: Patch is failing for nested fields in Topic in main branch#26835
Fix #26834: Patch is failing for nested fields in Topic in main branch#26835
Conversation
github-actions
bot
added
backend
safe to test
![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
There was a problem hiding this comment.
Pull request overview
This PR fixes nested-field PATCH updates being skipped due to mismatched patchedFields vs. recordChange field names for entities whose nested fields are wrapped (e.g., Topic messageSchema.schemaFields, Container dataModel.columns), and adds tests to prevent regressions.
Changes:
- Update backend updaters (Topic/APIEndpoint/SearchIndex/Columns) to build
recordChangefield paths using the passed wrapper-prefixedfieldName(viaEntityUtil.getFieldName(...)prefixes). - Add backend tests: a unit test validating
shouldComparematching behavior for wrapper-prefixed field paths, and integration tests that patch Topic schema-field description/tags/glossary term. - Extend Playwright nested-column update test data to include Container with a nested struct column.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/playwright/utils/nestedColumnUpdatesUtils.ts | Adds Container support for nested-column row key extraction in Playwright utils |
| openmetadata-ui/src/main/resources/ui/playwright/support/entity/ContainerClass.ts | Adds nested struct column test data for Container schema |
| openmetadata-ui/src/main/resources/ui/playwright/constant/nestedColumnUpdates.ts | Registers Container entity for nested-children Playwright scenarios |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TopicRepository.java | Fixes schema field recordChange paths to include wrapper prefix for PATCH matching |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/APIEndpointRepository.java | Same as Topic, for request/response schema fields |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/SearchIndexRepository.java | Same as Topic, for nested search index fields |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java | Fixes column recordChange paths to include the provided wrapper prefix (e.g., dataModel.columns) |
| openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/ShouldCompareFieldNamesTest.java | Adds unit coverage ensuring wrapper-prefixed field paths match shouldCompare logic |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TopicResourceIT.java | Adds integration coverage for patching Topic schema-field description and tags |
| package org.openmetadata.service.jdbi3; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertTrue; |
There was a problem hiding this comment.
Missing standard Apache 2.0 license header at the top of this new Java test file. Other tests in this module include the header and builds typically enforce it (e.g., via license/checkstyle tooling). Add the repository’s standard license block before the package declaration.
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TopicRepository.java
Outdated
Show resolved
Hide resolved
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/APIEndpointRepository.java
Outdated
Show resolved
Hide resolved
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/SearchIndexRepository.java
Outdated
Show resolved
Hide resolved
Jest test CoverageUI tests summary
|
| updateColumnDataLength(stored, updated); | ||
| updateColumnPrecision(stored, updated); | ||
| updateColumnScale(stored, updated); | ||
| String columnPrefix = EntityUtil.getFieldName(fieldName, updated.getName()); |
There was a problem hiding this comment.
columnPrefix is built via EntityUtil.getFieldName(fieldName, updated.getName()), which does not quote names containing .. Previously EntityUtil.getColumnField(...) used FullyQualifiedName.build(...) and would generate keys like columns."a.b".description, avoiding ambiguous changeDescription paths. Please quote the column name segment (e.g., with FullyQualifiedName.quoteName(updated.getName())) when building the prefix so dotted column names are tracked correctly and PATCH field matching remains consistent.
| String columnPrefix = EntityUtil.getFieldName(fieldName, updated.getName()); | |
| String columnPrefix = | |
| EntityUtil.getFieldName(fieldName, FullyQualifiedName.quoteName(updated.getName())); |
| updateFieldDescription(stored, updated); | ||
| updateFieldDataTypeDisplay(stored, updated); | ||
| updateFieldDisplayName(stored, updated); | ||
| String schemaFieldPrefix = EntityUtil.getFieldName(fieldName, updated.getName()); |
There was a problem hiding this comment.
schemaFieldPrefix is constructed with EntityUtil.getFieldName(fieldName, updated.getName()), which won’t quote schema field names containing .. Previously EntityUtil.getSchemaField(...) used FullyQualifiedName.build(...) and would preserve dotted names as a single segment (e.g., messageSchema.schemaFields."a.b".description). Please quote updated.getName() (e.g., FullyQualifiedName.quoteName(...)) when building the prefix to avoid ambiguous changeDescription paths for dotted field names.
| String schemaFieldPrefix = EntityUtil.getFieldName(fieldName, updated.getName()); | |
| String schemaFieldPrefix = | |
| EntityUtil.getFieldName(fieldName, FullyQualifiedName.quoteName(updated.getName())); |
| updateFieldDescription(stored, updated); | ||
| updateFieldDataTypeDisplay(stored, updated); | ||
| updateFieldDisplayName(stored, updated); | ||
| String schemaFieldPrefix = EntityUtil.getFieldName(fieldName, updated.getName()); |
There was a problem hiding this comment.
schemaFieldPrefix is built using EntityUtil.getFieldName(fieldName, updated.getName()), which doesn’t quote names containing .. Since schema field names (especially for API payloads) can include dots, this can produce ambiguous changeDescription keys like responseSchema.schemaFields.user.email.description instead of ..."user.email".... Please quote updated.getName() when building the prefix (e.g., via FullyQualifiedName.quoteName(...)).
| String schemaFieldPrefix = EntityUtil.getFieldName(fieldName, updated.getName()); | |
| String schemaFieldPrefix = | |
| EntityUtil.getFieldName(fieldName, FullyQualifiedName.quoteName(updated.getName())); |
| updateFieldDescription(stored, updated); | ||
| updateFieldDataTypeDisplay(stored, updated); | ||
| updateFieldDisplayName(stored, updated); | ||
| String searchFieldPrefix = EntityUtil.getFieldName(fieldName, updated.getName()); |
There was a problem hiding this comment.
searchFieldPrefix is built via EntityUtil.getFieldName(fieldName, updated.getName()), which does not quote names containing .. Search index field names commonly include dots (e.g. user.email), and previously EntityUtil.getSearchIndexField(...) used FullyQualifiedName.build(...) to keep such names as a single segment. Please quote updated.getName() when building the prefix (e.g., FullyQualifiedName.quoteName(...)) so changeDescription keys remain unambiguous.
| String searchFieldPrefix = EntityUtil.getFieldName(fieldName, updated.getName()); | |
| String searchFieldPrefix = | |
| EntityUtil.getFieldName(fieldName, FullyQualifiedName.quoteName(updated.getName())); |
| * Tests that field names generated by entity updater helper methods (getFieldName, getSchemaField, | ||
| * getColumnField, getSearchIndexField) are compatible with the shouldCompare patchedFields matching | ||
| * logic. | ||
| * | ||
| * <p>This test exists because PR #25751 introduced shouldCompare optimization that skips field | ||
| * comparisons when the field name doesn't match the patchedFields extracted from the JSON patch | ||
| * path. Entities with wrapper objects (e.g., Topic's messageSchema wrapping schemaFields, | ||
| * Container's dataModel wrapping columns) are vulnerable to mismatches when helper methods hardcode | ||
| * the inner field name without the wrapper prefix. |
There was a problem hiding this comment.
The class Javadoc says the test validates field names generated by multiple helper methods (getSchemaField, getColumnField, getSearchIndexField), but the assertions currently only use EntityUtil.getFieldName(...). Consider either updating the Javadoc to match what’s actually being tested or adding coverage that exercises the other helper methods as well.
| * Tests that field names generated by entity updater helper methods (getFieldName, getSchemaField, | |
| * getColumnField, getSearchIndexField) are compatible with the shouldCompare patchedFields matching | |
| * logic. | |
| * | |
| * <p>This test exists because PR #25751 introduced shouldCompare optimization that skips field | |
| * comparisons when the field name doesn't match the patchedFields extracted from the JSON patch | |
| * path. Entities with wrapper objects (e.g., Topic's messageSchema wrapping schemaFields, | |
| * Container's dataModel wrapping columns) are vulnerable to mismatches when helper methods hardcode | |
| * the inner field name without the wrapper prefix. | |
| * Tests that field names generated by {@link EntityUtil#getFieldName(String...)} are compatible | |
| * with the {@code shouldCompare} patchedFields matching logic. | |
| * | |
| * <p>This test exists because PR #25751 introduced a {@code shouldCompare} optimization that | |
| * skips field comparisons when the field name doesn't match the patchedFields extracted from the | |
| * JSON patch path. Entities with wrapper objects (e.g., Topic's messageSchema wrapping | |
| * schemaFields, Container's dataModel wrapping columns) are vulnerable to mismatches when helper | |
| * methods hardcode the fully qualified field path. |
🟡 Playwright Results — all passed (12 flaky)✅ 2869 passed · ❌ 0 failed · 🟡 12 flaky · ⏭️ 175 skipped
🟡 12 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 |
Code Review 👍 Approved with suggestions 1 resolved / 2 findingsFixes nested fields patching for Topic entities by correcting the updateFieldDisplayName guard condition. Consider optimizing getCertificationClassification() to avoid repeated SettingsCache reads per entity. 💡 Performance: getCertificationClassification called repeatedly per entity📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:4589-4598 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:4494-4496 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:4717 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:6765 The method ✅ 1 resolved✅ Bug: updateFieldDisplayName guards on getDescription() instead of getDisplayName()
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
| Request request = | ||
| new Request( | ||
| "GET", String.format("/%s/_doc/%s?_source_includes=%s", TABLE_INDEX, entityId, field)); | ||
|
|
There was a problem hiding this comment.
getFieldFromDoc still hardcodes TABLE_INDEX when fetching the document, but the test now resolves entityIndexName dynamically (and uses it for updateEntityEmbedding). This can cause the test to read from a different index than the one being updated (e.g., when cluster alias changes index names). Pass the resolved index name into getFieldFromDoc and use it in the request path instead of TABLE_INDEX.
| // Generate initial embedding synchronously — no polling needed | ||
| vectorService.updateEntityEmbedding(table, entityIndexName); | ||
|
|
||
| String initialFingerprint = getFieldFromIndex(searchClient, tableId, "fingerprint"); | ||
| assertNotNull(initialFingerprint, "Initial fingerprint should exist"); | ||
| String initialFingerprint = getFieldFromDoc(searchClient, tableId, "fingerprint"); | ||
| assertNotNull(initialFingerprint, "Initial fingerprint should exist after sync embedding"); | ||
|
|
||
| // Patch description and re-generate embedding synchronously | ||
| table.setDescription("Revenue metrics for quarterly financial reporting analysis"); | ||
| client.tables().update(tableId, table); | ||
|
|
||
| Awaitility.await("Wait for embedding update after PATCH") | ||
| .atMost(Duration.ofSeconds(30)) | ||
| .pollInterval(Duration.ofSeconds(2)) | ||
| .ignoreExceptions() | ||
| .until( | ||
| () -> { | ||
| String fp = getFieldFromIndex(searchClient, tableId, "fingerprint"); | ||
| return fp != null && !fp.equals(initialFingerprint); | ||
| }); | ||
|
|
||
| String textToEmbed = getFieldFromIndex(searchClient, tableId, "textToEmbed"); | ||
| Table updated = client.tables().update(tableId, table); | ||
| vectorService.updateEntityEmbedding(updated, entityIndexName); | ||
|
|
There was a problem hiding this comment.
This test removed the Awaitility wait for the table search document to exist and now calls vectorService.updateEntityEmbedding(...) immediately after client.tables().create(...). The underlying OpenSearch implementation performs a _update without doc_as_upsert, which will fail if the entity search document hasn’t been indexed yet. This makes the test prone to flakiness/failures on slower indexing. Please restore a readiness wait (e.g., wait until the doc exists in the entity index) or change the embedding update path to upsert when the doc is missing.
|
|
Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
ShouldCompareFieldNamesTestvalidates field name patterns against patch matching logicThis will update automatically on new commits.