codeant-ai-for-open-source[bot] commented on PR #37026: URL: https://github.com/apache/superset/pull/37026#issuecomment-3731158032
## 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/37026/files#diff-1faf516041ad4f2b1e40936967d9593f74362c3e1d6f27d2d9b54fef65bb6ea3R250-R296'><strong>Inspector usage</strong></a><br>The code calls raw execute on `inspector.bind` (e.g., `inspector.bind.execute("SHOW CATALOGS")` / `inspector.bind.execute("SHOW DATABASES")`). Relying on `inspector.bind` may be fragile across SQLAlchemy versions and the connection is not acquired/closed explicitly. Consider using an explicit connection (context manager) and SQLAlchemy `text()` for better resource handling and compatibility.<br> - [ ] <a href='https://github.com/apache/superset/pull/37026/files#diff-1faf516041ad4f2b1e40936967d9593f74362c3e1d6f27d2d9b54fef65bb6ea3R173-R211'><strong>Catalog quoting</strong></a><br>In `adjust_engine_params`, `schema` is URL-quoted but `catalog` isn't. If catalogs can contain characters that need quoting (or could be user-provided), this may lead to invalid URLs or injection-like issues. Ensure catalog is validated/quoted consistently before being embedded into the database field.<br> - [ ] <a href='https://github.com/apache/superset/pull/37026/files#diff-1faf516041ad4f2b1e40936967d9593f74362c3e1d6f27d2d9b54fef65bb6ea3R214-R236'><strong>Schema parsing ambiguity</strong></a><br>`get_schema_from_engine_params` treats a database value without a dot as "ambiguous" and returns None. This is reasonable for the new "catalog.schema" convention, but callers must be aware that a plain `database` value will be ignored as schema. Double-check consumers expect this behavior and that `get_default_catalog` handles those cases correctly.<br> - [ ] <a href='https://github.com/apache/superset/pull/37026/files#diff-572b41d8d5c208beabdaac2fc5cb806039403156839eabd0d7b742ed10efce52R220-R238'><strong>Mock __getitem__ signature</strong></a><br>The tests create MagicMock rows and assign __getitem__ using lambdas that accept two arguments (self, key). When __getitem__ is set on an instance, Python will call it with one argument (key). This mismatch can cause TypeError when test code indexes the mock row. Use dicts, SimpleNamespace, or set the MagicMock's __getitem__.side_effect with a single-arg callable.<br> - [ ] <a href='https://github.com/apache/superset/pull/37026/files#diff-572b41d8d5c208beabdaac2fc5cb806039403156839eabd0d7b742ed10efce52R76-R86'><strong>Trailing dot expectation</strong></a><br>Several tests expect the database portion of the URL to include a trailing dot (e.g. "db1.", "default_catalog."). Confirm this behaviour is intentional and consistent with production code (DEFAULT_CATALOG, adjust_engine_params). In particular, ensure other codepaths that consume Database.database handle the trailing dot correctly (and that the trailing dot isn't accidentally interpreted as a schema name).<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]
