codeant-ai-for-open-source[bot] commented on code in PR #41099:
URL: https://github.com/apache/superset/pull/41099#discussion_r3418341711
##########
superset/result_set.py:
##########
@@ -73,9 +73,16 @@ def stringify_values(array: NDArray[Any]) -> NDArray[Any]:
obj[na_obj] = None
else:
try:
- # for simple string conversions
- # this handles odd character types better
- obj[...] = obj.astype(str)
+ val = obj.item()
+ if isinstance(val, (dict, list)):
+ # Use json.dumps to produce valid double-quoted JSON.
+ # Python's str() gives single-quoted repr like {'a': 1}
+ # which is not valid JSON and breaks the frontend cell
viewer.
+ obj[...] = stringify(val)
Review Comment:
**Suggestion:** `stringify(val)` can raise `TypeError` for dict/list values
containing non-JSON-serializable members (for example bytes or custom objects),
but this branch only catches `ValueError`, so result-set construction can now
fail at runtime instead of falling back to string conversion. Catch `TypeError`
in this path (or use a pessimistic serializer/fallback to `str`) to preserve
previous non-crashing behavior. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ SQL Lab queries crash on nested unserializable objects.
- ⚠️ Dashboards using those queries fail to render results.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note that `stringify()` in `superset/result_set.py:62-63` calls
`superset.utils.json.dumps(obj, default=json.json_iso_dttm_ser)`, and
`json_iso_dttm_ser`
in `superset/utils/json.py:114-137` delegates to `base_json_conv`
(`superset/utils/json.py:73-112`), which raises `TypeError` for any object
it cannot
convert (final `raise TypeError(f"Unserializable object {obj}...")` at line
111).
2. In `SupersetResultSet.__init__` (`superset/result_set.py:167-200,
212-239`), when Arrow
conversion fails for a column, the code falls back to stringification:
inside the `except
(pa.lib.ArrowInvalid, pa.lib.ArrowTypeError,
pa.lib.ArrowNotImplementedError, ValueError,
TypeError)` block, it calls `stringify_values(columns[column])` at
`superset/result_set.py:237`.
3. In `stringify_values` (`superset/result_set.py:66-89`), for each cell it
does `val =
obj.item()` (line 76) and, if `val` is a `dict` or `list`, executes
`obj[...] =
stringify(val)` (line 81). This call is wrapped in `try:` with an `except
ValueError:`
handler only (line 86), so any `TypeError` raised by `stringify(val)` will
**not** be
caught.
4. To reproduce, add a unit test similar to
`tests/unit_tests/result_set_test.py::test_stringify_with_null_integers`
that constructs
`data = [({"k": CustomType()},)]` where `CustomType` is a user-defined class
not handled
by `base_json_conv`, and a matching `cursor_description`. When the test
instantiates
`SupersetResultSet(data, description, BaseEngineSpec)` (see existing usage in
`tests/unit_tests/result_set_test.py:73` and production callers in
`superset/sql/execution/executor.py:162` and `superset/sql_lab.py:340`), the
Arrow
conversion for that column will fail, the fallback at
`superset/result_set.py:237` will
call `stringify_values`, and `stringify(val)` will attempt to serialize the
nested
`CustomType` instance. `json_iso_dttm_ser`/`base_json_conv` will raise
`TypeError`, which
propagates out of `stringify_values` (not caught by `except ValueError`) and
out of
`SupersetResultSet.__init__`, causing the SQL Lab query (or any caller
through
`SQLExecutor` / Celery `_execute_sql_statements`) to fail at runtime instead
of degrading
to a plain string representation as before.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7a4ca9ccd0db463385efd6747b0dfb22&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7a4ca9ccd0db463385efd6747b0dfb22&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/result_set.py
**Line:** 76:81
**Comment:**
*Type Error: `stringify(val)` can raise `TypeError` for dict/list
values containing non-JSON-serializable members (for example bytes or custom
objects), but this branch only catches `ValueError`, so result-set construction
can now fail at runtime instead of falling back to string conversion. Catch
`TypeError` in this path (or use a pessimistic serializer/fallback to `str`) to
preserve previous non-crashing behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41099&comment_hash=a8cbf5fbf9bc326478a7c5cd1fd6f3a0ad73232a7b599cd9f5a80b1db21c2b26&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41099&comment_hash=a8cbf5fbf9bc326478a7c5cd1fd6f3a0ad73232a7b599cd9f5a80b1db21c2b26&reaction=dislike'>👎</a>
--
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]
