codeant-ai-for-open-source[bot] commented on PR #37042: URL: https://github.com/apache/superset/pull/37042#issuecomment-3737034897
## 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/37042/files#diff-853ee58717e3332c1812bd7b221bacf8af70db85bfd678fc13dae6b3c22d1a45R77-R204'><strong>Type mismatch / incomplete removal</strong></a><br>The cleanup assumes dashboard metadata uses specific types (ints for lists, strings for dict keys, JSON strings for some fields). In practice the same ID may be stored as a string or int (or fields may already be dicts vs JSON strings). This can lead to orphan references not being removed (e.g., 'timed_refresh_immune_slices' containing string IDs, 'default_filters' already being a dict, or native filter 'excluded' lists containing strings). Consider normalizing types and guarding JSON parsing to avoid leaving stale references.<br> - [ ] <a href='https://github.com/apache/superset/pull/37042/files#diff-853ee58717e3332c1812bd7b221bacf8af70db85bfd678fc13dae6b3c22d1a45R141-R153'><strong>Robustness of JSON parsing</strong></a><br>The code calls json.loads on metadata['default_filters'] without guarding against already-parsed dicts or invalid JSON. This may raise exceptions and roll back the transaction during chart deletion. Add safer parsing (check types and handle exceptions) to avoid failing the delete flow.<br> - [ ] <a href='https://github.com/apache/superset/pull/37042/files#diff-12c86ff9cc1f3ab90d143f8ea15575947df37f63a349b097f589e9cde5c43fefR101-R116'><strong>Silent data loss</strong></a><br>The updated comprehensions drop entries that don't map to a known chart (e.g., when a referenced chart was deleted). This is intentional to avoid KeyError, but it may silently remove important user metadata (expanded slices, default filters, filter scopes). Review whether telemetry/logging or a warning should be emitted so users/admins are aware that metadata was pruned during import.<br> - [ ] <a href='https://github.com/apache/superset/pull/37042/files#diff-12c86ff9cc1f3ab90d143f8ea15575947df37f63a349b097f589e9cde5c43fefR76-R81'><strong>Type mismatch risk</strong></a><br>Membership checks against `id_map` are inconsistent: some checks use `if old_id in id_map` (assumes metadata values are ints) while others use `if int(old_id) in id_map` (assumes metadata keys are strings). If metadata sometimes stores IDs as strings and sometimes as ints, the current checks may silently skip valid references or raise ValueError when casting. Normalize or defensively handle both types to avoid missed mappings or exceptions.<br> - [ ] <a href='https://github.com/apache/superset/pull/37042/files#diff-853ee58717e3332c1812bd7b221bacf8af70db85bfd678fc13dae6b3c22d1a45R41-R55'><strong>Performance (N+1 queries)</strong></a><br>The current implementation queries dashboards per chart in a loop: for each chart it executes a query to find dashboards that contain it. Deleting many charts will yield N+1 queries and may be slow. It's better to fetch all affected dashboards once (for all chart IDs) and process them in-memory to remove references for all charts in a single pass.<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]
