codeant-ai-for-open-source[bot] commented on PR #36680: URL: https://github.com/apache/superset/pull/36680#issuecomment-3661991334
## 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/36680/files#diff-0bdb04238f9bd9aaff3a6e2e70343333b8a411c3b5c73b1afe63981dd63bb3e1R121-R127'><strong>Silent No-op on Invalid query_context</strong></a><br>If `output["query_context"]` is invalid JSON, the code falls back to an empty dict and will not add `form_data` even if migration happened. Confirm that this is intentional and acceptable for charts missing or corrupt `query_context`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36680/files#diff-ef8122d1ec1d36dc03c5a3063c56f488d31704c51cf1f5f67b8e9af5e1f15883R73-R103'><strong>Session cleanup not guaranteed</strong></a><br>`session.close()` and the final `session.commit()` are called after the loop, but if an unexpected error escapes the loop (e.g., from `paginated_update`), the session may not be closed. Use a try/finally around session usage to guarantee cleanup and consider rolling back on errors.<br> - [ ] <a href='https://github.com/apache/superset/pull/36680/files#diff-ef8122d1ec1d36dc03c5a3063c56f488d31704c51cf1f5f67b8e9af5e1f15883R86-R90'><strong>Empty-string form_data skipped</strong></a><br>The code only attempts to fix `form_data` when it is truthy and a string. If `form_data` is an empty string (""), the current check will skip it, leaving broken state in the DB. Empty strings were likely introduced by the bug and should be handled (attempt parse or normalize to an empty dict) rather than silently ignored.<br> - [ ] <a href='https://github.com/apache/superset/pull/36680/files#diff-ef8122d1ec1d36dc03c5a3063c56f488d31704c51cf1f5f67b8e9af5e1f15883R95-R100'><strong>Broad exception swallowing</strong></a><br>A bare `except Exception` hides underlying errors and doesn't log stack traces. This can make debugging migration failures difficult. It also doesn't perform a rollback on unexpected session errors. Prefer logging the exception details and ensuring session state is safe.<br> - [ ] <a href='https://github.com/apache/superset/pull/36680/files#diff-c9ed4b75f05b493930963aa14443948fab6f7f8bae37135f200fcfd82b2bc305R176-R214'><strong>Insufficient test coverage</strong></a><br>The new test only verifies the case when `query_context["form_data"]` is already a dict. It does not exercise the prior-bug scenario where `form_data` was stored as a JSON string inside `query_context` (the problem this PR aims to fix). Add a test that starts with `query_context["form_data"]` as a JSON string to ensure the migration always converts it to a dict.<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]
