Skip to content

Fix test operation to respect JSON types (boolean vs number) - #187

Open
cognis-digital wants to merge 1 commit into
stefankoegl:masterfrom
cognis-digital:fix/test-op-json-type-equality
Open

cognis-digital wants to merge 1 commit into
stefankoegl:masterfrom
cognis-digital:fix/test-op-json-type-equality

Conversation

@cognis-digital

Copy link
Copy Markdown

What

The test operation currently compares values with a plain !=. Because Python treats bool as a
subclass of int (True == 1, False == 0), a test against the JSON true/false literal
incorrectly succeeds against the numbers 1/0, and vice versa.

import jsonpatch
# Each of these wrongly succeeds today (no JsonPatchTestFailed raised):
jsonpatch.apply_patch({"baz": 1},    [{"op": "test", "path": "/baz", "value": True}])
jsonpatch.apply_patch({"baz": True}, [{"op": "test", "path": "/baz", "value": 1}])
jsonpatch.apply_patch({"baz": 0},    [{"op": "test", "path": "/baz", "value": False}])

Why

RFC 6902 §4.6 defines test equality as requiring the two values to be "of the same JSON type". A
boolean literal and a number are different JSON types and must not compare equal. This mirrors the
intent already present in DiffBuilder._compare_values, which uses json.dumps specifically so it
can "recognize the difference between 1 and True".

How

Adds a small recursive helper, _compare_json_values, that keeps booleans distinct from numbers,
still compares numbers numerically (1 == 1.0), and recurses into arrays and objects so the rule
also holds for nested values. TestOperation.apply uses it in place of !=. The error type and
message are unchanged; there is no public API change.

Tests

Adds regression tests covering scalar bool/number mismatches in both directions, the 0/false
case, 1/1.0 numeric equality (still passes), same-typed booleans (still pass), and nested
[1] vs [true] / {"q": 1} vs {"q": true} cases. Full suite: 114 passing.

RFC 6902 requires the test operation to only consider values equal when
they are of the same JSON type. Because Python treats bool as a subclass
of int (True == 1, False == 0), a test against the boolean true/false
literal wrongly succeeded against the numbers 1/0 (and vice versa).

Introduce a small recursive equality helper that keeps booleans distinct
from numbers while still comparing numbers numerically (1 == 1.0) and
recursing into arrays and objects. Wire it into TestOperation.apply and
add regression tests.
@feiiiiii5

Copy link
Copy Markdown

Validated this independently and it looks complete for the test op.

I wrote a 40-case probe from RFC 8259 sections 4, 6 and 9 rather than from the diff, then ran it against both revisions through the public apply_patch: the bool/number pairs in both directions including 0/false; the one-number-type rule (1 vs 1.0, 100 vs 1e2, 0 vs -0.0); number vs string and vs null; null vs false; array vs object and vs string; each of those same pairs nested inside an array, inside an object, two levels down and three levels down; object member order insensitivity; array order and length; and an escape sequence against the literal character for both a non-ASCII and an astral character.

base     29/40
224d70c  40/40

python tests.py on the branch is 120 tests, OK.

One structural note, since it is the part that is easy to get wrong: the recursion is doing real work in both directions, not just for the nested cases. A shallow type(a) is type(b) guard at the top would pass all the scalar bool/number cases and still miss every nested one, but it would also reject 1 against 1.0, which section 6 says are one value rather than two. Checking bool before int and then recursing is what satisfies both halves at once.

Separately, the same root cause is still live in the diff path on this branch, in case you would rather land the two together:

>>> jsonpatch.make_patch({"a": [1]}, {"a": [True]})
[]                                        # empty diff: treated as equal
>>> jsonpatch.make_patch({"a": 1}, {"a": True})
[{'op': 'replace', 'path': '/a', 'value': True}]

The scalar case already behaves correctly on main; only the list and object comparison conflates them. I read #181 as covering that path, but I have not run that branch, so treat this as an observation about #187 rather than a cross-check of #181.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants