From c72894725ec1ac88c86f15a33fe9e7922cc0419c Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Tue, 25 May 2021 17:13:17 -0400 Subject: [PATCH] fix: Fix Big Query API for POST w/ no parameters (#14822) * Update schemas.py * Update bigquery.py * Fix tests Co-authored-by: Beto Dealmeida --- superset/databases/schemas.py | 49 +++++++++++++++------------- superset/db_engine_specs/bigquery.py | 6 ++-- tests/databases/api_tests.py | 2 +- tests/databases/schema_tests.py | 22 ++++++++----- 4 files changed, 45 insertions(+), 34 deletions(-) diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index e95296740b..5428c8596e 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -233,6 +233,12 @@ class DatabaseParametersSchemaMixin: values=fields.Raw(), description="DB-specific parameters for configuration", ) + configuration_method = EnumField( + ConfigurationMethod, + by_value=True, + description=configuration_method_description, + missing=ConfigurationMethod.SQLALCHEMY_FORM, + ) # pylint: disable=no-self-use, unused-argument @pre_load @@ -252,7 +258,8 @@ class DatabaseParametersSchemaMixin: # frontend is not passing engine inside parameters engine = data.pop("engine", None) or parameters.pop("engine", None) - if parameters: + configuration_method = data.get("configuration_method") + if configuration_method == ConfigurationMethod.DYNAMIC_FORM: if not engine: raise ValidationError( [ @@ -269,19 +276,26 @@ class DatabaseParametersSchemaMixin: ) engine_spec = engine_specs[engine] - if hasattr(engine_spec, "build_sqlalchemy_uri"): - serialized_encrypted_extra = data.get("encrypted_extra", "{}") - try: - encrypted_extra = json.loads(serialized_encrypted_extra) - except json.decoder.JSONDecodeError: - encrypted_extra = {} - - data[ - "sqlalchemy_uri" - ] = engine_spec.build_sqlalchemy_uri( # type: ignore - parameters, encrypted_extra + if not hasattr(engine_spec, "build_sqlalchemy_uri"): + raise ValidationError( + [ + _( + 'Engine spec "InvalidEngine" does not support ' + "being configured via individual parameters." + ) + ] ) + serialized_encrypted_extra = data.get("encrypted_extra", "{}") + try: + encrypted_extra = json.loads(serialized_encrypted_extra) + except json.decoder.JSONDecodeError: + encrypted_extra = {} + + data["sqlalchemy_uri"] = engine_spec.build_sqlalchemy_uri( # type: ignore + parameters, encrypted_extra + ) + return data @@ -325,11 +339,6 @@ class DatabasePostSchema(Schema, DatabaseParametersSchemaMixin): allow_ctas = fields.Boolean(description=allow_ctas_description) allow_cvas = fields.Boolean(description=allow_cvas_description) allow_dml = fields.Boolean(description=allow_dml_description) - configuration_method = EnumField( - ConfigurationMethod, - by_value=True, - description=configuration_method_description, - ) force_ctas_schema = fields.String( description=force_ctas_schema_description, allow_none=True, @@ -367,12 +376,6 @@ class DatabasePutSchema(Schema, DatabaseParametersSchemaMixin): description=cache_timeout_description, allow_none=True ) expose_in_sqllab = fields.Boolean(description=expose_in_sqllab_description) - configuration_method = EnumField( - ConfigurationMethod, - by_value=True, - allow_none=True, - description=configuration_method_description, - ) allow_run_async = fields.Boolean(description=allow_run_async_description) allow_csv_upload = fields.Boolean(description=allow_csv_upload_description) allow_ctas = fields.Boolean(description=allow_ctas_description) diff --git a/superset/db_engine_specs/bigquery.py b/superset/db_engine_specs/bigquery.py index a7ce77c580..5f46bb3cfa 100644 --- a/superset/db_engine_specs/bigquery.py +++ b/superset/db_engine_specs/bigquery.py @@ -307,10 +307,12 @@ class BigQueryEngineSpec(BaseEngineSpec): @classmethod def build_sqlalchemy_uri( - cls, _: BigQueryParametersType, encrypted_extra: Optional[Dict[str, str]] = None + cls, _: BigQueryParametersType, encrypted_extra: Optional[Dict[str, Any]] = None ) -> str: if encrypted_extra: - project_id = encrypted_extra.get("project_id") + project_id = encrypted_extra.get("credentials_info", {}).get("project_id") + + if project_id: return f"{cls.drivername}://{project_id}" raise SupersetGenericDBErrorException( diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index 9ad0b1970e..3e82a7eb47 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -510,7 +510,7 @@ class TestDatabaseApi(SupersetTestCase): self.login(username="admin") database_data = { "database_name": "test-database-updated", - "configuration_method": ConfigurationMethod.DYNAMIC_FORM, + "configuration_method": ConfigurationMethod.SQLALCHEMY_FORM, } uri = f"api/v1/database/{test_database.id}" rv = self.client.put(uri, json=database_data) diff --git a/tests/databases/schema_tests.py b/tests/databases/schema_tests.py index 6e10e21786..80859a7496 100644 --- a/tests/databases/schema_tests.py +++ b/tests/databases/schema_tests.py @@ -21,6 +21,7 @@ from marshmallow import fields, Schema, ValidationError from superset.databases.schemas import DatabaseParametersSchemaMixin from superset.db_engine_specs.base import BasicParametersMixin +from superset.models.core import ConfigurationMethod class DummySchema(Schema, DatabaseParametersSchemaMixin): @@ -39,31 +40,34 @@ class InvalidEngine: def test_database_parameters_schema_mixin(get_engine_specs): get_engine_specs.return_value = {"dummy_engine": DummyEngine} payload = { + "engine": "dummy_engine", + "configuration_method": ConfigurationMethod.DYNAMIC_FORM, "parameters": { - "engine": "dummy_engine", "username": "username", "password": "password", "host": "localhost", "port": 12345, "database": "dbname", - } + }, } schema = DummySchema() result = schema.load(payload) assert result == { - "sqlalchemy_uri": "dummy://username:password@localhost:12345/dbname" + "configuration_method": ConfigurationMethod.DYNAMIC_FORM, + "sqlalchemy_uri": "dummy://username:password@localhost:12345/dbname", } def test_database_parameters_schema_mixin_no_engine(): payload = { + "configuration_method": ConfigurationMethod.DYNAMIC_FORM, "parameters": { "username": "username", "password": "password", "host": "localhost", "port": 12345, "dbname": "dbname", - } + }, } schema = DummySchema() try: @@ -80,14 +84,15 @@ def test_database_parameters_schema_mixin_no_engine(): def test_database_parameters_schema_mixin_invalid_engine(get_engine_specs): get_engine_specs.return_value = {} payload = { + "engine": "dummy_engine", + "configuration_method": ConfigurationMethod.DYNAMIC_FORM, "parameters": { - "engine": "dummy_engine", "username": "username", "password": "password", "host": "localhost", "port": 12345, "dbname": "dbname", - } + }, } schema = DummySchema() try: @@ -102,14 +107,15 @@ def test_database_parameters_schema_mixin_invalid_engine(get_engine_specs): def test_database_parameters_schema_no_mixin(get_engine_specs): get_engine_specs.return_value = {"invalid_engine": InvalidEngine} payload = { + "engine": "invalid_engine", + "configuration_method": ConfigurationMethod.DYNAMIC_FORM, "parameters": { - "engine": "invalid_engine", "username": "username", "password": "password", "host": "localhost", "port": 12345, "database": "dbname", - } + }, } schema = DummySchema() try: