MINOR: regression in export field setter#27447
Conversation
github-actions
Bot
added
Ingestion
safe to test
There was a problem hiding this comment.
Pull request overview
Fixes a regression in the entity history-by-timestamp flow where returned historical entities were missing expected fields (e.g., service) by re-applying field population logic after deserializing history snapshots.
Changes:
- Update
listEntityHistoryByTimestamp(...)to callsetFieldsInBulk(...)on deserialized history entities. - Add an integration test to ensure the
/historyresponse for Databases includes a non-nullservicefield across returned versions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java | Re-hydrates fields for entities returned by the history-by-timestamp query. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DatabaseResourceIT.java | Adds coverage to assert /databases/.../history returns versions that include service. |
|
|
||
| List<T> entities = JsonUtils.readObjects(jsons, getEntityClass()); | ||
| hydrateHistoryEntities(entities); | ||
| setFieldsInBulk(putFields, entities); |
There was a problem hiding this comment.
Calling setFieldsInBulk(putFields, entities) here will re-hydrate relationship-backed fields from the current DB state (owners/tags/domains/etc.) and can overwrite the historical snapshot data returned by the /history-by-timestamp query. It also increases the cost of the endpoint significantly compared to returning the stored snapshots.
Consider switching to a minimal history hydration strategy (e.g., only populate required default references like service when missing, without fetching other relationship fields), or restoring the previous lightweight hook-based approach so that /history continues to reflect the stored version snapshots.
| setFieldsInBulk(putFields, entities); |
There was a problem hiding this comment.
@copilot hydrateHistoryEntities is a no-op operations and entities come without their fields. Serialization of the json then fails (i.e. Database entities would come without the service field)
There was a problem hiding this comment.
Agreed — setFieldsInBulk is essential since the deserialized history snapshots don't carry populated relationship fields (e.g. service). In commit 3fb282c I restored the hydrateHistoryEntities(entities) call after setFieldsInBulk (as the reviewer suggested) and updated the Javadoc so the hook is clearly positioned as an extension point for subclasses needing additional hydration on top of the core setFieldsInBulk pass.
|
|
||
| List<T> entities = JsonUtils.readObjects(jsons, getEntityClass()); | ||
| hydrateHistoryEntities(entities); | ||
| setFieldsInBulk(putFields, entities); |
There was a problem hiding this comment.
This change removes the only call site of hydrateHistoryEntities(...), leaving the method (and its Javadoc describing intentionally lightweight history hydration) effectively dead/inaccurate. Either call the hook again (possibly after deserialization and before pagination cursor computation) or remove/update the hook and documentation to match the new behavior.
| setFieldsInBulk(putFields, entities); | |
| setFieldsInBulk(putFields, entities); | |
| hydrateHistoryEntities(entities); |
There was a problem hiding this comment.
@copilot hydrateHistoryEntities is a no-op operations and entities come without their fields. Serialization of the json then fails (i.e. Database entities would come without the service field)
🔴 Playwright Results — 3 failure(s), 22 flaky✅ 3634 passed · ❌ 3 failed · 🟡 22 flaky · ⏭️ 111 skipped
Genuine Failures (failed on all attempts)❌
|
…date javadoc Agent-Logs-Url: https://github.com/open-metadata/OpenMetadata/sessions/6541c953-9da7-4b9b-9bca-3d3a4792516b Co-authored-by: TeddyCr <13626425+TeddyCr@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. 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 ✅ ApprovedRestores the export field setter functionality to resolve the regression. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
|
Failed to cherry-pick changes to the 1.12.6 branch. |
Describe your changes:
Fix regression introduced in entity history where fields are not set anymore
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>