yeachan153 opened a new issue, #20565:
URL: https://github.com/apache/superset/issues/20565
The button `Allow DML` when unchecked blocks quite a bit more than DML. For
example, DDL statements are blocked, as are non-modifying statements like
`DESCRIBE`. If unchecked, it also returns some confusing errors, for example
running a query with incorrect syntax:
```sql
with a as(
select *
from bla
),
select * from a
```
or
```sql
DESCRIBE table X
```
returns the error:
### How to reproduce the bug
1. Tick `Allow DML` in your DB setting
2. Run queries above on a table that exists
### Expected results
- Unticking allow DML should block DML statements only. Alternatively, if we
want to block any kind of edit access, we should rename that in the front end
to something else.
- Queries with syntax errors should return with an error highlighting
there's an issue with the syntax, instead of stating only select queries are
allowed
### Environment
- browser type and version: `chrome`
- superset version: `latest & 1.4.1`
- python version: `3.8.12`
- postgres version: `13.4`
### Checklist
Make sure to follow these steps before submitting your issue - thank you!
- [X] I have checked the superset logs for python stacktraces and included
it here as text if there are any.
- [X] I have reproduced the issue with at least the latest released version
of superset.
- [X] I have checked the issue tracker for the same issue and I haven't
found one similar.
### Additional context
Error gets raised
[here](https://github.com/apache/superset/blob/932e304ffbd14c46b2d816743c50c6aa7832fca2/superset/sql_lab.py#L223)
by the check
[here](https://github.com/apache/superset/blob/932e304ffbd14c46b2d816743c50c6aa7832fca2/superset/db_engine_specs/base.py#L1445).
Examining the `is_select` method
[here](https://github.com/apache/superset/blob/932e304ffbd14c46b2d816743c50c6aa7832fca2/superset/sql_parse.py#L219),
we rely on sqlparse's `get_type` method, which is implemented
[here](https://github.com/andialbrecht/sqlparse/blob/83e5381fc320f06f932d10bc0691ad970ef7962f/sqlparse/sql.py#L406).
We can see a few things:
- The `get_type` method also checks up front whether the SQL syntax is
correct, and if not, returns it as `UNKNOWN`. This is what is happening with
the first query, because the token that follows the subquery is punctuation,
not DML. This should probably be separate methods, one to check the type, one
to check the syntax in sqlparse.
- Running a `DESCRIBE table X` returns an 'UNKNOWN' type from that method,
so that gets blocked from running
- Unchecking `Allow DML` blocks far more than just DML statements, it will
also block DDL statements for instance.
### Proposed direction
I'm unsure whether we would want to support syntax validation in Superset,
or if that's the responsibility of the DB engine. If we want to push the
responsibility down to the engine for snytax, then we can replace the
`get_type` method by just checking each token, and whether they contain DML or
DDL. For example something like:
```python
if not database.allow_dml:
for cur_query in parsed_query._parsed:
for token in cur_query.tokens:
if token.ttype == DML and token.value.lower() != "select":
raise SupersetErrorException(
SupersetError(
message=__(f"DML statement `{token.value}` found in
query: {str(cur_query)}"),
error_type=SupersetErrorType.DML_NOT_ALLOWED_ERROR,
level=ErrorLevel.ERROR,
)
)
elif token.ttype == DDL:
raise SupersetErrorException(
SupersetError(
message=__(f"DDL statement `{token.value}` found in
query: {str(cur_query)}"),
error_type=SupersetErrorType.DDL_NOT_ALLOWED_ERROR,
level=ErrorLevel.ERROR,
)
)
```
If we do want to support syntax validation in Superset, that might be
difficult since every DB has different syntax. In any case, it probably
shouldn't happen in `get_type` since that results in confusing errors for the
end user.
--
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]
