Fix soft-delete and restore handling for charts linked to dashboards during dashboard delete/restore operations (#22168)

* Fix soft-delete and restore handling for charts linked to dashboards during dashboard delete/restore operations

* remove the search script no longer required

* resolve conflict
This commit is contained in:
sonika-shah 2025-07-15 17:24:33 +05:30 committed by GitHub
parent f93e6758a6
commit 0cab4d1cbf
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 390 additions and 40 deletions

View File

@ -589,7 +589,7 @@ public final class Entity {
public static void restoreEntity(String updatedBy, String entityType, UUID entityId) {
EntityRepository<?> dao = getEntityRepository(entityType);
dao.restoreEntity(updatedBy, entityType, entityId);
dao.restoreEntity(updatedBy, entityId);
}
public static <T> String getEntityTypeFromClass(Class<T> clz) {

View File

@ -16,6 +16,7 @@ package org.openmetadata.service.jdbi3;
import static org.openmetadata.common.utils.CommonUtil.listOrEmpty;
import static org.openmetadata.schema.type.Include.ALL;
import static org.openmetadata.schema.type.Include.NON_DELETED;
import static org.openmetadata.service.Entity.CHART;
import static org.openmetadata.service.Entity.DASHBOARD;
import static org.openmetadata.service.Entity.FIELD_DESCRIPTION;
import static org.openmetadata.service.Entity.FIELD_TAGS;
@ -25,7 +26,10 @@ import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
import org.jdbi.v3.sqlobject.transaction.Transaction;
import org.openmetadata.schema.EntityInterface;
import org.openmetadata.schema.entity.data.Chart;
@ -46,6 +50,7 @@ import org.openmetadata.service.util.EntityUtil;
import org.openmetadata.service.util.EntityUtil.Fields;
import org.openmetadata.service.util.FullyQualifiedName;
@Slf4j
public class DashboardRepository extends EntityRepository<Dashboard> {
private static final String DASHBOARD_UPDATE_FIELDS = "charts,dataModels";
private static final String DASHBOARD_PATCH_FIELDS = "charts,dataModels";
@ -197,6 +202,144 @@ public class DashboardRepository extends EntityRepository<Dashboard> {
fields.contains("usageSummary") ? dashboard.getUsageSummary() : null);
}
// Override soft delete behavior to handle charts through HAS relation.
@Transaction
@Override
protected void deleteChildren(
UUID dashboardId, boolean recursive, boolean hardDelete, String updatedBy) {
super.deleteChildren(dashboardId, recursive, hardDelete, updatedBy);
// Load all charts linked to this dashboard
List<CollectionDAO.EntityRelationshipRecord> chartRecords =
daoCollection
.relationshipDAO()
.findTo(dashboardId, DASHBOARD, Relationship.HAS.ordinal(), CHART);
if (chartRecords.isEmpty()) {
return;
}
// Batch-load dashboard relationships for these charts
List<CollectionDAO.EntityRelationshipObject> dashboardRelationships =
daoCollection
.relationshipDAO()
.findFromBatch(
chartRecords.stream()
.map(record -> record.getId().toString())
.distinct()
.collect(Collectors.toList()),
Relationship.HAS.ordinal(),
DASHBOARD);
Set<UUID> nonDeletedDashboards =
daoCollection
.dashboardDAO()
.findEntitiesByIds(
dashboardRelationships.stream()
.map(rel -> UUID.fromString(rel.getFromId()))
.distinct()
.collect(Collectors.toList()),
Include.NON_DELETED)
.stream()
.map(Dashboard::getId)
.filter(id -> !id.equals(dashboardId)) // (excluding the current dashboard
.collect(Collectors.toSet());
// For deletion: get charts whose linked dashboards (excluding the current dashboard)
// have no other nondeleted dashboards.
List<CollectionDAO.EntityRelationshipRecord> filteredChartRecordsToBeDeleted =
new ArrayList<>();
for (CollectionDAO.EntityRelationshipRecord record : chartRecords) {
UUID chartId = record.getId();
boolean hasOtherNonDeletedDashboard = false;
for (CollectionDAO.EntityRelationshipObject rel : dashboardRelationships) {
UUID relFromId = UUID.fromString(rel.getFromId());
UUID relToId = UUID.fromString(rel.getToId());
if (relToId.equals(chartId) && nonDeletedDashboards.contains(relFromId)) {
hasOtherNonDeletedDashboard = true;
break;
}
}
if (!hasOtherNonDeletedDashboard) {
filteredChartRecordsToBeDeleted.add(record);
}
}
deleteChildren(filteredChartRecordsToBeDeleted, hardDelete, updatedBy);
}
// Override restore behavior to handle charts through HAS relation.
@Transaction
@Override
protected void restoreChildren(UUID dashboardId, String updatedBy) {
super.restoreChildren(dashboardId, updatedBy);
// Load all charts linked to this dashboard
List<CollectionDAO.EntityRelationshipRecord> chartRecords =
daoCollection
.relationshipDAO()
.findTo(dashboardId, DASHBOARD, Relationship.HAS.ordinal(), CHART);
if (chartRecords.isEmpty()) {
return;
}
// Batch-load dashboard relationships for these charts
List<CollectionDAO.EntityRelationshipObject> dashboardRelationships =
daoCollection
.relationshipDAO()
.findFromBatch(
chartRecords.stream()
.map(record -> record.getId().toString())
.distinct()
.collect(Collectors.toList()),
Relationship.HAS.ordinal(),
DASHBOARD);
Set<UUID> deletedDashboards =
daoCollection
.dashboardDAO()
.findEntitiesByIds(
dashboardRelationships.stream()
.map(rel -> UUID.fromString(rel.getFromId()))
.distinct()
.collect(Collectors.toList()),
Include.DELETED)
.stream()
.map(Dashboard::getId)
.filter(id -> !id.equals(dashboardId)) // (excluding the current dashboard
.collect(Collectors.toSet());
// For restore: get charts whose linked dashboards (excluding the current dashboard)
// are all nondeleted.
List<CollectionDAO.EntityRelationshipRecord> filteredChartRecordsToBeRestored =
new ArrayList<>();
for (CollectionDAO.EntityRelationshipRecord chartRecord : chartRecords) {
UUID chartId = chartRecord.getId();
boolean hasOtherDeletedDashboard = false;
for (CollectionDAO.EntityRelationshipObject relationship : dashboardRelationships) {
UUID relFromId = UUID.fromString(relationship.getFromId());
UUID relToId = UUID.fromString(relationship.getToId());
if (relToId.equals(chartId) && deletedDashboards.contains(relFromId)) {
hasOtherDeletedDashboard = true;
break;
}
}
if (!hasOtherDeletedDashboard) {
filteredChartRecordsToBeRestored.add(chartRecord);
}
}
for (CollectionDAO.EntityRelationshipRecord record : filteredChartRecordsToBeRestored) {
LOG.info("Recursively restoring {} {}", record.getType(), record.getId());
Entity.restoreEntity(updatedBy, record.getType(), record.getId());
}
}
@Override
public void restorePatchAttributes(Dashboard original, Dashboard updated) {
// Patch can't make changes to following fields. Ignore the changes

View File

@ -1172,7 +1172,7 @@ public abstract class EntityRepository<T extends EntityInterface> {
updated.setUpdatedAt(System.currentTimeMillis());
// If the entity state is soft-deleted, recursively undelete the entity and it's children
if (Boolean.TRUE.equals(original.getDeleted())) {
restoreEntity(updated.getUpdatedBy(), entityType, original.getId());
restoreEntity(updated.getUpdatedBy(), original.getId());
}
// Update the attributes and relationships of an entity
@ -1196,7 +1196,7 @@ public abstract class EntityRepository<T extends EntityInterface> {
updated.setUpdatedAt(System.currentTimeMillis());
// If the entity state is soft-deleted, recursively undelete the entity and it's children
if (Boolean.TRUE.equals(original.getDeleted())) {
restoreEntity(updated.getUpdatedBy(), entityType, original.getId());
restoreEntity(updated.getUpdatedBy(), original.getId());
}
// Update the attributes and relationships of an entity
@ -1571,7 +1571,7 @@ public abstract class EntityRepository<T extends EntityInterface> {
}
@Transaction
private void deleteChildren(UUID id, boolean recursive, boolean hardDelete, String updatedBy) {
protected void deleteChildren(UUID id, boolean recursive, boolean hardDelete, String updatedBy) {
// If an entity being deleted contains other **non-deleted** children entities, it can't be
// deleted
List<EntityRelationshipRecord> childrenRecords =
@ -2364,22 +2364,9 @@ public abstract class EntityRepository<T extends EntityInterface> {
}
@Transaction
public final PutResponse<T> restoreEntity(String updatedBy, String entityType, UUID id) {
public final PutResponse<T> restoreEntity(String updatedBy, UUID id) {
// If an entity being restored contains other **deleted** children entities, restore them
List<EntityRelationshipRecord> records =
daoCollection.relationshipDAO().findTo(id, entityType, Relationship.CONTAINS.ordinal());
if (!records.isEmpty()) {
// Restore all the contained entities
for (EntityRelationshipRecord entityRelationshipRecord : records) {
LOG.info(
"Recursively restoring {} {}",
entityRelationshipRecord.getType(),
entityRelationshipRecord.getId());
Entity.restoreEntity(
updatedBy, entityRelationshipRecord.getType(), entityRelationshipRecord.getId());
}
}
restoreChildren(id, updatedBy);
// Finally set entity deleted flag to false
LOG.info("Restoring the {} {}", entityType, id);
@ -2399,6 +2386,20 @@ public abstract class EntityRepository<T extends EntityInterface> {
}
}
@Transaction
protected void restoreChildren(UUID id, String updatedBy) {
// Restore deleted children entities
List<CollectionDAO.EntityRelationshipRecord> records =
daoCollection.relationshipDAO().findTo(id, entityType, Relationship.CONTAINS.ordinal());
if (!records.isEmpty()) {
// Recursively restore all contained entities
for (CollectionDAO.EntityRelationshipRecord record : records) {
LOG.info("Recursively restoring {} {}", record.getType(), record.getId());
Entity.restoreEntity(updatedBy, record.getType(), record.getId());
}
}
}
public final void addRelationship(
UUID fromId, UUID toId, String fromEntity, String toEntity, Relationship relationship) {
addRelationship(fromId, toId, fromEntity, toEntity, relationship, false);

View File

@ -555,7 +555,7 @@ public abstract class EntityResource<T extends EntityInterface, K extends Entity
new OperationContext(entityType, MetadataOperation.EDIT_ALL);
authorizer.authorize(securityContext, operationContext, getResourceContextById(id));
PutResponse<T> response =
repository.restoreEntity(securityContext.getUserPrincipal().getName(), entityType, id);
repository.restoreEntity(securityContext.getUserPrincipal().getName(), id);
repository.restoreFromSearch(response.getEntity());
addHref(uriInfo, response.getEntity());
LOG.info(

View File

@ -1022,16 +1022,6 @@ public class SearchRepository {
new ImmutablePair<>(
REMOVE_TAGS_CHILDREN_SCRIPT,
Collections.singletonMap("fqn", entity.getFullyQualifiedName())));
case Entity.DASHBOARD -> {
String scriptTxt =
String.format(
"if (ctx._source.dashboards.size() == 1) { ctx._source.put('deleted', '%s') }",
true);
searchClient.softDeleteOrRestoreChildren(
indexMapping.getChildAliases(clusterAlias),
scriptTxt,
List.of(new ImmutablePair<>("dashboards.id", docId)));
}
case Entity.TEST_SUITE -> {
TestSuite testSuite = (TestSuite) entity;
if (Boolean.TRUE.equals(testSuite.getBasic())) {
@ -1082,16 +1072,6 @@ public class SearchRepository {
indexMapping.getChildAliases(clusterAlias),
scriptTxt,
List.of(new ImmutablePair<>("service.id", docId)));
case Entity.DASHBOARD -> {
scriptTxt =
String.format(
"if (ctx._source.dashboards.size() == 1) { ctx._source.put('deleted', '%s') }",
delete);
searchClient.softDeleteOrRestoreChildren(
indexMapping.getChildAliases(clusterAlias),
scriptTxt,
List.of(new ImmutablePair<>("dashboards.id", docId)));
}
default -> {
List<String> indexNames = indexMapping.getChildAliases(clusterAlias);
if (!indexNames.isEmpty()) {

View File

@ -17,12 +17,15 @@ import static jakarta.ws.rs.core.Response.Status.BAD_REQUEST;
import static jakarta.ws.rs.core.Response.Status.NOT_FOUND;
import static jakarta.ws.rs.core.Response.Status.OK;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.openmetadata.service.Entity.FIELD_DELETED;
import static org.openmetadata.service.exception.CatalogExceptionMessage.entityNotFound;
import static org.openmetadata.service.security.SecurityUtil.authHeaders;
import static org.openmetadata.service.util.EntityUtil.fieldAdded;
import static org.openmetadata.service.util.EntityUtil.fieldDeleted;
import static org.openmetadata.service.util.EntityUtil.fieldUpdated;
import static org.openmetadata.service.util.TestUtils.ADMIN_AUTH_HEADERS;
import static org.openmetadata.service.util.TestUtils.UpdateType.MINOR_UPDATE;
import static org.openmetadata.service.util.TestUtils.assertEntityReferenceNames;
@ -33,6 +36,7 @@ import static org.openmetadata.service.util.TestUtils.assertResponseContains;
import java.io.IOException;
import java.net.URISyntaxException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@ -41,14 +45,18 @@ import org.apache.http.client.HttpResponseException;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInfo;
import org.openmetadata.schema.api.data.CreateChart;
import org.openmetadata.schema.api.data.CreateDashboard;
import org.openmetadata.schema.api.services.CreateDashboardService;
import org.openmetadata.schema.entity.data.Chart;
import org.openmetadata.schema.entity.data.Dashboard;
import org.openmetadata.schema.entity.services.DashboardService;
import org.openmetadata.schema.type.ChangeDescription;
import org.openmetadata.schema.type.EntityReference;
import org.openmetadata.schema.type.Include;
import org.openmetadata.service.Entity;
import org.openmetadata.service.resources.EntityResourceTest;
import org.openmetadata.service.resources.charts.ChartResourceTest;
import org.openmetadata.service.resources.dashboards.DashboardResource.DashboardList;
import org.openmetadata.service.resources.services.DashboardServiceResourceTest;
import org.openmetadata.service.util.FullyQualifiedName;
@ -58,6 +66,7 @@ import org.openmetadata.service.util.TestUtils;
@Slf4j
public class DashboardResourceTest extends EntityResourceTest<Dashboard, CreateDashboard> {
public static final String SUPERSET_INVALID_SERVICE = "invalid_superset_service";
private final ChartResourceTest chartResourceTest = new ChartResourceTest();
public DashboardResourceTest() {
super(
@ -184,6 +193,223 @@ public class DashboardResourceTest extends EntityResourceTest<Dashboard, CreateD
authHeaders(DATA_CONSUMER.getName()));
}
@Test
void test_deleteDashboard_chartBelongsToSingleDashboard_chartIsDeleted_thenRestored(TestInfo test)
throws IOException {
// Create charts first using ChartResourceTest
List<String> chartFqns = new ArrayList<>();
for (int i = 0; i < 3; i++) {
CreateChart createChart =
chartResourceTest.createRequest(test, i).withService(METABASE_REFERENCE.getName());
Chart chart = chartResourceTest.createEntity(createChart, ADMIN_AUTH_HEADERS);
chartFqns.add(chart.getFullyQualifiedName());
}
// Create a dashboard with these charts
CreateDashboard create =
createRequest(test)
.withService(METABASE_REFERENCE.getFullyQualifiedName())
.withCharts(chartFqns);
Dashboard dashboard = createAndCheckEntity(create, ADMIN_AUTH_HEADERS);
// Verify the dashboard has charts
assertNotNull(dashboard.getCharts());
assertFalse(dashboard.getCharts().isEmpty());
// Store all chart IDs for verification after dashboard deletion
List<EntityReference> charts = dashboard.getCharts();
// For each chart, verify it belongs only to this dashboard
for (EntityReference chartRef : charts) {
// Get the chart entity with dashboards field included
Chart chart = chartResourceTest.getEntity(chartRef.getId(), "dashboards", ADMIN_AUTH_HEADERS);
// Check chart has exactly one dashboard
assertEquals(
1,
chart.getDashboards().size(),
"Chart should belong to exactly one dashboard before deletion test");
// Verify it's the dashboard we created
assertEquals(
dashboard.getId(),
chart.getDashboards().getFirst().getId(),
"Chart should belong to our test dashboard");
}
// Delete the dashboard
dashboard = deleteAndCheckEntity(dashboard, ADMIN_AUTH_HEADERS);
// Now verify that the charts are actually soft deleted and not hard deleted
List<Chart> deletedCharts = new ArrayList<>();
for (EntityReference chartRef : charts) {
// Get the chart with include=deleted
Map<String, String> queryParams = new HashMap<>();
queryParams.put("include", Include.DELETED.value());
Chart deletedChart =
chartResourceTest.getEntity(chartRef.getId(), queryParams, "", ADMIN_AUTH_HEADERS);
// Verify the chart is marked as deleted
assertTrue(deletedChart.getDeleted(), "Chart should be marked as deleted");
assertEquals(
chartRef.getId(), deletedChart.getId(), "Found chart should match the original chart ID");
deletedCharts.add(deletedChart);
}
// Restore the dashboard
ChangeDescription change = getChangeDescription(dashboard, MINOR_UPDATE);
fieldUpdated(change, FIELD_DELETED, true, false);
restoreAndCheckEntity(dashboard, ADMIN_AUTH_HEADERS, change);
// Verify charts are also restored when their parent dashboard is restored
for (Chart deletedChart : deletedCharts) {
// Verify chart is accessible again
Chart restoredChart =
chartResourceTest.getEntity(deletedChart.getId(), "dashboards", ADMIN_AUTH_HEADERS);
assertFalse(
restoredChart.getDeleted(),
"Chart should not be marked as deleted after dashboard restoration");
// Verify chart is associated with the restored dashboard
assertEquals(
1,
restoredChart.getDashboards().size(),
"Chart should have exactly one dashboard after restoration");
assertEquals(
dashboard.getId(),
restoredChart.getDashboards().getFirst().getId(),
"Chart should be associated with the restored dashboard");
}
}
@Test
void test_chartWithMultipleDashboards_deleteAndRestoreOneDashboard(TestInfo test)
throws IOException {
// Create charts first using ChartResourceTest
List<String> chartFqns = new ArrayList<>();
for (int i = 0; i < 3; i++) {
CreateChart createChart =
chartResourceTest.createRequest(test, i).withService(METABASE_REFERENCE.getName());
Chart chart = chartResourceTest.createEntity(createChart, ADMIN_AUTH_HEADERS);
chartFqns.add(chart.getFullyQualifiedName());
}
// Create first dashboard with all charts
CreateDashboard create1 =
createRequest(getEntityName(test) + "-first")
.withService(METABASE_REFERENCE.getFullyQualifiedName())
.withCharts(chartFqns);
Dashboard dashboard1 = createAndCheckEntity(create1, ADMIN_AUTH_HEADERS);
// Create second dashboard with the same charts
CreateDashboard create2 =
createRequest(getEntityName(test) + "-second")
.withService(METABASE_REFERENCE.getFullyQualifiedName())
.withCharts(chartFqns);
final Dashboard dashboard2 = createAndCheckEntity(create2, ADMIN_AUTH_HEADERS);
// Store all chart IDs for verification after dashboard deletion
List<EntityReference> charts = dashboard1.getCharts();
// Delete the first dashboard
final Dashboard deletedDashboard1 = deleteAndCheckEntity(dashboard1, ADMIN_AUTH_HEADERS);
// Verify charts are still accessible (not deleted)
for (EntityReference chartRef : charts) {
// Get the chart entity with dashboards field included
Chart chart = chartResourceTest.getEntity(chartRef.getId(), "dashboards", ADMIN_AUTH_HEADERS);
// Check chart still exists
assertNotNull(chart);
// Verify the chart itself isn't deleted
assertFalse(chart.getDeleted(), "Chart should not be marked as deleted");
// Count non-deleted dashboards
long nonDeletedDashboards =
chart.getDashboards().stream().filter(d -> !Boolean.TRUE.equals(d.getDeleted())).count();
assertEquals(
1,
nonDeletedDashboards,
"Chart should belong to exactly one non-deleted dashboard after first dashboard deletion");
// Verify one of the dashboards is the non-deleted second dashboard
boolean hasSecondDashboard =
chart.getDashboards().stream()
.anyMatch(
d ->
d.getId().equals(dashboard2.getId()) && !Boolean.TRUE.equals(d.getDeleted()));
assertTrue(hasSecondDashboard, "Chart should still belong to second test dashboard");
}
// Restore the first dashboard
ChangeDescription change = getChangeDescription(deletedDashboard1, MINOR_UPDATE);
fieldUpdated(change, FIELD_DELETED, true, false);
restoreAndCheckEntity(deletedDashboard1, ADMIN_AUTH_HEADERS, change);
// Verify charts now have associations with both dashboards
for (EntityReference chartRef : charts) {
// Get the chart entity with dashboards field included
Chart chart = chartResourceTest.getEntity(chartRef.getId(), "dashboards", ADMIN_AUTH_HEADERS);
// Verify chart is still not deleted
assertFalse(chart.getDeleted(), "Chart should not be marked as deleted");
// Count non-deleted dashboards - should be 2 now
long nonDeletedDashboards =
chart.getDashboards().stream().filter(d -> !Boolean.TRUE.equals(d.getDeleted())).count();
assertEquals(
2,
nonDeletedDashboards,
"Chart should belong to two non-deleted dashboards after first dashboard restoration");
// Verify both dashboards are associated with the chart
boolean hasFirstDashboard =
chart.getDashboards().stream()
.anyMatch(
d ->
d.getId().equals(deletedDashboard1.getId())
&& !Boolean.TRUE.equals(d.getDeleted()));
boolean hasSecondDashboard =
chart.getDashboards().stream()
.anyMatch(
d ->
d.getId().equals(dashboard2.getId()) && !Boolean.TRUE.equals(d.getDeleted()));
assertTrue(
hasFirstDashboard,
"Chart should be associated with the first dashboard after restoration");
assertTrue(hasSecondDashboard, "Chart should still be associated with the second dashboard");
}
// Now delete both dashboards
deleteEntity(deletedDashboard1.getId(), true, false, ADMIN_AUTH_HEADERS);
deleteEntity(dashboard2.getId(), true, false, ADMIN_AUTH_HEADERS);
// Verify charts are now soft deleted
for (EntityReference chartRef : charts) {
// Verify chart cannot be retrieved normally
assertResponse(
() -> chartResourceTest.getEntity(chartRef.getId(), "", ADMIN_AUTH_HEADERS),
NOT_FOUND,
entityNotFound(Entity.CHART, chartRef.getId()));
// Verify chart can be retrieved with include=deleted parameter
Map<String, String> queryParams = new HashMap<>();
queryParams.put("include", Include.DELETED.value());
Chart deletedChart =
chartResourceTest.getEntity(chartRef.getId(), queryParams, "", ADMIN_AUTH_HEADERS);
// Verify the chart is marked as deleted
assertTrue(
deletedChart.getDeleted(),
"Chart should be marked as deleted after all dashboards are deleted");
assertEquals(
chartRef.getId(), deletedChart.getId(), "Found chart should match the original chart ID");
}
}
@Override
public Dashboard validateGetWithDifferentFields(Dashboard dashboard, boolean byName)
throws HttpResponseException {