betodealmeida commented on a change in pull request #19242:
URL: https://github.com/apache/superset/pull/19242#discussion_r829472474
##########
File path: superset/connectors/sqla/utils.py
##########
@@ -119,3 +120,22 @@ def get_virtual_table_metadata(dataset: "SqlaTable") ->
List[Dict[str, str]]:
except Exception as ex:
raise SupersetGenericDBErrorException(message=str(ex)) from ex
return cols
+
+
+def allow_adhoc_subquery(raw_sql: str) -> bool:
Review comment:
Ah, I was confused when I saw the use of this function on the file above
(`superset/connectors/sqla/models.py`), I expected this to return `True` or
`False`, but it never returns `False`.
I think it might be clearer to rename this function and change its signature
to something like:
```python
def validate_adhoc_subquery(raw_sql: str) -> None
```
Then in the file above it will be easier to understand what's going on.
Also, can you add a docstring to this function?
##########
File path: superset/connectors/sqla/utils.py
##########
@@ -119,3 +120,22 @@ def get_virtual_table_metadata(dataset: "SqlaTable") ->
List[Dict[str, str]]:
except Exception as ex:
raise SupersetGenericDBErrorException(message=str(ex)) from ex
return cols
+
+
+def allow_adhoc_subquery(raw_sql: str) -> bool:
+ # pylint: disable=import-outside-toplevel
+ from superset import is_feature_enabled
+
+ if is_feature_enabled("ALLOW_ADHOC_SUBQUERY"):
+ return True
+
+ statement = sqlparse.parse(raw_sql)[0]
+ if has_table_query(statement):
Review comment:
I think it's safe to assume the ad-hoc expression will have a single
statement, but we might want to err on the side of caution just in case and do:
```python
for statement in sqlparse.parse(raw_sql):
if has_table_query(statement):
raise ...
```
##########
File path: superset/connectors/sqla/models.py
##########
@@ -885,7 +886,8 @@ def adhoc_metric_to_sqla(
elif expression_type == utils.AdhocMetricExpressionType.SQL:
tp = self.get_template_processor()
expression = tp.process_template(cast(str,
metric["sqlExpression"]))
- sqla_metric = literal_column(expression)
+ if allow_adhoc_subquery(expression):
Review comment:
If this condition is not true then `sqla_metric` will be undefined in
line 894.
##########
File path: superset/connectors/sqla/models.py
##########
@@ -1165,7 +1169,7 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
# if groupby field equals a selected column
elif selected in columns_by_name:
outer = columns_by_name[selected].get_sqla_col()
- else:
+ elif allow_adhoc_subquery(selected):
Review comment:
Same here, we need a fallback value for `outer` when
`allow_adhoc_subquery(selected)` is false.
##########
File path: superset/errors.py
##########
@@ -138,10 +139,12 @@ class SupersetErrorType(str, Enum):
1034: _("The port number is invalid."),
1035: _("Failed to start remote query on a worker."),
1036: _("The database was deleted."),
+ 1037: _("Custom SQL does not allow subquery."),
Review comment:
```suggestion
1037: _("Custom SQL fields cannot contain subqueries."),
```
(keeping it consistent with Aaron's suggestion)
--
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]
