sadpandajoe commented on code in PR #45761:
URL: https://github.com/apache/superset/pull/45761#discussion_r4392754784
##########
superset/key_value/types.py:
##########
@@ -115,6 +115,9 @@ def decode(self, value: bytes) -> Any: ...
class JsonKeyValueCodec(KeyValueCodec):
def encode(self, value: dict[Any, Any]) -> bytes:
+ # JSON has no binary type; raw bytes must not be silently stringified
+ if isinstance(value, (bytes, bytearray)):
Review Comment:
This guard only catches a top-level `bytes`. `encode({"payload": b"hello"})`
used to raise, but `base_json_conv` now stringifies nested bytes (to `"hello"`,
or `"[bytes]"` when not decodable), so binary data nested in a dict is silently
altered, which is what the comment above says must not happen. Should nested
bytes be rejected as well, or is that lossy conversion intended?
##########
superset/key_value/types.py:
##########
@@ -29,6 +28,7 @@
KeyValueCodecDecodeException,
KeyValueCodecEncodeException,
)
+from superset.utils import json
Review Comment:
Switching this import changes decoding for existing rows: stdlib `json`
wrote and read `NaN`/`Infinity`, but `superset.utils.json.loads` defaults to
`allow_nan=False`, so `decode` on an already-stored entry like `{"score": NaN}`
raises a `JSONDecodeError`, which isn't a `TypeError` and escapes unwrapped.
New writes also turn those values into `null` via `ignore_nan`. Do any callers
(e.g. extension persistent state) store non-finite floats, and if so should
`decode` keep accepting legacy payloads?
--
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]
