codeant-ai-for-open-source[bot] commented on PR #36740: URL: https://github.com/apache/superset/pull/36740#issuecomment-3670946760
## 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/36740/files#diff-ab9d79ea2966de7def32ea0b3ea3246f28ec76a3462285656b1361080ed10873R140-R190'><strong>Possible Bug</strong></a><br>In `map_chart_params` the code now passes an extra positional argument `template_context` into `_build_chart_mapping_prompt`, but the `_build_chart_mapping_prompt` implementation was not updated to accept it. This will raise a TypeError at runtime ("takes X positional arguments but Y were given").<br> - [ ] <a href='https://github.com/apache/superset/pull/36740/files#diff-b58e3292f02e70d64351c7d96bb1051940f3980a47f2b602639ebfef928cb864R1337-R1374'><strong>Numeric-detection bug</strong></a><br>The numeric-column fallback logic passes a column name into _is_numeric_type when actual_types are missing, causing wrong detection (it expects a type string). This can lead to injecting SUM() over a non-numeric column or mis-choosing COUNT(*) unnecessarily.<br> - [ ] <a href='https://github.com/apache/superset/pull/36740/files#diff-bcb3868b9e29aea5f6e87ec641120dae35a86d732501f488a7a6dbdeb450f5c1R2929-R2930'><strong>Type Safety</strong></a><br>The new code assumes `metadata["template_info"]` is a dict and calls `.get()` on it. If that value is a non-dict (e.g., a boolean, string, or None), calling `.get()` will raise an AttributeError. Guarding the type or normalizing the value is required.<br> - [ ] <a href='https://github.com/apache/superset/pull/36740/files#diff-f6a877e9c3a368e3c41256960e6bddff588b7d26f5ad750d5378f2fd65722508R710-R711'><strong>JSON decoding robustness</strong></a><br>`json.loads(dashboard.json_metadata or "{}")` can raise if `json_metadata` is invalid JSON. Previously the code did the same, but because this method is used for templates listing, consider whether a malformed dashboard should break the listing or should be handled gracefully (fallback to `{}`) and logged.<br> - [ ] <a href='https://github.com/apache/superset/pull/36740/files#diff-e08d4289195bc48b345e970b540d02839859547c3898de1d82adb7e11006c2f7R138-R139'><strong>JSON decoding error</strong></a><br>The new code calls `json.loads(dashboard.json_metadata or "{}")` directly. If `dashboard.json_metadata` contains malformed JSON this will raise `JSONDecodeError` and abort analysis. Other helper methods in this module also call `json.loads` without protection.<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]
