sadpandajoe commented on code in PR #45761:
URL: https://github.com/apache/superset/pull/45761#discussion_r4406509402
##########
tests/unit_tests/key_value/codec_test.py:
##########
@@ -134,3 +148,33 @@ def test_binary_codec_round_trips():
raw = b"\x00\x01binary\xffdata"
assert codec.decode(raw) == raw
assert codec.encode(codec.decode(raw)) == raw
+
+
+def test_json_codec_rejects_bytes():
+ """Raw bytes must raise rather than be silently stringified."""
+ with pytest.raises(KeyValueCodecEncodeException):
+ JsonKeyValueCodec().encode(b"hello")
+
+
+def test_json_codec_rejects_nested_bytes():
Review Comment:
The bytes guard walks dict keys, tuples, sets, frozensets, bytearray and
memoryview, but the tests only cover root bytes, a dict value and a list item.
Dropping the key traversal or the tuple/set branch would leave every test green
while `{b"hello": 1}`, `{"payload": (b"hello",)}` or `{"payload": {b"hello"}}`
silently stringify to `"hello"` through `base_json_conv`. Could these inputs be
parametrized here, each asserting `KeyValueCodecEncodeException`?
##########
superset/key_value/types.py:
##########
@@ -113,16 +113,38 @@ def encode(self, value: Any) -> bytes: ...
def decode(self, value: bytes) -> Any: ...
+def _contains_bytes(value: Any) -> bool:
+ """Whether ``value`` is, or nests, a raw bytes-like object."""
+ if isinstance(value, (bytes, bytearray, memoryview)):
+ return True
+ if isinstance(value, dict):
+ return any(_contains_bytes(k) or _contains_bytes(v) for k, v in
value.items())
+ if isinstance(value, (list, tuple, set, frozenset)):
+ return any(_contains_bytes(v) for v in value)
+ return False
+
+
class JsonKeyValueCodec(KeyValueCodec):
def encode(self, value: dict[Any, Any]) -> bytes:
+ # JSON has no binary type; bytes, top-level or nested, must not be
+ # silently stringified
+ if _contains_bytes(value):
+ raise KeyValueCodecEncodeException("Cannot JSON-encode a bytes
value")
try:
- return bytes(json.dumps(value), encoding="utf-8")
+ # keep NaN/Infinity round-trippable, as stdlib json did, rather
than
+ # coercing them to null
+ return bytes(
+ json.dumps(value, allow_nan=True, ignore_nan=False),
Review Comment:
Switching to `superset.utils.json.dumps` also changes how existing supported
values encode: `simplejson` defaults to `namedtuple_as_object=True`, and
`dumps` here doesn't override it. A `namedtuple` (e.g. `Point(1, 2)`) that
stdlib `json` wrote as `[1, 2]` is now stored as `{"x": 1, "y": 2}`, so a
caller that reads it back with `value["point"][0]` hits a `KeyError` on the
next decode. Is any caller expected to store namedtuples through this codec,
and if not, should that be called out as part of the intentional behavior
change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
