From 172c42a3f46f7dc5d6554160b4dd780643181063 Mon Sep 17 00:00:00 2001 From: Arash Date: Fri, 30 Apr 2021 11:06:07 -0400 Subject: [PATCH 01/14] db migration for dbs --- ...0256cea_add_save_option_column_to_db_model.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py index e3e3c20a3a5e2..26ebdf20ff436 100644 --- a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py +++ b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py @@ -17,14 +17,22 @@ """add_save_form_column_to_db_model Revision ID: 453530256cea +<<<<<<< HEAD Revises: f1410ed7ec95 +======= +Revises: d416d0d715cc +>>>>>>> db migration for dbs Create Date: 2021-04-30 10:55:07.009994 """ # revision identifiers, used by Alembic. revision = "453530256cea" +<<<<<<< HEAD down_revision = "f1410ed7ec95" +======= +down_revision = "d416d0d715cc" +>>>>>>> db migration for dbs import sqlalchemy as sa from alembic import op @@ -33,14 +41,22 @@ def upgrade(): with op.batch_alter_table("dbs") as batch_op: batch_op.add_column( +<<<<<<< HEAD sa.Column( "configuration_method", sa.VARCHAR(255), server_default="sqlalchemy_form", ) +======= + sa.Column("save_option", sa.VARCHAR(255), server_default="SQL_ALCHEMY") +>>>>>>> db migration for dbs ) def downgrade(): with op.batch_alter_table("dbs") as batch_op: +<<<<<<< HEAD batch_op.drop_column("configuration_method") +======= + batch_op.drop_column("save_option") +>>>>>>> db migration for dbs From 04150732d9dff3209a1f38f9315685987707b5bc Mon Sep 17 00:00:00 2001 From: Arash Date: Mon, 3 May 2021 18:32:13 -0400 Subject: [PATCH 02/14] first draft at logic --- superset/databases/api.py | 1 + superset/databases/commands/create.py | 12 +++++++++-- superset/databases/commands/update.py | 11 +++++++++- ...6cea_add_save_option_column_to_db_model.py | 20 ------------------- superset/models/core.py | 14 +++++++++++-- 5 files changed, 33 insertions(+), 25 deletions(-) diff --git a/superset/databases/api.py b/superset/databases/api.py index 5b795a879f4aa..e34f91798a845 100644 --- a/superset/databases/api.py +++ b/superset/databases/api.py @@ -104,6 +104,7 @@ class DatabaseRestApi(BaseSupersetModelRestApi): "expose_in_sqllab", "allow_run_async", "allow_csv_upload", + "configuration_method", "allow_ctas", "allow_cvas", "allow_dml", diff --git a/superset/databases/commands/create.py b/superset/databases/commands/create.py index cb4c3baead05b..a0ca14bd400bb 100644 --- a/superset/databases/commands/create.py +++ b/superset/databases/commands/create.py @@ -33,6 +33,7 @@ from superset.databases.commands.test_connection import TestConnectionDatabaseCommand from superset.databases.dao import DatabaseDAO from superset.extensions import db, event_logger, security_manager +from superset.models.core import ConfigurationMethod logger = logging.getLogger(__name__) @@ -78,7 +79,9 @@ def validate(self) -> None: exceptions: List[ValidationError] = list() sqlalchemy_uri: Optional[str] = self._properties.get("sqlalchemy_uri") database_name: Optional[str] = self._properties.get("database_name") - + configuration_method: Optional[str] = self._properties.get( + "configuration_method" + ) if not sqlalchemy_uri: exceptions.append(DatabaseRequiredFieldValidationError("sqlalchemy_uri")) if not database_name: @@ -87,7 +90,12 @@ def validate(self) -> None: # Check database_name uniqueness if not DatabaseDAO.validate_uniqueness(database_name): exceptions.append(DatabaseExistsValidationError()) - + if configuration_method: + if ConfigurationMethod(configuration_method) not in { + ConfigurationMethod.SQLALCHEMY_URI, + ConfigurationMethod.DYNAMIC_FORM, + }: + exceptions.append(DatabaseExistsValidationError()) if exceptions: exception = DatabaseInvalidError() exception.add_list(exceptions) diff --git a/superset/databases/commands/update.py b/superset/databases/commands/update.py index 7c48e743aedb8..479b2484da546 100644 --- a/superset/databases/commands/update.py +++ b/superset/databases/commands/update.py @@ -32,7 +32,7 @@ ) from superset.databases.dao import DatabaseDAO from superset.extensions import db, security_manager -from superset.models.core import Database +from superset.models.core import ConfigurationMethod, Database logger = logging.getLogger(__name__) @@ -75,12 +75,21 @@ def validate(self) -> None: if not self._model: raise DatabaseNotFoundError() database_name: Optional[str] = self._properties.get("database_name") + configuration_method: Optional[str] = self._properties.get( + "configuration_method" + ) if database_name: # Check database_name uniqueness if not DatabaseDAO.validate_update_uniqueness( self._model_id, database_name ): exceptions.append(DatabaseExistsValidationError()) + if configuration_method: + if ConfigurationMethod(configuration_method) not in { + ConfigurationMethod.SQLALCHEMY_URI, + ConfigurationMethod.DYNAMIC_FORM, + }: + exceptions.append(DatabaseExistsValidationError()) if exceptions: exception = DatabaseInvalidError() exception.add_list(exceptions) diff --git a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py index 26ebdf20ff436..d4dc1963e09a9 100644 --- a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py +++ b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py @@ -17,22 +17,14 @@ """add_save_form_column_to_db_model Revision ID: 453530256cea -<<<<<<< HEAD -Revises: f1410ed7ec95 -======= Revises: d416d0d715cc ->>>>>>> db migration for dbs Create Date: 2021-04-30 10:55:07.009994 """ # revision identifiers, used by Alembic. revision = "453530256cea" -<<<<<<< HEAD -down_revision = "f1410ed7ec95" -======= down_revision = "d416d0d715cc" ->>>>>>> db migration for dbs import sqlalchemy as sa from alembic import op @@ -41,22 +33,10 @@ def upgrade(): with op.batch_alter_table("dbs") as batch_op: batch_op.add_column( -<<<<<<< HEAD - sa.Column( - "configuration_method", - sa.VARCHAR(255), - server_default="sqlalchemy_form", - ) -======= sa.Column("save_option", sa.VARCHAR(255), server_default="SQL_ALCHEMY") ->>>>>>> db migration for dbs ) def downgrade(): with op.batch_alter_table("dbs") as batch_op: -<<<<<<< HEAD - batch_op.drop_column("configuration_method") -======= batch_op.drop_column("save_option") ->>>>>>> db migration for dbs diff --git a/superset/models/core.py b/superset/models/core.py index 54247281e96b3..ab532077432f6 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -16,13 +16,13 @@ # under the License. # pylint: disable=line-too-long,unused-argument,ungrouped-imports """A collection of ORM sqlalchemy models for Superset""" +import enum import json import logging import textwrap from contextlib import closing from copy import deepcopy from datetime import datetime -from enum import Enum from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Type import numpy @@ -36,6 +36,7 @@ Column, create_engine, DateTime, + Enum, ForeignKey, Integer, MetaData, @@ -99,6 +100,11 @@ class CssTemplate(Model, AuditMixinNullable): css = Column(Text, default="") +class ConfigurationMethod(str, enum.Enum): + SQLALCHEMY_URI = "SQLALCHEMY_URI" + DYNAMIC_FORM = "DYNAMIC_FORM" + + class Database( Model, AuditMixinNullable, ImportExportMixin ): # pylint: disable=too-many-public-methods @@ -118,6 +124,9 @@ class Database( cache_timeout = Column(Integer) select_as_create_table_as = Column(Boolean, default=False) expose_in_sqllab = Column(Boolean, default=True) + configuration_method = Column( + Enum(ConfigurationMethod), server_default=ConfigurationMethod.SQLALCHEMY_URI + ) allow_run_async = Column(Boolean, default=False) allow_csv_upload = Column(Boolean, default=False) allow_ctas = Column(Boolean, default=False) @@ -207,6 +216,7 @@ def data(self) -> Dict[str, Any]: "id": self.id, "name": self.database_name, "backend": self.backend, + "configurationMethod": self.configuration_method, "allow_multi_schema_metadata_fetch": self.allow_multi_schema_metadata_fetch, "allows_subquery": self.allows_subquery, "allows_cost_estimate": self.allows_cost_estimate, @@ -722,7 +732,7 @@ class Log(Model): # pylint: disable=too-few-public-methods referrer = Column(String(1024)) -class FavStarClassName(str, Enum): +class FavStarClassName(str, enum.Enum): CHART = "slice" DASHBOARD = "Dashboard" From 4c2c77ed97ebfa0a680a430af64b2e40ac536c6f Mon Sep 17 00:00:00 2001 From: Arash Date: Tue, 4 May 2021 18:48:09 -0400 Subject: [PATCH 03/14] added unit tests --- superset/databases/api.py | 1 + superset/databases/commands/update.py | 2 +- superset/databases/schemas.py | 5 +- ...6cea_add_save_option_column_to_db_model.py | 4 +- tests/databases/api_tests.py | 72 ++++++++++++++++--- tests/databases/commands_tests.py | 10 +-- 6 files changed, 75 insertions(+), 19 deletions(-) diff --git a/superset/databases/api.py b/superset/databases/api.py index e34f91798a845..18cd963bf115f 100644 --- a/superset/databases/api.py +++ b/superset/databases/api.py @@ -231,6 +231,7 @@ def post(self) -> Response: 500: $ref: '#/components/responses/500' """ + if not request.is_json: return self.response_400(message="Request is not JSON") try: diff --git a/superset/databases/commands/update.py b/superset/databases/commands/update.py index 479b2484da546..83b9d02140ecc 100644 --- a/superset/databases/commands/update.py +++ b/superset/databases/commands/update.py @@ -89,7 +89,7 @@ def validate(self) -> None: ConfigurationMethod.SQLALCHEMY_URI, ConfigurationMethod.DYNAMIC_FORM, }: - exceptions.append(DatabaseExistsValidationError()) + exceptions.append(DatabaseInvalidError()) if exceptions: exception = DatabaseInvalidError() exception.add_list(exceptions) diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index 8251289df88ee..16f0335ee49f6 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -23,6 +23,7 @@ from flask_babel import lazy_gettext as _ from marshmallow import EXCLUDE, fields, pre_load, Schema, validates_schema from marshmallow.validate import Length, ValidationError +from marshmallow_enum import EnumField from sqlalchemy import MetaData from sqlalchemy.engine.url import make_url from sqlalchemy.exc import ArgumentError @@ -30,7 +31,7 @@ from superset.db_engine_specs import get_engine_specs from superset.db_engine_specs.base import BasicParametersMixin from superset.exceptions import CertificateException, SupersetSecurityException -from superset.models.core import PASSWORD_MASK +from superset.models.core import ConfigurationMethod, PASSWORD_MASK from superset.security.analytics_db_safety import check_sqlalchemy_uri from superset.utils.core import markdown, parse_ssl_cert @@ -314,6 +315,7 @@ class Meta: # pylint: disable=too-few-public-methods 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) force_ctas_schema = fields.String( description=force_ctas_schema_description, allow_none=True, @@ -351,6 +353,7 @@ class Meta: # pylint: disable=too-few-public-methods 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_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/migrations/versions/453530256cea_add_save_option_column_to_db_model.py b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py index d4dc1963e09a9..6303e66cea39a 100644 --- a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py +++ b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py @@ -17,14 +17,14 @@ """add_save_form_column_to_db_model Revision ID: 453530256cea -Revises: d416d0d715cc +Revises: f1410ed7ec95 Create Date: 2021-04-30 10:55:07.009994 """ # revision identifiers, used by Alembic. revision = "453530256cea" -down_revision = "d416d0d715cc" +down_revision = "f1410ed7ec95" import sqlalchemy as sa from alembic import op diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index 70234bb5b6aed..caea2845427ac 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -37,7 +37,7 @@ from superset.db_engine_specs.mysql import MySQLEngineSpec from superset.db_engine_specs.postgres import PostgresEngineSpec from superset.errors import SupersetError -from superset.models.core import Database +from superset.models.core import Database, ConfigurationMethod from superset.models.reports import ReportSchedule, ReportScheduleType from superset.utils.core import get_example_database, get_main_database from tests.base_tests import SupersetTestCase @@ -229,6 +229,7 @@ def test_create_database(self): database_data = { "database_name": "test-create-database", "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, + "configuration_method": ConfigurationMethod.SQLALCHEMY_URI, "server_cert": None, "extra": json.dumps(extra), } @@ -242,6 +243,34 @@ def test_create_database(self): db.session.delete(model) db.session.commit() + def test_create_database_invalid_configuration_method(self): + """ + Database API: Test create + """ + extra = { + "metadata_params": {}, + "engine_params": {}, + "metadata_cache_timeout": {}, + "schemas_allowed_for_csv_upload": [], + } + + self.login(username="admin") + example_db = get_example_database() + if example_db.backend == "sqlite": + return + database_data = { + "database_name": "test-create-database", + "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, + "configuration_method": "BAD_FORM", + "server_cert": None, + "extra": json.dumps(extra), + } + + uri = "api/v1/database/" + rv = self.client.post(uri, json=database_data) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual(rv.status_code, 400) + def test_create_database_server_cert_validate(self): """ Database API: Test create server cert validation @@ -345,6 +374,7 @@ def test_create_database_unique_validate(self): database_data = { "database_name": "examples", "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, + "configuration_method": ConfigurationMethod.SQLALCHEMY_URI, } uri = "api/v1/database/" @@ -413,6 +443,7 @@ def test_create_database_conn_fail(self): database_data = { "database_name": "test-create-database-wrong-password", "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, + "configuration_method": ConfigurationMethod.SQLALCHEMY_URI, } uri = "api/v1/database/" @@ -434,7 +465,10 @@ def test_update_database(self): "test-database", example_db.sqlalchemy_uri_decrypted ) self.login(username="admin") - database_data = {"database_name": "test-database-updated"} + database_data = { + "database_name": "test-database-updated", + "configuration_method": ConfigurationMethod.DYNAMIC_FORM, + } uri = f"api/v1/database/{test_database.id}" rv = self.client.put(uri, json=database_data) self.assertEqual(rv.status_code, 200) @@ -535,6 +569,23 @@ def test_update_database_uri_validate(self): db.session.delete(test_database) db.session.commit() + def test_update_database_with_invalid_configuration_method(self): + """ + Database API: Test update + """ + example_db = get_example_database() + test_database = self.insert_database( + "test-database", example_db.sqlalchemy_uri_decrypted + ) + self.login(username="admin") + database_data = { + "database_name": "test-database-updated", + "configuration_method": "BAD_FORM", + } + uri = f"api/v1/database/{test_database.id}" + rv = self.client.put(uri, json=database_data) + self.assertEqual(rv.status_code, 400) + def test_delete_database(self): """ Database API: Test delete @@ -731,8 +782,8 @@ def test_database_schemas(self): """ Database API: Test database schemas """ - self.login("admin") - database = db.session.query(Database).first() + self.login(username="admin") + database = db.session.query(Database).filter_by(database_name="examples").one() schemas = database.get_all_schema_names() rv = self.client.get(f"api/v1/database/{database.id}/schemas/") @@ -856,16 +907,17 @@ def test_test_connection_failed(self): expected_response = { "errors": [ { - "message": "Could not load database driver: MssqlEngineSpec", - "error_type": "GENERIC_COMMAND_ERROR", - "level": "warning", + "message": 'Port None on hostname "url" refused the connection.', + "error_type": "CONNECTION_PORT_CLOSED_ERROR", + "level": "error", "extra": { + "engine_name": "Microsoft SQL", "issue_codes": [ { - "code": 1010, - "message": "Issue 1010 - Superset encountered an error while running a command.", + "code": 1008, + "message": "Issue 1008 - The port is closed.", } - ] + ], }, } ] diff --git a/tests/databases/commands_tests.py b/tests/databases/commands_tests.py index e7a3806d1de11..f5fd98acec728 100644 --- a/tests/databases/commands_tests.py +++ b/tests/databases/commands_tests.py @@ -74,7 +74,6 @@ def test_export_database_command(self, mock_g): "metadata.yaml", "databases/examples.yaml", "datasets/examples/energy_usage.yaml", - "datasets/examples/wb_health_population.yaml", "datasets/examples/birth_names.yaml", } expected_extra = { @@ -88,7 +87,8 @@ def test_export_database_command(self, mock_g): **expected_extra, "engine_params": {"connect_args": {"poll_interval": 0.1}}, } - + print("content keys", contents.keys()) + print("core files", core_files) assert core_files.issubset(set(contents.keys())) if example_db.backend == "postgresql": @@ -106,9 +106,9 @@ def test_export_database_command(self, mock_g): metadata = yaml.safe_load(contents["databases/examples.yaml"]) assert metadata == ( { - "allow_csv_upload": True, - "allow_ctas": True, - "allow_cvas": True, + "allow_csv_upload": False, + "allow_ctas": False, + "allow_cvas": False, "allow_run_async": False, "cache_timeout": None, "database_name": "examples", From 8855ff3a4ce52080e3ab8e41f60dada3535614a9 Mon Sep 17 00:00:00 2001 From: Arash Date: Fri, 7 May 2021 10:45:17 -0400 Subject: [PATCH 04/14] revisions --- superset/databases/schemas.py | 1 + .../453530256cea_add_save_option_column_to_db_model.py | 1 - superset/models/core.py | 4 ++-- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index 16f0335ee49f6..ce8539593a3b5 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -315,6 +315,7 @@ class Meta: # pylint: disable=too-few-public-methods 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 is used on the frontend to inform the backend whether to explode parameters or to provide only a sqlalchemy_uri configuration_method = EnumField(ConfigurationMethod, by_value=True) force_ctas_schema = fields.String( description=force_ctas_schema_description, diff --git a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py index 6303e66cea39a..5c34fe07f55f6 100644 --- a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py +++ b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py @@ -29,7 +29,6 @@ import sqlalchemy as sa from alembic import op - def upgrade(): with op.batch_alter_table("dbs") as batch_op: batch_op.add_column( diff --git a/superset/models/core.py b/superset/models/core.py index ab532077432f6..99255a49b6341 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -101,8 +101,8 @@ class CssTemplate(Model, AuditMixinNullable): class ConfigurationMethod(str, enum.Enum): - SQLALCHEMY_URI = "SQLALCHEMY_URI" - DYNAMIC_FORM = "DYNAMIC_FORM" + SQLALCHEMY_URI = "sqlalchemy_form" + DYNAMIC_FORM = "dynamic_form" class Database( From ae52d8af83b05ce0bab692334f9cd5d0c18d7932 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 7 May 2021 14:27:58 -0700 Subject: [PATCH 05/14] use strings for db values --- superset/models/core.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/superset/models/core.py b/superset/models/core.py index 99255a49b6341..98c6b10f16c7e 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -53,6 +53,7 @@ from sqlalchemy.pool import NullPool from sqlalchemy.schema import UniqueConstraint from sqlalchemy.sql import expression, Select +from sqlalchemy.sql.sqltypes import VARCHAR from superset import app, db_engine_specs, is_feature_enabled from superset.db_engine_specs.base import TimeGrain @@ -125,7 +126,7 @@ class Database( select_as_create_table_as = Column(Boolean, default=False) expose_in_sqllab = Column(Boolean, default=True) configuration_method = Column( - Enum(ConfigurationMethod), server_default=ConfigurationMethod.SQLALCHEMY_URI + String(255), server_default=ConfigurationMethod.SQLALCHEMY_URI.value ) allow_run_async = Column(Boolean, default=False) allow_csv_upload = Column(Boolean, default=False) From 8aeda42ecf573851a146290f2e044bfa962dd561 Mon Sep 17 00:00:00 2001 From: Arash Date: Fri, 7 May 2021 18:14:11 -0400 Subject: [PATCH 06/14] lint and revisions to tests --- superset/databases/schemas.py | 4 +++- superset/models/core.py | 2 -- tests/databases/api_tests.py | 12 ++++++------ tests/databases/commands_tests.py | 2 -- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index ce8539593a3b5..5f6cfd731b0af 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -315,7 +315,9 @@ class Meta: # pylint: disable=too-few-public-methods 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 is used on the frontend to inform the backend whether to explode parameters or to provide only a sqlalchemy_uri + # configuration_method is used on the frontend to + # inform the backend whether to explode parameters + # or to provide only a sqlalchemy_uri configuration_method = EnumField(ConfigurationMethod, by_value=True) force_ctas_schema = fields.String( description=force_ctas_schema_description, diff --git a/superset/models/core.py b/superset/models/core.py index 98c6b10f16c7e..5ad5373746fe2 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -36,7 +36,6 @@ Column, create_engine, DateTime, - Enum, ForeignKey, Integer, MetaData, @@ -53,7 +52,6 @@ from sqlalchemy.pool import NullPool from sqlalchemy.schema import UniqueConstraint from sqlalchemy.sql import expression, Select -from sqlalchemy.sql.sqltypes import VARCHAR from superset import app, db_engine_specs, is_feature_enabled from superset.db_engine_specs.base import TimeGrain diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index caea2845427ac..f28584802c5ab 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -895,7 +895,7 @@ def test_test_connection_failed(self): self.assertEqual(response, expected_response) data = { - "sqlalchemy_uri": "mssql+pymssql://url", + "sqlalchemy_uri": "postgres://url", "database_name": "examples", "impersonate_user": False, "server_cert": None, @@ -907,15 +907,15 @@ def test_test_connection_failed(self): expected_response = { "errors": [ { - "message": 'Port None on hostname "url" refused the connection.', - "error_type": "CONNECTION_PORT_CLOSED_ERROR", + "message": 'The host "url" might be down, and can\'t be reached on port 5432.', + "error_type": "CONNECTION_HOST_DOWN_ERROR", "level": "error", "extra": { - "engine_name": "Microsoft SQL", + "engine_name": "PostgreSQL", "issue_codes": [ { - "code": 1008, - "message": "Issue 1008 - The port is closed.", + "code": 1009, + "message": "Issue 1009 - The host might be down, and can't be reached on the provided port.", } ], }, diff --git a/tests/databases/commands_tests.py b/tests/databases/commands_tests.py index f5fd98acec728..259878d48683b 100644 --- a/tests/databases/commands_tests.py +++ b/tests/databases/commands_tests.py @@ -87,8 +87,6 @@ def test_export_database_command(self, mock_g): **expected_extra, "engine_params": {"connect_args": {"poll_interval": 0.1}}, } - print("content keys", contents.keys()) - print("core files", core_files) assert core_files.issubset(set(contents.keys())) if example_db.backend == "postgresql": From 3a6815b68e92852320dd2c9aebbd6cfd7ad3bc00 Mon Sep 17 00:00:00 2001 From: Arash Date: Tue, 11 May 2021 16:29:22 -0400 Subject: [PATCH 07/14] changed test back --- tests/databases/api_tests.py | 15 +++++++-------- tests/databases/commands_tests.py | 6 +++--- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index f28584802c5ab..448f8cab71889 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -895,7 +895,7 @@ def test_test_connection_failed(self): self.assertEqual(response, expected_response) data = { - "sqlalchemy_uri": "postgres://url", + "sqlalchemy_uri": "mssql+pymssql://url", "database_name": "examples", "impersonate_user": False, "server_cert": None, @@ -907,17 +907,16 @@ def test_test_connection_failed(self): expected_response = { "errors": [ { - "message": 'The host "url" might be down, and can\'t be reached on port 5432.', - "error_type": "CONNECTION_HOST_DOWN_ERROR", - "level": "error", + "message": "Could not load database driver: MssqlEngineSpec", + "error_type": "GENERIC_COMMAND_ERROR", + "level": "warning", "extra": { - "engine_name": "PostgreSQL", "issue_codes": [ { - "code": 1009, - "message": "Issue 1009 - The host might be down, and can't be reached on the provided port.", + "code": 1010, + "message": "Issue 1010 - Superset encountered an error while running a command.", } - ], + ] }, } ] diff --git a/tests/databases/commands_tests.py b/tests/databases/commands_tests.py index 259878d48683b..92152173ebc8b 100644 --- a/tests/databases/commands_tests.py +++ b/tests/databases/commands_tests.py @@ -104,9 +104,9 @@ def test_export_database_command(self, mock_g): metadata = yaml.safe_load(contents["databases/examples.yaml"]) assert metadata == ( { - "allow_csv_upload": False, - "allow_ctas": False, - "allow_cvas": False, + "allow_csv_upload": True, + "allow_ctas": True, + "allow_cvas": True, "allow_run_async": False, "cache_timeout": None, "database_name": "examples", From 7aacc07505786fe34a91fbbc61da9200a3f3eef8 Mon Sep 17 00:00:00 2001 From: Arash Date: Wed, 12 May 2021 17:54:22 -0400 Subject: [PATCH 08/14] added revisions for testing --- superset/databases/api.py | 1 + superset/databases/commands/create.py | 9 ----- superset/databases/commands/update.py | 6 --- superset/databases/schemas.py | 22 ++++++++--- tests/databases/api_tests.py | 57 ++++++++++++++++++++++++++- 5 files changed, 73 insertions(+), 22 deletions(-) diff --git a/superset/databases/api.py b/superset/databases/api.py index 18cd963bf115f..a47d5b4410962 100644 --- a/superset/databases/api.py +++ b/superset/databases/api.py @@ -147,6 +147,7 @@ class DatabaseRestApi(BaseSupersetModelRestApi): "allow_ctas", "allow_cvas", "allow_dml", + "configuration_method", "force_ctas_schema", "impersonate_user", "allow_multi_schema_metadata_fetch", diff --git a/superset/databases/commands/create.py b/superset/databases/commands/create.py index a0ca14bd400bb..c0f1d685bc177 100644 --- a/superset/databases/commands/create.py +++ b/superset/databases/commands/create.py @@ -79,9 +79,6 @@ def validate(self) -> None: exceptions: List[ValidationError] = list() sqlalchemy_uri: Optional[str] = self._properties.get("sqlalchemy_uri") database_name: Optional[str] = self._properties.get("database_name") - configuration_method: Optional[str] = self._properties.get( - "configuration_method" - ) if not sqlalchemy_uri: exceptions.append(DatabaseRequiredFieldValidationError("sqlalchemy_uri")) if not database_name: @@ -90,12 +87,6 @@ def validate(self) -> None: # Check database_name uniqueness if not DatabaseDAO.validate_uniqueness(database_name): exceptions.append(DatabaseExistsValidationError()) - if configuration_method: - if ConfigurationMethod(configuration_method) not in { - ConfigurationMethod.SQLALCHEMY_URI, - ConfigurationMethod.DYNAMIC_FORM, - }: - exceptions.append(DatabaseExistsValidationError()) if exceptions: exception = DatabaseInvalidError() exception.add_list(exceptions) diff --git a/superset/databases/commands/update.py b/superset/databases/commands/update.py index 83b9d02140ecc..ec3cb1a97fccc 100644 --- a/superset/databases/commands/update.py +++ b/superset/databases/commands/update.py @@ -84,12 +84,6 @@ def validate(self) -> None: self._model_id, database_name ): exceptions.append(DatabaseExistsValidationError()) - if configuration_method: - if ConfigurationMethod(configuration_method) not in { - ConfigurationMethod.SQLALCHEMY_URI, - ConfigurationMethod.DYNAMIC_FORM, - }: - exceptions.append(DatabaseInvalidError()) if exceptions: exception = DatabaseInvalidError() exception.add_list(exceptions) diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index 5f6cfd731b0af..d59c735efad2a 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -71,6 +71,11 @@ "all database schemas. For large data warehouse with thousands of " "tables, this can be expensive and put strain on the system." ) # pylint: disable=invalid-name +configuration_method_description = ( + "configuration_method is used on the frontend to" + " inform the backend whether to explode parameters" + " or to provide only a sqlalchemy_uri." +) impersonate_user_description = ( "If Presto, all the queries in SQL Lab are going to be executed as the " "currently logged on user who must have permission to run them.
" @@ -315,10 +320,12 @@ class Meta: # pylint: disable=too-few-public-methods 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 is used on the frontend to - # inform the backend whether to explode parameters - # or to provide only a sqlalchemy_uri - configuration_method = EnumField(ConfigurationMethod, by_value=True) + configuration_method = EnumField( + ConfigurationMethod, + by_value=True, + required=True, + description=configuration_method_description, + ) force_ctas_schema = fields.String( description=force_ctas_schema_description, allow_none=True, @@ -356,7 +363,12 @@ class Meta: # pylint: disable=too-few-public-methods description=cache_timeout_description, allow_none=True ) expose_in_sqllab = fields.Boolean(description=expose_in_sqllab_description) - configuration_method = EnumField(ConfigurationMethod, by_value=True) + 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/tests/databases/api_tests.py b/tests/databases/api_tests.py index 448f8cab71889..8832cbe5a8fba 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -269,7 +269,40 @@ def test_create_database_invalid_configuration_method(self): uri = "api/v1/database/" rv = self.client.post(uri, json=database_data) response = json.loads(rv.data.decode("utf-8")) - self.assertEqual(rv.status_code, 400) + assert response == { + "message": {"configuration_method": ["Invalid enum value BAD_FORM"]} + } + assert rv.status_code == 400 + + def test_create_database_no_configuration_method(self): + """ + Database API: Test create + """ + extra = { + "metadata_params": {}, + "engine_params": {}, + "metadata_cache_timeout": {}, + "schemas_allowed_for_csv_upload": [], + } + + self.login(username="admin") + example_db = get_example_database() + if example_db.backend == "sqlite": + return + database_data = { + "database_name": "test-create-database", + "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, + "server_cert": None, + "extra": json.dumps(extra), + } + + uri = "api/v1/database/" + rv = self.client.post(uri, json=database_data) + response = json.loads(rv.data.decode("utf-8")) + assert response == { + "message": {"configuration_method": ["Missing data for required field."]} + } + assert rv.status_code == 400 def test_create_database_server_cert_validate(self): """ @@ -584,7 +617,27 @@ def test_update_database_with_invalid_configuration_method(self): } uri = f"api/v1/database/{test_database.id}" rv = self.client.put(uri, json=database_data) - self.assertEqual(rv.status_code, 400) + response = json.loads(rv.data.decode("utf-8")) + assert response == { + "message": {"configuration_method": ["Invalid enum value BAD_FORM"]} + } + assert rv.status_code == 400 + + def test_update_database_with_no_configuration_method(self): + """ + Database API: Test update + """ + example_db = get_example_database() + test_database = self.insert_database( + "test-database", example_db.sqlalchemy_uri_decrypted + ) + self.login(username="admin") + database_data = { + "database_name": "test-database-updated", + } + uri = f"api/v1/database/{test_database.id}" + rv = self.client.put(uri, json=database_data) + assert rv.status_code == 200 def test_delete_database(self): """ From ec6fed62f03979f39c72bdf5e9ecebd162bfadcb Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Wed, 12 May 2021 22:02:30 -0400 Subject: [PATCH 09/14] Update superset/databases/commands/update.py Co-authored-by: Beto Dealmeida --- superset/databases/commands/update.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/databases/commands/update.py b/superset/databases/commands/update.py index ec3cb1a97fccc..9ec7462e5768d 100644 --- a/superset/databases/commands/update.py +++ b/superset/databases/commands/update.py @@ -32,7 +32,7 @@ ) from superset.databases.dao import DatabaseDAO from superset.extensions import db, security_manager -from superset.models.core import ConfigurationMethod, Database +from superset.models.core import Database logger = logging.getLogger(__name__) From 842a28673607c9b6cc1b8e0f5bc497d71ba86082 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Wed, 12 May 2021 22:02:37 -0400 Subject: [PATCH 10/14] Update superset/databases/schemas.py Co-authored-by: Beto Dealmeida --- superset/databases/schemas.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index d59c735efad2a..3ba65fbaba5c5 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -72,7 +72,7 @@ "tables, this can be expensive and put strain on the system." ) # pylint: disable=invalid-name configuration_method_description = ( - "configuration_method is used on the frontend to" + "Configuration_method is used on the frontend to" " inform the backend whether to explode parameters" " or to provide only a sqlalchemy_uri." ) From e4b6575a9b6a3e6fee8124f823334fff09678f26 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Wed, 12 May 2021 22:02:45 -0400 Subject: [PATCH 11/14] Update superset/models/core.py Co-authored-by: Beto Dealmeida --- superset/models/core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/models/core.py b/superset/models/core.py index 5ad5373746fe2..829db80c38456 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -215,7 +215,7 @@ def data(self) -> Dict[str, Any]: "id": self.id, "name": self.database_name, "backend": self.backend, - "configurationMethod": self.configuration_method, + "configuration_method": self.configuration_method, "allow_multi_schema_metadata_fetch": self.allow_multi_schema_metadata_fetch, "allows_subquery": self.allows_subquery, "allows_cost_estimate": self.allows_cost_estimate, From 7da586ef6e4048419d180cf58f8801b7670eb307 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Wed, 12 May 2021 22:03:27 -0400 Subject: [PATCH 12/14] Update superset/databases/commands/update.py Co-authored-by: Beto Dealmeida --- superset/databases/commands/update.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/superset/databases/commands/update.py b/superset/databases/commands/update.py index 9ec7462e5768d..7c48e743aedb8 100644 --- a/superset/databases/commands/update.py +++ b/superset/databases/commands/update.py @@ -75,9 +75,6 @@ def validate(self) -> None: if not self._model: raise DatabaseNotFoundError() database_name: Optional[str] = self._properties.get("database_name") - configuration_method: Optional[str] = self._properties.get( - "configuration_method" - ) if database_name: # Check database_name uniqueness if not DatabaseDAO.validate_update_uniqueness( From 62038d6b4b9e1dad9f369d89bafc8287e81ee4b1 Mon Sep 17 00:00:00 2001 From: Arash Date: Wed, 12 May 2021 22:12:08 -0400 Subject: [PATCH 13/14] got rid of extra imports added new test --- superset/databases/commands/create.py | 1 - ...6cea_add_save_option_column_to_db_model.py | 7 +++++- superset/models/core.py | 4 ++-- tests/databases/api_tests.py | 22 ++++++++++++++----- 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/superset/databases/commands/create.py b/superset/databases/commands/create.py index c0f1d685bc177..ec3d3458b9c5d 100644 --- a/superset/databases/commands/create.py +++ b/superset/databases/commands/create.py @@ -33,7 +33,6 @@ from superset.databases.commands.test_connection import TestConnectionDatabaseCommand from superset.databases.dao import DatabaseDAO from superset.extensions import db, event_logger, security_manager -from superset.models.core import ConfigurationMethod logger = logging.getLogger(__name__) diff --git a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py index 5c34fe07f55f6..0e052dc7c8e45 100644 --- a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py +++ b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py @@ -29,10 +29,15 @@ import sqlalchemy as sa from alembic import op + def upgrade(): with op.batch_alter_table("dbs") as batch_op: batch_op.add_column( - sa.Column("save_option", sa.VARCHAR(255), server_default="SQL_ALCHEMY") + sa.Column( + "configuration_method", + sa.VARCHAR(255), + server_default="sqlalchemy_form", + ) ) diff --git a/superset/models/core.py b/superset/models/core.py index 829db80c38456..5594ccc61876a 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -100,7 +100,7 @@ class CssTemplate(Model, AuditMixinNullable): class ConfigurationMethod(str, enum.Enum): - SQLALCHEMY_URI = "sqlalchemy_form" + SQLALCHEMY_FORM = "sqlalchemy_form" DYNAMIC_FORM = "dynamic_form" @@ -124,7 +124,7 @@ class Database( select_as_create_table_as = Column(Boolean, default=False) expose_in_sqllab = Column(Boolean, default=True) configuration_method = Column( - String(255), server_default=ConfigurationMethod.SQLALCHEMY_URI.value + String(255), server_default=ConfigurationMethod.SQLALCHEMY_FORM.value ) allow_run_async = Column(Boolean, default=False) allow_csv_upload = Column(Boolean, default=False) diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index 8832cbe5a8fba..f83f88ff87b37 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -229,7 +229,7 @@ def test_create_database(self): database_data = { "database_name": "test-create-database", "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, - "configuration_method": ConfigurationMethod.SQLALCHEMY_URI, + "configuration_method": ConfigurationMethod.SQLALCHEMY_FORM, "server_cert": None, "extra": json.dumps(extra), } @@ -240,12 +240,13 @@ def test_create_database(self): self.assertEqual(rv.status_code, 201) # Cleanup model = db.session.query(Database).get(response.get("id")) + assert model.configuration_method == ConfigurationMethod.SQLALCHEMY_FORM db.session.delete(model) db.session.commit() def test_create_database_invalid_configuration_method(self): """ - Database API: Test create + Database API: Test create with an invalid configuration method. """ extra = { "metadata_params": {}, @@ -276,7 +277,7 @@ def test_create_database_invalid_configuration_method(self): def test_create_database_no_configuration_method(self): """ - Database API: Test create + Database API: Test create with no config method. """ extra = { "metadata_params": {}, @@ -316,6 +317,7 @@ def test_create_database_server_cert_validate(self): database_data = { "database_name": "test-create-database-invalid-cert", "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, + "configuration_method": ConfigurationMethod.SQLALCHEMY_FORM, "server_cert": "INVALID CERT", } @@ -338,6 +340,7 @@ def test_create_database_json_validate(self): database_data = { "database_name": "test-create-database-invalid-json", "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, + "configuration_method": ConfigurationMethod.SQLALCHEMY_FORM, "encrypted_extra": '{"A": "a", "B", "C"}', "extra": '["A": "a", "B", "C"]', } @@ -378,6 +381,7 @@ def test_create_database_extra_metadata_validate(self): database_data = { "database_name": "test-create-database-invalid-extra", "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, + "configuration_method": ConfigurationMethod.SQLALCHEMY_FORM, "extra": json.dumps(extra), } @@ -407,7 +411,7 @@ def test_create_database_unique_validate(self): database_data = { "database_name": "examples", "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, - "configuration_method": ConfigurationMethod.SQLALCHEMY_URI, + "configuration_method": ConfigurationMethod.SQLALCHEMY_FORM, } uri = "api/v1/database/" @@ -427,6 +431,7 @@ def test_create_database_uri_validate(self): database_data = { "database_name": "test-database-invalid-uri", "sqlalchemy_uri": "wrong_uri", + "configuration_method": ConfigurationMethod.SQLALCHEMY_FORM, } uri = "api/v1/database/" @@ -448,6 +453,7 @@ def test_create_database_fail_sqllite(self): database_data = { "database_name": "test-create-sqlite-database", "sqlalchemy_uri": "sqlite:////some.db", + "configuration_method": ConfigurationMethod.SQLALCHEMY_FORM, } uri = "api/v1/database/" @@ -476,7 +482,7 @@ def test_create_database_conn_fail(self): database_data = { "database_name": "test-create-database-wrong-password", "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted, - "configuration_method": ConfigurationMethod.SQLALCHEMY_URI, + "configuration_method": ConfigurationMethod.SQLALCHEMY_FORM, } uri = "api/v1/database/" @@ -623,6 +629,9 @@ def test_update_database_with_invalid_configuration_method(self): } assert rv.status_code == 400 + db.session.delete(test_database) + db.session.commit() + def test_update_database_with_no_configuration_method(self): """ Database API: Test update @@ -639,6 +648,9 @@ def test_update_database_with_no_configuration_method(self): rv = self.client.put(uri, json=database_data) assert rv.status_code == 200 + db.session.delete(test_database) + db.session.commit() + def test_delete_database(self): """ Database API: Test delete From 3d05a710c626f79503aee37ca18bc584a432426c Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Fri, 14 May 2021 17:44:28 -0400 Subject: [PATCH 14/14] Update superset/databases/schemas.py Co-authored-by: Beto Dealmeida --- superset/databases/schemas.py | 6 +++--- .../453530256cea_add_save_option_column_to_db_model.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py index 3ba65fbaba5c5..ffe5bbdc4b3bf 100644 --- a/superset/databases/schemas.py +++ b/superset/databases/schemas.py @@ -72,9 +72,9 @@ "tables, this can be expensive and put strain on the system." ) # pylint: disable=invalid-name configuration_method_description = ( - "Configuration_method is used on the frontend to" - " inform the backend whether to explode parameters" - " or to provide only a sqlalchemy_uri." + "Configuration_method is used on the frontend to " + "inform the backend whether to explode parameters " + "or to provide only a sqlalchemy_uri." ) impersonate_user_description = ( "If Presto, all the queries in SQL Lab are going to be executed as the " diff --git a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py index 0e052dc7c8e45..e3e3c20a3a5e2 100644 --- a/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py +++ b/superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py @@ -43,4 +43,4 @@ def upgrade(): def downgrade(): with op.batch_alter_table("dbs") as batch_op: - batch_op.drop_column("save_option") + batch_op.drop_column("configuration_method")