Skip to content
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

Prod 2435: bq connection test #5138

Merged
merged 12 commits into from
Jul 31, 2024
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import json
from typing import List, Optional, Union

from google.cloud.bigquery import Client as BigQueryClient
from pydantic import EmailStr, Field, parse_obj_as, validator
from pydantic.main import BaseModel

Expand Down Expand Up @@ -47,6 +48,9 @@
v = json.loads(v)
return parse_obj_as(KeyfileCreds, v)

def get_client(self) -> BigQueryClient:
return BigQueryClient.from_service_account_info(self.keyfile_creds.dict())

Check warning on line 52 in src/fides/api/schemas/connection_configuration/connection_secrets_bigquery.py

View check run for this annotation

Codecov / codecov/patch

src/fides/api/schemas/connection_configuration/connection_secrets_bigquery.py#L52

Added line #L52 was not covered by tests


class BigQueryDocsSchema(BigQuerySchema, NoValidationSchema):
"""BigQuery Secrets Schema for API Docs"""
23 changes: 23 additions & 0 deletions src/fides/api/service/connectors/sql_connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,29 @@
"""Query wrapper corresponding to the input execution_node."""
return BigQueryQueryConfig(node)

# Overrides SQLConnector.test_connection
def test_connection(self) -> Optional[ConnectionTestStatus]:
Comment on lines +513 to +514
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be a question of preference, but i tend to put these sort of comments as proper method docstrings, and that plays nicely with many IDEs. i would note here that we're specifically testing with the native bigquery python client that's used for the d&d workflows, even though it is not yet used for the DSR workflows. something like this?

Suggested change
# Overrides SQLConnector.test_connection
def test_connection(self) -> Optional[ConnectionTestStatus]:
def test_connection(self) -> Optional[ConnectionTestStatus]:
"""
Overrides SQLConnector.test_connection with a BigQuery-specific connection test.
The connection is tested using the native python client for BigQuery, since that is what's used
by the detection and discovery workflows/codepaths. We will eventually look to migrate the
rest of this class, used for DSR execution, to also make use of the native bigquery client.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"""
Overrides SQLConnector.test_connection with a BigQuery-specific connection test.

The connection is tested using the native python client for BigQuery, since that is what's used
by the detection and discovery workflows/codepaths.
TODO: migrate the rest of this class, used for DSR execution, to also make use of the native bigquery client.
"""
try:
bq_schema = BigQuerySchema(**self.configuration.secrets or {})
client = bq_schema.get_client()

Check warning on line 524 in src/fides/api/service/connectors/sql_connector.py

View check run for this annotation

Codecov / codecov/patch

src/fides/api/service/connectors/sql_connector.py#L522-L524

Added lines #L522 - L524 were not covered by tests
all_projects = [project for project in client.list_projects()]
if all_projects:
return ConnectionTestStatus.succeeded
logger.error("No Bigquery Projects found with the provided credentials.")
raise ConnectionException(

Check warning on line 529 in src/fides/api/service/connectors/sql_connector.py

View check run for this annotation

Codecov / codecov/patch

src/fides/api/service/connectors/sql_connector.py#L527-L529

Added lines #L527 - L529 were not covered by tests
"No Bigquery Projects found with the provided credentials."
)
except Exception as e:
logger.exception(f"Error testing connection to remote BigQuery {str(e)}")
raise ConnectionException(f"Connection error: {e}")

Check warning on line 534 in src/fides/api/service/connectors/sql_connector.py

View check run for this annotation

Codecov / codecov/patch

src/fides/api/service/connectors/sql_connector.py#L532-L534

Added lines #L532 - L534 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is solid for the exception that's raised up (and hopefully gets to the UI?) in addition, it might be nice to log the exception here too, ideally with a full stack trace - since this endpoint is specifically for troubleshooting, giving as much error information as possible can be helpful.

you can use logger.exception - i think that may give you a stack trace, or you may need to still specify some args/options to get a full stack trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logger exception here: 16d65db#diff-15895ed412c26a8f5c097a7eddfa11273da29a8d999e7c342bdeb475224fff8aR537 I do see the exception in logs if I induce one


def mask_data(
self,
node: ExecutionNode,
Expand Down
1 change: 1 addition & 0 deletions tests/fixtures/bigquery_fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ def bigquery_test_engine() -> Generator:

connector: BigQueryConnector = get_connector(connection_config)
engine = connector.client()
connector.test_connection()
yield engine
engine.dispose()

Expand Down