Repository navigation
test(bigquery): verify string-literal escaping against a real GoogleSQL engine - #44546
Conversation
Code Review Agent Run #8459ceActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44546 +/- ##
===========================================
+ Coverage 65.02% 83.30% +18.28%
===========================================
Files 3039 3039
Lines 198382 198382
Branches 46213 46213
===========================================
+ Hits 128992 165260 +36268
+ Misses 66489 29898 -36591
- Partials 2901 3224 +323
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3e5f158 to
fe7821f
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. In the current implementation, if a DAO lookup or lazy datasource loading (such as To resolve this, you should wrap the annotation-related lookups and cache-key generation in a 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 |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
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-129The 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
Code Review Agent Run #873076Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
7f2a133 to
d7bf852
Compare
Code Review Agent Run #a3bbe1Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
github_actions
There was a problem hiding this comment.
Code Review Agent Run #a7b4d7
Actionable Suggestions - 1
-
tests/testcontainers/db_engine_specs/_bigquery_container.py - 1
- Missing docstrings on new helpers · Line 45-70
Review Details
-
Files reviewed - 3 · Commit Range:
d7bf852..a4d1960- requirements/development.in
- tests/testcontainers/db_engine_specs/_bigquery_container.py
- tests/testcontainers/db_engine_specs/test_bigquery.py
-
Files skipped - 1
- .github/workflows/testcontainers.yml - Reason: Filter setting
-
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
Code Review Agent Run #0b9931Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #4c7745Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
56581e2 to
690d1ce
Compare
Code Review Agent Run #62b438Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Ran the suite against the emulator (5 passed) and both compile paths side by side. Two of @sadpandajoe's points still measure as open at 690d1ce, so flagging rather than approving. The emulator is genuine GoogleSQL, so the title's claim holds. Details inline.
CodeAnt PR Risk: Low Risk
Assessed commit: |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Earlier points are fixed at 9cef70d. The apostrophe/percent round-trips still pass with the BigQuery literal patch removed, so they do not pin it yet (inline). Merging master should clear the OpenAPI-drift and GAQ Playwright reds, which come from the stale base.
…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>
… matrix testcontainers-python has no released BigQueryContainer -- it's still an open, unmerged upstream PR (testcontainers/testcontainers-python#1121), missing from every published release including the current latest (4.15.0). Importing it from testcontainers.community.google as this test did previously would ImportError on any real installation. Vendors a local BigQueryContainer (tests/testcontainers/db_engine_specs/ _bigquery_container.py) wrapping the same goccy/bigquery-emulator image, following the same pattern this suite already uses for StarRocks and ClickHouse when testcontainers-python has no dedicated module. Adds the missing `bigquery` entry to testcontainers.yml's matrix so this coverage actually runs in CI, and documents why no extra is needed for it in requirements/development.in. Verified locally against real Docker with the same env vars the CI job uses -- all 4 tests pass. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds the shared require_driver() guard around the testcontainers import so environments without the testcontainers/BigQuery extras skip this module at collection time instead of failing, matching the pattern the other per-dialect testcontainers tests already use. Also adds one-line docstrings to the vendored BigQueryContainer class and its methods. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_insert_and_find() wrapped the compiled literal in sa.text() and executed it via Connection.execute(), which lets SQLAlchemy's text-clause compiler double every literal % before the BigQuery DBAPI's own %% -> % de-escaping runs. That differs from Superset's actual execution path (BaseEngineSpec.execute() calls cursor.execute(query) on a raw DBAPI cursor, no separate parameters), so the test could round-trip a value that the real code path would mangle. Switch to engine.raw_connection() + cursor.execute(), matching production exactly, and add a test pinning the one case where that distinction matters: a literal doubled percent sign in the data is actually collapsed to one by the DBAPI's unconditional de-escape, a pre-existing DBAPI-level limitation unrelated to the literal_processor fix this module otherwise verifies. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…per docstring Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ring Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
9cef70d to
deed8dd
Compare
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Approving: the compiled-literal assertion and the doubled-percent check both fail when _monkeypatch_bigquery_string_literal is removed, and the docstrings match compile_sqla_query and the BigQuery DBAPI.
3 participants
SUMMARY
Adds testcontainers coverage that runs Superset's actual BigQuery string-literal escaping (
_monkeypatch_bigquery_string_literalinsuperset/db_engine_specs/bigquery.py) against a real GoogleSQL engine (thegoccy/bigquery-emulator, via the newBigQueryContainerin testcontainers/testcontainers-python#1121), instead of only reasoning about it from thesqlalchemy-bigquerydialect source and the BigQuery DBAPI'spyformathandling.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 viapytest.ini) andrequire_driver("testcontainers.community.google"), matching the existingtests/testcontainers/db_engine_specs/convention. Needs Docker running locally, or runs via.github/workflows/testcontainers.yml:All 4 tests pass locally against
superset/db_engine_specs/bigquery.pyunmodified.ADDITIONAL INFORMATION
🤖 Generated with Claude Code