fix: only normalize values for hashing with operators that match
DeepHash._hash ran normalize_value_for_hashing of every registered custom operator on every value, without ever consulting the operator's match method. An operator scoped to one type therefore rewrote values of every other type. When such an operator returns None for the values it was never meant to see, all of those values hash to the same thing and unequal iterables compare as equal under ignore_order=True: [1, 2] and [2, 3] were reported as identical. Ask the operator whether it applies before normalizing. DeepHash walks a single object at a time and has no t1/t2 pair, so the value is passed to match as both sides via a small level-like view, which keeps operators scoped by type or by path behaving as they do while diffing. Operators that define no match method are still applied to everything. Fixes #606
This commit is contained in:
1 parent
25afc42a2c
commit
1476898aa3
2 files changed
+86
No files matched your search
@@ -78,6 +78,29 @@ def sha1hex(obj: Union[str, bytes]) -> str:
|
||||
default_hasher: Callable[[Union[str, bytes]], str] = sha256hex
|
||||
|
||||
|
||||
class HashLevel:
|
||||
"""
|
||||
Minimal level-like view of a single object, used to ask a custom operator whether
|
||||
it applies to a value that is about to be hashed.
|
||||
|
||||
DeepHash walks one object at a time, so there is no t1/t2 pair to hand to the
|
||||
operator's match method. The value is therefore exposed as both t1 and t2, which
|
||||
makes an operator scoped by type or by path behave the same way it would when the
|
||||
value is compared against itself.
|
||||
"""
|
||||
__slots__ = ('t1', 't2', 'up', 'down', '_path')
|
||||
|
||||
def __init__(self, obj: Any, parent: str) -> None:
|
||||
self.t1 = obj
|
||||
self.t2 = obj
|
||||
self.up = None
|
||||
self.down = None
|
||||
self._path = parent
|
||||
|
||||
def path(self, *args: Any, **kwargs: Any) -> str:
|
||||
return self._path
|
||||
|
||||
|
||||
def combine_hashes_lists(items: List[List[str]], prefix: Union[str, bytes]) -> str:
|
||||
"""
|
||||
Combines lists of hashes into one hash
|
||||
@@ -633,11 +656,30 @@ class DeepHash(Base):
|
||||
return (type(obj), obj)
|
||||
return obj
|
||||
|
||||
@staticmethod
|
||||
def _operator_matches(operator: Any, obj: Any, parent: str) -> bool:
|
||||
"""
|
||||
Whether a custom operator is in scope for the value that is about to be hashed.
|
||||
|
||||
An operator that declines to handle a value when diffing must not be allowed to
|
||||
rewrite that same value when hashing it for ignore_order=True, otherwise an
|
||||
operator scoped to one type silently normalizes every other type as well.
|
||||
|
||||
Operators without a match method are considered to apply to everything, which
|
||||
keeps older operators working.
|
||||
"""
|
||||
match = getattr(operator, 'match', None)
|
||||
if match is None:
|
||||
return True
|
||||
return bool(match(HashLevel(obj, parent)))
|
||||
|
||||
def _hash(self, obj: Any, parent: str, parents_ids: frozenset = EMPTY_FROZENSET) -> HashTuple:
|
||||
"""The main hash method"""
|
||||
counts = 1
|
||||
if self.custom_operators is not None:
|
||||
for operator in self.custom_operators:
|
||||
if not self._operator_matches(operator, obj, parent):
|
||||
continue
|
||||
func = getattr(operator, 'normalize_value_for_hashing', None)
|
||||
if func is None:
|
||||
raise NotImplementedError(f"{operator.__class__.__name__} needs to define a normalize_value_for_hashing method to be compatible with ignore_order=True or iterable_compare_func.".format(operator))
|
||||
|
||||
@@ -426,3 +426,47 @@ class TestOperators:
|
||||
diff3 = DeepDiff(deepcopy(d1), deepcopy(d2), ignore_order=True, custom_operators=[RemoveGUIDsOperatorIgnoreOrderReady(types=[str])])
|
||||
assert not diff3, "We shouldn't have a diff because we have normalized the string values to be all the same vlues."
|
||||
|
||||
def test_normalize_value_for_hashing_is_scoped_by_match(self):
|
||||
normalized = []
|
||||
|
||||
class StrOnlyOperator(BaseOperatorPlus):
|
||||
def match(self, level) -> bool:
|
||||
return isinstance(level.t1, str) and isinstance(level.t2, str)
|
||||
|
||||
def give_up_diffing(self, level, diff_instance) -> bool:
|
||||
return level.t1 == level.t2
|
||||
|
||||
def normalize_value_for_hashing(self, parent: Any, obj: Any) -> Any:
|
||||
normalized.append(obj)
|
||||
return obj
|
||||
|
||||
t1 = [1, 2]
|
||||
t2 = [2, 3]
|
||||
expected = DeepDiff(t1, t2, ignore_order=True)
|
||||
|
||||
diff = DeepDiff(t1, t2, ignore_order=True, custom_operators=[StrOnlyOperator()])
|
||||
assert expected == diff, "An operator that only matches strings must not change how integers are diffed."
|
||||
assert [] == normalized, (
|
||||
"normalize_value_for_hashing must not be called for values the operator does not match.")
|
||||
|
||||
def test_normalize_value_for_hashing_not_called_for_unmatched_types(self):
|
||||
normalized = []
|
||||
|
||||
class LowerCaseStringsOperator(BaseOperatorPlus):
|
||||
def match(self, level) -> bool:
|
||||
return isinstance(level.t1, str) and isinstance(level.t2, str)
|
||||
|
||||
def give_up_diffing(self, level, diff_instance) -> bool:
|
||||
return False
|
||||
|
||||
def normalize_value_for_hashing(self, parent: Any, obj: Any) -> Any:
|
||||
# Raises AttributeError if it is handed anything other than a string.
|
||||
normalized.append(obj)
|
||||
return obj.lower()
|
||||
|
||||
DeepDiff(["A", 1], ["a", 1], ignore_order=True,
|
||||
custom_operators=[LowerCaseStringsOperator()])
|
||||
assert normalized, "The operator should still be applied to the values it matches."
|
||||
assert all(isinstance(obj, str) for obj in normalized), (
|
||||
"A string-only operator must never be handed a non-string to normalize.")
|
||||
|
||||
Reference in new issue
Block a user