Repository navigation
Compare read-only containers recursively in test operations - #257
liugedragon wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
2 open issues
What changed in this PR
This PR updates JSON Patch test operation equality semantics so read-only mapping types (e.g., MappingProxyType) are compared structurally instead of via Python’s default equality, preventing True/1 and False/0 from being treated as equal.
Changes:
- Broaden
_json_equalmapping comparison fromMutableMappingtoMapping. - Add regression tests ensuring
testfails for bool-vs-int mismatches on read-only mappings and does not mutate input documents on success/failure.
| File | Description |
|---|---|
jsonpatch.py |
Fixes _json_equal to structurally compare read-only mappings, preserving RFC6902 bool/number distinction. |
tests.py |
Adds regression coverage for MappingProxyType and nested read-only mapping scenarios (success and failure). |
Review effort: Lite
| if isinstance(first, bool) or isinstance(second, bool): | ||
| return isinstance(first, bool) and isinstance(second, bool) and \ | ||
| first == second | ||
| if isinstance(first, MutableMapping) and \ | ||
| isinstance(second, MutableMapping): | ||
| if isinstance(first, Mapping) and isinstance(second, Mapping): | ||
| return len(first) == len(second) and all( | ||
| key in second and _json_equal(first[key], second[key]) | ||
| for key in first) |
There was a problem hiding this comment.
Addressed in 51b32f1. The equality check now accepts non-text Sequence implementations, consistent with the stubs and the immutable-container handling in _restore; strings and bytes stay scalar, and write operations still require mutable containers.
The new regressions cover tuple/list comparisons in both directions, a generic read-only sequence, nested containers, Boolean/number distinctions, numeric equivalence, and unchanged inputs. Of the 95 new subcases, 33 fail on the previous PR revision. The updated suite passes locally: 322 unit tests (one skipped) and 26 standard-profile property tests.
| def test_test_read_only_mapping_preserves_boolean_types(self): | ||
| for mapping_type in (dict, MappingProxyType): | ||
| for nested in (False, True): | ||
| for actual, expected in ((True, 1), (False, 0), | ||
| (1, True), (0, False)): | ||
| with self.subTest(mapping_type=mapping_type, nested=nested, | ||
| actual=actual, expected=expected): | ||
| mapping = mapping_type({'a': actual}) | ||
| doc = {'nested': mapping} if nested else mapping | ||
| path = '/nested' if nested else '' | ||
| original = json.dumps(doc, default=dict) | ||
| patch = [{'op': 'test', 'path': path, | ||
| 'value': {'a': expected}}] | ||
|
|
||
| with self.assertRaises(jsonpatch.JsonPatchTestFailed): | ||
| jsonpatch.apply_patch(doc, patch, in_place=True) | ||
|
|
||
| self.assertEqual(json.dumps(doc, default=dict), original) |
liugedragon
changed the title
Whole read-only containers can fall back to Python equality instead of applying JSON equality recursively. For example,
MappingProxyType({'a': True})passes a roottestagainst{'a': 1}, and(True,)passes against(1,). Equivalent list and tuple values can also compare unequal.Use
Mappingand non-textSequencein_json_equal. Both are already accepted by the module and documented in its stubs; immutable sequences are also handled by the existing restoration code. Objects and arrays are compared recursively under the RFC 6902 rules. Strings and bytes remain scalar values, and mutation requirements are unchanged.Regression coverage includes root and nested read-only mappings, tuple/list comparisons in both directions, a generic read-only sequence, Boolean/number rejection, numeric
1/1.0equivalence, length/order differences, text/bytes exclusions, and preservation of input identity and content. The sequence follow-up adds 95 subcases; 33 fail on the previous PR revision and pass after the fix.Validation on Windows / Python 3.13.4:
python tests.py: 322 tests run, OK (1 skipped).python property_tests.py: 26 tests, OK.git diff --check: passed.Earlier validation: a 5,000-example single-process stress run hit a Hypothesis MemoryError and replay error; the affected two properties each passed at the same budget in fresh processes. On the previous CI revision, all unit-test and property-test matrix steps passed; coverage-upload jobs failed while downloading the Coveralls binary/checksum.