codeant-ai-for-open-source[bot] commented on PR #36422: URL: https://github.com/apache/superset/pull/36422#issuecomment-3612281930
## 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/36422/files#diff-5ef4c372b486020864523a8a78ea0b03e45739ca44ef3d756e25ab35215728ceR174-R177'><strong>Fragile generic extraction</strong></a><br>The code assumes `cls.__orig_bases__[0]` always exists and contains a parameterized base so `get_args(...)[0]` will succeed. In many runtime/class-definition scenarios (non-parameterized subclasses, dynamic/class-decorated subclasses, different Python versions), `__orig_bases__` may be absent, empty, or not contain type args, which will raise AttributeError/IndexError/TypeError. This can cause subclass initialization to fail or set `model_cls` incorrectly.<br> - [ ] <a href='https://github.com/apache/superset/pull/36422/files#diff-5ef4c372b486020864523a8a78ea0b03e45739ca44ef3d756e25ab35215728ceR174-R177'><strong>Lack of defensive handling / logging</strong></a><br>There is no fallback behavior or logging if extracting the generic parameter fails. Silent failures may make it hard to debug why `model_cls` is None or incorrect. Consider explicit try/except and logging and a safe fallback to avoid surprising runtime errors.<br> - [ ] <a href='https://github.com/apache/superset/pull/36422/files#diff-5f033bad25aaf2149f779007abedc88dd0c7f970b58b1a47a9853053968268bfR54-R54'><strong>Type mismatch / StatementError</strong></a><br>The code passes `self._tables` directly into `SqlaTable.id.in_(...)` without validating or converting values. If `self._tables` contains strings, UUIDs, or other non-integer values (or empty values), SQLAlchemy may raise a StatementError or return unexpected results. Ensure IDs are converted/validated to the column type before building the query.<br> - [ ] <a href='https://github.com/apache/superset/pull/36422/files#diff-5f033bad25aaf2149f779007abedc88dd0c7f970b58b1a47a9853053968268bfR48-R59'><strong>Missing input validation</strong></a><br>There's no check that `self._tables` is a sequence of valid IDs (e.g., list of ints). Malformed input could cause silent failures (empty result) or runtime exceptions. The function should validate input shape and provide a clear validation error.<br> - [ ] <a href='https://github.com/apache/superset/pull/36422/files#diff-5f033bad25aaf2149f779007abedc88dd0c7f970b58b1a47a9853053968268bfR54-R54'><strong>Reuse DAO / base_filter</strong></a><br>The code queries `SqlaTable` directly via `db.session.query(...)`. If there is a DAO for `SqlaTable` (or a `find_by_ids` helper on a DAO), using it would centralize type conversion and base_filter application and avoid duplicating logic. Consider using the DAO to respect filters and conversion behavior.<br> - [ ] <a href='https://github.com/apache/superset/pull/36422/files#diff-bcb3868b9e29aea5f6e87ec641120dae35a86d732501f488a7a6dbdeb450f5c1R1012-R1016'><strong>Potential wrong selected column (schema vs catalog)</strong></a><br>In the catalogs-access block the code still queries `SqlaTable.schema` but later reads `table.catalog`. This mismatch can lead to missing values or attribute errors on the returned rows. Ensure the query selects the `catalog` column (or returns full model instances) when you later access `table.catalog`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36422/files#diff-d640880c4193c106ced4c1d10e91c4e058bf0d09dd2b05a6e09bf9ea348f2ac1R68-R70'><strong>Unbounded template rendering time</strong></a><br>Template rendering is executed before the validator call and is not protected by the validation timeout. A complex template could hang or take a long time; there is no timeout around template processing. Consider executing rendering inside a timeout (or a separate shorter timeout) to avoid long-running requests.<br> - [ ] <a href='https://github.com/apache/superset/pull/36422/files#diff-d640880c4193c106ced4c1d10e91c4e058bf0d09dd2b05a6e09bf9ea348f2ac1R66-R66'><strong>template_params type</strong></a><br>The code reads template parameters with .get("template_params", {}) but if the incoming payload contains "template_params": null (or a non-dict), process_template may receive None or an unexpected type and raise a confusing error. The code should coerce or validate the value to ensure a mapping is passed to Jinja processing.<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]
