korbit-ai[bot] commented on code in PR #26803:
URL: https://github.com/apache/superset/pull/26803#discussion_r2129026436
##########
superset/extensions/pylint.py:
##########
@@ -72,6 +73,41 @@ def visit_call(self, node: nodes.Call) -> None:
self.add_message("consider-using-transaction", node=node)
+class SQLParsingLibraryImportChecker(BaseChecker):
+ name = "sql-parsing-library-import-checker"
+ priority = 0
+ msgs = {
+ "C9999": (
+ "Disallowed SQL parsing import used",
+ "disallowed-import",
+ "Used when a disallowed import is used in a specific file.",
+ ),
+ }
+
+ def _is_disallowed(self, file_path: Path, root_mod: str) -> bool:
+ # True if sqlglot is imported outside superset/sql,
+ # or if any forbidden library is imported anywhere
Review Comment:
### Missing rationale in SQL parser import restrictions <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The comment doesn't explain WHY sqlglot is only allowed in superset/sql or
why other libraries are forbidden.
###### Why this matters
Without understanding the rationale behind these import restrictions,
developers might be tempted to circumvent them or may inadvertently introduce
security/maintenance issues.
###### Suggested change ∙ *Feature Preview*
def _is_disallowed(self, file_path: Path, root_mod: str) -> bool:
# We restrict sqlglot to superset/sql to centralize SQL parsing
logic and prevent
# inconsistent parsing across the codebase. Other SQL parsing
libraries are forbidden
# to maintain consistent SQL handling through sqlglot.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9d5c4ef5-52e3-4418-b6ff-79b125606e0b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9d5c4ef5-52e3-4418-b6ff-79b125606e0b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9d5c4ef5-52e3-4418-b6ff-79b125606e0b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9d5c4ef5-52e3-4418-b6ff-79b125606e0b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9d5c4ef5-52e3-4418-b6ff-79b125606e0b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:333dae35-a5f1-42d2-94f8-afc7e310c3eb -->
[](333dae35-a5f1-42d2-94f8-afc7e310c3eb)
##########
superset/extensions/pylint.py:
##########
@@ -72,6 +73,41 @@
self.add_message("consider-using-transaction", node=node)
+class SQLParsingLibraryImportChecker(BaseChecker):
+ name = "sql-parsing-library-import-checker"
+ priority = 0
+ msgs = {
+ "C9999": (
+ "Disallowed SQL parsing import used",
+ "disallowed-import",
+ "Used when a disallowed import is used in a specific file.",
+ ),
+ }
+
+ def _is_disallowed(self, file_path: Path, root_mod: str) -> bool:
+ # True if sqlglot is imported outside superset/sql,
+ # or if any forbidden library is imported anywhere
+ in_superset_sql = file_path.match("**/superset/sql/**")
+ return (root_mod == "sqlglot" and not in_superset_sql) or root_mod in {
+ "sqlparse",
+ "sqloxide",
+ }
+
+ def visit_import(self, node: nodes.Import) -> None:
+ root_file = Path(node.root().file or "")
Review Comment:
### Silent fallback masks file path errors <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using empty string as fallback when node.root().file is None could mask
potential issues with file path resolution
###### Why this matters
Silent fallback to empty string could lead to unexpected behavior in path
matching logic and make debugging more difficult when the file path is
unexpectedly missing
###### Suggested change ∙ *Feature Preview*
Consider logging a warning when file path is None or raising an exception if
this is an unexpected state:
```python
root_file = Path(node.root().file if node.root().file is not None else
logging.warning("File path is None in SQLParsingLibraryImportChecker")
or "")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b721bbe4-6e48-468d-aa53-c40b70768ee5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b721bbe4-6e48-468d-aa53-c40b70768ee5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b721bbe4-6e48-468d-aa53-c40b70768ee5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b721bbe4-6e48-468d-aa53-c40b70768ee5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b721bbe4-6e48-468d-aa53-c40b70768ee5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:06f7c64f-6a8b-41b2-9e4f-bf46ea939784 -->
[](06f7c64f-6a8b-41b2-9e4f-bf46ea939784)
--
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]
