codeant-ai-for-open-source[bot] commented on PR #36596: URL: https://github.com/apache/superset/pull/36596#issuecomment-3647978535
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36596/files#diff-e6f64c514ae699668591d627dee7bccf53df788d8a07caa98632e3cc9d5ef019R103-R116'><strong>Serialization mismatch</strong></a><br>The exception handling uses `dataclasses.asdict()` to serialize `SupersetError` instances. If `SupersetError` relies on a custom `to_dict()` (and/or dataclass-specific post-init processing that mutates `extra` like adding `issue_codes`), `dataclasses.asdict()` may bypass that logic or fail if the class is not a dataclass. This can result in missing fields (e.g., `extra.issue_codes`) or runtime errors in production.<br> - [ ] <a href='https://github.com/apache/superset/pull/36596/files#diff-e6f64c514ae699668591d627dee7bccf53df788d8a07caa98632e3cc9d5ef019R112-R116'><strong>Dataclass conversion safety</strong></a><br>The new code calls dataclasses.asdict(...) on values coming from exceptions without checking whether the objects are actually dataclasses or None. If an item in `ex.errors` or `ex.error` is not a dataclass (or is None), dataclasses.asdict may raise a TypeError. This should be validated or guarded to avoid runtime failures in the error handling path.<br> - [ ] <a href='https://github.com/apache/superset/pull/36596/files#diff-e6f64c514ae699668591d627dee7bccf53df788d8a07caa98632e3cc9d5ef019R117-R120'><strong>Error payload shape consistency</strong></a><br>The GAQ task now returns a list of dicts (SIP-40 style) for Superset exceptions but falls back to a list with a dict containing only `message`. Other async tasks (e.g., load_explore_json_into_cache) use different shapes (sometimes lists of strings). Confirm the frontend and async job consumers accept the mixed shapes, or normalize the fallback to the SIP-40 structure to maintain a consistent API.<br> - [ ] <a href='https://github.com/apache/superset/pull/36596/files#diff-e6f64c514ae699668591d627dee7bccf53df788d8a07caa98632e3cc9d5ef019R112-R118'><strong>Fragile fallback message extraction</strong></a><br>The fallback for non-Superset exceptions uses `str(ex.message if hasattr(ex, "message") else ex)`. Accessing `ex.message` is non-standard for many exception types and may lead to confusing output. Also, wrapping non-string objects with `str()` may hide structured error info.<br> - [ ] <a href='https://github.com/apache/superset/pull/36596/files#diff-59b211fe4b63fc495afb8aa19edd283727304c66561c2ac030fd5d865ca39838R72-R101'><strong>Test coupling to internal representation</strong></a><br>The new tests assert on the exact structure returned by `update_job(..., errors=...)`. If the implementation uses a different serialization strategy (e.g., `to_dict()` vs `dataclasses.asdict()`), tests may pass/fail depending on subtle implementation details. Consider making assertions resilient to serialization variations.<br> </td></tr> </table> -- 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]
