Repository navigation
Conversation
seperman
left a comment
There was a problem hiding this comment.
Static review: #636 Skip excluded objects before calling custom operators
Summary: This reorders the start of DeepDiff._diff so that _skip_this(level) runs before _use_custom_operator(level). Custom operators' match()/give_up_diffing() are no longer called for paths excluded by exclude_paths, exclude_regex_paths, exclude_types, the *_obj_callback options, or paths outside include_paths. As far as I can tell from reading the code, this fixes the scenario in #633.
Overlap with other PRs
- I found no other PR, open or merged, for #633. Nothing merged on
devaddresses it. - The patch merges cleanly (
git merge-tree) with #634, #620 and #630. It only touches the top of_diff(diff.py:2139-2149), which none of those PRs modify.
Security
No security concerns found in static review.
Correctness
-
New order in
deepdiff/diff.py:2139-2149:- identity shortcut, only when there are no custom operators
_skip_this- custom operators
- identity shortcut
Without custom operators the behaviour is the same as before: both early returns still happen, and the cheap identity check now comes first.
-
With custom operators, the identity check still runs after the operators (diff.py:2148). Operators therefore still see identical objects on included paths. The existing
test_custom_operators_should_not_equalrelies on that: it reports'unexpected:still'for10vs10. It keeps working. -
Minor behaviour change: when custom operators are set,
_skip_thisnow runs for identical objects too. Soexclude_obj_callback/include_obj_callbackmay be called more often than before. This only matters for callbacks with side effects.
Tests
tests/test_operators.py:18and:51look like they would fail ondev. Ondevthe operator'smatch()is called forroot['b'], and in the excluded-root case theUnexpectedOperator.matchraises. They pass with the reorder.- The tests cover the shorthand and full
exclude_paths, regex, callback,exclude_types, and bothprevent_defaultvalues. - I don't see a CI run for this branch yet.
Suggestions
- Optional: add a case with
include_paths, checking that operators are not invoked outside the included subtree, since_skip_thiscovers that too. - Optional: a CHANGELOG line, and a sentence in
custom.rstsaying that operators are not called for excluded paths. Some users may have relied on the old behaviour.
AI-assisted static review; the code was not executed.
When a path is excluded, DeepDiff still calls custom operators'
match()andgive_up_diffing()methods before discarding the comparison result. This can run expensive comparisons or raise exceptions for objects that the caller explicitly excluded.Apply the existing exclusion check before invoking custom operators. Preserve the identity shortcut when there are no custom operators, and continue allowing custom operators to inspect identical objects on included paths.
Closes #633.
Adds 10 regression cases covering shorthand and full paths, regex and callback exclusions, excluded roots and types, both operator return values, and custom reports for identical objects on included paths. All 10 fail on the unchanged
devbaseline. The original issue example now compares onlyroot['a']and still returns an empty diff.Validation on Windows, Python 3.12.7, with dependencies from
uv.lock:python -X utf8 -m pytest tests --benchmark-disable --cov=deepdiff --cov-report=term --tb=short -q: 1,281 passed, 45 skipped, 1 failed; 92% coverage.TestDeltaClassPollution.test_remote_code_execution, expectsposix.systembut receivesnt.systemon Windows. The same failure was reproduced on baseline59acdd9cbb50fdbc30ef7cde2aa6f290ac0a5192using the same environment.python -m flake8 deepdiff tests/test_operators.py --count --select=E9,F63,F7,F82 --show-source --statistics: passed.git diff --check: passed.The full Python/platform CI matrix and optional slow tests have not been run locally.