From 00adf708a1e988f2ac20b19756ca3a2e0277fc41 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Thu, 16 Sep 2021 10:20:01 -0700 Subject: [PATCH] Fix #505: Tag remove patch is not working for chart entity; Fix #504: Changing Tier on dashboard does not replace the previously selected Tier (#509) * Fix #505: Tag remove patch is not working for chart entity; Fix #504: Changing Tier on dashboard does not replace the previously selected Tier * Fix #505: Tag remove patch is not working for chart entity; Fix #504: Changing Tier on dashboard does not replace the previously selected Tier --- .../catalog/jdbi3/ChartRepository.java | 3 +- .../catalog/jdbi3/DashboardRepository.java | 2 + .../resources/charts/ChartResourceTest.java | 9 ++-- .../dashboards/DashboardResourceTest.java | 52 +++++++++++++++---- 4 files changed, 52 insertions(+), 14 deletions(-) diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/ChartRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/ChartRepository.java index f6448d32c9b..45b2778f948 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/ChartRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/ChartRepository.java @@ -224,7 +224,8 @@ public abstract class ChartRepository { EntityReference newOwner = EntityUtil.populateOwner(userDAO(), teamDAO(), updated.getOwner()); EntityReference newService = updated.getService(); - + // Remove previous tags. Merge tags from the update and the existing tags + EntityUtil.removeTags(tagDAO(), original.getFullyQualifiedName()); updated.setHref(null); updated.setOwner(null); updated.setService(null); diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/DashboardRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/DashboardRepository.java index f1d5e2222c0..0bf2f6800eb 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/DashboardRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/DashboardRepository.java @@ -291,6 +291,8 @@ public abstract class DashboardRepository { EntityReference newOwner = EntityUtil.populateOwner(userDAO(), teamDAO(), updated.getOwner()); EntityReference newService = updated.getService(); + // Remove previous tags. Merge tags from the update and the existing tags + EntityUtil.removeTags(tagDAO(), original.getFullyQualifiedName()); updated.setHref(null); updated.setOwner(null); diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/charts/ChartResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/charts/ChartResourceTest.java index f51944585d5..bf36a00b319 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/charts/ChartResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/charts/ChartResourceTest.java @@ -89,6 +89,8 @@ public class ChartResourceTest extends CatalogApplicationTest { public static EntityReference SUPERSET_REFERENCE; public static EntityReference LOOKER_REFERENCE; public static final TagLabel USER_ADDRESS_TAG_LABEL = new TagLabel().withTagFQN("User.Address"); + public static final TagLabel TIER_1 = new TagLabel().withTagFQN("Tier.Tier1"); + @BeforeAll @@ -401,7 +403,7 @@ public class ChartResourceTest extends CatalogApplicationTest { assertNull(chart.getDescription()); assertNull(chart.getOwner()); assertNotNull(chart.getService()); - List chartTags = singletonList(USER_ADDRESS_TAG_LABEL); + List chartTags = List.of(USER_ADDRESS_TAG_LABEL); chart = getChart(chart.getId(), "service,owner,tags", adminAuthHeaders()); chart.getService().setHref(null); // href is readonly and not patchable @@ -410,14 +412,13 @@ public class ChartResourceTest extends CatalogApplicationTest { chart = patchChartAttributesAndCheck(chart, "description", TEAM_OWNER1, chartTags, adminAuthHeaders()); chart.setOwner(TEAM_OWNER1); // Get rid of href and name returned in the response for owner chart.setService(SUPERSET_REFERENCE); // Get rid of href and name returned in the response for service - chart.setTags(singletonList(USER_ADDRESS_TAG_LABEL)); - + chartTags = List.of(USER_ADDRESS_TAG_LABEL, TIER_1); // Replace description, tier, owner chart = patchChartAttributesAndCheck(chart, "description1", USER_OWNER1, chartTags, adminAuthHeaders()); chart.setOwner(USER_OWNER1); // Get rid of href and name returned in the response for owner chart.setService(SUPERSET_REFERENCE); // Get rid of href and name returned in the response for service - + chartTags = List.of(TIER_1); // Remove description, tier, owner patchChartAttributesAndCheck(chart, null, null, chartTags, adminAuthHeaders()); } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java index fa73d4695a3..279eddef12a 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java @@ -36,9 +36,11 @@ import org.openmetadata.catalog.exception.CatalogExceptionMessage; import org.openmetadata.catalog.resources.charts.ChartResourceTest; import org.openmetadata.catalog.resources.dashboards.DashboardResource.DashboardList; import org.openmetadata.catalog.resources.services.DashboardServiceResourceTest; +import org.openmetadata.catalog.resources.tags.TagResourceTest; import org.openmetadata.catalog.resources.teams.TeamResourceTest; import org.openmetadata.catalog.resources.teams.UserResourceTest; import org.openmetadata.catalog.type.EntityReference; +import org.openmetadata.catalog.type.TagLabel; import org.openmetadata.catalog.util.EntityUtil; import org.openmetadata.catalog.util.JsonUtils; import org.openmetadata.catalog.util.TestUtils; @@ -53,7 +55,9 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.UUID; +import java.util.stream.Collectors; +import static java.util.Collections.singletonList; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static javax.ws.rs.core.Response.Status.CONFLICT; import static javax.ws.rs.core.Response.Status.CREATED; @@ -81,6 +85,9 @@ public class DashboardResourceTest extends CatalogApplicationTest { public static EntityReference SUPERSET_REFERENCE; public static EntityReference LOOKER_REFERENCE; public static List CHART_REFERENCES; + public static final TagLabel TIER_1 = new TagLabel().withTagFQN("Tier.Tier1"); + public static final TagLabel USER_ADDRESS_TAG_LABEL = new TagLabel().withTagFQN("User.Address"); + @BeforeAll public static void setup(TestInfo test) throws HttpResponseException { @@ -437,21 +444,22 @@ public class DashboardResourceTest extends CatalogApplicationTest { dashboard = getDashboard(dashboard.getId(), "service,owner,usageSummary", adminAuthHeaders()); dashboard.getService().setHref(null); // href is readonly and not patchable + List dashboardTags = singletonList(TIER_1); // Add description, owner when previously they were null dashboard = patchDashboardAttributesAndCheck(dashboard, "description", - TEAM_OWNER1, adminAuthHeaders()); + TEAM_OWNER1, dashboardTags, adminAuthHeaders()); dashboard.setOwner(TEAM_OWNER1); // Get rid of href and name returned in the response for owner dashboard.setService(SUPERSET_REFERENCE); // Get rid of href and name returned in the response for service - + dashboardTags = singletonList(USER_ADDRESS_TAG_LABEL); // Replace description, tier, owner dashboard = patchDashboardAttributesAndCheck(dashboard, "description1", - USER_OWNER1, adminAuthHeaders()); + USER_OWNER1, dashboardTags, adminAuthHeaders()); dashboard.setOwner(USER_OWNER1); // Get rid of href and name returned in the response for owner dashboard.setService(SUPERSET_REFERENCE); // Get rid of href and name returned in the response for service // Remove description, tier, owner - patchDashboardAttributesAndCheck(dashboard, null, null, adminAuthHeaders()); + patchDashboardAttributesAndCheck(dashboard, null, null, dashboardTags, adminAuthHeaders()); } @Test @@ -541,7 +549,8 @@ public class DashboardResourceTest extends CatalogApplicationTest { Map authHeaders) throws HttpResponseException { create.withCharts(charts); Dashboard dashboard = createDashboard(create, authHeaders); - validateDashboard(dashboard, create.getDescription(), create.getOwner(), create.getService(), charts); + validateDashboard(dashboard, create.getDescription(), create.getOwner(), create.getService(), create.getTags(), + charts); return getAndValidate(dashboard.getId(), create, authHeaders); } @@ -643,7 +652,8 @@ public class DashboardResourceTest extends CatalogApplicationTest { private static Dashboard validateDashboard(Dashboard dashboard, String expectedDescription, EntityReference expectedOwner, EntityReference expectedService, - List charts) { + List expectedTags, + List charts) throws HttpResponseException { assertNotNull(dashboard.getId()); assertNotNull(dashboard.getHref()); assertEquals(expectedDescription, dashboard.getDescription()); @@ -663,6 +673,7 @@ public class DashboardResourceTest extends CatalogApplicationTest { assertEquals(expectedService.getType(), dashboard.getService().getType()); } validateDashboardCharts(dashboard, charts); + validateTags(expectedTags, dashboard.getTags()); return dashboard; } @@ -681,22 +692,45 @@ public class DashboardResourceTest extends CatalogApplicationTest { } } + private static void validateTags(List expectedList, List actualList) + throws HttpResponseException { + if (expectedList == null) { + return; + } + // When tags from the expected list is added to an entity, the derived tags for those tags are automatically added + // So add to the expectedList, the derived tags before validating the tags + List updatedExpectedList = new ArrayList<>(expectedList); + for (TagLabel expected : expectedList) { + List derived = EntityUtil.getDerivedTags(expected, TagResourceTest.getTag(expected.getTagFQN(), + adminAuthHeaders())); + updatedExpectedList.addAll(derived); + } + updatedExpectedList = updatedExpectedList.stream().distinct().collect(Collectors.toList()); + + assertTrue(actualList.containsAll(updatedExpectedList)); + assertTrue(updatedExpectedList.containsAll(actualList)); + } + private Dashboard patchDashboardAttributesAndCheck(Dashboard dashboard, String newDescription, - EntityReference newOwner, Map authHeaders) + EntityReference newOwner, List tags, + Map authHeaders) throws JsonProcessingException, HttpResponseException { String dashboardJson = JsonUtils.pojoToJson(dashboard); // Update the table attributes dashboard.setDescription(newDescription); dashboard.setOwner(newOwner); + dashboard.setTags(tags); // Validate information returned in patch response has the updates Dashboard updatedDashboard = patchDashboard(dashboardJson, dashboard, authHeaders); - validateDashboard(updatedDashboard, dashboard.getDescription(), newOwner, null); + validateDashboard(updatedDashboard, dashboard.getDescription(), newOwner, null, tags, + dashboard.getCharts()); // GET the table and Validate information returned Dashboard getDashboard = getDashboard(dashboard.getId(), "service,owner", authHeaders); - validateDashboard(getDashboard, dashboard.getDescription(), newOwner, null); + validateDashboard(updatedDashboard, dashboard.getDescription(), newOwner, null, tags, + dashboard.getCharts()); return updatedDashboard; }