codeant-ai-for-open-source[bot] commented on PR #36367: URL: https://github.com/apache/superset/pull/36367#issuecomment-3613742009
## 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/36367/files#diff-94d3bd02afcbd1983033ed9c33e9efb466cb9a64e3b1f3954e6e0d627f70d0efR53-R68'><strong>Circular import risk</strong></a><br>New type imports (`TimeGrainDict`, `ExplorableData`, `QueryObjectDict`) are imported at module runtime. If those modules import models (directly or indirectly) this can create a circular import causing runtime ImportError. Prefer type-only imports under TYPE_CHECKING or avoid top-level runtime imports for typing-only objects.<br> - [ ] <a href='https://github.com/apache/superset/pull/36367/files#diff-af5e15a55ff9c81f441eb6b9b10dae13b2ee17fec614d3e3ad0057ed0dc814aeR88-R88'><strong>Circular import risk</strong></a><br>The new top-level import `from superset.explorables.base import TimeGrainDict` can create a circular import at runtime if `superset.explorables.base` imports modules that in turn import models (directly or indirectly). This may lead to ImportError or partially initialized modules in some import orders or test setups. Consider avoiding a runtime import at module scope.<br> - [ ] <a href='https://github.com/apache/superset/pull/36367/files#diff-090abddb6d9306c432a5642a19da74b293f5fe31f6b3ac707bf0314d421693c8R99-R99'><strong>Possible circular import</strong></a><br>`Explorable` is imported at runtime (not under TYPE_CHECKING) even though the module uses postponed annotations. This risks creating an import cycle (or at least unnecessary runtime coupling) between utils.core and superset.explorables.base. Importing types at runtime can break application startup or cause hard-to-debug import errors.<br> - [ ] <a href='https://github.com/apache/superset/pull/36367/files#diff-af5e15a55ff9c81f441eb6b9b10dae13b2ee17fec614d3e3ad0057ed0dc814aeR269-R277'><strong>Missing None guard</strong></a><br>`get_time_grains` calls `self.database.grains()` without checking whether `self.database` is present. In other methods (e.g., `cache_timeout`) there is a guard for `self.database` possibly being None. If `self.database` is None at runtime, calling `.grains()` will raise AttributeError.<br> - [ ] <a href='https://github.com/apache/superset/pull/36367/files#diff-76c6a0b5c14a297c62ac3207416870471affdb153c5ae12b96d7b7da9304e9ddR540-R548'><strong>Mutating shared datasource data</strong></a><br>The code assigns `datasource.data` to `datasource_data` and then mutates it by adding `"owners"`. If `datasource.data` returns a reference to an internal/shared dict (or an ORM-managed structure), this will mutate the original object and may cause surprising side effects. Consider copying the data before mutation.<br> - [ ] <a href='https://github.com/apache/superset/pull/36367/files#diff-76c6a0b5c14a297c62ac3207416870471affdb153c5ae12b96d7b7da9304e9ddR541-R544'><strong>Non-deterministic data shape</strong></a><br>`datasource.data` may not always be a mapping/dict (or may be None). The code assumes it's a dict-shaped ExplorableData and assigns it directly to `datasource_data`. If it's not a mapping this could raise runtime errors later when code expects dict-like access.<br> - [ ] <a href='https://github.com/apache/superset/pull/36367/files#diff-4728134bd9ac8d67f3f5de136b7076db886e572122cb83f09dfb1861264e07fbR199-R296'><strong>Backwards compatibility risk</strong></a><br>The PR replaces `BaseDatasourceData`/`QueryData` with a unified `ExplorableData`. Other modules may still reference the old TypedDict names or rely on slightly different field shapes. Ensure all references were updated and add transitional types or shims where necessary to avoid type/behaviour mismatches.<br> - [ ] <a href='https://github.com/apache/superset/pull/36367/files#diff-94d3bd02afcbd1983033ed9c33e9efb466cb9a64e3b1f3954e6e0d627f70d0efR243-R284'><strong>TypedDict mismatch / incomplete data</strong></a><br>The `data` property is now annotated to return `ExplorableData`. While TypedDict is total=False (optional fields), ensure the produced dict follows expected frontend semantics (field names and value formats). Some fields present in other explorable types (e.g., `uid`, `column_formats`, `cache_timeout`) are omitted — verify callers won't rely on them and the shape/types (tuples vs dicts, ordering) are consistent.<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]
