gabotorresruiz commented on code in PR #42087:
URL: https://github.com/apache/superset/pull/42087#discussion_r3730860607
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -1177,3 +1179,86 @@ def
test_validate_stored_expression_rejects_subquery_around_jinja(
None,
"(SELECT password FROM ab_user LIMIT 1) {# x #}",
)
+
+
+def test_get_sqla_col_validates_stored_expression_at_query_time(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A stored calculated-column expression must be validated at the query sink,
+ not only at save time. ``get_sqla_col`` routes the expression through
+ ``validate_adhoc_subquery`` so a disallowed sub-query is rejected even when
+ it reaches the query with the save-time check bypassed (templating, the
+ create path, or older data). Locks in the query-time gate.
+ """
+ tc = TableColumn(
+ column_name="leak",
+ expression="(SELECT password FROM ab_user LIMIT 1)",
+ )
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "sqlite"
+ spy = mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ message="Sub-queries are not allowed in stored expressions.",
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
+ with pytest.raises(SupersetSecurityException):
+ tc.get_sqla_col()
+ spy.assert_called_once()
+
+
+def test_get_timestamp_expression_validates_stored_expression_at_query_time(
+ mocker: MockerFixture,
+) -> None:
+ """
+ The timestamp-expression sink must enforce the same query-time gate as
+ ``get_sqla_col``: a stored datetime column expression is routed through
+ ``validate_adhoc_subquery`` before it reaches ``literal_column``, so a
+ disallowed sub-query is rejected on the time-grained query path too.
+ """
+ tc = TableColumn(
+ column_name="ds",
+ expression="(SELECT ts FROM ab_user LIMIT 1)",
+ )
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "sqlite"
+ spy = mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ message="Sub-queries are not allowed in stored expressions.",
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
+ with pytest.raises(SupersetSecurityException):
+ tc.get_timestamp_expression(time_grain=None)
+ spy.assert_called_once()
+
+
+def test_get_sqla_col_falls_back_when_stored_expression_unparseable(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A stored expression using dialect-specific syntax that sqlglot cannot parse
+ (e.g. ``DATE_ADD(ds, 1)`` on MySQL) pre-dates the query-time gate and went
+ to the query unparsed. A parse failure must fall back to the raw expression
+ rather than break the query; a genuine sub-query still parses and is
caught.
+ """
+ tc = TableColumn(column_name="ds", expression="DATE_ADD(ds, 1)")
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "mysql"
+ mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetParseError("DATE_ADD(ds, 1)", "mysql"),
+ )
+ literal = mocker.patch("superset.connectors.sqla.models.literal_column")
+ tc.get_sqla_col()
+ # The raw expression reaches ``literal_column`` unchanged; no exception.
+ assert literal.call_args.args[0] == "DATE_ADD(ds, 1)"
Review Comment:
Agreeing with @rusackas and the bot thread here, and I would go one step
further rather than only adding `spy.assert_called_once()`.
All three new tests patch `superset.models.helpers.validate_adhoc_subquery`,
which is the thing being wired in, so between them they prove the call happens
and nothing else. I ran them against master with only this test file overlaid:
the two `..._at_query_time` tests fail there, so those are real regression
tests, but this one passes, so it currently pins nothing.
The save-time tests right above in this same file already show the pattern
that does not need the mock: build a `MagicMock(spec=Database)` with a backend
and let the real validator run. The query-time sinks work the same way. I ran
these two against the branch and both pass, and the first one is the sharp one
since it fails against master:
```python
def _stored_col(expression: str, backend: str, mocker: MockerFixture) ->
TableColumn:
tc = TableColumn(column_name="ds", expression=expression)
tc.table = mocker.MagicMock()
tc.table.database.backend = backend
tc.table.catalog = None
tc.table.schema = "public"
tc.db_engine_spec.engine = backend
return tc
def test_get_sqla_col_rejects_stored_subquery(mocker: MockerFixture) -> None:
tc = _stored_col("(SELECT 1)", "mysql", mocker)
with pytest.raises(SupersetSecurityException):
tc.get_sqla_col()
def test_get_sqla_col_falls_back_when_unparseable(mocker: MockerFixture) ->
None:
literal = mocker.patch("superset.connectors.sqla.models.literal_column")
_stored_col("DATE_ADD(ds, 1)", "mysql", mocker).get_sqla_col()
assert literal.call_args.args[0] == "DATE_ADD(ds, 1)"
```
That way the fallback case is anchored to a real MySQL parse failure instead
of a mocked one, and the rejection case proves the policy rather than the
wiring. The metric sink and `convert_tbl_column_to_sqla_col` have no test at
all right now, which is also what codecov is pointing at with the uncovered
lines. One test each on the same helper would close it.
--
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]
