Repository navigation
BUG: Reject extra JSON content in strict comparisons #567
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,11 +49,11 @@ def _generate_pprint_json(value): | |
| return json.dumps(value, sort_keys=True, indent=4) | ||
|
|
||
|
|
||
| def _is_dict_same(expected, actual, ignore_value_of_keys, ignore_missing_keys): | ||
| for key in expected: | ||
| if key in ignore_value_of_keys: | ||
| continue | ||
| # Strict mode already checked the key sets; containment still needs this check. | ||
| if key not in actual: | ||
| return False, \ | ||
| Stack().append( | ||
|
|
@@ -62,16 +62,18 @@ def _is_dict_same(expected, actual, ignore_value_of_keys): | |
| expected, | ||
| actual)) | ||
|
|
||
| are_same_flag, stack = _are_same( | ||
| expected[key], actual[key], ignore_value_of_keys, ignore_missing_keys) | ||
| if not are_same_flag: | ||
| return False, \ | ||
| stack.append(StackItem('Different values', expected[key], actual[key])) | ||
| return True, Stack() | ||
|
|
||
|
|
||
| def _is_list_same(expected, actual, ignore_value_of_keys, ignore_missing_keys): | ||
| for i in range(len(expected)): | ||
| are_same_flag, stack = _are_same( | ||
| expected[i], actual[i], ignore_value_of_keys, ignore_missing_keys) | ||
| if not are_same_flag: | ||
| return False, \ | ||
| stack.append( | ||
|
|
@@ -114,45 +116,46 @@ def _are_same(expected, actual, ignore_value_of_keys, ignore_missing_keys=False) | |
| if type(expected) in (int, str, bool, float): | ||
| return expected == actual, Stack() | ||
|
|
||
|
Member
There was a problem hiding this comment.
🤖 Comment generated by Claude (Claude Code), posted on behalf of @fedorov.
Contributor
Author
There was a problem hiding this comment. Removed the unused |
||
| 10BC8 | ||
| if isinstance(expected, dict): | ||
| if not ignore_missing_keys: | ||
| expected_keys = set(expected) - ignore_value_of_keys | ||
| actual_keys = set(actual) - ignore_value_of_keys | ||
| if expected_keys != actual_keys: | ||
| return False, Stack().append( | ||
| StackItem('Different keys: Missing keys: {0}; Extra keys: {1}' | ||
| .format(sorted(expected_keys - actual_keys), | ||
| sorted(actual_keys - expected_keys)), expected, actual)) | ||
| return _is_dict_same(expected, actual, ignore_value_of_keys, | ||
| ignore_missing_keys) | ||
|
|
||
| if isinstance(expected, list): | ||
| if (len(expected) > len(actual) or | ||
| (not ignore_missing_keys and len(expected) != len(actual))): | ||
| return False, Stack().append( | ||
| StackItem('Length Mismatch: Expected Length: {0}, Actual Length: {1}' | ||
| .format(len(expected), len(actual)), expected, actual)) | ||
| return _is_list_same(expected, actual, ignore_value_of_keys, | ||
| ignore_missing_keys) | ||
|
|
||
| return False, Stack().append(StackItem('Unhandled Type: {0}'.format(type(expected)), expected, actual)) | ||
|
|
||
|
|
||
| def are_same(original_a, original_b, ignore_list_order_recursively=False, ignore_missing_keys=False, ignore_value_of_keys=None): | ||
| ignore_value_of_keys = set(ignore_value_of_keys or ()) | ||
| a = _bottom_up_sort(original_a) if ignore_list_order_recursively else original_a | ||
| b = _bottom_up_sort(original_b) if ignore_list_order_recursively else original_b | ||
| return _are_same(a, b, ignore_value_of_keys, ignore_missing_keys) | ||
|
|
||
|
|
||
| def contains(expected_original, actual_original, ignore_list_order_recursively=False, ignore_value_of_keys=None): | ||
| ignore_value_of_keys = set(ignore_value_of_keys or ()) | ||
| actual = _bottom_up_sort(actual_original) if ignore_list_order_recursively else actual_original | ||
| expected = _bottom_up_sort(expected_original) if ignore_list_order_recursively else expected_original | ||
| return _are_same(expected, actual, ignore_value_of_keys, True) | ||
|
|
||
|
|
||
| def json_are_same(a, b, ignore_list_order_recursively=False, ignore_value_of_keys=None): | ||
| return are_same(json.loads(a), json.loads(b), | ||
| ignore_list_order_recursively=ignore_list_order_recursively, | ||
| ignore_value_of_keys=ignore_value_of_keys) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| import json | ||
| from pathlib import Path | ||
| import subprocess | ||
| import sys | ||
| import tempfile | ||
| import unittest | ||
|
|
||
| import jsoncompare | ||
|
|
||
|
|
||
| class TestJSONCompare(unittest.TestCase): | ||
| def test_equal_nested_values(self): | ||
| value = {'segments': [{'id': 1, 'data': [1, 2, 2]}], 'empty': None} | ||
| self.assertTrue(jsoncompare.are_same(value, value)[0]) | ||
|
|
||
| def test_extra_dictionary_key_is_not_equal(self): | ||
| self.assertFalse(jsoncompare.are_same({'a': 1}, {'a': 1, 'b': 2})[0]) | ||
|
|
||
| def test_missing_dictionary_key_is_not_equal(self): | ||
| self.assertFalse(jsoncompare.are_same({'a': 1, 'b': 2}, {'a': 1})[0]) | ||
|
|
||
| def test_equal_length_different_dictionary_keys(self): | ||
| self.assertFalse(jsoncompare.are_same({'a': 1}, {'b': 1})[0]) | ||
|
|
||
| def test_key_diagnostics_name_missing_and_extra_keys(self): | ||
| expected = {'z': 1, 'a': 2, 'ignored_expected': 3} | ||
| actual = {'y': 1, 'b': 2, 'ignored_actual': 3} | ||
| for left, right in [(expected, actual), | ||
| ({'nested': expected}, {'nested': actual})]: | ||
| with self.subTest(expected=left, actual=right): | ||
| same, stack = jsoncompare.are_same( | ||
| left, right, | ||
| ignore_value_of_keys=['ignored_expected', 'ignored_actual']) | ||
| self.assertFalse(same) | ||
| self.assertIn("Missing keys: ['a', 'z']", str(stack)) | ||
| self.assertIn("Extra keys: ['b', 'y']", str(stack)) | ||
|
|
||
| def test_nested_extra_key_is_not_equal(self): | ||
| self.assertFalse(jsoncompare.are_same({'a': {'b': 1}}, | ||
| {'a': {'b': 1, 'c': 2}})[0]) | ||
|
|
||
| def test_extra_list_item_is_not_equal(self): | ||
| self.assertFalse(jsoncompare.are_same([1], [1, 2])[0]) | ||
|
|
||
| def test_short_duplicate_list_is_not_equal(self): | ||
| self.assertFalse(jsoncompare.are_same([1, 1], [1])[0]) | ||
|
|
||
| def test_nested_list_length_is_checked(self): | ||
| self.assertFalse(jsoncompare.are_same({'a': [1]}, {'a': [1, 1]})[0]) | ||
|
|
||
| def test_ignored_dictionary_keys_in_either_operand(self): | ||
| for a, b in [({'x': 1}, {'x': 1, '@schema': 'new'}), | ||
| ({'x': 1, '@schema': 'old'}, {'x': 1}), | ||
| ({'x': 1, '@schema': 'old'}, | ||
| {'x': 1, '@schema': 'new'})]: | ||
| with self.subTest(a=a, b=b): | ||
| self.assertTrue(jsoncompare.are_same( | ||
| a, b, ignore_value_of_keys=['@schema'])[0]) | ||
|
|
||
| def test_ignored_key_does_not_hide_unexpected_key(self): | ||
| self.assertFalse(jsoncompare.are_same( | ||
| {'a': 1, '@schema': 'old'}, {'a': 1, 'extra': 2}, | ||
| ignore_value_of_keys=['@schema'])[0]) | ||
|
|
||
| def test_ignored_key_iterable_applies_at_every_depth(self): | ||
| expected = {'nested': {'value': 1, 'ignored': 'old'}, | ||
| 'items': [{'value': 2, 'ignored': 'old'}], 'ignored': 'old'} | ||
| actual = {'nested': {'value': 1}, 'items': [{'value': 2}]} | ||
| for compare in [jsoncompare.are_same, jsoncompare.contains]: | ||
| with self.subTest(compare=compare.__name__): | ||
| self.assertTrue(compare( | ||
| expected, actual, ignore_value_of_keys=iter(['ignored']))[0]) | ||
|
|
||
| def test_contains_keeps_nested_subset_semantics(self): | ||
| expected = {'a': {'b': 1}, 'values': [1]} | ||
| actual = {'a': {'b': 1, 'c': 2}, 'values': [1, 2], 'extra': 3} | ||
| self.assertTrue(jsoncompare.contains(expected, actual)[0]) | ||
| self.assertTrue(jsoncompare.are_same( | ||
| expected, actual, ignore_missing_keys=True)[0]) | ||
|
|
||
| def test_contains_rejects_missing_expected_data(self): | ||
| for expected, actual in [({'a': 1}, {}), ([1, 1], [1]), | ||
| ({'a': [1, 1]}, {'a': [1]})]: | ||
| with self.subTest(expected=expected, actual=actual): | ||
| self.assertFalse(jsoncompare.contains(expected, actual)[0]) | ||
|
|
||
| def test_list_order_and_duplicate_counts(self): | ||
| self.assertFalse(jsoncompare.are_same([1, 2], [2, 1])[0]) | ||
| self.assertTrue(jsoncompare.are_same( | ||
| [1, 2], [2, 1], ignore_list_order_recursively=True)[0]) | ||
| self.assertFalse(jsoncompare.are_same( | ||
| [1, 1], [1], ignore_list_order_recursively=True)[0]) | ||
|
|
||
| def test_type_checks_are_unchanged(self): | ||
| for expected, actual in [(True, 1), (1, 1.0), ([], {}), (None, 0)]: | ||
| with self.subTest(expected=expected, actual=actual): | ||
| self.assertFalse(jsoncompare.are_same(expected, actual)[0]) | ||
|
|
||
| def test_json_wrapper_forwards_ignored_keys(self): | ||
| self.assertTrue(jsoncompare.json_are_same( | ||
| '{"a": 1, "@schema": "old"}', '{"a": 1}', | ||
| ignore_value_of_keys=['@schema'])[0]) | ||
| self.assertFalse(jsoncompare.json_are_same( | ||
| '{"a": 1}', '{"a": 1, "extra": 2}', | ||
| ignore_value_of_keys=['@schema'])[0]) | ||
|
|
||
| def test_cli_strict_exit_status(self): | ||
| script = Path(jsoncompare.__file__).with_name('comparejson.py') | ||
| cases = [({'a': 1}, {'a': 1}, 0, None), | ||
| ({'a': 1}, {'a': 1, '@schema': 'new'}, 0, None), | ||
| ({'a': 1}, {'a': 1, 'extra': 2}, 1, "Extra keys: ['extra']"), | ||
| ({'a': {'b': 1}}, {'a': {'b': 1, 'extra': 2}}, 1, "Extra keys: ['extra']"), | ||
| ({'a': 1, 'missing': 2}, {'a': 1}, 1, "Missing keys: ['missing']"), | ||
| ({'a': [1]}, {'a': [1, 1]}, 1, 'Length Mismatch')] | ||
| with tempfile.TemporaryDirectory() as directory: | ||
| left = Path(directory) / 'expected.json' | ||
| right = Path(directory) / 'actual.json' | ||
| for expected, actual, code, diagnostic in cases: | ||
| with self.subTest(expected=expected, actual=actual): | ||
| left.write_text(json.dumps(expected), encoding='utf-8') | ||
| right.write_text(json.dumps(actual), encoding='utf-8') | ||
| result = subprocess.run( | ||
| [sys.executable, '-B', str(script), str(left), str(right)], | ||
| capture_output=True, text=True, check=False) | ||
| self.assertEqual(result.returncode, code, result.stdout) | ||
| if diagnostic: | ||
| self.assertIn(diagnostic, result.stdout) | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| unittest.main() |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unittest discover -s <source>/utilwritesutil/__pycache__/*.pycinto the source tree, so an out-of-sourcectestrun leaves the checkout dirty, which is a problem for read-only or shared source trees. Adding-Bto the Python invocation (or settingPYTHONDONTWRITEBYTECODE=1viaset_tests_properties(... ENVIRONMENT ...)) avoids it.🤖 Comment generated by Claude (Claude Code), posted on behalf of @fedorov.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
-Bto the utility CTest, all nine metadata-comparison commands, and CLI test subprocesses. The out-of-source harness ran all ten Python CTests withPYTHONDONTWRITEBYTECODEunset and created no source-tree.pycfiles.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fedorov edorov, thank you again for the thorough review. I've addressed all seven points in commit
6f8ee01and responded to each comment inline.When you have a chance, would you mind taking another look? I'd be happy to address anything that remains.
Thanks again!