Skip to content

MINOR: regression in export field setter #27447

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

Merged
TeddyCr merged 2 commits into from
Apr 17, 2026
+53 −5
Merged

MINOR: regression in export field setter #27447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08adc3e
fix: regression in export field setter
TeddyCr Apr 17, 2026
3fb282c
fix: restore hydrateHistoryEntities hook after setFieldsInBulk and up…
Copilot Apr 17, 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
47 changes: 47 additions & 0 deletions ...etadata-integration-tests/src/test/java/org/openmetadata/it/tests/DatabaseResourceIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -35,13 +37,15 @@
import org.openmetadata.schema.type.Column;
import org.openmetadata.schema.type.ColumnDataType;
import org.openmetadata.schema.type.EntityHistory;
import org.openmetadata.schema.type.EntityReference;
import org.openmetadata.schema.type.api.BulkOperationResult;
import org.openmetadata.schema.type.csv.CsvImportResult;
import org.openmetadata.sdk.client.OpenMetadataClient;
import org.openmetadata.sdk.exceptions.InvalidRequestException;
import org.openmetadata.sdk.fluent.Databases;
import org.openmetadata.sdk.models.ListParams;
import org.openmetadata.sdk.models.ListResponse;
import org.openmetadata.sdk.network.HttpMethod;
import org.openmetadata.service.util.FullyQualifiedName;

/**
Expand Down Expand Up @@ -1702,4 +1706,47 @@ void testRegexListDatabase_excludeMode(TestNamespace ns) {
databases.stream().noneMatch(d -> d.getName().startsWith("temp")),
"Excluded databases should not appear in results");
}

@Test
void test_listEntityHistoryByTimestamp_returnsServiceField(TestNamespace ns) throws Exception {
OpenMetadataClient client = SdkClients.adminClient();
long startTs = System.currentTimeMillis();

CreateDatabase createRequest = createRequest(ns.prefix("history_service_field"), ns);
Database database = createEntity(createRequest);

database.setDescription("Updated for history test - " + System.currentTimeMillis());
patchEntity(database.getId().toString(), database);

long endTs = System.currentTimeMillis();
String basePath = getResourcePath() + "history";

String response =
client
.getHttpClient()
.executeForString(
HttpMethod.GET,
basePath + "?startTs=" + startTs + "&endTs=" + endTs + "&limit=10",
null);

ObjectMapper mapper = new ObjectMapper();
JsonNode result = mapper.readTree(response);
JsonNode data = result.get("data");

assertTrue(data.isArray(), "Data should be an array");
assertTrue(data.size() > 0, "Should have at least one version in the time range");

for (JsonNode entityNode : data) {
assertTrue(
entityNode.has("service") && !entityNode.get("service").isNull(),
"Each database version must include the required 'service' field, but got: "
+ entityNode);

Database deserialized = mapper.treeToValue(entityNode, Database.class);
EntityReference service = deserialized.getService();
assertNotNull(service, "Deserialized database must have a non-null service reference");
assertNotNull(service.getId(), "Service reference must have an id");
assertNotNull(service.getType(), "Service reference must have a type");
}
}
}
11 changes: 6 additions & 5 deletions 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 @@ -2227,6 +2227,7 @@ public final ResultList<T> listEntityHistoryByTimestamp(
fetchLimit);

List<T> entities = JsonUtils.readObjects(jsons, getEntityClass());
setFieldsInBulk(putFields, entities);
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
setFieldsInBulk(putFields, entities);

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

@TeddyCr TeddyCr Apr 17, 2026

Choose a reason for hiding this comment

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

@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)

Copy link
Copy Markdown
Contributor

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

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.

Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
setFieldsInBulk(putFields, entities);
setFieldsInBulk(putFields, entities);
hydrateHistoryEntities(entities);

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

@TeddyCr TeddyCr Apr 17, 2026

Choose a reason for hiding this comment

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

@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)

hydrateHistoryEntities(entities);

int total = getVersionCountCached(tableName, startTs, endTs, entityType);
Expand Down Expand Up @@ -2265,14 +2266,14 @@ public final ResultList<T> listEntityHistoryByTimestamp(
}

/**
* Hook to hydrate entities returned from {@link #listEntityHistoryByTimestamp(long, long, String,
* String, int)}.
* Hook called after {@link #setFieldsInBulk} for entities returned from {@link
* #listEntityHistoryByTimestamp(long, long, String, String, int)}.
*
* <p>Default behavior is intentionally lightweight: return the historical snapshots as stored in
* the extension table and avoid expensive relationship re-hydration for each row.
* <p>Subclasses may override to perform additional, entity-specific hydration of history
* snapshots without overriding the core field-population logic in {@code setFieldsInBulk}.
*/
protected void hydrateHistoryEntities(List<T> entities) {
// Historical snapshots are already serialized versions; avoid N+1 hydration on /history.
// No additional hydration by default.
}

private String decodeAndValidateCursor(String cursor) {
Expand Down
Loading