[GitHub] [superset] ofekisr commented on a change in pull request #17702: chore(sql): clean up invalid filter clause exception types

[GitBox <gi...@apache.org>]

ofekisr commented on a change in pull request #17702:
URL: https://github.com/apache/superset/pull/17702#discussion_r765677496



##########
File path: superset/sql_parse.py
##########
@@ -378,3 +380,23 @@ def set_or_update_query_limit(self, new_limit: int, force: bool = False) -> str:
         for i in statement.tokens:
             str_res += str(i.value)
         return str_res
+
+
+def validate_filter_clause(clause: str) -> None:

Review comment:
       The module and its class responsible are just parsing a query nothing else. I don't like either of the utils functions inside x the module. so I think it should be extracted to a different module

##########
File path: superset/sql_parse.py
##########
@@ -378,3 +380,23 @@ def set_or_update_query_limit(self, new_limit: int, force: bool = False) -> str:
         for i in statement.tokens:
             str_res += str(i.value)
         return str_res
+
+
+def validate_filter_clause(clause: str) -> None:
+    if sqlparse.format(clause, strip_comments=True) != sqlparse.format(clause):
+        raise SupersetQueryParseException("Filter clause contains comment")
+
+    statements = sqlparse.parse(clause)
+    if len(statements) != 1:

Review comment:
       same as previous comments 

##########
File path: superset/sql_parse.py
##########
@@ -378,3 +380,23 @@ def set_or_update_query_limit(self, new_limit: int, force: bool = False) -> str:
         for i in statement.tokens:
             str_res += str(i.value)
         return str_res
+
+
+def validate_filter_clause(clause: str) -> None:
+    if sqlparse.format(clause, strip_comments=True) != sqlparse.format(clause):
+        raise SupersetQueryParseException("Filter clause contains comment")
+
+    statements = sqlparse.parse(clause)
+    if len(statements) != 1:
+        raise SupersetQueryParseException("Filter clause contains multiple queries")
+    open_parens = 0
+
+    for token in statements[0]:

Review comment:
       same as previous comments

##########
File path: superset/sql_parse.py
##########
@@ -378,3 +380,23 @@ def set_or_update_query_limit(self, new_limit: int, force: bool = False) -> str:
         for i in statement.tokens:
             str_res += str(i.value)
         return str_res
+
+
+def validate_filter_clause(clause: str) -> None:
+    if sqlparse.format(clause, strip_comments=True) != sqlparse.format(clause):
+        raise SupersetQueryParseException("Filter clause contains comment")

Review comment:
       1. This is not a parsing error but a constrained error for validation
   2. IMO I would encapsulate it under a private method that tells me what is validating like _validate_clause_does_not_contains_comments, even the if under encapsulated method "is_caluse_contains_comments
   3. Actually the code doesn't tell me why comments in clause are bad, anyway I don't like comments in code so instead of writing a comment describe it I would encapsulate bunch of the same purposes under a method with name describe the purpose 




-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org