codeant-ai-for-open-source[bot] commented on PR #35009: URL: https://github.com/apache/superset/pull/35009#issuecomment-3659744995
## 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/35009/files#diff-cfa1d3a85df93fa1642114b4b6b1da99caaa68b7a54df76029b4e46664b71edeR72-R99'><strong>Fallback behaviour risk</strong></a><br>When an undefined template function is detected the code returns the original SQL (unrendered). This is a deliberate fallback, but it can cause templates to reach the SQL engine unrendered and lead to runtime SQL errors or unexpected behavior. Ensure this is acceptable in all contexts or consider a safer sanitization/explicit error path.<br> - [ ] <a href='https://github.com/apache/superset/pull/35009/files#diff-cfa1d3a85df93fa1642114b4b6b1da99caaa68b7a54df76029b4e46664b71edeR69-R74'><strong>Broad exception handler</strong></a><br>The code catches `Exception` and then checks for a specific `UndefinedTemplateFunctionException`. This pattern can accidentally swallow unrelated exceptions during template rendering and make debugging harder. Prefer catching the specific exception directly to avoid masking other errors.<br> - [ ] <a href='https://github.com/apache/superset/pull/35009/files#diff-cfa1d3a85df93fa1642114b4b6b1da99caaa68b7a54df76029b4e46664b71edeR76-R85'><strong>SQL comment stripping fragility</strong></a><br>`_strip_sql_comments` relies on `SQLScript` and its `format(comments=False)` implementation. If parsing/formatting fails for a given engine or malformed SQL, this will raise and break validation. Consider making comment-stripping robust with a safe fallback to the original SQL.<br> - [ ] <a href='https://github.com/apache/superset/pull/35009/files#diff-2803d15408a51d2724a474c4ce31763b9e11c92f9eb41777b5ba79378c4f1239R778-R785'><strong>Fragile parsing</strong></a><br>The new logic parses the stringified UndefinedError to extract an undefined name using a regex and then searches the SQL text for a function-like occurrence. Relying on the textual format of exception messages is brittle (may vary across Jinja versions/locales) and can produce false positives/negatives. Consider using Jinja's parsing/meta API or more robust error attributes instead.<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]
