Copilot commented on code in PR #40642:
URL: https://github.com/apache/superset/pull/40642#discussion_r3337905279
##########
superset/jinja_context.py:
##########
@@ -354,6 +354,18 @@ def get_filters(self, column: str, remove_filter: bool =
False) -> list[Filter]:
- you want to have the ability for filter inside the main query
for speed
purposes
+ Always pass filter values through proper parameterization rather than
+ building SQL by hand. Use the ``where_in`` filter for list membership
and
+ bind individual values as query parameters instead of interpolating
them
+ directly into the SQL string.
+
+ .. warning::
+
+ Do not manually escape filter values (for example, with
+ ``replace("'", "''")``). Hand-rolled escaping is error-prone and
easy
+ to get wrong across dialects. Rely on the ``where_in`` filter and
+ parameter binding so values are quoted safely by the engine.
Review Comment:
The docstring recommends “bind individual values as query parameters” /
“parameter binding”, but in this Jinja context the `where_in` filter renders
SQL literals using SQLAlchemy’s dialect rules (`literal_binds=True`) rather
than passing runtime bound parameters. This wording is misleading and may
confuse users into thinking actual parameter binding is happening.
##########
tests/unit_tests/db_engine_specs/test_base.py:
##########
@@ -1283,3 +1283,55 @@ def
test_start_oauth2_dance_falls_back_to_url_for(mocker: MockerFixture) -> None
error = exc_info.value.error
assert error.extra["redirect_uri"] == fallback_uri
+
+
+def test_get_table_names_strips_schema_with_regex_metacharacters(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that get_table_names strips a schema prefix containing regex
+ metacharacters without raising and without mangling unrelated names.
+ """
+ schema = "a.b(c)"
+
+ inspector = mocker.MagicMock()
+ inspector.get_table_names.return_value = [
+ f"{schema}.orders",
+ "axbyc.other",
+ ]
+
+ database = mocker.MagicMock()
+
+ spec = BaseEngineSpec
+ mocker.patch.object(spec, "try_remove_schema_from_table_name", True)
+
+ tables = spec.get_table_names(database, inspector, schema)
+
+ # The real schema prefix is stripped; the look-alike name is left intact
+ # because the metacharacters are escaped before being used as a regex.
+ assert tables == {"orders", "axbyc.other"}
+
+
+def test_get_view_names_strips_schema_with_regex_metacharacters(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that get_view_names strips a schema prefix containing regex
+ metacharacters without raising and without mangling unrelated names.
+ """
+ schema = "a.b(c)"
+
+ inspector = mocker.MagicMock()
+ inspector.get_view_names.return_value = [
+ f"{schema}.report",
+ "axbyc.other",
+ ]
+
+ database = mocker.MagicMock()
+
+ spec = BaseEngineSpec
+ mocker.patch.object(spec, "try_remove_schema_from_table_name", True)
+
+ views = spec.get_view_names(database, inspector, schema)
+
+ assert views == {"report", "axbyc.other"}
Review Comment:
Same as the table-name test: `axbyc.other` would not have matched the old
unescaped regex for schema `a.b(c)`, so it doesn’t verify the regression. Use a
value like `axbc.other` that would have been incorrectly stripped previously.
##########
tests/unit_tests/db_engine_specs/test_base.py:
##########
@@ -1283,3 +1283,55 @@ def
test_start_oauth2_dance_falls_back_to_url_for(mocker: MockerFixture) -> None
error = exc_info.value.error
assert error.extra["redirect_uri"] == fallback_uri
+
+
+def test_get_table_names_strips_schema_with_regex_metacharacters(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that get_table_names strips a schema prefix containing regex
+ metacharacters without raising and without mangling unrelated names.
+ """
+ schema = "a.b(c)"
+
+ inspector = mocker.MagicMock()
+ inspector.get_table_names.return_value = [
+ f"{schema}.orders",
+ "axbyc.other",
+ ]
+
+ database = mocker.MagicMock()
+
+ spec = BaseEngineSpec
+ mocker.patch.object(spec, "try_remove_schema_from_table_name", True)
+
+ tables = spec.get_table_names(database, inspector, schema)
+
+ # The real schema prefix is stripped; the look-alike name is left intact
+ # because the metacharacters are escaped before being used as a regex.
+ assert tables == {"orders", "axbyc.other"}
Review Comment:
The “look-alike name” in this test (`axbyc.other`) would *not* have matched
the previous unescaped regex either, so the test wouldn’t fail without the
`re.escape()` fix. Use a value like `axbc.other`, which the old pattern
`^a.b(c)\.` would incorrectly match due to `.` acting as a wildcard.
##########
superset/db_engine_specs/base.py:
##########
@@ -1746,7 +1746,9 @@ def get_table_names( # pylint: disable=unused-argument
raise cls.get_dbapi_mapped_exception(ex) from ex
if schema and cls.try_remove_schema_from_table_name:
- tables = {re.sub(f"^{schema}\\.", "", table) for table in tables}
+ tables = {
+ re.sub(f"^{re.escape(schema)}\\.", "", table) for table in
tables
+ }
Review Comment:
`re.escape(schema)` is recomputed for every entry in the set comprehension.
Since `schema` is constant within this method, compute it once for readability
and to avoid repeated work.
##########
superset/db_engine_specs/base.py:
##########
@@ -1774,7 +1776,9 @@ def get_view_names( # pylint: disable=unused-argument
raise cls.get_dbapi_mapped_exception(ex) from ex
if schema and cls.try_remove_schema_from_table_name:
- views = {re.sub(f"^{schema}\\.", "", view) for view in views}
+ views = {
+ re.sub(f"^{re.escape(schema)}\\.", "", view) for view in views
+ }
Review Comment:
Same as above: compute `re.escape(schema)` once before the comprehension to
avoid repeating it for each view name.
--
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]
