fix(bigquery): ensure BigQuery client uses project from engine URI#41975
fix(bigquery): ensure BigQuery client uses project from engine URI#41975Abhiiishek44 wants to merge 2 commits into
Conversation
Code Review Agent Run #4f9425Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| "Could not import libraries needed to connect to BigQuery." | ||
| ) | ||
|
|
||
| project = engine.url.host or None |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for the new local project variable to satisfy the type-hint requirement. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new local variable project is clearly type-annotatable as str | None, but it is introduced without a type hint. This matches the custom rule requiring type hints on new or modified Python variables that can be annotated.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/db_engine_specs/bigquery.py
**Line:** 731:731
**Comment:**
*Custom Rule: Add an explicit type annotation for the new local `project` variable to satisfy the type-hint requirement.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| credentials_info = {"project_id": "credential-project"} | ||
| credentials = mock.Mock() | ||
| engine = mock.MagicMock() | ||
| engine.url = make_url("bigquery://uri-project") | ||
| engine.dialect.credentials_info = credentials_info | ||
| create_credentials = mocker.patch( | ||
| "superset.db_engine_specs.bigquery.service_account.Credentials." | ||
| "from_service_account_info", | ||
| return_value=credentials, | ||
| ) | ||
| client = mocker.patch("superset.db_engine_specs.bigquery.bigquery.Client") |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for newly introduced local variables in this test to satisfy the type-hint requirement. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The added test introduces several local variables without type annotations, including mock objects and the credentials_info dict. This matches the type-hint rule for newly added Python code, so the suggestion is a real violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_bigquery.py
**Line:** 439:449
**Comment:**
*Custom Rule: Add explicit type annotations for newly introduced local variables in this test to satisfy the type-hint requirement.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| credentials = mock.Mock() | ||
| engine = mock.MagicMock() | ||
| engine.url = make_url("bigquery://uri-project") | ||
| engine.dialect.credentials_info = None | ||
| get_default_credentials = mocker.patch( | ||
| "superset.db_engine_specs.bigquery.google.auth.default", | ||
| return_value=(credentials, "credential-project"), | ||
| ) | ||
| client = mocker.patch("superset.db_engine_specs.bigquery.bigquery.Client") |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for the new local variables in this test, including mocked objects and patched call handles. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new test block assigns mock and patched objects to local variables without type hints. That is a direct match for the Python type-hint rule on newly introduced code.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_bigquery.py
**Line:** 463:471
**Comment:**
*Custom Rule: Add explicit type annotations for the new local variables in this test, including mocked objects and patched call handles.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41975 +/- ##
==========================================
- Coverage 64.88% 55.95% -8.93%
==========================================
Files 2742 2746 +4
Lines 153378 153674 +296
Branches 35168 35196 +28
==========================================
- Hits 99516 85989 -13527
- Misses 51958 66856 +14898
+ Partials 1904 829 -1075
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:
|
|
Looks like the fix only handles the |
|
Thanks. I’ll update |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
2 participants
SUMMARY
Fixes BigQuery metadata operations resolving datasets against the Application Default Credentials project instead of the project configured in the connection URI.
BigQueryEngineSpec._get_clientpreviously createdbigquery.Clientwithout specifying a project. When the credentials project differed from the project in a connection such asbigquery://data-project, unqualified BigQuery table references were resolved against the credentials project and could return a dataset-not-found error.This change:
engine.url.host.bigquery.Clientfor both service-account credentials and Application Default Credentials.Fixes #41906
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable. This is a backend-only change.
TESTING INSTRUCTIONS
Automated tests:
python -m pytest \ tests/unit_tests/db_engine_specs/test_bigquery.py \ -k "get_client_uses_uri_project" \ -vvResult:
Code-quality checks:
Result:
ADDITIONAL INFORMATION