Skip to content

Commit fbcf558

Browse files
jtdubclaude
andcommitted
Fix __hash__/__eq__ inconsistency in HConfigChild (#185)
__hash__ included new_in_config and order_weight but __eq__ intentionally excluded them, violating the Python invariant that a == b implies hash(a) == hash(b). __eq__ also checked tags but __hash__ did not include them. Align __hash__ to use the same fields as __eq__: text, tags, and children. Add five tests covering each dimension of the inconsistency and its practical impact on set deduplication and dict key lookup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 5634fbd commit fbcf558

2 files changed

Lines changed: 111 additions & 4 deletions

File tree

hier_config/child.py

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,7 @@ def __hash__(self) -> int:
6969
return hash(
7070
(
7171
self.text,
72-
# self.tags,
73-
# self.comments,
74-
self.new_in_config,
75-
self.order_weight,
72+
self.tags,
7673
*self.children,
7774
),
7875
)

tests/unit/test_child.py

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,3 +1144,113 @@ def test_idempotency_key_regex_trimmed_to_no_match() -> None:
11441144
key = driver._idempotency_key(child, (MatchRule(re_search=r"interface.*"),)) # noqa: SLF001 # pyright: ignore[reportPrivateUsage]
11451145
# Since "interface.*" doesn't match "logging console", should fall back to text
11461146
assert key == ("text|logging console",)
1147+
1148+
1149+
def test_child_hash_eq_consistency_new_in_config() -> None:
1150+
"""Test that equal HConfigChild objects have equal hashes regardless of new_in_config.
1151+
1152+
Validates the bug in issue #185: __hash__ includes new_in_config but __eq__ does not,
1153+
violating the Python invariant that a == b implies hash(a) == hash(b).
1154+
"""
1155+
platform = Platform.CISCO_IOS
1156+
config = get_hconfig(platform)
1157+
child1 = config.add_child("interface GigabitEthernet0/0")
1158+
config2 = get_hconfig(platform)
1159+
child2 = config2.add_child("interface GigabitEthernet0/0")
1160+
1161+
child1.new_in_config = False
1162+
child2.new_in_config = True
1163+
1164+
# These two children compare as equal (same text, no tags, no children)
1165+
assert child1 == child2
1166+
# Python invariant: equal objects must have equal hashes
1167+
assert hash(child1) == hash(child2)
1168+
1169+
1170+
def test_child_hash_eq_consistency_order_weight() -> None:
1171+
"""Test that equal HConfigChild objects have equal hashes regardless of order_weight.
1172+
1173+
Validates the bug in issue #185: __hash__ includes order_weight but __eq__ does not,
1174+
violating the Python invariant that a == b implies hash(a) == hash(b).
1175+
"""
1176+
platform = Platform.CISCO_IOS
1177+
config = get_hconfig(platform)
1178+
child1 = config.add_child("interface GigabitEthernet0/0")
1179+
config2 = get_hconfig(platform)
1180+
child2 = config2.add_child("interface GigabitEthernet0/0")
1181+
1182+
child1.order_weight = 0
1183+
child2.order_weight = 100
1184+
1185+
# These two children compare as equal (same text, no tags, no children)
1186+
assert child1 == child2
1187+
# Python invariant: equal objects must have equal hashes
1188+
assert hash(child1) == hash(child2)
1189+
1190+
1191+
def test_child_hash_eq_consistency_tags() -> None:
1192+
"""Test that __hash__ and __eq__ agree on whether tags affect equality.
1193+
1194+
Validates the bug in issue #185: __eq__ checks tags but __hash__ does not include
1195+
tags, meaning two objects that compare unequal could have the same hash (not a
1196+
correctness violation, but inconsistent) while also raising the question of whether
1197+
tags should be part of the hash.
1198+
"""
1199+
platform = Platform.CISCO_IOS
1200+
config = get_hconfig(platform)
1201+
child1 = config.add_child("interface GigabitEthernet0/0")
1202+
config2 = get_hconfig(platform)
1203+
child2 = config2.add_child("interface GigabitEthernet0/0")
1204+
1205+
child1.tags = frozenset({"safe"})
1206+
child2.tags = frozenset()
1207+
1208+
# __eq__ considers tags, so these are unequal
1209+
assert child1 != child2
1210+
# Since they are unequal, their hashes should differ to avoid excessive collisions
1211+
# (not strictly required by the invariant, but required for correctness in reverse:
1212+
# if hash(a) != hash(b) then a != b must hold — currently tags are in __eq__ but
1213+
# not __hash__, so unequal objects can share a hash, which means dict/set lookup
1214+
# will fall back to __eq__ unexpectedly)
1215+
assert hash(child1) != hash(child2)
1216+
1217+
1218+
def test_child_set_deduplication_with_new_in_config() -> None:
1219+
"""Test that equal HConfigChild objects are deduplicated correctly in sets.
1220+
1221+
Validates the practical impact of issue #185: when new_in_config differs,
1222+
two logically equal children occupy different set buckets, causing duplicates.
1223+
"""
1224+
platform = Platform.CISCO_IOS
1225+
config = get_hconfig(platform)
1226+
child1 = config.add_child("interface GigabitEthernet0/0")
1227+
config2 = get_hconfig(platform)
1228+
child2 = config2.add_child("interface GigabitEthernet0/0")
1229+
1230+
child1.new_in_config = False
1231+
child2.new_in_config = True
1232+
1233+
assert child1 == child2
1234+
# Equal objects must collapse to one entry in a set
1235+
assert len({child1, child2}) == 1
1236+
1237+
1238+
def test_child_dict_key_lookup_with_order_weight() -> None:
1239+
"""Test that HConfigChild objects with differing order_weight work as dict keys.
1240+
1241+
Validates the practical impact of issue #185: when order_weight differs, a
1242+
logically equal child cannot be found as a dict key.
1243+
"""
1244+
platform = Platform.CISCO_IOS
1245+
config = get_hconfig(platform)
1246+
child1 = config.add_child("interface GigabitEthernet0/0")
1247+
config2 = get_hconfig(platform)
1248+
child2 = config2.add_child("interface GigabitEthernet0/0")
1249+
1250+
child1.order_weight = 0
1251+
child2.order_weight = 100
1252+
1253+
assert child1 == child2
1254+
lookup: dict[HConfigChild, str] = {child1: "found"}
1255+
# child2 is equal to child1, so it must find the same dict entry
1256+
assert lookup[child2] == "found"

0 commit comments

Comments
 (0)