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 <ashish@getcollate.io>

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<Team> {
         .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<TeamHierarchy> 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<TeamHierarchy> team1Children = team1.getChildren();
-    List<TeamHierarchy> team2Children = team2.getChildren();
-    if (team1Children != null && team2Children != null) {
-      List<TeamHierarchy> toMerge = new ArrayList<>(team1Children);
-      toMerge.retainAll(team2Children);
-
-      for (TeamHierarchy n : toMerge) mergeTrees(n, team2Children.get(team2Children.indexOf(n)));
-    }
-    if (team2Children != null) {
-      List<TeamHierarchy> 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<TeamHierarchy> listHierarchy(ListFilter filter, int limit, Boolean isJoinable) {
     Fields fields = getFields(PARENTS_FIELD);
-    Map<UUID, TeamHierarchy> map = new HashMap<>();
     ResultList<Team> resultList = listAfter(null, fields, filter, limit, null);
     List<Team> allTeams = resultList.getData();
     List<Team> joinableTeams =
@@ -392,42 +349,61 @@ public class TeamRepository extends EntityRepository<Team> {
             .filter(Boolean.TRUE.equals(isJoinable) ? Team::getIsJoinable : t -> true)
             .filter(t -> !t.getName().equals(ORGANIZATION_NAME))
             .toList();
-    // build hierarchy of joinable teams
+
-    joinableTeams.forEach(
+    Map<UUID, TeamHierarchy> hierarchyMap = new HashMap<>();
-        team -> {
+    for (Team team : joinableTeams) {
-          Team currentTeam = team;
+      if (!hierarchyMap.containsKey(team.getId())) {
-          TeamHierarchy currentHierarchy = getTeamHierarchy(team);
+        hierarchyMap.put(team.getId(), getTeamHierarchy(team));
-          while (currentTeam != null
+      }
-              && !currentTeam.getParents().isEmpty()
+    }
-              && !currentTeam.getParents().get(0).getName().equals(ORGANIZATION_NAME)) {
+
-            EntityReference parentRef = currentTeam.getParents().get(0);
+    for (Team team : joinableTeams) {
-            Team parent =
+      for (EntityReference parentRef : team.getParents()) {
+        if (parentRef.getName().equals(ORGANIZATION_NAME)) {
+          continue;
+        }
+
+        Team parentTeam =
             allTeams.stream()
                 .filter(t -> t.getId().equals(parentRef.getId()))
                 .findFirst()
-                    .orElseThrow(() -> new IllegalArgumentException(TEAM_HIERARCHY));
+                .orElse(null);
-            currentHierarchy =
+        if (parentTeam == null) {
-                getTeamHierarchy(parent).withChildren(new ArrayList<>(List.of(currentHierarchy)));
+          continue;
-            if (map.containsKey(parent.getId())) {
+        }
-              TeamHierarchy parentTeam = map.get(parent.getId());
+
-              currentHierarchy = mergeTrees(parentTeam, currentHierarchy);
+        hierarchyMap.putIfAbsent(parentTeam.getId(), getTeamHierarchy(parentTeam));
-              currentTeam =
+        TeamHierarchy parentNode = hierarchyMap.get(parentTeam.getId());
-                  allTeams.stream()
+        TeamHierarchy childNode = hierarchyMap.get(team.getId());
-                      .filter(t -> t.getId().equals(parent.getId()))
+
-                      .findFirst()
+        if (parentNode.getChildren() == null) {
-                      .orElseThrow(() -> new IllegalArgumentException(TEAM_HIERARCHY));
+          parentNode.setChildren(new ArrayList<>());
-            } else {
+        }
-              currentTeam = parent;
+
+        boolean childAlreadyAdded =
+            parentNode.getChildren().stream()
+                .anyMatch(child -> child.getId().equals(childNode.getId()));
+        if (!childAlreadyAdded) {
+          parentNode.getChildren().add(childNode);
         }
       }
-          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());
+    Set<UUID> childIds = new HashSet<>();
+    for (TeamHierarchy node : hierarchyMap.values()) {
+      if (node.getChildren() != null) {
+        for (TeamHierarchy child : node.getChildren()) {
+          childIds.add(child.getId());
+        }
+      }
+    }
+    List<TeamHierarchy> topLevelNodes =
+        hierarchyMap.values().stream()
+            .filter(node -> !childIds.contains(node.getId()))
+            .sorted(Comparator.comparing(TeamHierarchy::getName))
+            .collect(Collectors.toList());
+
+    return topLevelNodes;
   }
 
   private List<EntityReference> getUsers(Team team) {

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<Team, CreateTeam> {
     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<TeamHierarchy> 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<Team, CreateTeam> {
         defaultRoles,
         policies);
   }
+
+  void verifyNoDuplicateChildrenTeam(TeamHierarchy parent) {
+    if (parent.getChildren() != null) {
+      Set<UUID> seenChildIds = new HashSet<>();
+      for (TeamHierarchy child : parent.getChildren()) {
+        assertTrue(
+            seenChildIds.add(child.getId()),
+            "Duplicate child " + child.getId() + " found under parent " + parent.getId());
+        verifyNoDuplicateChildrenTeam(child);
+      }
+    }
+  }
 }

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