fix(gsheets): pass service_account_info via adapter_kwargs #38443
fix(gsheets): pass service_account_info via adapter_kwargs #38443piopy wants to merge 1 commit intoapache:masterfrom
Conversation
…params_from_encrypted_extra Fixes apache#38442 The existing assert_called_with assertions in test_validate_parameters_catalog and test_validate_parameters_catalog_and_credentials were updated to reflect the corrected call signature. The previous assertions were inadvertently testing the broken behavior.
Code Review Agent Run #6ffb40Actionable 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes Google Sheets (Shillelagh) service account authentication by ensuring service_account_info is passed via connect_args -> adapter_kwargs -> gsheetsapi, matching Shillelagh’s expected adapter configuration.
Changes:
- Update
GSheetsEngineSpec.update_params_from_encrypted_extrato moveservice_account_info(and copycatalog) intoconnect_args.adapter_kwargs.gsheetsapi. - Update
GSheetsEngineSpec.validate_parametersto pass credentials/subject viaconnect_argsinstead of top-levelcreate_engine()kwargs. - Update unit tests to assert the corrected
create_engine()call signature and add coverage for the newupdate_params_from_encrypted_extrabehavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
superset/db_engine_specs/gsheets.py |
Fixes engine creation/parameter shaping so Shillelagh receives service_account_info under connect_args.adapter_kwargs.gsheetsapi. |
tests/unit_tests/db_engine_specs/test_gsheets.py |
Updates assertions for the corrected call signature and expands test coverage for parameter updates from encrypted extra. |
| engine = create_engine( | ||
| "gsheets://", | ||
| service_account_info=encrypted_credentials, | ||
| subject=subject, | ||
| connect_args={ | ||
| "adapter_kwargs": { | ||
| "gsheetsapi": { | ||
| "service_account_info": encrypted_credentials, | ||
| "subject": subject, | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
| conn = engine.connect() | ||
| idx = 0 |
There was a problem hiding this comment.
validate_parameters opens a SQLAlchemy connection via engine.connect() but never closes it. Since this method can return early (eg, missing name/url) or loop over multiple entries, this can leak connections/handles; wrap the connection in a context manager (or try/finally) and ensure the connection (and optionally the engine) is closed/disposed.
|
The suggestion to wrap the SQLAlchemy connection in a context manager (or use try/finally) is valid and improves the code by preventing potential connection leaks. The validate_parameters method opens a connection that may not be closed if the method returns early due to missing parameters or during iteration, which could exhaust database handles over time. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #38443 +/- ##
==========================================
+ Coverage 64.81% 64.97% +0.15%
==========================================
Files 1815 2515 +700
Lines 71912 126743 +54831
Branches 22915 29230 +6315
==========================================
+ Hits 46608 82345 +35737
- Misses 25304 42969 +17665
- Partials 0 1429 +1429
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixes #38442
The existing assert_called_with assertions in test_validate_parameters_catalog and test_validate_parameters_catalog_and_credentials were updated to reflect the corrected call signature. The previous assertions were inadvertently testing the broken behavior.
SUMMARY
When using Google Sheets with service account authentication, the connection
fails because
service_account_infois passed as a top-level keyword argumentto
create_engine()instead of being nested underconnect_args → adapter_kwargs → gsheetsapi, as required by shillelagh.This affects both
update_params_from_encrypted_extraandvalidate_parameters.Solution:
update_params_from_encrypted_extra: wrapservice_account_infoandcataloginsideconnect_args.adapter_kwargs.gsheetsapivalidate_parameters: useconnect_args={"adapter_kwargs": {"gsheetsapi": {...}}}instead of top-level kwargs in
create_engine()BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Uploaded in Issue 38442
TESTING INSTRUCTIONS
Verified manually with Superset 6.0.0 deployed via Docker Compose with a
Google service account JSON credential + pytest
ADDITIONAL INFORMATION