diff --git a/go/hierarchy_test.go b/go/hierarchy_test.go index ac0f052..8689e09 100644 --- a/go/hierarchy_test.go +++ b/go/hierarchy_test.go @@ -1,6 +1,8 @@ package orgdatacore import ( + "context" + "encoding/json" "testing" ) @@ -165,6 +167,22 @@ func TestGetHierarchyPathNameCollision(t *testing.T) { if !service.IsEmployeeInOrg("euser", "acme") { t.Error("Expected euser to be in org 'acme' via the name-colliding team hierarchy") } + + // Reciprocal: requesting the same name with the team_group type must + // resolve the team_group, not be rejected because a team shares the name. + tgPath := service.GetHierarchyPath("shared", "team_group") + tgExpected := []HierarchyPathEntry{ + {Name: "shared", Type: "team_group"}, + {Name: "acme", Type: "org"}, + } + if len(tgPath) != len(tgExpected) { + t.Fatalf("team_group path: expected %d entries, got %d: %+v", len(tgExpected), len(tgPath), tgPath) + } + for i, exp := range tgExpected { + if tgPath[i] != exp { + t.Errorf("team_group path entry %d: expected %+v, got %+v", i, exp, tgPath[i]) + } + } } func TestGetHierarchyPathNoData(t *testing.T) { @@ -303,3 +321,107 @@ func TestGetDescendantsTreeNoData(t *testing.T) { t.Error("Expected nil tree when no data loaded") } } + +// TestGetDescendantsTreeNameCollision covers a team_group and a team that share +// a name. The tree must key both the children map and the visited set by +// (name, type): keying by name alone merges the two nodes' children and stops +// the recursion early, producing a wrong tree. +func TestGetDescendantsTreeNameCollision(t *testing.T) { + service := NewService() + service.data = &Data{ + Lookups: Lookups{ + Orgs: map[string]Org{"acme": {Name: "acme", Type: "org"}}, + TeamGroups: map[string]TeamGroup{ + "shared": {Name: "shared", Type: "team_group", Parent: &ParentInfo{Name: "acme", Type: "org"}}, + }, + Teams: map[string]Team{ + "shared": {Name: "shared", Type: "team", Parent: &ParentInfo{Name: "shared", Type: "team_group"}}, + "leaf": {Name: "leaf", Type: "team", Parent: &ParentInfo{Name: "shared", Type: "team"}}, + }, + }, + } + + // acme(org) -> shared(team_group) -> shared(team) -> leaf(team) + tree := service.GetDescendantsTree("acme") + if tree == nil { + t.Fatal("Expected non-nil tree") + } + if len(tree.Children) != 1 || tree.Children[0].Name != "shared" || tree.Children[0].Type != "team_group" { + t.Fatalf("Expected acme's only child to be shared/team_group, got %+v", tree.Children) + } + tg := tree.Children[0] + if len(tg.Children) != 1 || tg.Children[0].Type != "team" { + t.Fatalf("Expected team_group shared's only child to be shared/team, got %+v", tg.Children) + } + team := tg.Children[0] + if len(team.Children) != 1 || team.Children[0].Name != "leaf" { + t.Fatalf("Expected team shared's only child to be leaf, got %+v", team.Children) + } +} + +// TestMixedCaseTypesNormalizedAtLoad verifies hierarchy types are canonicalized +// to lowercase at the load boundary, so traversal resolves regardless of the +// casing the data source used for entity and parent types. +func TestMixedCaseTypesNormalizedAtLoad(t *testing.T) { + data := &Data{ + Lookups: Lookups{ + Employees: map[string]Employee{"u1": {UID: "u1", Email: "u1@example.com"}}, + Orgs: map[string]Org{"acme": {Name: "acme", Type: "ORG"}}, + TeamGroups: map[string]TeamGroup{ + "grp": {Name: "grp", Type: "Team_Group", Parent: &ParentInfo{Name: "acme", Type: "ORG"}}, + }, + Teams: map[string]Team{ + "t": {Name: "t", Type: "TEAM", Parent: &ParentInfo{Name: "grp", Type: "TEAM_GROUP"}}, + }, + }, + Indexes: Indexes{ + Membership: MembershipIndex{MembershipIndex: map[string][]MembershipInfo{ + "u1": {{Name: "t", Type: "TEAM"}}, + }}, + }, + } + jsonData, err := json.Marshal(data) + if err != nil { + t.Fatalf("marshal: %v", err) + } + service := NewService() + if err := service.LoadFromDataSource(context.Background(), NewFakeDataSource(string(jsonData))); err != nil { + t.Fatalf("load: %v", err) + } + + // Descendants resolve: the children map keys on parent type "ORG"/"TEAM_GROUP" + // which, once normalized, match the canonical lowercase child types. + tree := service.GetDescendantsTree("acme") + if tree == nil { + t.Fatal("Expected non-nil tree") + } + if len(tree.Children) != 1 || tree.Children[0].Name != "grp" { + t.Fatalf("Expected acme's only child to be grp, got %+v", tree.Children) + } + grp := tree.Children[0] + if len(grp.Children) != 1 || grp.Children[0].Name != "t" { + t.Fatalf("Expected grp's only child to be t, got %+v", grp.Children) + } + + // Hierarchy path entries carry canonical lowercase types (from normalized + // parent refs), so consumers need not defensively lowercase them. A + // caller-supplied type in any casing normalizes to the same canonical path, + // including the first entry (which is derived from the caller's argument). + wantTypes := []string{"team", "team_group", "org"} + for _, callerType := range []string{"team", "TEAM", "Team"} { + path := service.GetHierarchyPath("t", callerType) + if len(path) != len(wantTypes) { + t.Fatalf("GetHierarchyPath(t, %q): expected %d path entries, got %+v", callerType, len(wantTypes), path) + } + for i, wt := range wantTypes { + if path[i].Type != wt { + t.Errorf("GetHierarchyPath(t, %q): path[%d].Type = %q, want %q", callerType, i, path[i].Type, wt) + } + } + } + + // Entity own types are normalized in the lookups too. + if tm := service.GetTeamByName("t"); tm == nil || tm.Type != "team" { + t.Errorf("expected team 't' type normalized to 'team', got %+v", tm) + } +} diff --git a/go/organization_test.go b/go/organization_test.go index 207bdce..9d83ee5 100644 --- a/go/organization_test.go +++ b/go/organization_test.go @@ -311,3 +311,49 @@ func TestOrgInfoTypes(t *testing.T) { } } } + +// TestGetUserOrganizationsNameCollision covers a hierarchy where a team shares +// its parent team_group's name. Deduping the result by (name, type) — not name +// alone — is required, otherwise the team_group is wrongly dropped because a +// team with the same name was already recorded. +func TestGetUserOrganizationsNameCollision(t *testing.T) { + service := NewService() + service.data = &Data{ + Lookups: Lookups{ + Teams: map[string]Team{ + "shared": {Name: "shared", Type: "team", Parent: &ParentInfo{Name: "shared", Type: "team_group"}}, + }, + TeamGroups: map[string]TeamGroup{ + "shared": {Name: "shared", Type: "team_group", Parent: &ParentInfo{Name: "acme", Type: "org"}}, + }, + Orgs: map[string]Org{"acme": {Name: "acme", Type: "org"}}, + }, + Indexes: Indexes{ + Membership: MembershipIndex{MembershipIndex: map[string][]MembershipInfo{ + "euser": {{Name: "shared", Type: "team"}}, + }}, + SlackIDMappings: SlackIDMappings{SlackUIDToUID: map[string]string{"Suser": "euser"}}, + }, + } + + // euser is a member of team "shared" -> team_group "shared" -> org "acme". + // All three must appear, each with its own type. + expected := []OrgInfo{ + {Name: "shared", Type: OrgTypeTeam}, + {Name: "shared", Type: OrgTypeTeamGroup}, + {Name: "acme", Type: OrgTypeOrganization}, + } + result := service.GetUserOrganizations("Suser") + for _, exp := range expected { + found := false + for _, actual := range result { + if actual.Name == exp.Name && actual.Type == exp.Type { + found = true + break + } + } + if !found { + t.Errorf("GetUserOrganizations missing %+v; got %+v", exp, result) + } + } +} diff --git a/go/service.go b/go/service.go index 6f1fefa..c59af3d 100644 --- a/go/service.go +++ b/go/service.go @@ -44,6 +44,8 @@ func (s *Service) LoadFromDataSource(ctx context.Context, source DataSource) err return NewLoadError(source.String(), fmt.Errorf("failed to parse JSON: %w", err)) } + normalizeData(&orgData) + if err := validateData(&orgData); err != nil { return NewLoadError(source.String(), err) } @@ -424,7 +426,7 @@ func (s *Service) isEmployeeInOrg(uid string, orgName string) bool { if m.Type == string(MembershipTeam) { hierarchyPath := s.computeHierarchyPath(m.Name, "team") for _, entry := range hierarchyPath { - if strings.ToLower(entry.Type) == "org" && entry.Name == orgName { + if entry.Type == "org" && entry.Name == orgName { return true } } @@ -458,19 +460,24 @@ func (s *Service) GetUserOrganizations(slackUserID string) []OrgInfo { } var orgs []OrgInfo - seen := make(map[string]bool) + // Dedupe by (name, type): names are not unique across types, so keying on + // name alone would drop a legitimately distinct entity (e.g. a team_group + // sharing a team's name) from the result. + seen := make(map[HierarchyPathEntry]bool) for _, m := range s.data.Indexes.Membership.MembershipIndex[uid] { switch m.Type { case string(MembershipOrg): - if !seen[m.Name] { + key := HierarchyPathEntry{Name: m.Name, Type: "org"} + if !seen[key] { orgs = append(orgs, OrgInfo{Name: m.Name, Type: OrgTypeOrganization}) - seen[m.Name] = true + seen[key] = true } case string(MembershipTeam): - if !seen[m.Name] { + key := HierarchyPathEntry{Name: m.Name, Type: "team"} + if !seen[key] { orgs = append(orgs, OrgInfo{Name: m.Name, Type: OrgTypeTeam}) - seen[m.Name] = true + seen[key] = true } hierarchyPath := s.computeHierarchyPath(m.Name, "team") addHierarchyPathItems(&orgs, &seen, hierarchyPath) @@ -479,7 +486,7 @@ func (s *Service) GetUserOrganizations(slackUserID string) []OrgInfo { return orgs } -func addHierarchyPathItems(orgs *[]OrgInfo, seen *map[string]bool, hierarchyPath []HierarchyPathEntry) { +func addHierarchyPathItems(orgs *[]OrgInfo, seen *map[HierarchyPathEntry]bool, hierarchyPath []HierarchyPathEntry) { typeToOrgInfoType := map[string]OrgInfoType{ "org": OrgTypeOrganization, "pillar": OrgTypePillar, @@ -491,13 +498,14 @@ func addHierarchyPathItems(orgs *[]OrgInfo, seen *map[string]bool, hierarchyPath if i == 0 { continue } - if !(*seen)[entry.Name] { - orgType, ok := typeToOrgInfoType[strings.ToLower(entry.Type)] + key := HierarchyPathEntry{Name: entry.Name, Type: entry.Type} + if !(*seen)[key] { + orgType, ok := typeToOrgInfoType[entry.Type] if !ok { orgType = OrgTypeOrganization } *orgs = append(*orgs, OrgInfo{Name: entry.Name, Type: orgType}) - (*seen)[entry.Name] = true + (*seen)[key] = true } } } @@ -558,6 +566,31 @@ func (s *Service) getEntityType(entityName string) string { return "" } +// entityExists reports whether an entity of the given type exists. +// Unlike getEntityType, it checks the type-specific lookup, so it correctly +// resolves names shared across types (e.g. a team and a team_group both named +// "shared"). Must be called with s.mu held. +func (s *Service) entityExists(entityName, entityType string) bool { + if s.data == nil { + return false + } + switch strings.ToLower(entityType) { + case "team": + _, ok := s.data.Lookups.Teams[entityName] + return ok + case "org": + _, ok := s.data.Lookups.Orgs[entityName] + return ok + case "pillar": + _, ok := s.data.Lookups.Pillars[entityName] + return ok + case "team_group": + _, ok := s.data.Lookups.TeamGroups[entityName] + return ok + } + return false +} + // computeHierarchyPath builds the hierarchy path by walking parent references. // Must be called with s.mu held. func (s *Service) computeHierarchyPath(entityName, entityType string) []HierarchyPathEntry { @@ -572,12 +605,16 @@ func (s *Service) computeHierarchyPath(entityName, entityType string) []Hierarch return []HierarchyPathEntry{} } } else { - // Validate entity exists with given type - actualType := s.getEntityType(entityName) - if actualType == "" || !strings.EqualFold(actualType, entityType) { + // Normalize the caller-supplied type to canonical lowercase so the + // first path entry carries the same casing as the parent-derived + // entries below (which are normalized at load). + entityType = strings.ToLower(entityType) + // Validate the entity exists with the requested type specifically. + // Names are not unique across types, so we must check the type's own + // lookup rather than inferring a single type from the name. + if !s.entityExists(entityName, entityType) { return []HierarchyPathEntry{} } - entityType = actualType } path := []HierarchyPathEntry{{Name: entityName, Type: entityType}} @@ -699,39 +736,47 @@ func (s *Service) GetDescendantsTree(entityName string) *HierarchyNode { return nil } - // Build children map by scanning all entities - childrenMap := make(map[string][]struct{ name, typ string }) + // Build children map keyed by the parent's (name, type). Names are not + // unique across types, so keying by name alone would merge the children of + // different same-named parents into a single bucket. Parent types are + // canonical lowercase (normalized at load), matching the child types below. + childrenMap := make(map[HierarchyPathEntry][]struct{ name, typ string }) for name, team := range s.data.Lookups.Teams { if team.Parent != nil { - childrenMap[team.Parent.Name] = append(childrenMap[team.Parent.Name], struct{ name, typ string }{name, "team"}) + pk := HierarchyPathEntry{Name: team.Parent.Name, Type: team.Parent.Type} + childrenMap[pk] = append(childrenMap[pk], struct{ name, typ string }{name, "team"}) } } for name, org := range s.data.Lookups.Orgs { if org.Parent != nil { - childrenMap[org.Parent.Name] = append(childrenMap[org.Parent.Name], struct{ name, typ string }{name, "org"}) + pk := HierarchyPathEntry{Name: org.Parent.Name, Type: org.Parent.Type} + childrenMap[pk] = append(childrenMap[pk], struct{ name, typ string }{name, "org"}) } } for name, pillar := range s.data.Lookups.Pillars { if pillar.Parent != nil { - childrenMap[pillar.Parent.Name] = append(childrenMap[pillar.Parent.Name], struct{ name, typ string }{name, "pillar"}) + pk := HierarchyPathEntry{Name: pillar.Parent.Name, Type: pillar.Parent.Type} + childrenMap[pk] = append(childrenMap[pk], struct{ name, typ string }{name, "pillar"}) } } for name, tg := range s.data.Lookups.TeamGroups { if tg.Parent != nil { - childrenMap[tg.Parent.Name] = append(childrenMap[tg.Parent.Name], struct{ name, typ string }{name, "team_group"}) + pk := HierarchyPathEntry{Name: tg.Parent.Name, Type: tg.Parent.Type} + childrenMap[pk] = append(childrenMap[pk], struct{ name, typ string }{name, "team_group"}) } } - // Build tree recursively - var buildNode func(name, typ string, visited map[string]bool) HierarchyNode - buildNode = func(name, typ string, visited map[string]bool) HierarchyNode { - if visited[name] { + // Build tree recursively, guarding cycles by (name, type) for the same reason. + var buildNode func(name, typ string, visited map[HierarchyPathEntry]bool) HierarchyNode + buildNode = func(name, typ string, visited map[HierarchyPathEntry]bool) HierarchyNode { + key := HierarchyPathEntry{Name: name, Type: typ} + if visited[key] { return HierarchyNode{Name: name, Type: typ, Children: []HierarchyNode{}} } - visited[name] = true + visited[key] = true - children := childrenMap[name] + children := childrenMap[key] childNodes := make([]HierarchyNode, 0, len(children)) for _, c := range children { childNodes = append(childNodes, buildNode(c.name, c.typ, visited)) @@ -740,7 +785,7 @@ func (s *Service) GetDescendantsTree(entityName string) *HierarchyNode { return HierarchyNode{Name: name, Type: typ, Children: childNodes} } - node := buildNode(entityName, entityType, make(map[string]bool)) + node := buildNode(entityName, entityType, make(map[HierarchyPathEntry]bool)) return &node } @@ -1228,3 +1273,44 @@ func validateData(data *Data) error { } return nil } + +// normalizeData canonicalizes hierarchy type values to lowercase across the +// loaded data. Entity type values (team/org/pillar/team_group) can arrive from +// the data source in any casing, yet call sites key maps and compare on them. +// Canonicalizing here — at the deserialization boundary — is what lets query +// code read a type directly instead of defensively lowercasing it everywhere. +func normalizeData(data *Data) { + for name, team := range data.Lookups.Teams { + team.Type = strings.ToLower(team.Type) + if team.Parent != nil { + team.Parent.Type = strings.ToLower(team.Parent.Type) + } + data.Lookups.Teams[name] = team + } + for name, org := range data.Lookups.Orgs { + org.Type = strings.ToLower(org.Type) + if org.Parent != nil { + org.Parent.Type = strings.ToLower(org.Parent.Type) + } + data.Lookups.Orgs[name] = org + } + for name, pillar := range data.Lookups.Pillars { + pillar.Type = strings.ToLower(pillar.Type) + if pillar.Parent != nil { + pillar.Parent.Type = strings.ToLower(pillar.Parent.Type) + } + data.Lookups.Pillars[name] = pillar + } + for name, tg := range data.Lookups.TeamGroups { + tg.Type = strings.ToLower(tg.Type) + if tg.Parent != nil { + tg.Parent.Type = strings.ToLower(tg.Parent.Type) + } + data.Lookups.TeamGroups[name] = tg + } + for _, members := range data.Indexes.Membership.MembershipIndex { + for i := range members { + members[i].Type = strings.ToLower(members[i].Type) + } + } +} diff --git a/python/orgdatacore/_async.py b/python/orgdatacore/_async.py index 2794f1b..06761b1 100644 --- a/python/orgdatacore/_async.py +++ b/python/orgdatacore/_async.py @@ -532,19 +532,27 @@ def _get_hierarchy_path( if self._data is None: return [] + # Normalize the caller-supplied type to canonical lowercase so the first + # path entry carries the same casing as the parent-derived entries below + # (which are normalized at load). + entity_type = entity_type.lower() entity = self._get_entity_by_type(entity_name, entity_type) if entity is None: return [] path = [HierarchyPathEntry(name=entity_name, type=entity_type)] - visited = {entity_name} + # Guard against cycles by tracking visited entities by both name and + # type. Different entity types can share a name (e.g. a team and its + # parent team_group), so keying on name alone would stop the walk + # prematurely. + visited = {(entity_name, entity_type)} current: Team | Org | Pillar | TeamGroup | None = entity while current and current.parent: parent = current.parent - if parent.name in visited: + if (parent.name, parent.type) in visited: break - visited.add(parent.name) + visited.add((parent.name, parent.type)) path.append(HierarchyPathEntry(name=parent.name, type=parent.type)) current = self._get_entity_by_type(parent.name, parent.type) @@ -596,8 +604,10 @@ async def get_descendants_tree(self, entity_name: str) -> HierarchyNode | None: if not entity_type: return None - # Build children map by scanning all entities - children_map: dict[str, list[tuple[str, str]]] = {} + # Build children map keyed by the parent's (name, type). Names are + # not unique across types, so keying by name alone would merge the + # children of different same-named parents into a single bucket. + children_map: dict[tuple[str, str], list[tuple[str, str]]] = {} all_entities: list[tuple[str, Team | Org | Pillar | TeamGroup, str]] = [ *( (name, info, "team") @@ -619,15 +629,16 @@ async def get_descendants_tree(self, entity_name: str) -> HierarchyNode | None: for name, info, etype in all_entities: if info.parent: - if info.parent.name not in children_map: - children_map[info.parent.name] = [] - children_map[info.parent.name].append((name, etype)) + key = (info.parent.name, info.parent.type) + children_map.setdefault(key, []).append((name, etype)) - def build_node(name: str, type_: str, visited: set[str]) -> HierarchyNode: - if name in visited: + def build_node( + name: str, type_: str, visited: set[tuple[str, str]] + ) -> HierarchyNode: + if (name, type_) in visited: return HierarchyNode(name=name, type=type_, children=()) - visited.add(name) - children = children_map.get(name, []) + visited.add((name, type_)) + children = children_map.get((name, type_), []) child_nodes = tuple(build_node(n, t, visited) for n, t in children) return HierarchyNode(name=name, type=type_, children=child_nodes) @@ -647,7 +658,10 @@ async def get_user_organizations(self, slack_user_id: str) -> list[OrgInfo]: memberships = self._data.indexes.membership.membership_index.get(uid, ()) result: list[OrgInfo] = [] - seen: set[str] = set() + # Dedupe by (name, type): names are not unique across types, so + # keying on name alone would drop a legitimately distinct entity + # (e.g. a team_group sharing a team's name) from the result. + seen: set[tuple[str, str]] = set() type_to_org_info_type = { "org": OrgInfoType.ORGANIZATION, @@ -658,24 +672,27 @@ async def get_user_organizations(self, slack_user_id: str) -> list[OrgInfo]: for m in memberships: if m.type == MembershipType.ORG: - if m.name not in seen: + key = (m.name, "org") + if key not in seen: result.append( OrgInfo(name=m.name, type=OrgInfoType.ORGANIZATION) ) - seen.add(m.name) + seen.add(key) elif m.type == MembershipType.TEAM: - if m.name not in seen: + key = (m.name, "team") + if key not in seen: result.append(OrgInfo(name=m.name, type=OrgInfoType.TEAM)) - seen.add(m.name) + seen.add(key) hierarchy_path = self._get_hierarchy_path(m.name, "team") for entry in hierarchy_path[1:]: - if entry.name not in seen: + entry_key = (entry.name, entry.type) + if entry_key not in seen: org_type = type_to_org_info_type.get( - entry.type.lower(), OrgInfoType.ORGANIZATION + entry.type, OrgInfoType.ORGANIZATION ) result.append(OrgInfo(name=entry.name, type=org_type)) - seen.add(entry.name) + seen.add(entry_key) return result diff --git a/python/orgdatacore/_service.py b/python/orgdatacore/_service.py index a1e90d4..d6b1358 100644 --- a/python/orgdatacore/_service.py +++ b/python/orgdatacore/_service.py @@ -686,22 +686,27 @@ def get_user_organizations(self, slack_user_id: str) -> list[OrgInfo]: memberships = self._data.indexes.membership.membership_index.get(uid, ()) orgs: list[OrgInfo] = [] - seen: set[str] = set() + # Dedupe by (name, type): names are not unique across types, so + # keying on name alone would drop a legitimately distinct entity + # (e.g. a team_group sharing a team's name) from the result. + seen: set[tuple[str, str]] = set() for membership in memberships: if membership.type == MembershipType.ORG: - if membership.name not in seen: + key = (membership.name, "org") + if key not in seen: orgs.append( OrgInfo(name=membership.name, type=OrgInfoType.ORGANIZATION) ) - seen.add(membership.name) + seen.add(key) elif membership.type == MembershipType.TEAM: - if membership.name not in seen: + key = (membership.name, "team") + if key not in seen: orgs.append( OrgInfo(name=membership.name, type=OrgInfoType.TEAM) ) - seen.add(membership.name) + seen.add(key) hierarchy_path = self._get_hierarchy_path(membership.name, "team") self._add_hierarchy_path_items(orgs, seen, tuple(hierarchy_path)) @@ -711,7 +716,7 @@ def get_user_organizations(self, slack_user_id: str) -> list[OrgInfo]: def _add_hierarchy_path_items( self, orgs: list[OrgInfo], - seen: set[str], + seen: set[tuple[str, str]], hierarchy_path: tuple[HierarchyPathEntry, ...], ) -> None: """Add hierarchy path items to the orgs list, avoiding duplicates.""" @@ -722,12 +727,13 @@ def _add_hierarchy_path_items( "team": OrgInfoType.PARENT_TEAM, } for entry in hierarchy_path[1:]: - if entry.name not in seen: + key = (entry.name, entry.type) + if key not in seen: org_type = type_to_org_info_type.get( - entry.type.lower(), OrgInfoType.ORGANIZATION + entry.type, OrgInfoType.ORGANIZATION ) orgs.append(OrgInfo(name=entry.name, type=org_type)) - seen.add(entry.name) + seen.add(key) def _get_uid_from_slack_id(self, slack_id: str) -> str: """Get the UID for a given Slack ID.""" @@ -910,6 +916,10 @@ def _get_hierarchy_path( if self._data is None: return [] + # Normalize the caller-supplied type to canonical lowercase so the first + # path entry carries the same casing as the parent-derived entries below + # (which are normalized at load). + entity_type = entity_type.lower() entity = self._get_entity_by_type(entity_name, entity_type) if entity is None: return [] @@ -947,8 +957,10 @@ def get_descendants_tree(self, entity_name: str) -> HierarchyNode | None: if self._data is None: return None - # Build children map by scanning all entities - children_map: dict[str, list[tuple[str, str]]] = {} + # Build children map keyed by the parent's (name, type). Names are + # not unique across types, so keying by name alone would merge the + # children of different same-named parents into a single bucket. + children_map: dict[tuple[str, str], list[tuple[str, str]]] = {} all_entities: list[tuple[str, Team | Org | Pillar | TeamGroup, str]] = [ *( (name, info, "team") @@ -974,15 +986,16 @@ def get_descendants_tree(self, entity_name: str) -> HierarchyNode | None: for name, info, etype in all_entities: if info.parent: - if info.parent.name not in children_map: - children_map[info.parent.name] = [] - children_map[info.parent.name].append((name, etype)) + key = (info.parent.name, info.parent.type) + children_map.setdefault(key, []).append((name, etype)) - def build_node(name: str, type_: str, visited: set[str]) -> HierarchyNode: - if name in visited: + def build_node( + name: str, type_: str, visited: set[tuple[str, str]] + ) -> HierarchyNode: + if (name, type_) in visited: return HierarchyNode(name=name, type=type_, children=()) - visited.add(name) - children = children_map.get(name, []) + visited.add((name, type_)) + children = children_map.get((name, type_), []) child_nodes = tuple(build_node(n, t, visited) for n, t in children) return HierarchyNode(name=name, type=type_, children=child_nodes) diff --git a/python/orgdatacore/_types.py b/python/orgdatacore/_types.py index f4f9e0c..a819a48 100644 --- a/python/orgdatacore/_types.py +++ b/python/orgdatacore/_types.py @@ -5,7 +5,7 @@ from enum import StrEnum from typing import Any, BinaryIO, Protocol -from pydantic import BaseModel, ConfigDict, Field, model_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator class PIIMode(StrEnum): @@ -240,7 +240,23 @@ class Group(BaseModel): resolved_context: tuple[ContextItemInfo, ...] = () -class ParentInfo(BaseModel): +class _CanonicalType(BaseModel): + """Base for hierarchy entities whose ``type`` must be canonical lowercase. + + Entity type values (team/org/pillar/team_group) can arrive from the data + source in any casing, yet several call sites key maps or compare on them. + Canonicalizing to lowercase here — at the deserialization boundary — is what + lets query code read ``type`` directly instead of defensively lowercasing it + at every consumption site. + """ + + @field_validator("type", check_fields=False) + @classmethod + def _canonicalize_type(cls, value: str) -> str: + return value.lower() + + +class ParentInfo(_CanonicalType): """Parent reference for hierarchy traversal.""" model_config = ConfigDict(frozen=True) @@ -249,7 +265,7 @@ class ParentInfo(BaseModel): type: str = "" -class Team(BaseModel): +class Team(_CanonicalType): """Represents a team in the organizational data.""" model_config = ConfigDict(frozen=True) @@ -263,7 +279,7 @@ class Team(BaseModel): group: Group = Field(default_factory=Group) -class Org(BaseModel): +class Org(_CanonicalType): """Represents an organization in the organizational data.""" model_config = ConfigDict(frozen=True) @@ -277,7 +293,7 @@ class Org(BaseModel): group: Group = Field(default_factory=Group) -class Pillar(BaseModel): +class Pillar(_CanonicalType): """Represents a pillar in the organizational hierarchy.""" model_config = ConfigDict(frozen=True) @@ -291,7 +307,7 @@ class Pillar(BaseModel): group: Group = Field(default_factory=Group) -class TeamGroup(BaseModel): +class TeamGroup(_CanonicalType): """Represents a team group in the organizational hierarchy.""" model_config = ConfigDict(frozen=True) @@ -370,7 +386,7 @@ class Lookups(BaseModel): components: dict[str, Component] = Field(default_factory=dict) -class MembershipInfo(BaseModel): +class MembershipInfo(_CanonicalType): """Represents a membership entry with name and type.""" model_config = ConfigDict(frozen=True) diff --git a/python/tests/test_async_service.py b/python/tests/test_async_service.py index 6cd49d4..05f0bf9 100644 --- a/python/tests/test_async_service.py +++ b/python/tests/test_async_service.py @@ -7,7 +7,22 @@ import pytest -from orgdatacore import AsyncService, DataLoadError +from orgdatacore import ( + AsyncService, + Data, + DataLoadError, + HierarchyPathEntry, + Indexes, + Lookups, + MembershipIndex, + MembershipInfo, + Org, + OrgInfo, + ParentInfo, + SlackIDMappings, + Team, + TeamGroup, +) from orgdatacore._internal.testing import create_test_data_json @@ -461,6 +476,25 @@ async def test_get_hierarchy_path(self) -> None: assert path[0].name == "test-squad" assert path[0].type == "team" + @pytest.mark.asyncio + async def test_get_hierarchy_path_caller_type_case_insensitive(self) -> None: + """A caller-supplied type in any casing resolves to the same path. + + The type is normalized to canonical lowercase, so the first entry (which + is derived from the caller's argument) carries lowercase like the rest. + """ + source = AsyncFakeDataSource(data=create_test_data_json()) + service = AsyncService() + await service.load_from_data_source(source) + + baseline = await service.get_hierarchy_path("test-squad", "team") + for caller_type in ("TEAM", "Team"): + path = await service.get_hierarchy_path("test-squad", caller_type) + assert [(e.name, e.type) for e in path] == [ + (e.name, e.type) for e in baseline + ], caller_type + assert baseline[0].type == "team" + @pytest.mark.asyncio async def test_get_descendants_tree(self) -> None: """Test getting descendants tree for an entity.""" @@ -681,3 +715,175 @@ def __str__(self) -> str: assert stop_called, "source.stop() should have been called" assert service._watcher_task is None assert not service._watcher_running + + @pytest.mark.asyncio + async def test_get_hierarchy_path_name_collision(self) -> None: + """A team sharing its parent team_group's name must still reach the org. + + The async walk must key visited entities by (name, type): keying by name + alone stops at the team and never reaches the org, which wrongly denies + org membership (e.g. clusterbot's "Hybrid Platforms" check). + """ + service = AsyncService() + service._data = Data( + lookups=Lookups( + teams={ + "shared": Team( + name="shared", + type="team", + parent=ParentInfo(name="shared", type="team_group"), + ), + }, + team_groups={ + "shared": TeamGroup( + name="shared", + type="team_group", + parent=ParentInfo(name="acme", type="org"), + ), + }, + orgs={"acme": Org(name="acme", type="org")}, + ), + indexes=Indexes( + membership=MembershipIndex( + membership_index={ + "euser": (MembershipInfo(name="shared", type="team"),), + }, + ), + ), + ) + + path = await service.get_hierarchy_path("shared", "team") + assert path == [ + HierarchyPathEntry(name="shared", type="team"), + HierarchyPathEntry(name="shared", type="team_group"), + HierarchyPathEntry(name="acme", type="org"), + ] + + # The membership symptom: the org is only reachable by walking past the + # name-colliding team_group. + assert await service.is_employee_in_org("euser", "acme") + + @pytest.mark.asyncio + async def test_get_descendants_tree_name_collision(self) -> None: + """A team_group and a team sharing a name must not merge their subtrees. + + The async tree must key both the children map and the visited set by + (name, type): keying by name alone merges the two nodes' children and + stops the recursion early, producing a wrong tree. + """ + service = AsyncService() + service._data = Data( + lookups=Lookups( + orgs={"acme": Org(name="acme", type="org")}, + team_groups={ + "shared": TeamGroup( + name="shared", + type="team_group", + parent=ParentInfo(name="acme", type="org"), + ), + }, + teams={ + "shared": Team( + name="shared", + type="team", + parent=ParentInfo(name="shared", type="team_group"), + ), + "leaf": Team( + name="leaf", + type="team", + parent=ParentInfo(name="shared", type="team"), + ), + }, + ), + ) + + # acme(org) -> shared(team_group) -> shared(team) -> leaf(team) + tree = await service.get_descendants_tree("acme") + assert tree is not None + assert [(c.name, c.type) for c in tree.children] == [("shared", "team_group")] + team_group = tree.children[0] + assert [(c.name, c.type) for c in team_group.children] == [("shared", "team")] + team = team_group.children[0] + assert [(c.name, c.type) for c in team.children] == [("leaf", "team")] + + @pytest.mark.asyncio + async def test_get_descendants_tree_mixed_case_parent_type(self) -> None: + """Descendants resolve even when parent types arrive in non-lowercase. + + Hierarchy types are canonicalized to lowercase at the deserialization + boundary, so keying the children map on the parent type resolves + regardless of the casing the data source used. + """ + service = AsyncService() + service._data = Data( + lookups=Lookups( + orgs={"acme": Org(name="acme", type="ORG")}, + team_groups={ + "grp": TeamGroup( + name="grp", + type="Team_Group", + parent=ParentInfo(name="acme", type="ORG"), + ), + }, + teams={ + "t": Team( + name="t", + type="TEAM", + parent=ParentInfo(name="grp", type="TEAM_GROUP"), + ), + }, + ), + ) + + # acme(org) -> grp(team_group, parent type "ORG") -> t(team, parent "TEAM_GROUP") + tree = await service.get_descendants_tree("acme") + assert tree is not None + assert [(c.name, c.type) for c in tree.children] == [("grp", "team_group")] + grp = tree.children[0] + assert [(c.name, c.type) for c in grp.children] == [("t", "team")] + + @pytest.mark.asyncio + async def test_get_user_organizations_name_collision(self) -> None: + """A team sharing its parent team_group's name must not be deduped away. + + Deduping the result by (name, type) — not name alone — is required, + otherwise the team_group is wrongly dropped because a team with the same + name was already recorded. + """ + service = AsyncService() + service._data = Data( + lookups=Lookups( + teams={ + "shared": Team( + name="shared", + type="team", + parent=ParentInfo(name="shared", type="team_group"), + ), + }, + team_groups={ + "shared": TeamGroup( + name="shared", + type="team_group", + parent=ParentInfo(name="acme", type="org"), + ), + }, + orgs={"acme": Org(name="acme", type="org")}, + ), + indexes=Indexes( + membership=MembershipIndex( + membership_index={ + "euser": (MembershipInfo(name="shared", type="team"),), + }, + ), + slack_id_mappings=SlackIDMappings( + slack_uid_to_uid={"Suser": "euser"}, + ), + ), + ) + + # euser is in team "shared" -> team_group "shared" -> org "acme". + # All three must appear, each with its own type. + result = await service.get_user_organizations("Suser") + assert OrgInfo(name="shared", type="Team") in result + assert OrgInfo(name="shared", type="Team Group") in result + assert OrgInfo(name="acme", type="Organization") in result diff --git a/python/tests/test_hierarchy.py b/python/tests/test_hierarchy.py index a02ee60..dd1c973 100644 --- a/python/tests/test_hierarchy.py +++ b/python/tests/test_hierarchy.py @@ -27,6 +27,21 @@ def test_get_hierarchy_path_for_team(self, service: Service) -> None: assert path[1].name == "test-org" assert path[1].type == "org" + def test_get_hierarchy_path_caller_type_case_insensitive( + self, service: Service + ) -> None: + """A caller-supplied type in any casing resolves to the same path. + + The type is normalized to canonical lowercase, so the first entry (which + is derived from the caller's argument) carries lowercase like the rest. + """ + for caller_type in ("team", "TEAM", "Team"): + path = service.get_hierarchy_path("test-team", caller_type) + assert [(e.name, e.type) for e in path] == [ + ("test-team", "team"), + ("test-org", "org"), + ], caller_type + def test_get_hierarchy_path_for_deep_team(self, service: Service) -> None: """Test getting hierarchy path for a deeply nested team.""" path = service.get_hierarchy_path("platform-team", "team") @@ -150,6 +165,13 @@ def test_get_hierarchy_path_name_collision(self) -> None: # name-colliding team_group. assert service.is_employee_in_org("euser", "acme") + # Reciprocal: requesting the same name with the team_group type must + # resolve the team_group, not be rejected because a team shares the name. + assert service.get_hierarchy_path("shared", "team_group") == [ + HierarchyPathEntry(name="shared", type="team_group"), + HierarchyPathEntry(name="acme", type="org"), + ] + class TestGetDescendantsTree: """Tests for get_descendants_tree method.""" @@ -238,3 +260,80 @@ def test_get_descendants_tree_no_data(self, empty_service: Service) -> None: """Test get_descendants_tree returns None when no data loaded.""" tree = empty_service.get_descendants_tree("test-org") assert tree is None + + def test_get_descendants_tree_name_collision(self) -> None: + """A team_group and a team sharing a name must not merge their subtrees. + + The tree must key both the children map and the visited set by + (name, type): keying by name alone merges the two nodes' children and + stops the recursion early, producing a wrong tree. + """ + service = Service() + service._data = Data( + lookups=Lookups( + orgs={"acme": Org(name="acme", type="org")}, + team_groups={ + "shared": TeamGroup( + name="shared", + type="team_group", + parent=ParentInfo(name="acme", type="org"), + ), + }, + teams={ + "shared": Team( + name="shared", + type="team", + parent=ParentInfo(name="shared", type="team_group"), + ), + "leaf": Team( + name="leaf", + type="team", + parent=ParentInfo(name="shared", type="team"), + ), + }, + ), + ) + + # acme(org) -> shared(team_group) -> shared(team) -> leaf(team) + tree = service.get_descendants_tree("acme") + assert tree is not None + assert [(c.name, c.type) for c in tree.children] == [("shared", "team_group")] + team_group = tree.children[0] + assert [(c.name, c.type) for c in team_group.children] == [("shared", "team")] + team = team_group.children[0] + assert [(c.name, c.type) for c in team.children] == [("leaf", "team")] + + def test_get_descendants_tree_mixed_case_parent_type(self) -> None: + """Descendants resolve even when parent types arrive in non-lowercase. + + Hierarchy types are canonicalized to lowercase at the deserialization + boundary, so keying the children map on the parent type resolves + regardless of the casing the data source used. + """ + service = Service() + service._data = Data( + lookups=Lookups( + orgs={"acme": Org(name="acme", type="ORG")}, + team_groups={ + "grp": TeamGroup( + name="grp", + type="Team_Group", + parent=ParentInfo(name="acme", type="ORG"), + ), + }, + teams={ + "t": Team( + name="t", + type="TEAM", + parent=ParentInfo(name="grp", type="TEAM_GROUP"), + ), + }, + ), + ) + + # acme(org) -> grp(team_group, parent type "ORG") -> t(team, parent "TEAM_GROUP") + tree = service.get_descendants_tree("acme") + assert tree is not None + assert [(c.name, c.type) for c in tree.children] == [("grp", "team_group")] + grp = tree.children[0] + assert [(c.name, c.type) for c in grp.children] == [("t", "team")] diff --git a/python/tests/test_organization.py b/python/tests/test_organization.py index 6d0612b..dc65fe1 100644 --- a/python/tests/test_organization.py +++ b/python/tests/test_organization.py @@ -2,7 +2,20 @@ import pytest -from orgdatacore import OrgInfo, Service +from orgdatacore import ( + Data, + Indexes, + Lookups, + MembershipIndex, + MembershipInfo, + Org, + OrgInfo, + ParentInfo, + Service, + SlackIDMappings, + Team, + TeamGroup, +) class TestGetOrgByName: @@ -130,6 +143,51 @@ def test_no_duplicate_organizations(self, service: Service) -> None: assert key not in seen, f"Duplicate organization: {org}" seen.add(key) + def test_name_collision(self) -> None: + """A team sharing its parent team_group's name must not be deduped away. + + Deduping the result by (name, type) — not name alone — is required, + otherwise the team_group is wrongly dropped because a team with the same + name was already recorded. + """ + service = Service() + service._data = Data( + lookups=Lookups( + teams={ + "shared": Team( + name="shared", + type="team", + parent=ParentInfo(name="shared", type="team_group"), + ), + }, + team_groups={ + "shared": TeamGroup( + name="shared", + type="team_group", + parent=ParentInfo(name="acme", type="org"), + ), + }, + orgs={"acme": Org(name="acme", type="org")}, + ), + indexes=Indexes( + membership=MembershipIndex( + membership_index={ + "euser": (MembershipInfo(name="shared", type="team"),), + }, + ), + slack_id_mappings=SlackIDMappings( + slack_uid_to_uid={"Suser": "euser"}, + ), + ), + ) + + # euser is in team "shared" -> team_group "shared" -> org "acme". + # All three must appear, each with its own type. + result = service.get_user_organizations("Suser") + assert OrgInfo(name="shared", type="Team") in result + assert OrgInfo(name="shared", type="Team Group") in result + assert OrgInfo(name="acme", type="Organization") in result + class TestOrganizationalHierarchy: """Tests for team-to-org inheritance."""