From 3cc1b8b97294310d5a84dd4d297d5fe20a01481a Mon Sep 17 00:00:00 2001 From: thiagoalessio Date: Tue, 1 Sep 2026 14:42:36 +0200 Subject: [PATCH 1/5] OCPCRT-641: Validate explicit entity type against its own lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetHierarchyPath validated an explicitly-requested entity type via getEntityType, which scans lookups in a fixed order and returns only the first-matching type for a name. When a team and a team_group shared a name, GetHierarchyPath(name, "team_group") was rejected and returned an empty path even though the team_group existed — the same "names are not unique across types" root cause as the primary fix, on the type- validation half of the function. Fix (Go): add an entityExists(name, type) helper that checks the type-specific lookup, and use it to validate an explicitly-supplied type. Name inference via getEntityType is kept only for the empty-type case. This aligns Go with the Python implementation, which already validated via _get_entity_by_type; no Python production change is needed. Tests: extend the name-collision case in both languages with the reciprocal team_group lookup, asserting it resolves to the org. The Go assertion fails on the pre-fix code. API parity harness still reports identical Go/Python output. Assisted-by: Claude Opus 4.8 --- go/hierarchy_test.go | 16 ++++++++++++++++ go/service.go | 33 +++++++++++++++++++++++++++++---- python/tests/test_hierarchy.py | 7 +++++++ 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/go/hierarchy_test.go b/go/hierarchy_test.go index ac0f052..182f925 100644 --- a/go/hierarchy_test.go +++ b/go/hierarchy_test.go @@ -165,6 +165,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) { diff --git a/go/service.go b/go/service.go index 6f1fefa..3f80305 100644 --- a/go/service.go +++ b/go/service.go @@ -558,6 +558,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 +597,12 @@ 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) { + // 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}} diff --git a/python/tests/test_hierarchy.py b/python/tests/test_hierarchy.py index a02ee60..5043bb8 100644 --- a/python/tests/test_hierarchy.py +++ b/python/tests/test_hierarchy.py @@ -150,6 +150,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.""" From 770687ec7bc711709330fa2a0e4d4d21decee10f Mon Sep 17 00:00:00 2001 From: thiagoalessio Date: Tue, 1 Sep 2026 14:50:38 +0200 Subject: [PATCH 2/5] OCPCRT-641: Fix GetDescendantsTree for entities sharing a name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The downward tree builder had the same "names are not unique across types" root cause in two places. The children map was keyed by parent name only, so children of same-named entities of different types (e.g. a team and a team_group both named "shared") were merged into one bucket. The recursion's visited set was likewise keyed by name only, so a same-named descendant was treated as already-visited and returned no children. Together these produced a structurally wrong tree. Fix: key both the children map and the visited set by (name, type) in both implementations — Go reuses the comparable HierarchyPathEntry as the key; Python uses a (name, type) tuple. Tests: add a name-collision case to the descendants tests in both languages (org -> team_group "shared" -> team "shared" -> leaf), asserting each level keeps its own single child. Both fail on the pre-fix code. API parity harness still reports identical Go/Python output. Assisted-by: Claude Opus 4.8 --- go/hierarchy_test.go | 37 ++++++++++++++++++++++++++++++ go/service.go | 33 +++++++++++++++----------- python/orgdatacore/_service.py | 21 +++++++++-------- python/tests/test_hierarchy.py | 42 ++++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 22 deletions(-) diff --git a/go/hierarchy_test.go b/go/hierarchy_test.go index 182f925..b7ede67 100644 --- a/go/hierarchy_test.go +++ b/go/hierarchy_test.go @@ -319,3 +319,40 @@ 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) + } +} diff --git a/go/service.go b/go/service.go index 3f80305..abb2706 100644 --- a/go/service.go +++ b/go/service.go @@ -724,39 +724,46 @@ 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. + 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)) @@ -765,7 +772,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 } diff --git a/python/orgdatacore/_service.py b/python/orgdatacore/_service.py index a1e90d4..e816a07 100644 --- a/python/orgdatacore/_service.py +++ b/python/orgdatacore/_service.py @@ -947,8 +947,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 +976,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/tests/test_hierarchy.py b/python/tests/test_hierarchy.py index 5043bb8..99a1318 100644 --- a/python/tests/test_hierarchy.py +++ b/python/tests/test_hierarchy.py @@ -245,3 +245,45 @@ 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")] From d32d046da718df33a38d0314c4a65db527519360 Mon Sep 17 00:00:00 2001 From: thiagoalessio Date: Tue, 1 Sep 2026 16:13:55 +0200 Subject: [PATCH 3/5] OCPCRT-641: Fix the async client's hierarchy name-collision defects The public AsyncService in _async.py is a full duplicate of the sync Service and was never updated by #29 or the preceding two commits, so it still carried the "names are not unique across types" root cause in two places: - _get_hierarchy_path keyed its visited set by name only. This is the primary #29 bug itself, still live in the async client: the upward walk stops at a team whose name collides with its parent team_group and never reaches the org, so async is_employee_in_org wrongly denies org membership (the original clusterbot "Hybrid Platforms" symptom). - get_descendants_tree keyed both its children map and visited set by name only, merging same-named entities' children and cutting the recursion short, producing a structurally wrong tree. Fix: key visited and the children map by (name, type), mirroring the already-fixed sync Service line for line. Tests: add name-collision cases for both async methods; both fail on the pre-fix code and pass after. Full suite, ruff, mypy --strict, and the Go/Python parity harness all pass. This is Python-only (Go has no async twin), so parity is unaffected. Assisted-by: Claude Opus 4.8 --- python/orgdatacore/_async.py | 31 +++++---- python/tests/test_async_service.py | 105 ++++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 13 deletions(-) diff --git a/python/orgdatacore/_async.py b/python/orgdatacore/_async.py index 2794f1b..1421e24 100644 --- a/python/orgdatacore/_async.py +++ b/python/orgdatacore/_async.py @@ -537,14 +537,18 @@ def _get_hierarchy_path( 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 +600,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 +625,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) diff --git a/python/tests/test_async_service.py b/python/tests/test_async_service.py index 6cd49d4..3930627 100644 --- a/python/tests/test_async_service.py +++ b/python/tests/test_async_service.py @@ -7,7 +7,20 @@ import pytest -from orgdatacore import AsyncService, DataLoadError +from orgdatacore import ( + AsyncService, + Data, + DataLoadError, + HierarchyPathEntry, + Indexes, + Lookups, + MembershipIndex, + MembershipInfo, + Org, + ParentInfo, + Team, + TeamGroup, +) from orgdatacore._internal.testing import create_test_data_json @@ -681,3 +694,93 @@ 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")] From aa5ac75aebd8d9fd356224f32d5507345dfc2aaf Mon Sep 17 00:00:00 2001 From: thiagoalessio Date: Tue, 1 Sep 2026 16:38:43 +0200 Subject: [PATCH 4/5] OCPCRT-641: Fix GetUserOrganizations dedup for entities sharing a name GetUserOrganizations (and its helper addHierarchyPathItems) deduped the returned entities with a set keyed by name only. Because names are not unique across types, a legitimately distinct entity was dropped whenever an earlier one shared its name -- e.g. a user in team "shared" whose hierarchy also contains team_group "shared" would get the team_group silently omitted from the result. Same root cause as the hierarchy-path and descendants-tree fixes; lower impact (an entry is missing rather than membership being denied), but the same family of bug. Fix: key the dedup set by (name, type) in all three implementations -- Go Service, Python Service, and the public async AsyncService (Go reuses the comparable HierarchyPathEntry; Python uses a (name, type) tuple). Tests: add a name-collision case to the user-organizations tests in Go, Python sync, and Python async (team "shared" -> team_group "shared" -> org "acme"), asserting all three entities appear with their own type. All fail on the pre-fix code. Full suites, ruff, mypy --strict, and the Go/Python parity harness pass. Assisted-by: Claude Opus 4.8 --- go/organization_test.go | 46 +++++++++++++++++++++++ go/service.go | 22 +++++++---- python/orgdatacore/_async.py | 20 ++++++---- python/orgdatacore/_service.py | 22 +++++++---- python/tests/test_async_service.py | 48 ++++++++++++++++++++++++ python/tests/test_organization.py | 60 +++++++++++++++++++++++++++++- 6 files changed, 194 insertions(+), 24 deletions(-) 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 abb2706..80a1cc8 100644 --- a/go/service.go +++ b/go/service.go @@ -458,19 +458,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 +484,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 +496,14 @@ func addHierarchyPathItems(orgs *[]OrgInfo, seen *map[string]bool, hierarchyPath if i == 0 { continue } - if !(*seen)[entry.Name] { + key := HierarchyPathEntry{Name: entry.Name, Type: strings.ToLower(entry.Type)} + if !(*seen)[key] { orgType, ok := typeToOrgInfoType[strings.ToLower(entry.Type)] if !ok { orgType = OrgTypeOrganization } *orgs = append(*orgs, OrgInfo{Name: entry.Name, Type: orgType}) - (*seen)[entry.Name] = true + (*seen)[key] = true } } } diff --git a/python/orgdatacore/_async.py b/python/orgdatacore/_async.py index 1421e24..492b13b 100644 --- a/python/orgdatacore/_async.py +++ b/python/orgdatacore/_async.py @@ -654,7 +654,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, @@ -665,24 +668,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.lower()) + if entry_key not in seen: org_type = type_to_org_info_type.get( entry.type.lower(), 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 e816a07..8cd44f9 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.lower()) + if key not in seen: org_type = type_to_org_info_type.get( entry.type.lower(), 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.""" diff --git a/python/tests/test_async_service.py b/python/tests/test_async_service.py index 3930627..0079590 100644 --- a/python/tests/test_async_service.py +++ b/python/tests/test_async_service.py @@ -17,7 +17,9 @@ MembershipIndex, MembershipInfo, Org, + OrgInfo, ParentInfo, + SlackIDMappings, Team, TeamGroup, ) @@ -784,3 +786,49 @@ async def test_get_descendants_tree_name_collision(self) -> None: 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_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_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.""" From 5dc7d38125e02c95e550ba43959a908bc046a819 Mon Sep 17 00:00:00 2001 From: thiagoalessio Date: Thu, 10 Sep 2026 16:18:57 +0200 Subject: [PATCH 5/5] OCPCRT-641: Normalize hierarchy types at boundaries, not call sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit's remaining finding: ParentInfo.type is a free-form string, but descendant/ancestor traversal keys maps and the visited set on the canonical lowercase type. A parent ref such as "TEAM_GROUP" produced a child-map key that could never match ("shared", "team_group"), so the descendant was silently dropped. The async hierarchy cycle check had the same case-sensitivity problem for caller-supplied types. Rather than defensively lowercasing at every consumption site (the approach the review sketched), canonicalize type to lowercase once, at the two boundaries where untrusted casing enters: - Data source: normalize every entity type, parent-ref type, and membership type as data is loaded. Go gets a normalizeData() pass in LoadFromDataSource; Python gets a _CanonicalType pydantic base whose field_validator lowercases type on Team/Org/Pillar/TeamGroup/ ParentInfo/MembershipInfo at deserialization. Redundant strings.ToLower /.lower() calls at the fixed call sites are removed, so query code now reads type directly. - Caller argument: GetHierarchyPath/get_hierarchy_path (sync + async) lowercase the caller-supplied entity_type before using it, so the first path entry — derived from that argument — carries canonical casing too, matching the parent-derived entries. Tests: add mixed-case regression cases in Go, Python sync, and Python async covering both a mixed-case ParentInfo.type descendant scenario and a mixed-case caller-supplied type. All fail on the pre-fix code. Full Go and Python suites, ruff, mypy --strict, and the Go/Python parity harness all pass. Assisted-by: Claude Opus 4.8 --- go/hierarchy_test.go | 69 ++++++++++++++++++++++++++++++ go/service.go | 56 ++++++++++++++++++++++-- python/orgdatacore/_async.py | 8 +++- python/orgdatacore/_service.py | 8 +++- python/orgdatacore/_types.py | 30 ++++++++++--- python/tests/test_async_service.py | 55 ++++++++++++++++++++++++ python/tests/test_hierarchy.py | 50 ++++++++++++++++++++++ 7 files changed, 261 insertions(+), 15 deletions(-) diff --git a/go/hierarchy_test.go b/go/hierarchy_test.go index b7ede67..8689e09 100644 --- a/go/hierarchy_test.go +++ b/go/hierarchy_test.go @@ -1,6 +1,8 @@ package orgdatacore import ( + "context" + "encoding/json" "testing" ) @@ -356,3 +358,70 @@ func TestGetDescendantsTreeNameCollision(t *testing.T) { 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/service.go b/go/service.go index 80a1cc8..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 } } @@ -496,9 +498,9 @@ func addHierarchyPathItems(orgs *[]OrgInfo, seen *map[HierarchyPathEntry]bool, h if i == 0 { continue } - key := HierarchyPathEntry{Name: entry.Name, Type: strings.ToLower(entry.Type)} + key := HierarchyPathEntry{Name: entry.Name, Type: entry.Type} if !(*seen)[key] { - orgType, ok := typeToOrgInfoType[strings.ToLower(entry.Type)] + orgType, ok := typeToOrgInfoType[entry.Type] if !ok { orgType = OrgTypeOrganization } @@ -603,6 +605,10 @@ func (s *Service) computeHierarchyPath(entityName, entityType string) []Hierarch return []HierarchyPathEntry{} } } else { + // 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. @@ -732,7 +738,8 @@ func (s *Service) GetDescendantsTree(entityName string) *HierarchyNode { // 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. + // 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 { @@ -1266,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 492b13b..06761b1 100644 --- a/python/orgdatacore/_async.py +++ b/python/orgdatacore/_async.py @@ -532,6 +532,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 [] @@ -682,10 +686,10 @@ async def get_user_organizations(self, slack_user_id: str) -> list[OrgInfo]: hierarchy_path = self._get_hierarchy_path(m.name, "team") for entry in hierarchy_path[1:]: - entry_key = (entry.name, entry.type.lower()) + 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_key) diff --git a/python/orgdatacore/_service.py b/python/orgdatacore/_service.py index 8cd44f9..d6b1358 100644 --- a/python/orgdatacore/_service.py +++ b/python/orgdatacore/_service.py @@ -727,10 +727,10 @@ def _add_hierarchy_path_items( "team": OrgInfoType.PARENT_TEAM, } for entry in hierarchy_path[1:]: - key = (entry.name, entry.type.lower()) + 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(key) @@ -916,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 [] 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 0079590..05f0bf9 100644 --- a/python/tests/test_async_service.py +++ b/python/tests/test_async_service.py @@ -476,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.""" @@ -787,6 +806,42 @@ async def test_get_descendants_tree_name_collision(self) -> None: 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. diff --git a/python/tests/test_hierarchy.py b/python/tests/test_hierarchy.py index 99a1318..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") @@ -287,3 +302,38 @@ def test_get_descendants_tree_name_collision(self) -> None: 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")]