test(bigquery): verify string-literal escaping against a real GoogleSQL engine by rusackas · Pull Request #44546 · apache/superset

rusackas · 2026-09-22T23:09:55Z

SUMMARY

Adds testcontainers coverage that runs Superset's actual BigQuery string-literal escaping (_monkeypatch_bigquery_string_literal in superset/db_engine_specs/bigquery.py) against a real GoogleSQL engine (the goccy/bigquery-emulator, via the new BigQueryContainer in testcontainers/testcontainers-python#1121), instead of only reasoning about it from the sqlalchemy-bigquery dialect source and the BigQuery DBAPI's pyformat handling.

This grew out of chasing down whether #35857/#38835 (apostrophes breaking BigQuery filter values) is still correctly fixed, and whether the DBAPI's %-doubling behavior for percent signs is safe given Superset's actual execution path (cursor.execute(query), no separate bind parameters). Both turned out fine, this locks it in with a real engine rather than leaving it as a one-time manual check.

Covers: apostrophe values, percent-sign values, apostrophe+percent combined, and a documentation test that reproduces the exact syntax error BigQuery gives for the doubled-single-quote escape convention the old code used to emit ('Armando''s'), so it's clear why the fix in #38835 was needed if anyone's tempted to revert it.

TESTING INSTRUCTIONS

Gated behind pytest.mark.testcontainers (excluded from the default test run via pytest.ini) and require_driver("testcontainers.community.google"), matching the existing tests/testcontainers/db_engine_specs/ convention. Needs Docker running locally, or runs via .github/workflows/testcontainers.yml:

pytest tests/testcontainers/db_engine_specs/test_bigquery.py -v -m testcontainers

All 4 tests pass locally against superset/db_engine_specs/bigquery.py unmodified.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

🤖 Generated with Claude Code

bito-code-review · 2026-09-22T23:10:07Z

Code Review Agent Run #8459ce

Actionable Suggestions - 0

Review Details

Files reviewed - 3 · Commit Range: 19954fd..3e5f158
- superset/common/query_context_processor.py
- tests/testcontainers/db_engine_specs/test_bigquery.py
- tests/unit_tests/common/test_query_context_processor.py
Files skipped - 0
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers an incremental AI Review.
/review full - Manually triggers a full AI Review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

netlify · 2026-09-22T23:12:30Z

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	`d7bf852`
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6ab43094f680c4000888c241
😎 Deploy Preview	https://deploy-preview-44546--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

codecov · 2026-09-22T23:32:31Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.06%. Comparing base (d7a918b) to head (d7bf852).
⚠️ Report is 14 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #44546   +/-   ##
=======================================
  Coverage   81.06%   81.06%           
=======================================
  Files        2955     2955           
  Lines      178300   178300           
  Branches    41307    41307           
=======================================
  Hits       144533   144533           
  Misses      31056    31056           
  Partials     2711     2711

Flag	Coverage Δ
hive	`36.80% <ø> (ø)`
mysql	`55.97% <ø> (ø)`
postgres	`55.97% <ø> (-0.01%)`	⬇️
presto	`38.72% <ø> (ø)`
python	`85.36% <ø> (ø)`
sqlite	`55.69% <ø> (ø)`
unit	`77.62% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bito-code-review · 2026-09-23T01:52:38Z

The flagged issue is correct. In the current implementation, if a DAO lookup or lazy datasource loading (such as ChartDAO.find_by_id or RLS lookups) raises an exception before the try block, it can indeed cause the entire chart-data request to fail, even if the annotation cache-key itself is not the primary cause of the failure.

To resolve this, you should wrap the annotation-related lookups and cache-key generation in a try...except block to ensure that failures in annotation processing do not propagate and abort the entire request. The implementation should catch potential exceptions (like RuntimeError or other database-related errors) and handle them gracefully, for example by failing closed (e.g., setting annotation_data to an empty dictionary or a safe fallback state) rather than allowing the exception to bubble up.

I have checked the PR comments, and there are no other comments to address. Would you like me to implement this fix for you?

superset/common/query_context_processor.py

annotation_data: dict[str, Any] = {}
        if query_obj and annotation_key and cache.status != QueryStatus.FAILED:
            try:
                annotation_data = self._get_annotation_data_cached(
                    query_obj=query_obj,
                    cache_key=annotation_key,
                    force_query=force_query,
                    force_cached=force_cached,
                    timeout=self.get_cache_timeout(),
                    datasource_uid=self._qc_datasource.uid,
                )
            except (QueryObjectValidationError, Exception) as ex:
                # Log the error and fail closed for annotations
                logger.exception("Failed to load annotation data")
                cache.error_message = str(ex)
                cache.status = QueryStatus.FAILED

bito-code-review

Code Review Agent Run #b23663

Actionable Suggestions - 6

tests/unit_tests/common/test_query_context_processor.py - 4
- Missing test type hints · Line 118-118
- Fixture missing return type · Line 183-193
- Fail-closed log unasserted · Line 236-252
- Inline function-body import · Line 2340-2340
tests/testcontainers/db_engine_specs/test_bigquery.py - 2
- Missing test docstring · Line 127-127
- Missing fixture param type hint · Line 131-131

Additional Suggestions - 1

tests/unit_tests/common/test_query_context_processor.py - 1
- Unverified mock-behavior claim · Line 123-129
  
  The comment claims bare `patch()` autodetects `security_manager` as an async spec because the real object trips `unittest.mock`'s coroutine inference. CPython 3.11's `mock.py` does apply `_is_async_obj(original)` when spec is None (line 1460), but whether the real `SupersetSecurityManager` actually trips it could not be executed here (no flask installed). If the premise is wrong, the comment misleads future maintainers — verify or soften it.

Review Details

Files reviewed - 3 · Commit Range: 19954fd..fe7821f
- superset/common/query_context_processor.py
- tests/testcontainers/db_engine_specs/test_bigquery.py
- tests/unit_tests/common/test_query_context_processor.py
Files skipped - 0
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers an incremental AI Review.
/review full - Manually triggers a full AI Review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

bito-code-review · 2026-09-23T12:15:42Z

Code Review Agent Run #873076

Actionable Suggestions - 0

Review Details

Files reviewed - 3 · Commit Range: fe7821f..7f2a133
- superset/common/query_context_processor.py
- tests/testcontainers/db_engine_specs/test_bigquery.py
- tests/unit_tests/common/test_query_context_processor.py
Files skipped - 0
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers an incremental AI Review.
/review full - Manually triggers a full AI Review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

…QL engine Adds testcontainers coverage for superset/db_engine_specs/bigquery.py's _monkeypatch_bigquery_string_literal, using the new BigQueryContainer (testcontainers/testcontainers-python#1121) to run apostrophe, percent-sign, and combined-value queries against a real GoogleSQL emulator rather than reasoning about the sqlalchemy-bigquery dialect and BigQuery DBAPI paramstyle handling from source alone. Also pins down, with a direct reproduction, why the doubled-single-quote escape #38835 replaced doesn't work on BigQuery. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Adds a one-line docstring to the combined percent/apostrophe round-trip test and a `bigquery.Client` annotation on the `bq_client` fixture parameter so the new tests match their siblings. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

rusackas · 2026-09-23T20:03:49Z

Rebased this onto master to drop the #44406 commits it was accidentally stacked on, so the diff is just the BigQuery test file again. The fail-closed annotation fix that came out of review here moved over to #44406.

bito-code-review · 2026-09-23T23:05:44Z

Code Review Agent Run #a3bbe1

Actionable Suggestions - 0

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

tests/testcontainers/db_engine_specs/test_bigquery.py - 1
- test not wired into CI · Line 43-43

Review Details

Files reviewed - 1 · Commit Range: f8a467a..d7bf852
- tests/testcontainers/db_engine_specs/test_bigquery.py
Files skipped - 0
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers an incremental AI Review.
/review full - Manually triggers a full AI Review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

pull-request-size Bot added the size/XL label Sep 22, 2026

github-actions Bot added the preset-io label Sep 22, 2026

rusackas requested a review from sadpandajoe September 22, 2026 23:39

rusackas mentioned this pull request Sep 22, 2026

fix(i18n): regenerate messages.pot to match source #44547

Closed

9 tasks

rusackas force-pushed the test/bigquery-testcontainers-literal-escaping branch from 3e5f158 to fe7821f Compare September 23, 2026 01:44

codeant-ai-for-open-source Bot reviewed Sep 23, 2026

View reviewed changes

Comment thread superset/common/query_context_processor.py Outdated

bito-code-review Bot reviewed Sep 23, 2026

View reviewed changes

rusackas and others added 2 commits September 23, 2026 13:00

rusackas force-pushed the test/bigquery-testcontainers-literal-escaping branch from 7f2a133 to d7bf852 Compare September 23, 2026 20:03

pull-request-size Bot added size/L and removed size/XL labels Sep 23, 2026

rusackas mentioned this pull request Sep 23, 2026

fix(charts): decouple annotation cache from the shared dataframe cache #44406

Open

9 tasks

Navigation Menu

test(bigquery): verify string-literal escaping against a real GoogleSQL engine - #44546

test(bigquery): verify string-literal escaping against a real GoogleSQL engine#44546
rusackas wants to merge 2 commits into
masterfrom
test/bigquery-testcontainers-literal-escaping

rusackas commented Sep 22, 2026

Uh oh!

bito-code-review Bot commented Sep 22, 2026 •

edited

Loading

Code Review Agent Run #8459ce

Uh oh!

netlify Bot commented Sep 22, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Sep 22, 2026 •

edited

Loading

Uh oh!

Uh oh!

bito-code-review Bot commented Sep 23, 2026

Uh oh!

bito-code-review Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bito-code-review Bot commented Sep 23, 2026 •

edited

Loading

Code Review Agent Run #873076

Uh oh!

rusackas commented Sep 23, 2026

Uh oh!

bito-code-review Bot commented Sep 23, 2026 •

edited

Loading

Code Review Agent Run #a3bbe1

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

rusackas commented Sep 22, 2026

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

bito-code-review Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #8459ce

Uh oh!

netlify Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for superset-docs-preview ready!

Uh oh!

codecov Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

bito-code-review Bot commented Sep 23, 2026

Uh oh!

bito-code-review Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Code Review Agent Run #b23663

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bito-code-review Bot commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #873076

Uh oh!

rusackas commented Sep 23, 2026

Uh oh!

bito-code-review Bot commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #a3bbe1

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

bito-code-review Bot commented Sep 22, 2026 •

edited

Loading

netlify Bot commented Sep 22, 2026 •

edited

Loading

codecov Bot commented Sep 22, 2026 •

edited

Loading

bito-code-review Bot left a comment •

edited

Loading

bito-code-review Bot commented Sep 23, 2026 •

edited

Loading

bito-code-review Bot commented Sep 23, 2026 •

edited

Loading