From b341d5e1cb3e7ab6788258af57cfefbfcf00f28d Mon Sep 17 00:00:00 2001 From: sonika-shah <58761340+sonika-shah@users.noreply.github.com> Date: Thu, 20 Feb 2025 16:44:22 +0530 Subject: [PATCH] fix - 17995 Avoid duplicate teams in team listing hierarchy (#19844) * fix - 17995 Avoid duplicate teams in team listing hierarchy * added check in playwright to see if the team is not repeated --------- Co-authored-by: Ashish Gupta --- .../service/jdbi3/TeamRepository.java | 136 ++++++++---------- .../resources/teams/TeamResourceTest.java | 55 +++++++ .../e2e/Features/TeamsHierarchy.spec.ts | 1 + 3 files changed, 112 insertions(+), 80 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TeamRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TeamRepository.java index e135095a485..0adc75487cd 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TeamRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TeamRepository.java @@ -37,7 +37,6 @@ import static org.openmetadata.service.exception.CatalogExceptionMessage.CREATE_ import static org.openmetadata.service.exception.CatalogExceptionMessage.DELETE_ORGANIZATION; import static org.openmetadata.service.exception.CatalogExceptionMessage.INVALID_GROUP_TEAM_CHILDREN_UPDATE; import static org.openmetadata.service.exception.CatalogExceptionMessage.INVALID_GROUP_TEAM_UPDATE; -import static org.openmetadata.service.exception.CatalogExceptionMessage.TEAM_HIERARCHY; import static org.openmetadata.service.exception.CatalogExceptionMessage.UNEXPECTED_PARENT; import static org.openmetadata.service.exception.CatalogExceptionMessage.invalidChild; import static org.openmetadata.service.exception.CatalogExceptionMessage.invalidParent; @@ -47,6 +46,7 @@ import static org.openmetadata.service.util.EntityUtil.*; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -340,51 +340,8 @@ public class TeamRepository extends EntityRepository { .withDescription(team.getDescription()); } - private TeamHierarchy deepCopy(TeamHierarchy team) { - TeamHierarchy newTeam = - new TeamHierarchy() - .withId(team.getId()) - .withTeamType(team.getTeamType()) - .withName(team.getName()) - .withDisplayName(team.getDisplayName()) - .withHref(team.getHref()) - .withFullyQualifiedName(team.getFullyQualifiedName()) - .withIsJoinable(team.getIsJoinable()); - if (team.getChildren() != null) { - List children = new ArrayList<>(); - for (TeamHierarchy n : team.getChildren()) { - children.add(deepCopy(n)); - } - newTeam.withChildren(children); - } - return newTeam; - } - - private TeamHierarchy mergeTrees(TeamHierarchy team1, TeamHierarchy team2) { - List team1Children = team1.getChildren(); - List team2Children = team2.getChildren(); - if (team1Children != null && team2Children != null) { - List toMerge = new ArrayList<>(team1Children); - toMerge.retainAll(team2Children); - - for (TeamHierarchy n : toMerge) mergeTrees(n, team2Children.get(team2Children.indexOf(n))); - } - if (team2Children != null) { - List toAdd = new ArrayList<>(team2Children); - if (team1Children != null) { - toAdd.removeAll(team1Children); - } else { - team1.setChildren(new ArrayList<>()); - } - for (TeamHierarchy n : toAdd) team1.getChildren().add(deepCopy(n)); - } - - return team1; - } - public List listHierarchy(ListFilter filter, int limit, Boolean isJoinable) { Fields fields = getFields(PARENTS_FIELD); - Map map = new HashMap<>(); ResultList resultList = listAfter(null, fields, filter, limit, null); List allTeams = resultList.getData(); List joinableTeams = @@ -392,42 +349,61 @@ public class TeamRepository extends EntityRepository { .filter(Boolean.TRUE.equals(isJoinable) ? Team::getIsJoinable : t -> true) .filter(t -> !t.getName().equals(ORGANIZATION_NAME)) .toList(); - // build hierarchy of joinable teams - joinableTeams.forEach( - team -> { - Team currentTeam = team; - TeamHierarchy currentHierarchy = getTeamHierarchy(team); - while (currentTeam != null - && !currentTeam.getParents().isEmpty() - && !currentTeam.getParents().get(0).getName().equals(ORGANIZATION_NAME)) { - EntityReference parentRef = currentTeam.getParents().get(0); - Team parent = - allTeams.stream() - .filter(t -> t.getId().equals(parentRef.getId())) - .findFirst() - .orElseThrow(() -> new IllegalArgumentException(TEAM_HIERARCHY)); - currentHierarchy = - getTeamHierarchy(parent).withChildren(new ArrayList<>(List.of(currentHierarchy))); - if (map.containsKey(parent.getId())) { - TeamHierarchy parentTeam = map.get(parent.getId()); - currentHierarchy = mergeTrees(parentTeam, currentHierarchy); - currentTeam = - allTeams.stream() - .filter(t -> t.getId().equals(parent.getId())) - .findFirst() - .orElseThrow(() -> new IllegalArgumentException(TEAM_HIERARCHY)); - } else { - currentTeam = parent; - } - } - UUID currentId = currentHierarchy.getId(); - if (!map.containsKey(currentId)) { - map.put(currentId, currentHierarchy); - } else { - map.put(currentId, mergeTrees(map.get(currentId), currentHierarchy)); - } - }); - return new ArrayList<>(map.values()); + + Map hierarchyMap = new HashMap<>(); + for (Team team : joinableTeams) { + if (!hierarchyMap.containsKey(team.getId())) { + hierarchyMap.put(team.getId(), getTeamHierarchy(team)); + } + } + + for (Team team : joinableTeams) { + for (EntityReference parentRef : team.getParents()) { + if (parentRef.getName().equals(ORGANIZATION_NAME)) { + continue; + } + + Team parentTeam = + allTeams.stream() + .filter(t -> t.getId().equals(parentRef.getId())) + .findFirst() + .orElse(null); + if (parentTeam == null) { + continue; + } + + hierarchyMap.putIfAbsent(parentTeam.getId(), getTeamHierarchy(parentTeam)); + TeamHierarchy parentNode = hierarchyMap.get(parentTeam.getId()); + TeamHierarchy childNode = hierarchyMap.get(team.getId()); + + if (parentNode.getChildren() == null) { + parentNode.setChildren(new ArrayList<>()); + } + + boolean childAlreadyAdded = + parentNode.getChildren().stream() + .anyMatch(child -> child.getId().equals(childNode.getId())); + if (!childAlreadyAdded) { + parentNode.getChildren().add(childNode); + } + } + } + + Set childIds = new HashSet<>(); + for (TeamHierarchy node : hierarchyMap.values()) { + if (node.getChildren() != null) { + for (TeamHierarchy child : node.getChildren()) { + childIds.add(child.getId()); + } + } + } + List topLevelNodes = + hierarchyMap.values().stream() + .filter(node -> !childIds.contains(node.getId())) + .sorted(Comparator.comparing(TeamHierarchy::getName)) + .collect(Collectors.toList()); + + return topLevelNodes; } private List getUsers(Team team) { diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/TeamResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/TeamResourceTest.java index 8caa04adeb4..bf7efeff840 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/TeamResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/TeamResourceTest.java @@ -64,9 +64,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Random; +import java.util.Set; import java.util.UUID; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -608,6 +610,47 @@ public class TeamResourceTest extends EntityResourceTest { patchEntityAndCheck(bu2, json, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); } + @Test + void test_hierarchyNoDuplicateForGroupInDept() throws HttpResponseException { + // Team hierarchy: BU -> Division -> Department -> Group + Team bu = createWithParents("buTest-341f887e", BUSINESS_UNIT, ORG_TEAM.getEntityReference()); + Team div = createWithParents("divTest-010f23ef", DIVISION, bu.getEntityReference()); + Team dept = createWithParents("deptTest-0574ff5c", DEPARTMENT, div.getEntityReference()); + Team group = createWithParents("groupTest-148facc0", GROUP, dept.getEntityReference()); + + List hierarchyList = getTeamsHierarchy(false, ADMIN_AUTH_HEADERS); + + TeamHierarchy buHierarchy = + hierarchyList.stream().filter(t -> t.getId().equals(bu.getId())).findFirst().orElse(null); + assertNotNull(buHierarchy, "BU node should be present in the hierarchy"); + + TeamHierarchy divHierarchy = + buHierarchy.getChildren().stream() + .filter(t -> t.getId().equals(div.getId())) + .findFirst() + .orElse(null); + assertNotNull(divHierarchy, "Division node should be present under BU"); + + TeamHierarchy deptHierarchy = + divHierarchy.getChildren().stream() + .filter(t -> t.getId().equals(dept.getId())) + .findFirst() + .orElse(null); + assertNotNull(deptHierarchy, "Department node should be present under Division"); + + TeamHierarchy groupHierarchy = + deptHierarchy.getChildren().stream() + .filter(t -> t.getId().equals(group.getId())) + .findFirst() + .orElse(null); + assertNotNull(groupHierarchy, "Group node should be present under Department"); + + // Verify that within each parent's children list, no node is duplicated + for (TeamHierarchy topLevel : hierarchyList) { + verifyNoDuplicateChildrenTeam(topLevel); + } + } + @Test void patch_isJoinable_200(TestInfo test) throws IOException { CreateTeam create = @@ -1354,4 +1397,16 @@ public class TeamResourceTest extends EntityResourceTest { defaultRoles, policies); } + + void verifyNoDuplicateChildrenTeam(TeamHierarchy parent) { + if (parent.getChildren() != null) { + Set seenChildIds = new HashSet<>(); + for (TeamHierarchy child : parent.getChildren()) { + assertTrue( + seenChildIds.add(child.getId()), + "Duplicate child " + child.getId() + " found under parent " + parent.getId()); + verifyNoDuplicateChildrenTeam(child); + } + } + } } diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/TeamsHierarchy.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/TeamsHierarchy.spec.ts index 93fe4330c27..4443132cd94 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/TeamsHierarchy.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/TeamsHierarchy.spec.ts @@ -91,6 +91,7 @@ test.describe('Add Nested Teams and Test TeamsSelectable', () => { const dropdown = page.locator('.ant-tree-select-dropdown'); await expect(dropdown).toContainText(teamName); + await expect(dropdown.getByText(teamName)).toHaveCount(1); } for (const teamName of teamNames) {