-
Notifications
You must be signed in to change notification settings - Fork 18.4k
test(bigquery): verify string-literal escaping against a real GoogleSQL engine #44546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
rusackas
wants to merge
4
commits into
master
Choose a base branch
from
test/bigquery-testcontainers-literal-escaping
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+235
−1
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
f8a467a
test(bigquery): verify string-literal escaping against a real GoogleS…
rusackas d7bf852
test(bigquery): add docstring and fixture type hint per review
rusackas a4d1960
test(bigquery): vendor a local BigQueryContainer and add it to the CI…
rusackas 615e64e
test(bigquery): guard optional import, add container docstrings
rusackas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
76 changes: 76 additions & 0 deletions
76
tests/testcontainers/db_engine_specs/_bigquery_container.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| # 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:latest", | ||
| 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) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| # 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 | ||
|
rusackas marked this conversation as resolved.
|
||
| .github/workflows/testcontainers.yml. | ||
|
|
||
| sc-120493-adjacent investigation: 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") | ||
|
|
||
| from google.cloud import bigquery # noqa: E402 | ||
|
rusackas marked this conversation as resolved.
|
||
| 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).""" | ||
| return str( | ||
| expr.compile(dialect=BigQueryDialect(), compile_kwargs={"literal_binds": True}) | ||
| ) | ||
|
|
||
|
|
||
| def _insert_and_find(engine: Engine, value: str) -> list[str]: | ||
| t = sa.table(TABLE, sa.column("name")) | ||
| with engine.connect() as conn: | ||
| conn.execute(sa.text(f"DELETE FROM {DATASET}.{TABLE} WHERE TRUE")) # noqa: S608 | ||
| insert_literal = _compiled_literal(sa.literal(value)) | ||
| conn.execute( | ||
| sa.text( | ||
| f"INSERT INTO {DATASET}.{TABLE} (name) VALUES ({insert_literal})" # noqa: S608 | ||
| ) | ||
| ) | ||
| where = _compiled_literal(t.c.name == value) | ||
| rows = conn.execute( | ||
| sa.text(f"SELECT name FROM {DATASET}.{TABLE} WHERE {where}") # noqa: S608 | ||
| ).fetchall() | ||
| return [row[0] for row in rows] | ||
|
|
||
|
|
||
| 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 _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: | ||
|
rusackas marked this conversation as resolved.
|
||
| """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_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) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.