-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
feat(oauth): adding necessary changes to support bigquery oauth #30674
base: master
Are you sure you want to change the base?
Conversation
@@ -225,6 +225,10 @@ def ping(engine: Engine) -> bool: | |||
# bubble up the exception to return proper status code | |||
raise | |||
except Exception as ex: | |||
if database.is_oauth2_enabled() and database.db_engine_spec.needs_oauth2( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BigQuery raises an error when running create_engine()
necessitating another needs_oauth2(ex)
check for the proper oauth2 exceptions to trigger the Oauth dance. Most DB's allow for an engine to be created without valid creds, and instead raises the exception on engine.connect()
@@ -1691,10 +1691,13 @@ def select_star( # pylint: disable=too-many-arguments | |||
return sql | |||
|
|||
@classmethod | |||
def estimate_statement_cost(cls, statement: str, cursor: Any) -> dict[str, Any]: | |||
def estimate_statement_cost( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Database is required to create the BigQuery Client when using OAuth2
@@ -351,7 +351,16 @@ def get_allow_cost_estimate(cls, extra: dict[str, Any]) -> bool: | |||
return True | |||
|
|||
@classmethod | |||
def estimate_statement_cost(cls, statement: str, cursor: Any) -> dict[str, Any]: | |||
def estimate_statement_cost( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aligning other specs to have database, and adding missing docstring
@@ -365,9 +365,12 @@ def get_schema_from_engine_params( | |||
return parse.unquote(database.split("/")[1]) | |||
|
|||
@classmethod | |||
def estimate_statement_cost(cls, statement: str, cursor: Any) -> dict[str, Any]: | |||
def estimate_statement_cost( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding database param
superset/models/core.py
Outdated
effective_username, | ||
access_token, | ||
) | ||
# Checking if the function signature can accept database as a param |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, here BigQuery needs the Database object to build the client. Checks if the function signature has database
as a param, then passes it.
@@ -192,3 +192,4 @@ class OAuth2ClientConfigSchema(Schema): | |||
) | |||
authorization_request_uri = fields.String(required=True) | |||
token_request_uri = fields.String(required=True) | |||
project_id = fields.String(required=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BigQuery takes project_id
as a param in its OAuth2
parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ideally we'd want to store the project id in the URI, to make it compatible with non-oauth2 use cases. And then later when you need you can grab it from database.sqlalchemy_uri.database
.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #30674 +/- ##
===========================================
+ Coverage 60.48% 70.82% +10.33%
===========================================
Files 1931 1987 +56
Lines 76236 80190 +3954
Branches 8568 9170 +602
===========================================
+ Hits 46114 56792 +10678
+ Misses 28017 21166 -6851
- Partials 2105 2232 +127
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but wondering if we need to modify _get_client
here to take a database object.
11f631e
to
96bcb1c
Compare
96bcb1c
to
6e42328
Compare
5872351
to
25cdd3a
Compare
@@ -106,6 +108,15 @@ export const OAuth2ClientField = ({ changeMethods, db }: FieldPropTypes) => { | |||
onChange={handleChange('scope')} | |||
/> | |||
</FormItem> | |||
{db.engine === Engines.BigQuery && ( | |||
<FormItem label="Project ID"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's localize the label
) | ||
# PR #30674 changed the signature of the method to include database. | ||
# This ensures that the change is backwards compatible | ||
sig = signature(self.db_engine_spec.update_impersonation_config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have sufficient test coverage for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I left a few comments on the handling of project_id
. Happy to hop on a meeting to discuss it in more detail.
|
||
interface OAuth2ClientInfo { | ||
id: string; | ||
secret: string; | ||
authorization_request_uri: string; | ||
token_request_uri: string; | ||
scope: string; | ||
project_id?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is BigQuery specific, it's better to not add it here, otherwise the interface loses its purpose. Ideally we want to keep only the common information that every oauth2 client has.
Can we have this in a separate attribute instead?
@@ -192,3 +192,4 @@ class OAuth2ClientConfigSchema(Schema): | |||
) | |||
authorization_request_uri = fields.String(required=True) | |||
token_request_uri = fields.String(required=True) | |||
project_id = fields.String(required=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ideally we'd want to store the project id in the URI, to make it compatible with non-oauth2 use cases. And then later when you need you can grab it from database.sqlalchemy_uri.database
.
SUMMARY
Adding needed changes to support future implementation of OAuth2 functionality for BigQuery.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION