From 232d6dc71576923339b0be234659523d9ccfbd4f Mon Sep 17 00:00:00 2001 From: AAfghahi Date: Wed, 29 Jun 2022 17:28:12 -0400 Subject: [PATCH 1/2] pylint --- superset/common/query_context_factory.py | 2 - superset/common/query_context_processor.py | 1 + superset/models/helpers.py | 102 ++++++++---------- superset/models/sql_lab.py | 4 +- superset/views/core.py | 10 +- .../charts/data/api_tests.py | 1 - 6 files changed, 55 insertions(+), 65 deletions(-) diff --git a/superset/common/query_context_factory.py b/superset/common/query_context_factory.py index eefc5e68301d1..dc43d28de9d58 100644 --- a/superset/common/query_context_factory.py +++ b/superset/common/query_context_factory.py @@ -82,8 +82,6 @@ def create( # pylint: disable=no-self-use def _convert_to_model(self, datasource: DatasourceDict) -> BaseDatasource: - from superset.datasource.dao import DatasourceDAO - from superset.utils.core import DatasourceType return DatasourceDAO.get_datasource( session=db.session, diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index a1b970cb0526f..770ba357e252b 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -185,6 +185,7 @@ def get_query_result(self, query_object: QueryObject) -> QueryResult: # support multiple queries from different data sources. # The datasource here can be different backend but the interface is common + # pylint: disable=import-outside-toplevel from superset.models.sql_lab import Query query = "" diff --git a/superset/models/helpers.py b/superset/models/helpers.py index ef48e2eaef597..be71b567c0f86 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. """a collection of model-related helper classes and functions""" +# pylint: disable=too-many-lines import json import logging import re @@ -23,10 +24,8 @@ from json.decoder import JSONDecodeError from typing import ( Any, - Callable, cast, Dict, - Hashable, List, Mapping, NamedTuple, @@ -45,6 +44,7 @@ import pandas as pd import pytz import sqlalchemy as sa +import sqlparse import yaml from flask import escape, g, Markup from flask_appbuilder import Model @@ -53,16 +53,16 @@ from flask_appbuilder.security.sqla.models import User from flask_babel import lazy_gettext as _ from jinja2.exceptions import TemplateError -from sqlalchemy import and_, or_, UniqueConstraint +from sqlalchemy import and_, Column, or_, UniqueConstraint from sqlalchemy.ext.declarative import declared_attr from sqlalchemy.orm import Mapper, Session from sqlalchemy.orm.exc import MultipleResultsFound -from sqlalchemy.sql.elements import ColumnClause, TextClause +from sqlalchemy.sql.elements import ColumnElement, literal_column, TextClause from sqlalchemy.sql.expression import Label, Select, TextAsFrom from sqlalchemy.sql.selectable import Alias, TableClause from sqlalchemy_utils import UUIDType -from superset import app, db, is_feature_enabled, security_manager +from superset import app, is_feature_enabled, security_manager from superset.advanced_data_type.types import AdvancedDataTypeResponse from superset.common.db_query_status import QueryStatus from superset.constants import EMPTY_STRING, NULL_STRING @@ -70,23 +70,20 @@ from superset.exceptions import ( AdvancedDataTypeResponseError, QueryClauseValidationException, + QueryObjectValidationError, SupersetSecurityException, ) from superset.extensions import feature_flag_manager -from superset.jinja_context import ( - BaseTemplateProcessor, - ExtraCache, - get_template_processor, -) -from superset.sql_parse import ( - extract_table_references, - has_table_query, - insert_rls, - ParsedQuery, - sanitize_clause, - Table as TableName, +from superset.jinja_context import BaseTemplateProcessor +from superset.sql_parse import has_table_query, insert_rls, ParsedQuery, sanitize_clause +from superset.superset_typing import ( + AdhocMetric, + FilterValue, + FilterValues, + Metric, + OrderBy, + QueryObjectDict, ) -from superset.superset_typing import AdhocColumn from superset.utils import core as utils from superset.utils.core import get_user_id @@ -95,6 +92,7 @@ from superset.db_engine_specs import BaseEngineSpec from superset.models.core import Database + config = app.config logger = logging.getLogger(__name__) @@ -118,9 +116,6 @@ def validate_adhoc_subquery( :raise SupersetSecurityException if sql contains sub-queries or nested sub-queries with table """ - # pylint: disable=import-outside-toplevel - from superset import is_feature_enabled - statements = [] for statement in sqlparse.parse(sql): if has_table_query(statement): @@ -639,24 +634,6 @@ def clone_model( return target.__class__(**data) -from typing import Any, Dict, List, NamedTuple - -import sqlparse -from sqlalchemy import Column -from sqlalchemy.sql.elements import ColumnElement, Label, literal_column - -from superset.exceptions import QueryObjectValidationError -from superset.superset_typing import ( - AdhocMetric, - FilterValue, - FilterValues, - Metric, - OrderBy, - QueryObjectDict, -) -from superset.utils import core as utils - - # todo(hugh): centralize where this code lives class QueryStringExtended(NamedTuple): applied_template_filters: Optional[List[str]] @@ -674,7 +651,7 @@ class SqlaQuery(NamedTuple): sqla_query: Select -class ExploreMixin: +class ExploreMixin: # pylint: disable=too-many-public-methods """ Allows any flask_appbuilder.Model (Query, Table, etc.) to be used to power a chart inside /explore @@ -761,7 +738,8 @@ def get_fetch_values_predicate(self) -> List[Any]: def get_extra_cache_keys(query_obj: Dict[str, Any]) -> List[str]: raise NotImplementedError() - def _process_sql_expression( # type: ignore + def _process_sql_expression( # type: ignore # pylint: disable=no-self-use + self, expression: Optional[str], database_id: int, schema: str, @@ -825,8 +803,8 @@ def _apply_cte(sql: str, cte: Optional[str]) -> str: sql = f"{cte}\n{sql}" return sql + @staticmethod def validate_adhoc_subquery( - self, sql: str, database_id: int, default_schema: str, @@ -841,8 +819,6 @@ def validate_adhoc_subquery( :raise SupersetSecurityException if sql contains sub-queries or nested sub-queries with table """ - # pylint: disable=import-outside-toplevel - from superset import is_feature_enabled statements = [] for statement in sqlparse.parse(sql): @@ -971,7 +947,9 @@ def assign_column_label(df: pd.DataFrame) -> Optional[pd.DataFrame]: return df try: - df = self.database.get_df(sql, self.schema, mutator=assign_column_label) # type: ignore + df = self.database.get_df( + sql, self.schema, mutator=assign_column_label # type: ignore + ) except Exception as ex: # pylint: disable=broad-except df = pd.DataFrame() status = QueryStatus.FAILED @@ -1053,7 +1031,7 @@ def get_from_clause( def adhoc_metric_to_sqla( self, metric: AdhocMetric, - columns_by_name: Dict[str, Dict[str, Any]], + columns_by_name: Dict[str, "TableColumn"], template_processor: Optional[BaseTemplateProcessor] = None, ) -> ColumnElement: """ @@ -1071,11 +1049,13 @@ def adhoc_metric_to_sqla( if expression_type == utils.AdhocMetricExpressionType.SIMPLE: metric_column = metric.get("column") or {} column_name = cast(str, metric_column.get("column_name")) - table_column: Optional[Dict[str, Any]] = columns_by_name.get(column_name) + table_column: Optional["TableColumn"] = columns_by_name.get(column_name) sqla_column = sa.column(column_name) + if table_column: + sqla_column = table_column.get_sqla_col() sqla_metric = self.sqla_aggregations[metric["aggregate"]](sqla_column) elif expression_type == utils.AdhocMetricExpressionType.SQL: - expression = _process_sql_expression( # type: ignore + expression = self._process_sql_expression( # type: ignore expression=metric["sqlExpression"], database_id=self.database_id, schema=self.schema, @@ -1151,7 +1131,9 @@ def _get_series_orderby( ) -> Column: if utils.is_adhoc_metric(series_limit_metric): assert isinstance(series_limit_metric, dict) - ob = self.adhoc_metric_to_sqla(series_limit_metric, columns_by_name) # type: ignore + ob = self.adhoc_metric_to_sqla( + series_limit_metric, columns_by_name # type: ignore + ) elif ( isinstance(series_limit_metric, str) and series_limit_metric in metrics_by_name @@ -1165,7 +1147,7 @@ def _get_series_orderby( def adhoc_column_to_sqla( self, - col: Type["AdhocColumn"], + col: Type["AdhocColumn"], # type: ignore template_processor: Optional[BaseTemplateProcessor] = None, ) -> ColumnElement: """ @@ -1177,7 +1159,7 @@ def adhoc_column_to_sqla( :rtype: sqlalchemy.sql.column """ label = utils.get_column_name(col) # type: ignore - expression = _process_sql_expression( # type: ignore + expression = self._process_sql_expression( # type: ignore expression=col["sqlExpression"], # type: ignore database_id=self.database_id, schema=self.schema, @@ -1282,12 +1264,14 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma if granularity not in self.dttm_cols and granularity is not None: granularity = self.main_dttm_col - columns_by_name: Dict[str, TableColumn] = { + columns_by_name: Dict[str, "TableColumn"] = { col.get("column_name"): col for col in self.columns # col.column_name: col for col in self.columns } - metrics_by_name: Dict[str, SqlMetric] = {m.metric_name: m for m in self.metrics} + metrics_by_name: Dict[str, "SqlMetric"] = { + m.metric_name: m for m in self.metrics + } if not granularity and is_timeseries: raise QueryObjectValidationError( @@ -1336,7 +1320,7 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma if isinstance(col, dict): col = cast(AdhocMetric, col) if col.get("sqlExpression"): - col["sqlExpression"] = _process_sql_expression( # type: ignore + col["sqlExpression"] = self._process_sql_expression( # type: ignore expression=col["sqlExpression"], database_id=self.database_id, schema=self.schema, @@ -1492,7 +1476,7 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma flt_col = flt["col"] val = flt.get("val") op = flt["op"].upper() - col_obj: Optional[TableColumn] = None + col_obj: Optional["TableColumn"] = None sqla_col: Optional[Column] = None if flt_col == utils.DTTM_ALIAS and is_timeseries and dttm_col: col_obj = dttm_col @@ -1638,7 +1622,9 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma where = extras.get("where") if where: try: - where = template_processor.process_template(f"({where})") # type: ignore + where = template_processor.process_template( # type: ignore + f"({where})" + ) except TemplateError as ex: raise QueryObjectValidationError( _( @@ -1650,7 +1636,9 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma having = extras.get("having") if having: try: - having = template_processor.process_template(f"({having})") # type: ignore + having = template_processor.process_template( # type: ignore + f"({having})" + ) except TemplateError as ex: raise QueryObjectValidationError( _( diff --git a/superset/models/sql_lab.py b/superset/models/sql_lab.py index c0e2215b947cd..d6a636e926d3c 100644 --- a/superset/models/sql_lab.py +++ b/superset/models/sql_lab.py @@ -56,7 +56,7 @@ from superset.db_engine_specs import BaseEngineSpec -class Query(Model, ExtraJSONMixin, ExploreMixin): +class Query(Model, ExtraJSONMixin, ExploreMixin): # pylint: disable=abstract-method """ORM model for SQL query Now that SQL Lab support multi-statement execution, an entry in this @@ -175,6 +175,7 @@ def sql_tables(self) -> List[Table]: @property def columns(self) -> List[ResultSetColumnType]: # todo(hughhh): move this logic into a base class + # pylint: disable=import-outside-toplevel from superset.utils.core import GenericDataType bool_types = ("BOOL",) @@ -193,6 +194,7 @@ def columns(self) -> List[ResultSetColumnType]: date_types = ("DATE", "TIME") str_types = ("VARCHAR", "STRING", "CHAR") columns = [] + col_type = "" for col in self.extra.get("columns", []): computed_column = {**col} col_type = col.get("type") diff --git a/superset/views/core.py b/superset/views/core.py index 15f2bef9a49f3..54b36421bbe26 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -649,7 +649,9 @@ def explore_json( errors=[ SupersetError( message=__( - "This chart type is not supported when using an unsaved query as a chart source. Create a dataset to visualize your data." + "This chart type is not supported when using an unsaved" + " query as a chart source. Create a dataset to visualize" + " your data." ), error_type=SupersetErrorType.VIZ_TYPE_REQUIRES_DATASET_ERROR, level=ErrorLevel.ERROR, @@ -755,7 +757,7 @@ def import_dashboards(self) -> FlaskResponse: @expose("/explore///", methods=["GET", "POST"]) @expose("/explore/", methods=["GET", "POST"]) @expose("/explore/p//", methods=["GET"]) - # pylint: disable=too-many-locals,too-many-branches,too-many-statements + # pylint: disable=too-many-locals,too-many-branches,too-many-statements, too-many-return-statements def explore( self, datasource_type: Optional[str] = None, @@ -791,9 +793,8 @@ def explore( value = GetFormDataCommand(parameters).run() initial_form_data = json.loads(value) if value else {} - from superset.datasource.dao import DatasourceDAO + # pylint: disable=import-outside-toplevel from superset.models.helpers import ExploreMixin - from superset.utils.core import DatasourceType # Handle SIP-68 Models or explore view # API will always use /explore/// to query @@ -896,6 +897,7 @@ def explore( "form_data": form_data, "datasource_id": datasource_id, "datasource_type": datasource_type, + "datasource_name": datasource_name, "slice": slc.data if slc else None, "standalone": standalone_mode, "force": force, diff --git a/tests/integration_tests/charts/data/api_tests.py b/tests/integration_tests/charts/data/api_tests.py index 73425fb58f68c..0baa1151cdd0e 100644 --- a/tests/integration_tests/charts/data/api_tests.py +++ b/tests/integration_tests/charts/data/api_tests.py @@ -332,7 +332,6 @@ def test_chart_data_applied_time_extras(self): {"column": "gender"}, {"column": "num"}, {"column": "name"}, - {"column": "__time_range"}, ], ) self.assertEqual( From aa2756ef115a2e6ccc9629d551d8c5830f0e135f Mon Sep 17 00:00:00 2001 From: AAfghahi Date: Tue, 5 Jul 2022 12:29:16 -0400 Subject: [PATCH 2/2] core_test fix --- superset/utils/core.py | 14 ++++++++++---- tests/integration_tests/charts/data/api_tests.py | 1 + 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/superset/utils/core.py b/superset/utils/core.py index 9617d6cd29d13..7610fcd2c9e0f 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -1729,14 +1729,20 @@ def is_test() -> bool: return strtobool(os.environ.get("SUPERSET_TESTENV", "false")) -def get_time_filter_status( +def get_time_filter_status( # pylint: disable=too-many-branches datasource: "BaseDatasource", applied_time_extras: Dict[str, str], ) -> Tuple[List[Dict[str, str]], List[Dict[str, str]]]: - # todo(hugh): fix this - # temporal_columns = {col.column_name for col in datasource.columns if col.is_dttm} - temporal_columns: Dict[str, Any] = {} + temporal_columns: Set[Any] + if datasource.type == "query": + temporal_columns = { + col.get("column_name") for col in datasource.columns if col.get("is_dttm") + } + else: + temporal_columns = { + col.column_name for col in datasource.columns if col.is_dttm + } applied: List[Dict[str, str]] = [] rejected: List[Dict[str, str]] = [] time_column = applied_time_extras.get(ExtraFiltersTimeColumnType.TIME_COL) diff --git a/tests/integration_tests/charts/data/api_tests.py b/tests/integration_tests/charts/data/api_tests.py index 0baa1151cdd0e..73425fb58f68c 100644 --- a/tests/integration_tests/charts/data/api_tests.py +++ b/tests/integration_tests/charts/data/api_tests.py @@ -332,6 +332,7 @@ def test_chart_data_applied_time_extras(self): {"column": "gender"}, {"column": "num"}, {"column": "name"}, + {"column": "__time_range"}, ], ) self.assertEqual(