You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/10/15 00:37:24 UTC
(superset) branch master updated: test(bigquery): verify string-literal escaping against a real GoogleSQL engine (#44546)
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 54d2421ded3 test(bigquery): verify string-literal escaping against a real GoogleSQL engine (#44546)
54d2421ded3 is described below
commit 54d2421ded3572dc2ae328b7d4fd698acec48c11
Author: Evan Rusackas <ev...@preset.io>
AuthorDate: Wed Oct 14 17:37:16 2026 -0700
test(bigquery): verify string-literal escaping against a real GoogleSQL engine (#44546)
Co-authored-by: Claude Sonnet 5 <no...@anthropic.com>
---
.github/workflows/testcontainers.yml | 1 +
requirements/development.in | 7 +-
.../db_engine_specs/_bigquery_container.py | 79 +++++++++
.../db_engine_specs/test_bigquery.py | 183 +++++++++++++++++++++
4 files changed, 269 insertions(+), 1 deletion(-)
diff --git a/.github/workflows/testcontainers.yml b/.github/workflows/testcontainers.yml
index 190a2d77a88..dd6e3a8ad90 100644
--- a/.github/workflows/testcontainers.yml
+++ b/.github/workflows/testcontainers.yml
@@ -87,6 +87,7 @@ jobs:
{"dialect": "risingwave", "timeout": 10},
{"dialect": "firebird", "timeout": 10},
{"dialect": "ydb", "timeout": 10},
+ {"dialect": "bigquery", "timeout": 10},
{"dialect": "oceanbase", "timeout": 20, "nightly_only": true}
]
run: |
diff --git a/requirements/development.in b/requirements/development.in
index 1982438d35d..bb6a708d4fa 100644
--- a/requirements/development.in
+++ b/requirements/development.in
@@ -43,7 +43,12 @@
# already provides. databend/risingwave/firebird/ydb are the same story:
# none has a dedicated testcontainers module, so each test uses a generic
# DockerContainer plus whatever driver its own extra above already
-# provides.
+# provides. bigquery is the same story too, and for the same reason as
+# db2/oceanbase above -- testcontainers-python has no BigQueryContainer in
+# any release yet (still an open upstream PR), so its test vendors one
+# locally against a generic DockerContainer (see
+# tests/testcontainers/db_engine_specs/_bigquery_container.py) using the
+# google-cloud-bigquery client the bigquery extra above already provides.
testcontainers[cockroachdb,cratedb,mongodb,mssql,mysql,oracle,postgres,trino]>=4.15.0,<5
# gevent (pulled in via the `gevent` extra above) has no direct version
diff --git a/tests/testcontainers/db_engine_specs/_bigquery_container.py b/tests/testcontainers/db_engine_specs/_bigquery_container.py
new file mode 100644
index 00000000000..d86423429b7
--- /dev/null
+++ b/tests/testcontainers/db_engine_specs/_bigquery_container.py
@@ -0,0 +1,79 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Locally vendored ``BigQueryContainer``.
+
+testcontainers-python has no released ``BigQueryContainer``: adding one is
+still an open, unmerged upstream PR (testcontainers/testcontainers-python
+#1121, tracking the older #393/#925) as of this writing, and the class does
+not exist in any published release (including the latest, 4.15.0) or on the
+project's default branch. Rather than depend on an unreleased upstream
+class, this follows the same pattern this test suite already uses for
+StarRocks and ClickHouse (see test_starrocks.py): wrap the container image
+directly with ``testcontainers.core.container.DockerContainer``. Delete this
+file and import from ``testcontainers.community.google`` instead once that
+PR ships in a release.
+
+Wraps `goccy/bigquery-emulator <https://github.com/goccy/bigquery-emulator>`_,
+a GoogleSQL implementation over an embedded SQLite database. It is not
+BigQuery itself, so treat query results as a strong signal rather than a
+guarantee for anything outside standard GoogleSQL (BigQuery-specific
+services like BigQuery ML, row access policies, or external tables are out
+of scope).
+"""
+
+from google.auth.credentials import AnonymousCredentials
+from google.cloud import bigquery
+from testcontainers.core.container import DockerContainer
+from testcontainers.core.waiting_utils import wait_for_logs
+
+
+class BigQueryContainer(DockerContainer):
+ """Wraps the `goccy/bigquery-emulator` image in a plain ``DockerContainer``."""
+
+ def __init__(
+ self,
+ image: str = (
+ "ghcr.io/goccy/bigquery-emulator@sha256:"
+ "f4e428d265a93dc5ce36c294e1c584c7c9b384117d47ab8ddbb63d8d50b7f393"
+ ),
+ project: str = "test-project",
+ port: int = 9050,
+ grpc_port: int = 9060,
+ **kwargs: object,
+ ) -> None:
+ """Configure the emulator's REST/gRPC ports and startup command."""
+ super().__init__(image=image, **kwargs)
+ self.project = project
+ self.port = port
+ self.grpc_port = grpc_port
+ self.with_exposed_ports(self.port, self.grpc_port)
+ self.with_command(f"--project={project} --port={port} --grpc-port={grpc_port}")
+
+ def get_rest_endpoint(self) -> str:
+ """Return the emulator's host-mapped REST API base URL."""
+ return (
+ f"http://{self.get_container_host_ip()}:{self.get_exposed_port(self.port)}"
+ )
+
+ def get_client(self, **kwargs: object) -> bigquery.Client:
+ """Wait for the emulator to be ready and return a client pointed at it."""
+ wait_for_logs(self, "REST server listening at", timeout=30.0)
+ kwargs.setdefault("project", self.project)
+ kwargs.setdefault("credentials", AnonymousCredentials())
+ kwargs.setdefault("client_options", {"api_endpoint": self.get_rest_endpoint()})
+ return bigquery.Client(**kwargs)
diff --git a/tests/testcontainers/db_engine_specs/test_bigquery.py b/tests/testcontainers/db_engine_specs/test_bigquery.py
new file mode 100644
index 00000000000..49b072abb1d
--- /dev/null
+++ b/tests/testcontainers/db_engine_specs/test_bigquery.py
@@ -0,0 +1,183 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Tests Superset's BigQuery string-literal escaping (superset/db_engine_specs/
+bigquery.py's ``_monkeypatch_bigquery_string_literal``) against a real
+GoogleSQL query engine, spun up on demand via testcontainers. Run via
+.github/workflows/testcontainers.yml.
+
+Background: an apostrophe in a filter value used to
+break BigQuery queries (apache/superset#35857 / #38835, doubled single
+quotes -- BigQuery rejects ``'Armando''s'`` as two adjacent string literals
+needing whitespace between them). The current fix backslash-escapes instead.
+Reasoning about correctness from the ``sqlalchemy-bigquery`` dialect source
+and the BigQuery DBAPI's ``pyformat`` paramstyle handling is necessary but
+not sufficient; this locks in the actual compiled-and-executed behavior
+against a real engine instead.
+"""
+
+from collections.abc import Iterator
+
+import pytest
+import sqlalchemy as sa
+from sqlalchemy.engine import Engine
+
+pytestmark = pytest.mark.testcontainers
+
+from ._driver import require_driver # noqa: E402
+
+require_driver("testcontainers.core.container")
+require_driver("sqlalchemy_bigquery")
+
+from google.cloud import bigquery # noqa: E402
+from sqlalchemy_bigquery import BigQueryDialect # noqa: E402
+
+# Importing this triggers _monkeypatch_bigquery_string_literal(), exactly as
+# it runs in a real Superset process.
+import superset.db_engine_specs.bigquery # noqa: E402, F401
+
+from ._bigquery_container import BigQueryContainer # noqa: E402
+
+DATASET = "ds"
+TABLE = "t"
+
+
+@pytest.fixture(scope="module")
+def bq_client() -> Iterator[bigquery.Client]:
+ with BigQueryContainer() as container:
+ client = container.get_client()
+ client.create_dataset(f"{client.project}.{DATASET}")
+ client.query(f"CREATE TABLE {DATASET}.{TABLE} (name STRING)").result()
+ yield client
+
+
+@pytest.fixture(scope="module")
+def engine(bq_client) -> Engine:
+ # user_supplied_client=true is a URL query param, not just a connect_args
+ # key: parse_url() only sets BigQueryDialect.create_connect_args() to
+ # accept the connect_args={"client": ...} override when it's present,
+ # otherwise it tries to build a client from real GCP credentials.
+ return sa.create_engine(
+ "bigquery://?user_supplied_client=true", connect_args={"client": bq_client}
+ )
+
+
+def _compiled_literal(expr: sa.ColumnElement) -> str:
+ """Render ``expr`` exactly as Superset's actual code path does: compiled
+ with ``literal_binds=True``, then executed as a plain string with no
+ separate bind parameters (superset.db_engine_specs.base.BaseEngineSpec
+ .execute() calls ``cursor.execute(query)``, nothing else).
+
+ This does not replicate ``Database.compile_sqla_query``'s ``%%`` -> ``%``
+ undo (applied because the BigQuery dialect doubles percents), so values
+ containing consecutive percent signs can differ from the full Superset
+ path; ``test_doubled_percent_value_is_collapsed_by_bigquery_dbapi`` pins
+ the behavior of this helper only."""
+ return str(
+ expr.compile(dialect=BigQueryDialect(), compile_kwargs={"literal_binds": True})
+ )
+
+
+def _insert_and_find(engine: Engine, value: str) -> list[str]:
+ """Round-trips ``value`` through the exact call superset.db_engine_specs.
+ base.BaseEngineSpec.execute() makes: ``cursor.execute(query)`` on a raw
+ DBAPI cursor, with no separate ``parameters`` argument and no SQLAlchemy
+ ``Connection``/``text()`` layer in between. Going through ``conn.execute
+ (sa.text(...))`` instead (as this helper used to) lets SQLAlchemy's own
+ text-clause compiler double every literal ``%`` to ``%%`` before the
+ DBAPI's unconditional ``%%`` -> ``%`` de-escaping runs, which can mask or
+ alter percent-sign handling that the raw cursor path would not."""
+ t = sa.table(TABLE, sa.column("name"))
+ raw_conn = engine.raw_connection()
+ try:
+ cursor = raw_conn.cursor()
+ cursor.execute(f"DELETE FROM {DATASET}.{TABLE} WHERE TRUE") # noqa: S608
+ insert_literal = _compiled_literal(sa.literal(value))
+ cursor.execute(
+ f"INSERT INTO {DATASET}.{TABLE} (name) VALUES ({insert_literal})" # noqa: S608
+ )
+ where = _compiled_literal(t.c.name == value)
+ cursor.execute(f"SELECT name FROM {DATASET}.{TABLE} WHERE {where}") # noqa: S608
+ return [row[0] for row in cursor.fetchall()]
+ finally:
+ raw_conn.close()
+
+
+def test_apostrophe_value_round_trips(engine: Engine) -> None:
+ """Regression test for apache/superset#35857: an apostrophe in a filter
+ value must not corrupt the compiled query or fail to match."""
+ assert _compiled_literal(sa.literal("O'Brien")) == "'O\\'Brien'"
+ assert _insert_and_find(engine, "O'Brien") == ["O'Brien"]
+
+
+def test_percent_sign_value_round_trips(engine: Engine) -> None:
+ """
+ A literal percent sign must survive Superset's actual execution path
+ unchanged. Superset's literal_processor does not double it (unlike the
+ upstream sqlalchemy-bigquery function it replaces), which is correct
+ specifically because Superset always executes via cursor.execute(query)
+ with no separate `parameters` -- the BigQuery DBAPI's own pyformat
+ handling only applies `%%` -> `%` de-escaping in that case
+ (google.cloud.bigquery.dbapi.cursor._format_operation), so a lone `%`
+ passes through untouched either way. A doubled `%%` would also survive
+ (de-escaped back to one `%`), so this test would not by itself catch a
+ regression toward doubling -- it exists to pin the actually-shipped
+ behavior, not to distinguish the two.
+ """
+ assert _insert_and_find(engine, "100% sure") == ["100% sure"]
+
+
+def test_combined_percent_and_apostrophe_round_trips(engine: Engine) -> None:
+ """Round-trips a value containing both a percent sign and an apostrophe."""
+ assert _insert_and_find(engine, "50% off for O'Brien") == ["50% off for O'Brien"]
+
+
+def test_doubled_percent_value_is_collapsed_by_bigquery_dbapi(engine: Engine) -> None:
+ """
+ Pins a lossy interaction between Superset's literal_processor, which
+ does not double `%`, and google.cloud.bigquery.dbapi.cursor.
+ _format_operation, which runs `operation.replace("%%", "%")` whenever
+ it is called with no `parameters` (always true for Superset's
+ cursor.execute(query) path), so one of two consecutive percent signs
+ is silently lost. It depends on the escaping under test: with
+ _monkeypatch_bigquery_string_literal removed, upstream doubles `%` and
+ this test fails. The full Superset path also collapses `%%` in
+ Database.compile_sqla_query; see _compiled_literal.
+ """
+ assert _insert_and_find(engine, "a%%b") == ["a%b"]
+
+
+def test_doubled_single_quotes_are_rejected_by_bigquery(
+ bq_client: bigquery.Client,
+) -> None:
+ """
+ Documents *why* the fix in #38835 was needed: BigQuery does not accept
+ the standard-SQL doubled-single-quote escape convention Superset used to
+ emit. If this test ever starts failing because the query succeeds, that
+ is a BigQuery/GoogleSQL behavior change worth knowing about, not a
+ Superset regression.
+
+ Goes through the raw client with retries disabled, not engine.connect():
+ the emulator reports this syntax error as a generic retryable INTERNAL
+ rather than a 400, so the client library's default retry policy spends
+ close to a minute retrying a failure that will never succeed.
+ """
+ with pytest.raises(Exception, match="concatenated string literals"):
+ bq_client.query(
+ f"SELECT * FROM {DATASET}.{TABLE} WHERE name IN ('Armando''s')", # noqa: S608
+ job_retry=None,
+ ).result(retry=None)