From f341025d80aacf7345e7c20f8463231b9197ea58 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Date: Sat, 19 Mar 2022 00:08:06 +0200 Subject: [PATCH 1/9] feat: add support for comments in adhoc clauses (#19248) * feat: add support for comments in adhoc clauses * sanitize remaining freeform clauses * sanitize adhoc having in frontend * address review comment --- .../src/query/processFilters.ts | 12 ++++- .../test/query/processFilters.test.ts | 10 ++-- superset/common/query_object.py | 10 ++-- superset/connectors/sqla/models.py | 23 +++++--- superset/sql_parse.py | 21 ++++++-- superset/utils/core.py | 7 ++- superset/viz.py | 10 ++-- .../charts/data/api_tests.py | 22 ++++++++ tests/unit_tests/sql_parse_tests.py | 54 +++++++++---------- 9 files changed, 109 insertions(+), 60 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/query/processFilters.ts b/superset-frontend/packages/superset-ui-core/src/query/processFilters.ts index 8ead77c0fc34b..239f1c49afbe5 100644 --- a/superset-frontend/packages/superset-ui-core/src/query/processFilters.ts +++ b/superset-frontend/packages/superset-ui-core/src/query/processFilters.ts @@ -23,6 +23,14 @@ import { QueryObjectFilterClause } from './types/Query'; import { isSimpleAdhocFilter } from './types/Filter'; import convertFilter from './convertFilter'; +function sanitizeClause(clause: string): string { + let sanitizedClause = clause; + if (clause.includes('--')) { + sanitizedClause = `${clause}\n`; + } + return `(${sanitizedClause})`; +} + /** Logic formerly in viz.py's process_query_filters */ export default function processFilters( formData: Partial, @@ -60,9 +68,9 @@ export default function processFilters( }); // some filter-related fields need to go in `extras` - extras.having = freeformHaving.map(exp => `(${exp})`).join(' AND '); + extras.having = freeformHaving.map(sanitizeClause).join(' AND '); extras.having_druid = simpleHaving; - extras.where = freeformWhere.map(exp => `(${exp})`).join(' AND '); + extras.where = freeformWhere.map(sanitizeClause).join(' AND '); return { filters: simpleWhere, diff --git a/superset-frontend/packages/superset-ui-core/test/query/processFilters.test.ts b/superset-frontend/packages/superset-ui-core/test/query/processFilters.test.ts index 267b416493e35..151c0363f16f0 100644 --- a/superset-frontend/packages/superset-ui-core/test/query/processFilters.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/query/processFilters.test.ts @@ -132,12 +132,12 @@ describe('processFilters', () => { { expressionType: 'SQL', clause: 'WHERE', - sqlExpression: 'tea = "jasmine"', + sqlExpression: "tea = 'jasmine'", }, { expressionType: 'SQL', clause: 'WHERE', - sqlExpression: 'cup = "large"', + sqlExpression: "cup = 'large' -- comment", }, { expressionType: 'SQL', @@ -147,13 +147,13 @@ describe('processFilters', () => { { expressionType: 'SQL', clause: 'HAVING', - sqlExpression: 'waitTime <= 180', + sqlExpression: 'waitTime <= 180 -- comment', }, ], }), ).toEqual({ extras: { - having: '(ice = 25 OR ice = 50) AND (waitTime <= 180)', + having: '(ice = 25 OR ice = 50) AND (waitTime <= 180 -- comment\n)', having_druid: [ { col: 'sweetness', @@ -166,7 +166,7 @@ describe('processFilters', () => { val: '50', }, ], - where: '(tea = "jasmine") AND (cup = "large")', + where: "(tea = 'jasmine') AND (cup = 'large' -- comment\n)", }, filters: [ { diff --git a/superset/common/query_object.py b/superset/common/query_object.py index fd988a36fac05..139dc27c580ef 100644 --- a/superset/common/query_object.py +++ b/superset/common/query_object.py @@ -30,7 +30,7 @@ QueryClauseValidationException, QueryObjectValidationError, ) -from superset.sql_parse import validate_filter_clause +from superset.sql_parse import sanitize_clause from superset.superset_typing import Column, Metric, OrderBy from superset.utils import pandas_postprocessing from superset.utils.core import ( @@ -272,7 +272,7 @@ def validate( try: self._validate_there_are_no_missing_series() self._validate_no_have_duplicate_labels() - self._validate_filters() + self._sanitize_filters() return None except QueryObjectValidationError as ex: if raise_exceptions: @@ -291,12 +291,14 @@ def _validate_no_have_duplicate_labels(self) -> None: ) ) - def _validate_filters(self) -> None: + def _sanitize_filters(self) -> None: for param in ("where", "having"): clause = self.extras.get(param) if clause: try: - validate_filter_clause(clause) + sanitized_clause = sanitize_clause(clause) + if sanitized_clause != clause: + self.extras[param] = sanitized_clause except QueryClauseValidationException as ex: raise QueryObjectValidationError(ex.message) from ex diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 23d3d326cd9a5..9261f97ae1b11 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -82,7 +82,10 @@ ) from superset.datasets.models import Dataset as NewDataset from superset.db_engine_specs.base import BaseEngineSpec, CTE_ALIAS, TimestampExpression -from superset.exceptions import QueryObjectValidationError +from superset.exceptions import ( + QueryClauseValidationException, + QueryObjectValidationError, +) from superset.jinja_context import ( BaseTemplateProcessor, ExtraCache, @@ -96,7 +99,7 @@ clone_model, QueryResult, ) -from superset.sql_parse import ParsedQuery +from superset.sql_parse import ParsedQuery, sanitize_clause from superset.superset_typing import ( AdhocColumn, AdhocMetric, @@ -887,6 +890,10 @@ def adhoc_metric_to_sqla( tp = self.get_template_processor() expression = tp.process_template(cast(str, metric["sqlExpression"])) validate_adhoc_subquery(expression) + try: + expression = sanitize_clause(expression) + except QueryClauseValidationException as ex: + raise QueryObjectValidationError(ex.message) from ex sqla_metric = literal_column(expression) else: raise QueryObjectValidationError("Adhoc metric expressionType is invalid") @@ -912,6 +919,10 @@ def adhoc_column_to_sqla( expression = template_processor.process_template(expression) if expression: validate_adhoc_subquery(expression) + try: + expression = sanitize_clause(expression) + except QueryClauseValidationException as ex: + raise QueryObjectValidationError(ex.message) from ex sqla_metric = literal_column(expression) return self.make_sqla_column_compatible(sqla_metric, label) @@ -1353,7 +1364,7 @@ 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(where) + where = template_processor.process_template(f"({where})") except TemplateError as ex: raise QueryObjectValidationError( _( @@ -1361,11 +1372,11 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma msg=ex.message, ) ) from ex - where_clause_and += [self.text(f"({where})")] + where_clause_and += [self.text(where)] having = extras.get("having") if having: try: - having = template_processor.process_template(having) + having = template_processor.process_template(f"({having})") except TemplateError as ex: raise QueryObjectValidationError( _( @@ -1373,7 +1384,7 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma msg=ex.message, ) ) from ex - having_clause_and += [self.text(f"({having})")] + having_clause_and += [self.text(having)] if apply_fetch_values_predicate and self.fetch_values_predicate: qry = qry.where(self.get_fetch_values_predicate()) if granularity: diff --git a/superset/sql_parse.py b/superset/sql_parse.py index f5523bab71e8d..95361b39a6a27 100644 --- a/superset/sql_parse.py +++ b/superset/sql_parse.py @@ -32,6 +32,7 @@ Where, ) from sqlparse.tokens import ( + Comment, CTE, DDL, DML, @@ -441,25 +442,35 @@ def set_or_update_query_limit(self, new_limit: int, force: bool = False) -> str: return str_res -def validate_filter_clause(clause: str) -> None: - if sqlparse.format(clause, strip_comments=True) != sqlparse.format(clause): - raise QueryClauseValidationException("Filter clause contains comment") - +def sanitize_clause(clause: str) -> str: + # clause = sqlparse.format(clause, strip_comments=True) statements = sqlparse.parse(clause) if len(statements) != 1: - raise QueryClauseValidationException("Filter clause contains multiple queries") + raise QueryClauseValidationException("Clause contains multiple statements") open_parens = 0 + previous_token = None for token in statements[0]: + if token.value == "/" and previous_token and previous_token.value == "*": + raise QueryClauseValidationException("Closing unopened multiline comment") + if token.value == "*" and previous_token and previous_token.value == "/": + raise QueryClauseValidationException("Unclosed multiline comment") if token.value in (")", "("): open_parens += 1 if token.value == "(" else -1 if open_parens < 0: raise QueryClauseValidationException( "Closing unclosed parenthesis in filter clause" ) + previous_token = token if open_parens > 0: raise QueryClauseValidationException("Unclosed parenthesis in filter clause") + if previous_token and previous_token.ttype in Comment: + if previous_token.value[-1] != "\n": + clause = f"{clause}\n" + + return clause + class InsertRLSState(str, Enum): """ diff --git a/superset/utils/core.py b/superset/utils/core.py index 36d59333d2ed3..d2527a32c72cf 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -98,6 +98,7 @@ SupersetException, SupersetTimeoutException, ) +from superset.sql_parse import sanitize_clause from superset.superset_typing import ( AdhocColumn, AdhocMetric, @@ -1366,10 +1367,12 @@ def split_adhoc_filters_into_base_filters( # pylint: disable=invalid-name } ) elif expression_type == "SQL": + sql_expression = adhoc_filter.get("sqlExpression") + sql_expression = sanitize_clause(sql_expression) if clause == "WHERE": - sql_where_filters.append(adhoc_filter.get("sqlExpression")) + sql_where_filters.append(sql_expression) elif clause == "HAVING": - sql_having_filters.append(adhoc_filter.get("sqlExpression")) + sql_having_filters.append(sql_expression) form_data["where"] = " AND ".join( ["({})".format(sql) for sql in sql_where_filters] ) diff --git a/superset/viz.py b/superset/viz.py index 4998da12dcd57..7544af5078059 100644 --- a/superset/viz.py +++ b/superset/viz.py @@ -62,14 +62,13 @@ from superset.exceptions import ( CacheLoadError, NullValueException, - QueryClauseValidationException, QueryObjectValidationError, SpatialException, SupersetSecurityException, ) from superset.extensions import cache_manager, security_manager from superset.models.helpers import QueryResult -from superset.sql_parse import validate_filter_clause +from superset.sql_parse import sanitize_clause from superset.superset_typing import ( Column, Metric, @@ -391,10 +390,9 @@ def query_obj(self) -> QueryObjectDict: # pylint: disable=too-many-locals for param in ("where", "having"): clause = self.form_data.get(param) if clause: - try: - validate_filter_clause(clause) - except QueryClauseValidationException as ex: - raise QueryObjectValidationError(ex.message) from ex + sanitized_clause = sanitize_clause(clause) + if sanitized_clause != clause: + self.form_data[param] = sanitized_clause # extras are used to query elements specific to a datasource type # for instance the extra where clause that applies only to Tables diff --git a/tests/integration_tests/charts/data/api_tests.py b/tests/integration_tests/charts/data/api_tests.py index bc8ec74feb0d6..c45c8d5064eaa 100644 --- a/tests/integration_tests/charts/data/api_tests.py +++ b/tests/integration_tests/charts/data/api_tests.py @@ -465,6 +465,28 @@ def test_with_invalid_where_parameter_closing_unclosed__400(self): assert rv.status_code == 400 + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_with_where_parameter_including_comment___200(self): + self.query_context_payload["queries"][0]["filters"] = [] + self.query_context_payload["queries"][0]["extras"]["where"] = "1 = 1 -- abc" + + rv = self.post_assert_metric(CHART_DATA_URI, self.query_context_payload, "data") + + assert rv.status_code == 200 + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_with_orderby_parameter_with_second_query__400(self): + self.query_context_payload["queries"][0]["filters"] = [] + self.query_context_payload["queries"][0]["orderby"] = [ + [ + {"expressionType": "SQL", "sqlExpression": "sum__num; select 1, 1",}, + True, + ], + ] + rv = self.post_assert_metric(CHART_DATA_URI, self.query_context_payload, "data") + + assert rv.status_code == 400 + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_with_invalid_having_parameter_closing_and_comment__400(self): self.query_context_payload["queries"][0]["filters"] = [] diff --git a/tests/unit_tests/sql_parse_tests.py b/tests/unit_tests/sql_parse_tests.py index 886eb368e4aa4..75f099e52b6e1 100644 --- a/tests/unit_tests/sql_parse_tests.py +++ b/tests/unit_tests/sql_parse_tests.py @@ -30,9 +30,9 @@ insert_rls, matches_table_name, ParsedQuery, + sanitize_clause, strip_comments_from_sql, Table, - validate_filter_clause, ) @@ -1142,52 +1142,46 @@ def test_strip_comments_from_sql() -> None: ) -def test_validate_filter_clause_valid(): +def test_sanitize_clause_valid(): # regular clauses - assert validate_filter_clause("col = 1") is None - assert validate_filter_clause("1=\t\n1") is None - assert validate_filter_clause("(col = 1)") is None - assert validate_filter_clause("(col1 = 1) AND (col2 = 2)") is None + assert sanitize_clause("col = 1") == "col = 1" + assert sanitize_clause("1=\t\n1") == "1=\t\n1" + assert sanitize_clause("(col = 1)") == "(col = 1)" + assert sanitize_clause("(col1 = 1) AND (col2 = 2)") == "(col1 = 1) AND (col2 = 2)" + assert sanitize_clause("col = 'abc' -- comment") == "col = 'abc' -- comment\n" - # Valid literal values that appear to be invalid - assert validate_filter_clause("col = 'col1 = 1) AND (col2 = 2'") is None - assert validate_filter_clause("col = 'select 1; select 2'") is None - assert validate_filter_clause("col = 'abc -- comment'") is None - - -def test_validate_filter_clause_closing_unclosed(): - with pytest.raises(QueryClauseValidationException): - validate_filter_clause("col1 = 1) AND (col2 = 2)") - - -def test_validate_filter_clause_unclosed(): - with pytest.raises(QueryClauseValidationException): - validate_filter_clause("(col1 = 1) AND (col2 = 2") + # Valid literal values that at could be flagged as invalid by a naive query parser + assert ( + sanitize_clause("col = 'col1 = 1) AND (col2 = 2'") + == "col = 'col1 = 1) AND (col2 = 2'" + ) + assert sanitize_clause("col = 'select 1; select 2'") == "col = 'select 1; select 2'" + assert sanitize_clause("col = 'abc -- comment'") == "col = 'abc -- comment'" -def test_validate_filter_clause_closing_and_unclosed(): +def test_sanitize_clause_closing_unclosed(): with pytest.raises(QueryClauseValidationException): - validate_filter_clause("col1 = 1) AND (col2 = 2") + sanitize_clause("col1 = 1) AND (col2 = 2)") -def test_validate_filter_clause_closing_and_unclosed_nested(): +def test_sanitize_clause_unclosed(): with pytest.raises(QueryClauseValidationException): - validate_filter_clause("(col1 = 1)) AND ((col2 = 2)") + sanitize_clause("(col1 = 1) AND (col2 = 2") -def test_validate_filter_clause_multiple(): +def test_sanitize_clause_closing_and_unclosed(): with pytest.raises(QueryClauseValidationException): - validate_filter_clause("TRUE; SELECT 1") + sanitize_clause("col1 = 1) AND (col2 = 2") -def test_validate_filter_clause_comment(): +def test_sanitize_clause_closing_and_unclosed_nested(): with pytest.raises(QueryClauseValidationException): - validate_filter_clause("1 = 1 -- comment") + sanitize_clause("(col1 = 1)) AND ((col2 = 2)") -def test_validate_filter_clause_subquery_comment(): +def test_sanitize_clause_multiple(): with pytest.raises(QueryClauseValidationException): - validate_filter_clause("(1 = 1 -- comment\n)") + sanitize_clause("TRUE; SELECT 1") def test_sqlparse_issue_652(): From d645579cdd64b7fe7f9dde4a8da000dd7db51ce9 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 18 Mar 2022 16:01:27 -0700 Subject: [PATCH 2/9] chore!: update mutator to take kwargs (#19083) * update mutator to take kwargs * update updating.md * lint * test that the database name is properly passed in to the mutator --- UPDATING.md | 4 +++- superset/config.py | 11 +++++----- superset/connectors/sqla/models.py | 7 ++++++- superset/db_engine_specs/base.py | 7 ++++++- superset/sql_lab.py | 4 +++- superset/sql_validators/presto_db.py | 7 ++++++- tests/integration_tests/model_tests.py | 29 +++++++++++++++++++++++++- 7 files changed, 57 insertions(+), 12 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 4272ecc56bb32..c28975387efe3 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -37,7 +37,9 @@ assists people when migrating to a new version. - [17984](https://github.com/apache/superset/pull/17984): Default Flask SECRET_KEY has changed for security reasons. You should always override with your own secret. Set `PREVIOUS_SECRET_KEY` (ex: PREVIOUS_SECRET_KEY = "\2\1thisismyscretkey\1\2\\e\\y\\y\\h") with your previous key and use `superset re-encrypt-secrets` to rotate you current secrets - [15254](https://github.com/apache/superset/pull/15254): Previously `QUERY_COST_FORMATTERS_BY_ENGINE`, `SQL_VALIDATORS_BY_ENGINE` and `SCHEDULED_QUERIES` were expected to be defined in the feature flag dictionary in the `config.py` file. These should now be defined as a top-level config, with the feature flag dictionary being reserved for boolean only values. - [17539](https://github.com/apache/superset/pull/17539): all Superset CLI commands (init, load_examples and etc) require setting the FLASK_APP environment variable (which is set by default when `.flaskenv` is loaded) -- [18970](https://github.com/apache/superset/pull/18970): Changes feature flag for the legacy datasource editor (DISABLE_LEGACY_DATASOURCE_EDITOR) in config.py to True, thus disabling the feature from being shown in the client. +- [18970](https://github.com/apache/superset/pull/18970): Changes feature +flag for the legacy datasource editor (DISABLE_LEGACY_DATASOURCE_EDITOR) in config.py to True, thus disabling the feature from being shown in the client. +- [19083](https://github.com/apache/superset/pull/19083): Updates the mutator function in the config file to take a sql argument and a list of kwargs. Any `SQL_QUERY_MUTATOR` config function overrides will need to be updated to match the new set of params. It is advised regardless of the dictionary args that you list in your function arguments, to keep **kwargs as the last argument to allow for any new kwargs to be passed in. - [19017](https://github.com/apache/superset/pull/19017): Removes Python 3.7 support. - [19142](https://github.com/apache/superset/pull/19142): Changes feature flag for versioned export(VERSIONED_EXPORT) to be true. - [19107](https://github.com/apache/superset/pull/19107): Feature flag `SQLLAB_BACKEND_PERSISTENCE` is now on by default, which enables persisting SQL Lab tabs in the backend instead of the browser's `localStorage`. diff --git a/superset/config.py b/superset/config.py index dc93ad7f5de17..86e35be718702 100644 --- a/superset/config.py +++ b/superset/config.py @@ -40,7 +40,6 @@ from flask_appbuilder.security.manager import AUTH_DB from pandas._libs.parsers import STR_NA_VALUES # pylint: disable=no-name-in-module from typing_extensions import Literal -from werkzeug.local import LocalProxy from superset.constants import CHANGE_ME_SECRET_KEY from superset.jinja_context import BaseTemplateProcessor @@ -1048,14 +1047,14 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC( # pylint: disable=invalid-name # The use case is can be around adding some sort of comment header # with information such as the username and worker node information # -# def SQL_QUERY_MUTATOR(sql, user_name, security_manager, database): +# def SQL_QUERY_MUTATOR(sql, user_name=user_name, security_manager=security_manager, database=database): # dttm = datetime.now().isoformat() # return f"-- [SQL LAB] {username} {dttm}\n{sql}" +# For backward compatibility, you can unpack any of the above arguments in your +# function definition, but keep the **kwargs as the last argument to allow new args +# to be added later without any errors. def SQL_QUERY_MUTATOR( # pylint: disable=invalid-name,unused-argument - sql: str, - user_name: Optional[str], - security_manager: LocalProxy, - database: "Database", + sql: str, **kwargs: Any ) -> str: return sql diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 9261f97ae1b11..62ae8c9ebaf95 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -776,7 +776,12 @@ def mutate_query_from_config(self, sql: str) -> str: sql_query_mutator = config["SQL_QUERY_MUTATOR"] if sql_query_mutator: username = utils.get_username() - sql = sql_query_mutator(sql, username, security_manager, self.database) + sql = sql_query_mutator( + sql, + user_name=username, + security_manager=security_manager, + database=self.database, + ) return sql def get_template_processor(self, **kwargs: Any) -> BaseTemplateProcessor: diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index d7e457baa8c01..e867f200d91a8 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -1138,7 +1138,12 @@ def process_statement( sql = parsed_query.stripped() sql_query_mutator = current_app.config["SQL_QUERY_MUTATOR"] if sql_query_mutator: - sql = sql_query_mutator(sql, user_name, security_manager, database) + sql = sql_query_mutator( + sql, + user_name=user_name, + security_manager=security_manager, + database=database, + ) return sql diff --git a/superset/sql_lab.py b/superset/sql_lab.py index 8fac419cf0ba6..613db963e31c1 100644 --- a/superset/sql_lab.py +++ b/superset/sql_lab.py @@ -225,7 +225,9 @@ def execute_sql_statement( # pylint: disable=too-many-arguments,too-many-locals sql = apply_limit_if_exists(database, increased_limit, query, sql) # Hook to allow environment-specific mutation (usually comments) to the SQL - sql = SQL_QUERY_MUTATOR(sql, user_name, security_manager, database) + sql = SQL_QUERY_MUTATOR( + sql, user_name=user_name, security_manager=security_manager, database=database + ) try: query.executed_sql = sql if log_query: diff --git a/superset/sql_validators/presto_db.py b/superset/sql_validators/presto_db.py index bf77f474a0004..6d0311f120efb 100644 --- a/superset/sql_validators/presto_db.py +++ b/superset/sql_validators/presto_db.py @@ -55,7 +55,12 @@ def validate_statement( # Hook to allow environment-specific mutation (usually comments) to the SQL sql_query_mutator = config["SQL_QUERY_MUTATOR"] if sql_query_mutator: - sql = sql_query_mutator(sql, user_name, security_manager, database) + sql = sql_query_mutator( + sql, + user_name=user_name, + security_manager=security_manager, + database=database, + ) # Transform the final statement to an explain call before sending it on # to presto to validate diff --git a/tests/integration_tests/model_tests.py b/tests/integration_tests/model_tests.py index 5ffa65e583ead..c6388601354d4 100644 --- a/tests/integration_tests/model_tests.py +++ b/tests/integration_tests/model_tests.py @@ -506,7 +506,7 @@ def test_sql_mutator(self): sql = tbl.get_query_str(query_obj) self.assertNotIn("-- COMMENT", sql) - def mutator(*args): + def mutator(*args, **kwargs): return "-- COMMENT\n" + args[0] app.config["SQL_QUERY_MUTATOR"] = mutator @@ -515,6 +515,33 @@ def mutator(*args): app.config["SQL_QUERY_MUTATOR"] = None + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_sql_mutator_different_params(self): + tbl = self.get_table(name="birth_names") + query_obj = dict( + groupby=[], + metrics=None, + filter=[], + is_timeseries=False, + columns=["name"], + granularity=None, + from_dttm=None, + to_dttm=None, + extras={}, + ) + sql = tbl.get_query_str(query_obj) + self.assertNotIn("-- COMMENT", sql) + + def mutator(sql, database=None, **kwargs): + return "-- COMMENT\n--" + "\n" + str(database) + "\n" + sql + + app.config["SQL_QUERY_MUTATOR"] = mutator + mutated_sql = tbl.get_query_str(query_obj) + self.assertIn("-- COMMENT", mutated_sql) + self.assertIn(tbl.database.name, mutated_sql) + + app.config["SQL_QUERY_MUTATOR"] = None + def test_query_with_non_existent_metrics(self): tbl = self.get_table(name="birth_names") From e1d0b83885d89c50e96c6eef872beffe0de7cf50 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Sun, 20 Mar 2022 21:31:44 -0700 Subject: [PATCH 3/9] chore: update updating with druid no sql deprecation (#19262) * update updating with druid no sql deprecation * Update UPDATING.md Co-authored-by: Beto Dealmeida Co-authored-by: Beto Dealmeida --- UPDATING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/UPDATING.md b/UPDATING.md index c28975387efe3..953d2c34c400c 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -43,6 +43,7 @@ flag for the legacy datasource editor (DISABLE_LEGACY_DATASOURCE_EDITOR) in conf - [19017](https://github.com/apache/superset/pull/19017): Removes Python 3.7 support. - [19142](https://github.com/apache/superset/pull/19142): Changes feature flag for versioned export(VERSIONED_EXPORT) to be true. - [19107](https://github.com/apache/superset/pull/19107): Feature flag `SQLLAB_BACKEND_PERSISTENCE` is now on by default, which enables persisting SQL Lab tabs in the backend instead of the browser's `localStorage`. +- [19262](https://github.com/apache/superset/pull/19262): As per SIPs 11 and 68, the native NoSQL Druid connector is deprecated as of 2.0 and will no longer be supported. Druid is still supported through SQLAlchemy via pydruid. ### Potential Downtime From dc575080d7e43d40b1734bb8f44fdc291cb95b11 Mon Sep 17 00:00:00 2001 From: Stephen Liu <750188453@qq.com> Date: Mon, 21 Mar 2022 15:20:04 +0800 Subject: [PATCH 4/9] feat: improve color consistency (save all labels) (#19038) --- .gitignore | 2 + superset-frontend/package-lock.json | 26 +++- superset-frontend/package.json | 2 + .../src/shared-controls/index.tsx | 3 + .../packages/superset-ui-core/package.json | 1 + .../src/color/CategoricalColorScale.ts | 17 ++- .../src/color/SharedLabelColorSingleton.ts | 130 ++++++++++++++++++ .../superset-ui-core/src/color/index.ts | 4 + .../color/SharedLabelColorSingleton.test.ts | 110 +++++++++++++++ .../legacy-plugin-chart-chord/src/Chord.js | 6 +- .../src/transformProps.js | 3 +- .../src/CountryMap.js | 21 ++- .../src/transformProps.js | 10 +- .../src/Histogram.jsx | 3 +- .../src/transformProps.js | 2 + .../src/Partition.js | 3 +- .../src/transformProps.js | 2 + .../legacy-plugin-chart-rose/src/Rose.js | 9 +- .../src/transformProps.js | 2 + .../src/SankeyLoop.js | 4 +- .../src/transformProps.js | 3 +- .../legacy-plugin-chart-sankey/src/Sankey.js | 4 +- .../src/transformProps.js | 3 +- .../src/Sunburst.js | 9 +- .../src/transformProps.js | 4 +- .../src/Treemap.js | 3 +- .../src/transformProps.js | 3 +- .../src/WorldMap.js | 24 +++- .../src/controlPanel.ts | 4 + .../src/transformProps.js | 12 +- .../src/CategoricalDeckGLContainer.jsx | 4 +- .../legacy-preset-chart-nvd3/src/NVD3Vis.js | 5 +- .../src/transformProps.js | 2 + .../src/BoxPlot/transformProps.ts | 7 +- .../src/Funnel/transformProps.ts | 3 +- .../src/Gauge/transformProps.ts | 5 +- .../src/Graph/transformProps.ts | 3 +- .../plugin-chart-echarts/src/Graph/types.ts | 43 +++--- .../src/MixedTimeseries/transformProps.ts | 16 ++- .../src/Pie/transformProps.ts | 3 +- .../src/Radar/transformProps.ts | 3 +- .../src/Timeseries/transformProps.ts | 15 +- .../src/Timeseries/transformers.ts | 13 +- .../src/Treemap/transformProps.ts | 5 +- .../src/chart/WordCloud.tsx | 15 +- .../src/legacyPlugin/transformProps.ts | 2 + .../src/plugin/transformProps.ts | 3 +- .../src/components/Chart/Chart.jsx | 2 + .../src/components/Chart/ChartRenderer.jsx | 5 + .../src/dashboard/actions/dashboardInfo.ts | 27 +++- .../src/dashboard/actions/dashboardLayout.js | 6 +- .../src/dashboard/actions/dashboardState.js | 49 ++++++- .../dashboard/actions/dashboardState.test.js | 8 +- .../src/dashboard/actions/hydrate.js | 23 +--- .../src/dashboard/components/Header/index.jsx | 17 ++- .../components/PropertiesModal/index.tsx | 28 +++- .../components/gridComponents/Chart.jsx | 6 + .../components/gridComponents/ChartHolder.jsx | 8 ++ .../src/dashboard/containers/Chart.jsx | 3 + .../containers/DashboardComponent.jsx | 2 + .../dashboard/containers/DashboardPage.tsx | 21 ++- .../src/dashboard/reducers/dashboardState.js | 9 ++ .../dashboard/reducers/dashboardState.test.js | 9 +- .../charts/getFormDataWithExtraFilters.ts | 6 + .../components/ExploreChartHeader/index.jsx | 18 ++- superset/dashboards/dao.py | 1 + superset/dashboards/schemas.py | 1 + .../integration_tests/dashboards/api_tests.py | 2 +- 68 files changed, 690 insertions(+), 137 deletions(-) create mode 100644 superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts create mode 100644 superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts diff --git a/.gitignore b/.gitignore index 81c44731de5d6..a23cbb9ba5a6e 100644 --- a/.gitignore +++ b/.gitignore @@ -108,3 +108,5 @@ release.json messages.mo docker/requirements-local.txt + +cache/ diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index 24c235692a7eb..e54aa9df3ecda 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -136,6 +136,7 @@ "rison": "^0.1.1", "scroll-into-view-if-needed": "^2.2.28", "shortid": "^2.2.6", + "tinycolor2": "^1.4.2", "urijs": "^1.19.8", "use-immer": "^0.6.0", "use-query-params": "^1.1.9", @@ -201,6 +202,7 @@ "@types/rison": "0.0.6", "@types/shortid": "^0.0.29", "@types/sinon": "^9.0.5", + "@types/tinycolor2": "^1.4.3", "@types/yargs": "12 - 15", "@typescript-eslint/eslint-plugin": "^5.3.0", "@typescript-eslint/parser": "^5.3.0", @@ -22525,6 +22527,11 @@ "resolved": "https://registry.npmjs.org/@types/text-encoding-utf-8/-/text-encoding-utf-8-1.0.2.tgz", "integrity": "sha512-AQ6zewa0ucLJvtUi5HsErbOFKAcQfRLt9zFLlUOvcXBy2G36a+ZDpCHSGdzJVUD8aNURtIjh9aSjCStNMRCcRQ==" }, + "node_modules/@types/tinycolor2": { + "version": "1.4.3", + "resolved": "https://registry.npmjs.org/@types/tinycolor2/-/tinycolor2-1.4.3.tgz", + "integrity": "sha512-Kf1w9NE5HEgGxCRyIcRXR/ZYtDv0V8FVPtYHwLxl0O+maGX0erE77pQlD0gpP+/KByMZ87mOA79SjifhSB3PjQ==" + }, "node_modules/@types/uglify-js": { "version": "3.0.4", "resolved": "https://registry.npmjs.org/@types/uglify-js/-/uglify-js-3.0.4.tgz", @@ -53688,9 +53695,9 @@ "integrity": "sha512-lBN9zLN/oAf68o3zNXYrdCt1kP8WsiGW8Oo2ka41b2IM5JL/S1CTyX1rW0mb/zSuJun0ZUrDxx4sqvYS2FWzPA==" }, "node_modules/tinycolor2": { - "version": "1.4.1", - "resolved": "https://registry.npmjs.org/tinycolor2/-/tinycolor2-1.4.1.tgz", - "integrity": "sha1-9PrTM0R7wLB9TcjpIJ2POaisd+g=", + "version": "1.4.2", + "resolved": "https://registry.npmjs.org/tinycolor2/-/tinycolor2-1.4.2.tgz", + "integrity": "sha512-vJhccZPs965sV/L2sU4oRQVAos0pQXwsvTLkWYdqJ+a8Q5kPFzJTuOFwy7UniPli44NKQGAglksjvOcpo95aZA==", "engines": { "node": "*" } @@ -58702,6 +58709,7 @@ "@types/prop-types": "^15.7.2", "@types/rison": "0.0.6", "@types/seedrandom": "^2.4.28", + "@types/tinycolor2": "^1.4.3", "@vx/responsive": "^0.0.199", "csstype": "^2.6.4", "d3-format": "^1.3.2", @@ -75807,6 +75815,7 @@ "@types/prop-types": "^15.7.2", "@types/rison": "0.0.6", "@types/seedrandom": "^2.4.28", + "@types/tinycolor2": "^1.4.3", "@vx/responsive": "^0.0.199", "csstype": "^2.6.4", "d3-format": "^1.3.2", @@ -77790,6 +77799,11 @@ "resolved": "https://registry.npmjs.org/@types/text-encoding-utf-8/-/text-encoding-utf-8-1.0.2.tgz", "integrity": "sha512-AQ6zewa0ucLJvtUi5HsErbOFKAcQfRLt9zFLlUOvcXBy2G36a+ZDpCHSGdzJVUD8aNURtIjh9aSjCStNMRCcRQ==" }, + "@types/tinycolor2": { + "version": "1.4.3", + "resolved": "https://registry.npmjs.org/@types/tinycolor2/-/tinycolor2-1.4.3.tgz", + "integrity": "sha512-Kf1w9NE5HEgGxCRyIcRXR/ZYtDv0V8FVPtYHwLxl0O+maGX0erE77pQlD0gpP+/KByMZ87mOA79SjifhSB3PjQ==" + }, "@types/uglify-js": { "version": "3.0.4", "resolved": "https://registry.npmjs.org/@types/uglify-js/-/uglify-js-3.0.4.tgz", @@ -102080,9 +102094,9 @@ "integrity": "sha512-lBN9zLN/oAf68o3zNXYrdCt1kP8WsiGW8Oo2ka41b2IM5JL/S1CTyX1rW0mb/zSuJun0ZUrDxx4sqvYS2FWzPA==" }, "tinycolor2": { - "version": "1.4.1", - "resolved": "https://registry.npmjs.org/tinycolor2/-/tinycolor2-1.4.1.tgz", - "integrity": "sha1-9PrTM0R7wLB9TcjpIJ2POaisd+g=" + "version": "1.4.2", + "resolved": "https://registry.npmjs.org/tinycolor2/-/tinycolor2-1.4.2.tgz", + "integrity": "sha512-vJhccZPs965sV/L2sU4oRQVAos0pQXwsvTLkWYdqJ+a8Q5kPFzJTuOFwy7UniPli44NKQGAglksjvOcpo95aZA==" }, "tinyqueue": { "version": "2.0.3", diff --git a/superset-frontend/package.json b/superset-frontend/package.json index d8161d0d7d637..31a95ca672a02 100644 --- a/superset-frontend/package.json +++ b/superset-frontend/package.json @@ -196,6 +196,7 @@ "rison": "^0.1.1", "scroll-into-view-if-needed": "^2.2.28", "shortid": "^2.2.6", + "tinycolor2": "^1.4.2", "urijs": "^1.19.8", "use-immer": "^0.6.0", "use-query-params": "^1.1.9", @@ -261,6 +262,7 @@ "@types/rison": "0.0.6", "@types/shortid": "^0.0.29", "@types/sinon": "^9.0.5", + "@types/tinycolor2": "^1.4.3", "@types/yargs": "12 - 15", "@typescript-eslint/eslint-plugin": "^5.3.0", "@typescript-eslint/parser": "^5.3.0", diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx index aee3717938c9e..ff15a6f4a92da 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx @@ -205,6 +205,9 @@ const linear_color_scheme: SharedControlConfig<'ColorSchemeControl'> = { renderTrigger: true, schemes: () => sequentialSchemeRegistry.getMap(), isLinear: true, + mapStateToProps: state => ({ + dashboardId: state?.form_data?.dashboardId, + }), }; const secondary_metric: SharedControlConfig<'MetricsControl'> = { diff --git a/superset-frontend/packages/superset-ui-core/package.json b/superset-frontend/packages/superset-ui-core/package.json index 0b9713bc4346b..28937d298ad6c 100644 --- a/superset-frontend/packages/superset-ui-core/package.json +++ b/superset-frontend/packages/superset-ui-core/package.json @@ -42,6 +42,7 @@ "@types/math-expression-evaluator": "^1.2.1", "@types/rison": "0.0.6", "@types/seedrandom": "^2.4.28", + "@types/tinycolor2": "^1.4.3", "@types/fetch-mock": "^7.3.3", "@types/enzyme": "^3.10.5", "@types/prop-types": "^15.7.2", diff --git a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts index 63b2cb55f67cd..d34960dac0973 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -22,12 +22,12 @@ import { scaleOrdinal, ScaleOrdinal } from 'd3-scale'; import { ExtensibleFunction } from '../models'; import { ColorsLookup } from './types'; import stringifyAndTrim from './stringifyAndTrim'; +import getSharedLabelColor from './SharedLabelColorSingleton'; // Use type augmentation to correct the fact that // an instance of CategoricalScale is also a function - interface CategoricalColorScale { - (x: { toString(): string }): string; + (x: { toString(): string }, y?: number): string; } class CategoricalColorScale extends ExtensibleFunction { @@ -46,7 +46,7 @@ class CategoricalColorScale extends ExtensibleFunction { * (usually CategoricalColorNamespace) and supersede this.forcedColors */ constructor(colors: string[], parentForcedColors?: ColorsLookup) { - super((value: string) => this.getColor(value)); + super((value: string, sliceId?: number) => this.getColor(value, sliceId)); this.colors = colors; this.scale = scaleOrdinal<{ toString(): string }, string>(); @@ -55,20 +55,27 @@ class CategoricalColorScale extends ExtensibleFunction { this.forcedColors = {}; } - getColor(value?: string) { + getColor(value?: string, sliceId?: number) { const cleanedValue = stringifyAndTrim(value); + const sharedLabelColor = getSharedLabelColor(); + const parentColor = this.parentForcedColors && this.parentForcedColors[cleanedValue]; if (parentColor) { + sharedLabelColor.addSlice(cleanedValue, parentColor, sliceId); return parentColor; } const forcedColor = this.forcedColors[cleanedValue]; if (forcedColor) { + sharedLabelColor.addSlice(cleanedValue, forcedColor, sliceId); return forcedColor; } - return this.scale(cleanedValue); + const color = this.scale(cleanedValue); + sharedLabelColor.addSlice(cleanedValue, color, sliceId); + + return color; } /** diff --git a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts new file mode 100644 index 0000000000000..227b565276a94 --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -0,0 +1,130 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import tinycolor from 'tinycolor2'; +import { CategoricalColorNamespace } from '.'; +import makeSingleton from '../utils/makeSingleton'; + +export class SharedLabelColor { + sliceLabelColorMap: Record>; + + constructor() { + // { sliceId1: { label1: color1 }, sliceId2: { label2: color2 } } + this.sliceLabelColorMap = {}; + } + + getColorMap( + colorNamespace?: string, + colorScheme?: string, + updateColorScheme?: boolean, + ) { + if (colorScheme) { + const categoricalNamespace = + CategoricalColorNamespace.getNamespace(colorNamespace); + const colors = categoricalNamespace.getScale(colorScheme).range(); + const sharedLabels = this.getSharedLabels(); + const generatedColors: tinycolor.Instance[] = []; + let sharedLabelMap; + + if (sharedLabels.length) { + const multiple = Math.ceil(sharedLabels.length / colors.length); + const ext = 5; + const analogousColors = colors.map(color => { + const result = tinycolor(color).analogous(multiple + ext); + return result.slice(ext); + }); + + // [[A, AA, AAA], [B, BB, BBB]] => [A, B, AA, BB, AAA, BBB] + while (analogousColors[analogousColors.length - 1]?.length) { + analogousColors.forEach(colors => + generatedColors.push(colors.shift() as tinycolor.Instance), + ); + } + sharedLabelMap = sharedLabels.reduce( + (res, label, index) => ({ + ...res, + [label.toString()]: generatedColors[index]?.toHexString(), + }), + {}, + ); + } + + const labelMap = Object.keys(this.sliceLabelColorMap).reduce( + (res, sliceId) => { + const colorScale = categoricalNamespace.getScale(colorScheme); + return { + ...res, + ...Object.keys(this.sliceLabelColorMap[sliceId]).reduce( + (res, label) => ({ + ...res, + [label]: updateColorScheme + ? colorScale(label) + : this.sliceLabelColorMap[sliceId][label], + }), + {}, + ), + }; + }, + {}, + ); + + return { + ...labelMap, + ...sharedLabelMap, + }; + } + return undefined; + } + + addSlice(label: string, color: string, sliceId?: number) { + if (!sliceId) return; + this.sliceLabelColorMap[sliceId] = { + ...this.sliceLabelColorMap[sliceId], + [label]: color, + }; + } + + removeSlice(sliceId: number) { + delete this.sliceLabelColorMap[sliceId]; + } + + clear() { + this.sliceLabelColorMap = {}; + } + + getSharedLabels() { + const tempLabels = new Set(); + const result = new Set(); + Object.keys(this.sliceLabelColorMap).forEach(sliceId => { + const colorMap = this.sliceLabelColorMap[sliceId]; + Object.keys(colorMap).forEach(label => { + if (tempLabels.has(label) && !result.has(label)) { + result.add(label); + } else { + tempLabels.add(label); + } + }); + }); + return [...result]; + } +} + +const getInstance = makeSingleton(SharedLabelColor); + +export default getInstance; diff --git a/superset-frontend/packages/superset-ui-core/src/color/index.ts b/superset-frontend/packages/superset-ui-core/src/color/index.ts index 0f7ce6194c6e3..e1cde3ba3e2d5 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/index.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/index.ts @@ -32,5 +32,9 @@ export * from './SequentialScheme'; export { default as ColorSchemeRegistry } from './ColorSchemeRegistry'; export * from './colorSchemes'; export * from './utils'; +export { + default as getSharedLabelColor, + SharedLabelColor, +} from './SharedLabelColorSingleton'; export const BRAND_COLOR = '#00A699'; diff --git a/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts new file mode 100644 index 0000000000000..560bd56e6805f --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts @@ -0,0 +1,110 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + CategoricalScheme, + getCategoricalSchemeRegistry, + getSharedLabelColor, + SharedLabelColor, +} from '@superset-ui/core'; + +describe('SharedLabelColor', () => { + beforeAll(() => { + getCategoricalSchemeRegistry() + .registerValue( + 'testColors', + new CategoricalScheme({ + id: 'testColors', + colors: ['red', 'green', 'blue'], + }), + ) + .registerValue( + 'testColors2', + new CategoricalScheme({ + id: 'testColors2', + colors: ['yellow', 'green', 'blue'], + }), + ); + }); + + beforeEach(() => { + getSharedLabelColor().clear(); + }); + + it('has default value out-of-the-box', () => { + expect(getSharedLabelColor()).toBeInstanceOf(SharedLabelColor); + }); + + describe('.addSlice(value, color, sliceId)', () => { + it('should add to valueSliceMap when first adding label', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + expect(sharedLabelColor.sliceLabelColorMap).toHaveProperty('1', { + a: 'red', + }); + }); + + it('do nothing when sliceId is undefined', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red'); + expect(sharedLabelColor.sliceLabelColorMap).toEqual({}); + }); + }); + + describe('.remove(sliceId)', () => { + it('should remove sliceId', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.removeSlice(1); + expect(sharedLabelColor.sliceLabelColorMap).toEqual({}); + }); + }); + + describe('.getColorMap(namespace, scheme, updateColorScheme)', () => { + it('return undefined when scheme is undefined', () => { + const sharedLabelColor = getSharedLabelColor(); + const colorMap = sharedLabelColor.getColorMap(); + expect(colorMap).toBeUndefined(); + }); + + it('return undefined value if pass updateColorScheme', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.addSlice('b', 'blue', 2); + const colorMap = sharedLabelColor.getColorMap('', 'testColors2', true); + expect(colorMap).toEqual({ a: 'yellow', b: 'yellow' }); + }); + + it('return color value if not pass updateColorScheme', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.addSlice('b', 'blue', 2); + const colorMap = sharedLabelColor.getColorMap('', 'testColors'); + expect(colorMap).toEqual({ a: 'red', b: 'blue' }); + }); + + it('return color value if shared label exit', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.addSlice('a', 'blue', 2); + const colorMap = sharedLabelColor.getColorMap('', 'testColors'); + expect(colorMap).not.toEqual({}); + }); + }); +}); diff --git a/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js b/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js index d36083e6cb46b..d0aed798de916 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js +++ b/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js @@ -36,7 +36,7 @@ const propTypes = { }; function Chord(element, props) { - const { data, width, height, numberFormat, colorScheme } = props; + const { data, width, height, numberFormat, colorScheme, sliceId } = props; element.innerHTML = ''; @@ -93,7 +93,7 @@ function Chord(element, props) { .append('path') .attr('id', (d, i) => `group${i}`) .attr('d', arc) - .style('fill', (d, i) => colorFn(nodes[i])); + .style('fill', (d, i) => colorFn(nodes[i], sliceId)); // Add a text label. const groupText = group.append('text').attr('x', 6).attr('dy', 15); @@ -121,7 +121,7 @@ function Chord(element, props) { .on('mouseover', d => { chord.classed('fade', p => p !== d); }) - .style('fill', d => colorFn(nodes[d.source.index])) + .style('fill', d => colorFn(nodes[d.source.index], sliceId)) .attr('d', path); // Add an elaborate mouseover title for each chord. diff --git a/superset-frontend/plugins/legacy-plugin-chart-chord/src/transformProps.js b/superset-frontend/plugins/legacy-plugin-chart-chord/src/transformProps.js index 4c9d09517f606..7503ff4ea1ff7 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-chord/src/transformProps.js +++ b/superset-frontend/plugins/legacy-plugin-chart-chord/src/transformProps.js @@ -18,7 +18,7 @@ */ export default function transformProps(chartProps) { const { width, height, formData, queriesData } = chartProps; - const { yAxisFormat, colorScheme } = formData; + const { yAxisFormat, colorScheme, sliceId } = formData; return { colorScheme, @@ -26,5 +26,6 @@ export default function transformProps(chartProps) { height, numberFormat: yAxisFormat, width, + sliceId, }; } diff --git a/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js b/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js index ef26569199918..d363e8a5980b5 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js +++ b/superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js @@ -23,6 +23,7 @@ import { extent as d3Extent } from 'd3-array'; import { getNumberFormatter, getSequentialSchemeRegistry, + CategoricalColorNamespace, } from '@superset-ui/core'; import countries, { countryOptions } from './countries'; import './CountryMap.css'; @@ -45,17 +46,29 @@ const propTypes = { const maps = {}; function CountryMap(element, props) { - const { data, width, height, country, linearColorScheme, numberFormat } = - props; + const { + data, + width, + height, + country, + linearColorScheme, + numberFormat, + colorScheme, + sliceId, + } = props; const container = element; const format = getNumberFormatter(numberFormat); - const colorScale = getSequentialSchemeRegistry() + const linearColorScale = getSequentialSchemeRegistry() .get(linearColorScheme) .createLinearScale(d3Extent(data, v => v.metric)); + const colorScale = CategoricalColorNamespace.getScale(colorScheme); + const colorMap = {}; data.forEach(d => { - colorMap[d.country_id] = colorScale(d.metric); + colorMap[d.country_id] = colorScheme + ? colorScale(d.country_id, sliceId) + : linearColorScale(d.metric); }); const colorFn = d => colorMap[d.properties.ISO] || 'none'; diff --git a/superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js b/superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js index 4120f621f1a04..8789c3d2f34fe 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js +++ b/superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js @@ -18,7 +18,13 @@ */ export default function transformProps(chartProps) { const { width, height, formData, queriesData } = chartProps; - const { linearColorScheme, numberFormat, selectCountry } = formData; + const { + linearColorScheme, + numberFormat, + selectCountry, + colorScheme, + sliceId, + } = formData; return { width, @@ -27,5 +33,7 @@ export default function transformProps(chartProps) { country: selectCountry ? String(selectCountry).toLowerCase() : null, linearColorScheme, numberFormat, + colorScheme, + sliceId, }; } diff --git a/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx b/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx index 518272afdff2e..2c07267748614 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx +++ b/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx @@ -71,13 +71,14 @@ class CustomHistogram extends React.PureComponent { xAxisLabel, yAxisLabel, showLegend, + sliceId, } = this.props; const colorFn = CategoricalColorNamespace.getScale(colorScheme); const keys = data.map(d => d.key); const colorScale = scaleOrdinal({ domain: keys, - range: keys.map(x => colorFn(x)), + range: keys.map(x => colorFn(x, sliceId)), }); return ( diff --git a/superset-frontend/plugins/legacy-plugin-chart-histogram/src/transformProps.js b/superset-frontend/plugins/legacy-plugin-chart-histogram/src/transformProps.js index 4a5782c7172a5..1de223240498c 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-histogram/src/transformProps.js +++ b/superset-frontend/plugins/legacy-plugin-chart-histogram/src/transformProps.js @@ -27,6 +27,7 @@ export default function transformProps(chartProps) { xAxisLabel, yAxisLabel, showLegend, + sliceId, } = formData; return { @@ -41,5 +42,6 @@ export default function transformProps(chartProps) { xAxisLabel, yAxisLabel, showLegend, + sliceId, }; } diff --git a/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js b/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js index 224ff85af7d18..5355530cd536a 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js +++ b/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js @@ -119,6 +119,7 @@ function Icicle(element, props) { partitionThreshold, useRichTooltip, timeSeriesOption = 'not_time', + sliceId, } = props; const div = d3.select(element); @@ -385,7 +386,7 @@ function Icicle(element, props) { // Apply color scheme g.selectAll('rect').style('fill', d => { - d.color = colorFn(d.name); + d.color = colorFn(d.name, sliceId); return d.color; }); diff --git a/superset-frontend/plugins/legacy-plugin-chart-partition/src/transformProps.js b/superset-frontend/plugins/legacy-plugin-chart-partition/src/transformProps.js index d69de4ed52f0f..da58cd6160850 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-partition/src/transformProps.js +++ b/superset-frontend/plugins/legacy-plugin-chart-partition/src/transformProps.js @@ -30,6 +30,7 @@ export default function transformProps(chartProps) { partitionThreshold, richTooltip, timeSeriesOption, + sliceId, } = formData; const { verboseMap } = datasource; @@ -48,5 +49,6 @@ export default function transformProps(chartProps) { timeSeriesOption, useLogScale: logScale, useRichTooltip: richTooltip, + sliceId, }; } diff --git a/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js b/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js index 7ad4fd508ea56..4d7ef2b8ed995 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js +++ b/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js @@ -76,6 +76,7 @@ function Rose(element, props) { numberFormat, useRichTooltip, useAreaProportions, + sliceId, } = props; const div = d3.select(element); @@ -120,10 +121,10 @@ function Rose(element, props) { .map(v => ({ key: v.name, value: v.value, - color: colorFn(v.name), + color: colorFn(v.name, sliceId), highlight: v.id === d.arcId, })) - : [{ key: d.name, value: d.val, color: colorFn(d.name) }]; + : [{ key: d.name, value: d.val, color: colorFn(d.name, sliceId) }]; return { key: 'Date', @@ -132,7 +133,7 @@ function Rose(element, props) { }; } - legend.width(width).color(d => colorFn(d.key)); + legend.width(width).color(d => colorFn(d.key, sliceId)); legendWrap.datum(legendData(datum)).call(legend); tooltip.headerFormatter(timeFormat).valueFormatter(format); @@ -378,7 +379,7 @@ function Rose(element, props) { const arcs = ae .append('path') .attr('class', 'arc') - .attr('fill', d => colorFn(d.name)) + .attr('fill', d => colorFn(d.name, sliceId)) .attr('d', arc); function mousemove() { diff --git a/superset-frontend/plugins/legacy-plugin-chart-rose/src/transformProps.js b/superset-frontend/plugins/legacy-plugin-chart-rose/src/transformProps.js index 1e5407b16d9d2..b907e40ecffab 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-rose/src/transformProps.js +++ b/superset-frontend/plugins/legacy-plugin-chart-rose/src/transformProps.js @@ -24,6 +24,7 @@ export default function transformProps(chartProps) { numberFormat, richTooltip, roseAreaProportion, + sliceId, } = formData; return { @@ -35,5 +36,6 @@ export default function transformProps(chartProps) { numberFormat, useAreaProportions: roseAreaProportion, useRichTooltip: richTooltip, + sliceId, }; } diff --git a/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/SankeyLoop.js b/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/SankeyLoop.js index bddf570026ba3..4d6f059fdeb10 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/SankeyLoop.js +++ b/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/SankeyLoop.js @@ -84,7 +84,7 @@ function computeGraph(links) { } function SankeyLoop(element, props) { - const { data, width, height, colorScheme } = props; + const { data, width, height, colorScheme, sliceId } = props; const color = CategoricalColorNamespace.getScale(colorScheme); const margin = { ...defaultMargin, ...props.margin }; const innerWidth = width - margin.left - margin.right; @@ -109,7 +109,7 @@ function SankeyLoop(element, props) { value / sValue, )})`, ) - .linkColor(d => color(d.source.name)); + .linkColor(d => color(d.source.name, sliceId)); const div = select(element); div.selectAll('*').remove(); diff --git a/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/transformProps.js b/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/transformProps.js index b62639a87feec..76c0c220a7681 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/transformProps.js +++ b/superset-frontend/plugins/legacy-plugin-chart-sankey-loop/src/transformProps.js @@ -18,7 +18,7 @@ */ export default function transformProps(chartProps) { const { width, height, formData, queriesData, margin } = chartProps; - const { colorScheme } = formData; + const { colorScheme, sliceId } = formData; return { width, @@ -26,5 +26,6 @@ export default function transformProps(chartProps) { data: queriesData[0].data, colorScheme, margin, + sliceId, }; } diff --git a/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js b/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js index b847f754133bf..d8c8f61e441a8 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js +++ b/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js @@ -44,7 +44,7 @@ const propTypes = { const formatNumber = getNumberFormatter(NumberFormats.FLOAT); function Sankey(element, props) { - const { data, width, height, colorScheme } = props; + const { data, width, height, colorScheme, sliceId } = props; const div = d3.select(element); div.classed(`superset-legacy-chart-sankey`, true); const margin = { @@ -219,7 +219,7 @@ function Sankey(element, props) { .attr('width', sankey.nodeWidth()) .style('fill', d => { const name = d.name || 'N/A'; - d.color = colorFn(name.replace(/ .*/, '')); + d.color = colorFn(name, sliceId); return d.color; }) diff --git a/superset-frontend/plugins/legacy-plugin-chart-sankey/src/transformProps.js b/superset-frontend/plugins/legacy-plugin-chart-sankey/src/transformProps.js index 5297994fb9525..b8e9f05b284c3 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-sankey/src/transformProps.js +++ b/superset-frontend/plugins/legacy-plugin-chart-sankey/src/transformProps.js @@ -20,7 +20,7 @@ import { getLabelFontSize } from './utils'; export default function transformProps(chartProps) { const { width, height, formData, queriesData } = chartProps; - const { colorScheme } = formData; + const { colorScheme, sliceId } = formData; return { width, @@ -28,5 +28,6 @@ export default function transformProps(chartProps) { data: queriesData[0].data, colorScheme, fontSize: getLabelFontSize(width), + sliceId, }; } diff --git a/superset-frontend/plugins/legacy-plugin-chart-sunburst/src/Sunburst.js b/superset-frontend/plugins/legacy-plugin-chart-sunburst/src/Sunburst.js index 75bebdaa14034..2a9cc56f51fc6 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-sunburst/src/Sunburst.js +++ b/superset-frontend/plugins/legacy-plugin-chart-sunburst/src/Sunburst.js @@ -170,6 +170,7 @@ function Sunburst(element, props) { linearColorScheme, metrics, numberFormat, + sliceId, } = props; const responsiveClass = getResponsiveContainerClass(width); const isSmallWidth = responsiveClass === 's'; @@ -287,7 +288,7 @@ function Sunburst(element, props) { .attr('points', breadcrumbPoints) .style('fill', d => colorByCategory - ? categoricalColorScale(d.name) + ? categoricalColorScale(d.name, sliceId) : linearColorScale(d.m2 / d.m1), ); @@ -300,7 +301,7 @@ function Sunburst(element, props) { // Make text white or black based on the lightness of the background const col = d3.hsl( colorByCategory - ? categoricalColorScale(d.name) + ? categoricalColorScale(d.name, sliceId) : linearColorScale(d.m2 / d.m1), ); @@ -489,7 +490,7 @@ function Sunburst(element, props) { // For efficiency, filter nodes to keep only those large enough to see. const nodes = partition.nodes(root).filter(d => d.dx > 0.005); // 0.005 radians = 0.29 degrees - if (metrics[0] !== metrics[1] && metrics[1]) { + if (metrics[0] !== metrics[1] && metrics[1] && !colorScheme) { colorByCategory = false; const ext = d3.extent(nodes, d => d.m2 / d.m1); linearColorScale = getSequentialSchemeRegistry() @@ -507,7 +508,7 @@ function Sunburst(element, props) { .attr('fill-rule', 'evenodd') .style('fill', d => colorByCategory - ? categoricalColorScale(d.name) + ? categoricalColorScale(d.name, sliceId) : linearColorScale(d.m2 / d.m1), ) .style('opacity', 1) diff --git a/superset-frontend/plugins/legacy-plugin-chart-sunburst/src/transformProps.js b/superset-frontend/plugins/legacy-plugin-chart-sunburst/src/transformProps.js index 9952fc4992eb8..92c4d99f00e41 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-sunburst/src/transformProps.js +++ b/superset-frontend/plugins/legacy-plugin-chart-sunburst/src/transformProps.js @@ -18,7 +18,8 @@ */ export default function transformProps(chartProps) { const { width, height, formData, queriesData, datasource } = chartProps; - const { colorScheme, linearColorScheme, metric, secondaryMetric } = formData; + const { colorScheme, linearColorScheme, metric, secondaryMetric, sliceId } = + formData; const returnProps = { width, @@ -27,6 +28,7 @@ export default function transformProps(chartProps) { colorScheme, linearColorScheme, metrics: [metric, secondaryMetric], + sliceId, }; if (datasource && datasource.metrics) { diff --git a/superset-frontend/plugins/legacy-plugin-chart-treemap/src/Treemap.js b/superset-frontend/plugins/legacy-plugin-chart-treemap/src/Treemap.js index a155a050b360d..f218218ec8bbd 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-treemap/src/Treemap.js +++ b/superset-frontend/plugins/legacy-plugin-chart-treemap/src/Treemap.js @@ -87,6 +87,7 @@ function Treemap(element, props) { numberFormat, colorScheme, treemapRatio, + sliceId, } = props; const div = d3Select(element); div.classed('superset-legacy-chart-treemap', true); @@ -138,7 +139,7 @@ function Treemap(element, props) { .attr('id', d => `rect-${d.data.name}`) .attr('width', d => d.x1 - d.x0) .attr('height', d => d.y1 - d.y0) - .style('fill', d => colorFn(d.depth)); + .style('fill', d => colorFn(d.depth, sliceId)); cell .append('clipPath') diff --git a/superset-frontend/plugins/legacy-plugin-chart-treemap/src/transformProps.js b/superset-frontend/plugins/legacy-plugin-chart-treemap/src/transformProps.js index adba34c09b3dd..bbc577cb3db12 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-treemap/src/transformProps.js +++ b/superset-frontend/plugins/legacy-plugin-chart-treemap/src/transformProps.js @@ -18,7 +18,7 @@ */ export default function transformProps(chartProps) { const { width, height, formData, queriesData } = chartProps; - const { colorScheme, treemapRatio } = formData; + const { colorScheme, treemapRatio, sliceId } = formData; let { numberFormat } = formData; if (!numberFormat && chartProps.datasource && chartProps.datasource.metrics) { @@ -39,5 +39,6 @@ export default function transformProps(chartProps) { colorScheme, numberFormat, treemapRatio, + sliceId, }; } diff --git a/superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js b/superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js index c7253e10d0e68..0c81e98560166 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js +++ b/superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js @@ -23,6 +23,7 @@ import { extent as d3Extent } from 'd3-array'; import { getNumberFormatter, getSequentialSchemeRegistry, + CategoricalColorNamespace, } from '@superset-ui/core'; import Datamap from 'datamaps/dist/datamaps.world.min'; @@ -55,6 +56,8 @@ function WorldMap(element, props) { showBubbles, linearColorScheme, color, + colorScheme, + sliceId, } = props; const div = d3.select(element); div.classed('superset-legacy-chart-world-map', true); @@ -69,15 +72,24 @@ function WorldMap(element, props) { .domain([extRadius[0], extRadius[1]]) .range([1, maxBubbleSize]); - const colorScale = getSequentialSchemeRegistry() + const linearColorScale = getSequentialSchemeRegistry() .get(linearColorScheme) .createLinearScale(d3Extent(filteredData, d => d.m1)); - const processedData = filteredData.map(d => ({ - ...d, - radius: radiusScale(Math.sqrt(d.m2)), - fillColor: colorScale(d.m1), - })); + const colorScale = CategoricalColorNamespace.getScale(colorScheme); + + const processedData = filteredData.map(d => { + let color = linearColorScale(d.m1); + if (colorScheme) { + // use color scheme instead + color = colorScale(d.name, sliceId); + } + return { + ...d, + radius: radiusScale(Math.sqrt(d.m2)), + fillColor: color, + }; + }); const mapData = {}; processedData.forEach(d => { diff --git a/superset-frontend/plugins/legacy-plugin-chart-world-map/src/controlPanel.ts b/superset-frontend/plugins/legacy-plugin-chart-world-map/src/controlPanel.ts index ec8aafc7b872a..91664290dcb02 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-world-map/src/controlPanel.ts +++ b/superset-frontend/plugins/legacy-plugin-chart-world-map/src/controlPanel.ts @@ -106,6 +106,7 @@ const config: ControlPanelConfig = { }, ], ['color_picker'], + ['color_scheme'], ['linear_color_scheme'], ], }, @@ -126,6 +127,9 @@ const config: ControlPanelConfig = { color_picker: { label: t('Bubble Color'), }, + color_scheme: { + label: t('Categorical Color Scheme'), + }, linear_color_scheme: { label: t('Country Color Scheme'), }, diff --git a/superset-frontend/plugins/legacy-plugin-chart-world-map/src/transformProps.js b/superset-frontend/plugins/legacy-plugin-chart-world-map/src/transformProps.js index 464dd53afa4fc..3838ebfa5c10a 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-world-map/src/transformProps.js +++ b/superset-frontend/plugins/legacy-plugin-chart-world-map/src/transformProps.js @@ -20,8 +20,14 @@ import { rgb } from 'd3-color'; export default function transformProps(chartProps) { const { width, height, formData, queriesData } = chartProps; - const { maxBubbleSize, showBubbles, linearColorScheme, colorPicker } = - formData; + const { + maxBubbleSize, + showBubbles, + linearColorScheme, + colorPicker, + colorScheme, + sliceId, + } = formData; const { r, g, b } = colorPicker; return { @@ -32,5 +38,7 @@ export default function transformProps(chartProps) { showBubbles, linearColorScheme, color: rgb(r, g, b).hex(), + colorScheme, + sliceId, }; } diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.jsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.jsx index 7523c2e4ba907..64bfc0244a8ad 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.jsx +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.jsx @@ -46,7 +46,7 @@ function getCategories(fd, data) { if (d.cat_color != null && !categories.hasOwnProperty(d.cat_color)) { let color; if (fd.dimension) { - color = hexToRGB(colorFn(d.cat_color), c.a * 255); + color = hexToRGB(colorFn(d.cat_color, fd.sliceId), c.a * 255); } else { color = fixedColor; } @@ -212,7 +212,7 @@ export default class CategoricalDeckGLContainer extends React.PureComponent { return data.map(d => { let color; if (fd.dimension) { - color = hexToRGB(colorFn(d.cat_color), c.a * 255); + color = hexToRGB(colorFn(d.cat_color, fd.sliceId), c.a * 255); return { ...d, color }; } diff --git a/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js b/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js index 7f3d4d08d4fef..4d130d2139d0c 100644 --- a/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js +++ b/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js @@ -313,6 +313,7 @@ function nvd3Vis(element, props) { yAxis2ShowMinMax = false, yField, yIsLogScale, + sliceId, } = props; const isExplore = document.querySelector('#explorer-container') !== null; @@ -670,7 +671,9 @@ function nvd3Vis(element, props) { ); } else if (vizType !== 'bullet') { const colorFn = getScale(colorScheme); - chart.color(d => d.color || colorFn(cleanColorInput(d[colorKey]))); + chart.color( + d => d.color || colorFn(cleanColorInput(d[colorKey]), sliceId), + ); } if (isVizTypes(['line', 'area', 'bar', 'dist_bar']) && useRichTooltip) { diff --git a/superset-frontend/plugins/legacy-preset-chart-nvd3/src/transformProps.js b/superset-frontend/plugins/legacy-preset-chart-nvd3/src/transformProps.js index 454314c502f3a..7fc8669e6daf3 100644 --- a/superset-frontend/plugins/legacy-preset-chart-nvd3/src/transformProps.js +++ b/superset-frontend/plugins/legacy-preset-chart-nvd3/src/transformProps.js @@ -94,6 +94,7 @@ export default function transformProps(chartProps) { yAxisShowminmax, yAxis2Showminmax, yLogScale, + sliceId, } = formData; let { @@ -195,5 +196,6 @@ export default function transformProps(chartProps) { yAxis2ShowMinMax: yAxis2Showminmax, yField: y, yIsLogScale: yLogScale, + sliceId, }; } diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts index 8e89914b4d8a8..6f79ec6e274e1 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts @@ -63,6 +63,7 @@ export default function transformProps( xAxisTitleMargin, yAxisTitleMargin, yAxisTitlePosition, + sliceId, } = formData as BoxPlotQueryFormData; const colorFn = CategoricalColorNamespace.getScale(colorScheme as string); const numberFormatter = getNumberFormatter(numberFormat); @@ -98,9 +99,9 @@ export default function transformProps( datum[`${metric}__outliers`], ], itemStyle: { - color: colorFn(groupbyLabel), + color: colorFn(groupbyLabel, sliceId), opacity: isFiltered ? OpacityEnum.SemiTransparent : 0.6, - borderColor: colorFn(groupbyLabel), + borderColor: colorFn(groupbyLabel, sliceId), }, }; }); @@ -138,7 +139,7 @@ export default function transformProps( }, }, itemStyle: { - color: colorFn(groupbyLabel), + color: colorFn(groupbyLabel, sliceId), opacity: isFiltered ? OpacityEnum.SemiTransparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts index 3f3d84816d28f..88a0fe75c215f 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts @@ -103,6 +103,7 @@ export default function transformProps( showLabels, showLegend, emitFilter, + sliceId, }: EchartsFunnelFormData = { ...DEFAULT_LEGEND_FORM_DATA, ...DEFAULT_FUNNEL_FORM_DATA, @@ -145,7 +146,7 @@ export default function transformProps( value: datum[metricLabel], name, itemStyle: { - color: colorFn(name), + color: colorFn(name, sliceId), opacity: isFiltered ? OpacityEnum.SemiTransparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts index beecb475ace7a..0486ddf67298b 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts @@ -107,6 +107,7 @@ export default function transformProps( intervalColorIndices, valueFormatter, emitFilter, + sliceId, }: EchartsGaugeFormData = { ...DEFAULT_GAUGE_FORM_DATA, ...formData }; const data = (queriesData[0]?.data || []) as DataRecord[]; const numberFormatter = getNumberFormatter(numberFormat); @@ -147,7 +148,7 @@ export default function transformProps( value: data_point[getMetricLabel(metric as QueryFormMetric)] as number, name, itemStyle: { - color: colorFn(index), + color: colorFn(index, sliceId), }, title: { offsetCenter: [ @@ -175,7 +176,7 @@ export default function transformProps( item = { ...item, itemStyle: { - color: colorFn(index), + color: colorFn(index, sliceId), opacity: OpacityEnum.SemiTransparent, }, detail: { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts index ea9c6f1524970..905ecbfa8ebdb 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts @@ -184,6 +184,7 @@ export default function transformProps(chartProps: ChartProps): EchartsProps { baseEdgeWidth, baseNodeSize, edgeSymbol, + sliceId, }: EchartsGraphFormData = { ...DEFAULT_GRAPH_FORM_DATA, ...formData }; const metricLabel = getMetricLabel(metric); @@ -264,7 +265,7 @@ export default function transformProps(chartProps: ChartProps): EchartsProps { type: 'graph', categories: categoryList.map(c => ({ name: c, - itemStyle: { color: colorFn(c) }, + itemStyle: { color: colorFn(c, sliceId) }, })), layout, force: { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Graph/types.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Graph/types.ts index 76be9bb1a4e24..9cb35c1304450 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Graph/types.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Graph/types.ts @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ +import { QueryFormData } from '@superset-ui/core'; import { GraphNodeItemOption } from 'echarts/types/src/chart/graph/GraphSeries'; import { SeriesTooltipOption } from 'echarts/types/src/util/types'; import { @@ -27,32 +28,34 @@ import { export type EdgeSymbol = 'none' | 'circle' | 'arrow'; -export type EchartsGraphFormData = EchartsLegendFormData & { - source: string; - target: string; - sourceCategory?: string; - targetCategory?: string; - colorScheme?: string; - metric?: string; - layout?: 'none' | 'circular' | 'force'; - roam: boolean | 'scale' | 'move'; - draggable: boolean; - selectedMode?: boolean | 'multiple' | 'single'; - showSymbolThreshold: number; - repulsion: number; - gravity: number; - baseNodeSize: number; - baseEdgeWidth: number; - edgeLength: number; - edgeSymbol: string; - friction: number; -}; +export type EchartsGraphFormData = QueryFormData & + EchartsLegendFormData & { + source: string; + target: string; + sourceCategory?: string; + targetCategory?: string; + colorScheme?: string; + metric?: string; + layout?: 'none' | 'circular' | 'force'; + roam: boolean | 'scale' | 'move'; + draggable: boolean; + selectedMode?: boolean | 'multiple' | 'single'; + showSymbolThreshold: number; + repulsion: number; + gravity: number; + baseNodeSize: number; + baseEdgeWidth: number; + edgeLength: number; + edgeSymbol: string; + friction: number; + }; export type EChartGraphNode = Omit & { value: number; tooltip?: Pick; }; +// @ts-ignore export const DEFAULT_FORM_DATA: EchartsGraphFormData = { ...DEFAULT_LEGEND_FORM_DATA, source: '', diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts index c3d6e979a96a4..8ac08e39257b8 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -128,6 +128,7 @@ export default function transformProps( xAxisTitleMargin, yAxisTitleMargin, yAxisTitlePosition, + sliceId, }: EchartsMixedTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData }; const colorScale = CategoricalColorNamespace.getScale(colorScheme as string); @@ -177,6 +178,7 @@ export default function transformProps( yAxisIndex, filterState, seriesKey: entry.name, + sliceId, }); if (transformedSeries) series.push(transformedSeries); }); @@ -195,6 +197,7 @@ export default function transformProps( seriesKey: primarySeries.has(entry.name as string) ? `${entry.name} (1)` : entry.name, + sliceId, }); if (transformedSeries) series.push(transformedSeries); }); @@ -203,7 +206,9 @@ export default function transformProps( .filter((layer: AnnotationLayer) => layer.show) .forEach((layer: AnnotationLayer) => { if (isFormulaAnnotationLayer(layer)) - series.push(transformFormulaAnnotation(layer, data1, colorScale)); + series.push( + transformFormulaAnnotation(layer, data1, colorScale, sliceId), + ); else if (isIntervalAnnotationLayer(layer)) { series.push( ...transformIntervalAnnotation( @@ -211,11 +216,18 @@ export default function transformProps( data1, annotationData, colorScale, + sliceId, ), ); } else if (isEventAnnotationLayer(layer)) { series.push( - ...transformEventAnnotation(layer, data1, annotationData, colorScale), + ...transformEventAnnotation( + layer, + data1, + annotationData, + colorScale, + sliceId, + ), ); } else if (isTimeseriesAnnotationLayer(layer)) { series.push( diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts index a70855fa432f7..237f4ae001f70 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts @@ -109,6 +109,7 @@ export default function transformProps( showLegend, showLabelsThreshold, emitFilter, + sliceId, }: EchartsPieFormData = { ...DEFAULT_LEGEND_FORM_DATA, ...DEFAULT_PIE_FORM_DATA, @@ -162,7 +163,7 @@ export default function transformProps( value: datum[metricLabel], name, itemStyle: { - color: colorFn(name), + color: colorFn(name, sliceId), opacity: isFiltered ? OpacityEnum.SemiTransparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts index cd981a21a9b7e..b668e340350a8 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts @@ -91,6 +91,7 @@ export default function transformProps( showLegend, isCircle, columnConfig, + sliceId, }: EchartsRadarFormData = { ...DEFAULT_LEGEND_FORM_DATA, ...DEFAULT_RADAR_FORM_DATA, @@ -154,7 +155,7 @@ export default function transformProps( value: metricLabels.map(metricLabel => datum[metricLabel]), name: joinedName, itemStyle: { - color: colorFn(joinedName), + color: colorFn(joinedName, sliceId), opacity: isFiltered ? OpacityEnum.Transparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index b3a3d58c39958..1a2200db22097 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -125,6 +125,7 @@ export default function transformProps( xAxisTitleMargin, yAxisTitleMargin, yAxisTitlePosition, + sliceId, }: EchartsTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData }; const colorScale = CategoricalColorNamespace.getScale(colorScheme as string); @@ -198,6 +199,7 @@ export default function transformProps( showValueIndexes, thresholdValues, richTooltip, + sliceId, }); if (transformedSeries) series.push(transformedSeries); }); @@ -217,7 +219,9 @@ export default function transformProps( .filter((layer: AnnotationLayer) => layer.show) .forEach((layer: AnnotationLayer) => { if (isFormulaAnnotationLayer(layer)) - series.push(transformFormulaAnnotation(layer, data, colorScale)); + series.push( + transformFormulaAnnotation(layer, data, colorScale, sliceId), + ); else if (isIntervalAnnotationLayer(layer)) { series.push( ...transformIntervalAnnotation( @@ -225,11 +229,18 @@ export default function transformProps( data, annotationData, colorScale, + sliceId, ), ); } else if (isEventAnnotationLayer(layer)) { series.push( - ...transformEventAnnotation(layer, data, annotationData, colorScale), + ...transformEventAnnotation( + layer, + data, + annotationData, + colorScale, + sliceId, + ), ); } else if (isTimeseriesAnnotationLayer(layer)) { series.push( diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts index c357b2a40ee9d..7ce72695be2b5 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -84,6 +84,7 @@ export function transformSeries( thresholdValues?: number[]; richTooltip?: boolean; seriesKey?: OptionName; + sliceId?: number; }, ): SeriesOption | undefined { const { name } = series; @@ -105,6 +106,7 @@ export function transformSeries( thresholdValues = [], richTooltip, seriesKey, + sliceId, } = opts; const contexts = seriesContexts[name || ''] || []; const hasForecast = @@ -151,7 +153,7 @@ export function transformSeries( } // forcing the colorScale to return a different color for same metrics across different queries const itemStyle = { - color: colorScale(seriesKey || forecastSeries.name), + color: colorScale(seriesKey || forecastSeries.name, sliceId), opacity, }; let emphasis = {}; @@ -244,13 +246,14 @@ export function transformFormulaAnnotation( layer: FormulaAnnotationLayer, data: TimeseriesDataRecord[], colorScale: CategoricalColorScale, + sliceId?: number, ): SeriesOption { const { name, color, opacity, width, style } = layer; return { name, id: name, itemStyle: { - color: color || colorScale(name), + color: color || colorScale(name, sliceId), }, lineStyle: { opacity: parseAnnotationOpacity(opacity), @@ -269,6 +272,7 @@ export function transformIntervalAnnotation( data: TimeseriesDataRecord[], annotationData: AnnotationData, colorScale: CategoricalColorScale, + sliceId?: number, ): SeriesOption[] { const series: SeriesOption[] = []; const annotations = extractRecordAnnotations(layer, annotationData); @@ -323,7 +327,7 @@ export function transformIntervalAnnotation( markArea: { silent: false, itemStyle: { - color: color || colorScale(name), + color: color || colorScale(name, sliceId), opacity: parseAnnotationOpacity(opacity || AnnotationOpacity.Medium), emphasis: { opacity: 0.8, @@ -342,6 +346,7 @@ export function transformEventAnnotation( data: TimeseriesDataRecord[], annotationData: AnnotationData, colorScale: CategoricalColorScale, + sliceId?: number, ): SeriesOption[] { const series: SeriesOption[] = []; const annotations = extractRecordAnnotations(layer, annotationData); @@ -359,7 +364,7 @@ export function transformEventAnnotation( const lineStyle: LineStyleOption & DefaultStatesMixin['emphasis'] = { width, type: style as ZRLineType, - color: color || colorScale(name), + color: color || colorScale(name, sliceId), opacity: parseAnnotationOpacity(opacity), emphasis: { width: width ? width + 1 : width, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts index 2face71250d1a..25f3910b92c81 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts @@ -127,6 +127,7 @@ export default function transformProps( showUpperLabels, dashboardId, emitFilter, + sliceId, }: EchartsTreemapFormData = { ...DEFAULT_TREEMAP_FORM_DATA, ...formData, @@ -223,7 +224,7 @@ export default function transformProps( colorSaturation: COLOR_SATURATION, itemStyle: { borderColor: BORDER_COLOR, - color: colorFn(`${child.name}`), + color: colorFn(`${child.name}`, sliceId), borderWidth: BORDER_WIDTH, gapWidth: GAP_WIDTH, }, @@ -259,7 +260,7 @@ export default function transformProps( show: false, }, itemStyle: { - color: CategoricalColorNamespace.getColor(), + color: '#1FA8C9', }, }, ]; diff --git a/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx b/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx index 4a9683a6dd652..5d8ae0105e1af 100644 --- a/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx +++ b/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx @@ -25,7 +25,12 @@ import { DeriveEncoding, Encoder, } from 'encodable'; -import { SupersetThemeProps, withTheme, seedRandom } from '@superset-ui/core'; +import { + SupersetThemeProps, + withTheme, + seedRandom, + CategoricalColorScale, +} from '@superset-ui/core'; export const ROTATION = { flat: () => 0, @@ -58,6 +63,7 @@ export interface WordCloudProps extends WordCloudVisualProps { data: PlainObject[]; height: number; width: number; + sliceId: number; } export interface WordCloudState { @@ -210,12 +216,15 @@ class WordCloud extends React.PureComponent< render() { const { scaleFactor } = this.state; - const { width, height, encoding } = this.props; + const { width, height, encoding, sliceId } = this.props; const { words } = this.state; const encoder = this.createEncoder(encoding); encoder.channels.color.setDomainFromDataset(words); + const { getValueFromDatum } = encoder.channels.color; + const colorFn = encoder.channels.color.scale as CategoricalColorScale; + const viewBoxWidth = width * scaleFactor; const viewBoxHeight = height * scaleFactor; @@ -234,7 +243,7 @@ class WordCloud extends React.PureComponent< fontSize={`${w.size}px`} fontWeight={w.weight} fontFamily={w.font} - fill={encoder.channels.color.encodeDatum(w, '')} + fill={colorFn(getValueFromDatum(w) as string, sliceId)} textAnchor="middle" transform={`translate(${w.x}, ${w.y}) rotate(${w.rotate})`} > diff --git a/superset-frontend/plugins/plugin-chart-word-cloud/src/legacyPlugin/transformProps.ts b/superset-frontend/plugins/plugin-chart-word-cloud/src/legacyPlugin/transformProps.ts index 095714086bcc8..aec557d616d58 100644 --- a/superset-frontend/plugins/plugin-chart-word-cloud/src/legacyPlugin/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-word-cloud/src/legacyPlugin/transformProps.ts @@ -43,6 +43,7 @@ export default function transformProps(chartProps: ChartProps): WordCloudProps { series, sizeFrom = 0, sizeTo, + sliceId, } = formData as LegacyWordCloudFormData; const metricLabel = getMetricLabel(metric); @@ -77,5 +78,6 @@ export default function transformProps(chartProps: ChartProps): WordCloudProps { height, rotation, width, + sliceId, }; } diff --git a/superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/transformProps.ts b/superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/transformProps.ts index 02265298e97f7..4ed6f6405c9b6 100644 --- a/superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/transformProps.ts @@ -23,7 +23,7 @@ import { WordCloudFormData } from '../types'; export default function transformProps(chartProps: ChartProps): WordCloudProps { const { width, height, formData, queriesData } = chartProps; - const { encoding, rotation } = formData as WordCloudFormData; + const { encoding, rotation, sliceId } = formData as WordCloudFormData; return { data: queriesData[0].data, @@ -31,5 +31,6 @@ export default function transformProps(chartProps: ChartProps): WordCloudProps { height, rotation, width, + sliceId, }; } diff --git a/superset-frontend/src/components/Chart/Chart.jsx b/superset-frontend/src/components/Chart/Chart.jsx index 89fdd54546195..ab4fde84d2789 100644 --- a/superset-frontend/src/components/Chart/Chart.jsx +++ b/superset-frontend/src/components/Chart/Chart.jsx @@ -47,6 +47,7 @@ const propTypes = { // and merged with extra filter that current dashboard applying formData: PropTypes.object.isRequired, labelColors: PropTypes.object, + sharedLabelColors: PropTypes.object, width: PropTypes.number, height: PropTypes.number, setControlValue: PropTypes.func, @@ -70,6 +71,7 @@ const propTypes = { onFilterMenuOpen: PropTypes.func, onFilterMenuClose: PropTypes.func, ownState: PropTypes.object, + postTransformProps: PropTypes.func, }; const BLANK = {}; diff --git a/superset-frontend/src/components/Chart/ChartRenderer.jsx b/superset-frontend/src/components/Chart/ChartRenderer.jsx index 3c634ea32df8a..b814b6fde6d36 100644 --- a/superset-frontend/src/components/Chart/ChartRenderer.jsx +++ b/superset-frontend/src/components/Chart/ChartRenderer.jsx @@ -31,6 +31,7 @@ const propTypes = { initialValues: PropTypes.object, formData: PropTypes.object.isRequired, labelColors: PropTypes.object, + sharedLabelColors: PropTypes.object, height: PropTypes.number, width: PropTypes.number, setControlValue: PropTypes.func, @@ -48,6 +49,7 @@ const propTypes = { onFilterMenuOpen: PropTypes.func, onFilterMenuClose: PropTypes.func, ownState: PropTypes.object, + postTransformProps: PropTypes.func, source: PropTypes.oneOf(['dashboard', 'explore']), }; @@ -107,6 +109,7 @@ class ChartRenderer extends React.Component { nextProps.width !== this.props.width || nextProps.triggerRender || nextProps.labelColors !== this.props.labelColors || + nextProps.sharedLabelColors !== this.props.sharedLabelColors || nextProps.formData.color_scheme !== this.props.formData.color_scheme || nextProps.cacheBusterProp !== this.props.cacheBusterProp ); @@ -192,6 +195,7 @@ class ChartRenderer extends React.Component { filterState, formData, queriesResponse, + postTransformProps, } = this.props; // It's bad practice to use unprefixed `vizType` as classnames for chart @@ -260,6 +264,7 @@ class ChartRenderer extends React.Component { onRenderSuccess={this.handleRenderSuccess} onRenderFailure={this.handleRenderFailure} noResults={noResultsComponent} + postTransformProps={postTransformProps} /> ); } diff --git a/superset-frontend/src/dashboard/actions/dashboardInfo.ts b/superset-frontend/src/dashboard/actions/dashboardInfo.ts index 7b1b0017baa24..9a769101cfdca 100644 --- a/superset-frontend/src/dashboard/actions/dashboardInfo.ts +++ b/superset-frontend/src/dashboard/actions/dashboardInfo.ts @@ -23,6 +23,21 @@ import { ChartConfiguration, DashboardInfo } from '../reducers/types'; export const DASHBOARD_INFO_UPDATED = 'DASHBOARD_INFO_UPDATED'; +export function updateColorSchema( + metadata: Record, + labelColors: Record, +) { + const categoricalNamespace = CategoricalColorNamespace.getNamespace( + metadata?.color_namespace, + ); + const colorMap = isString(labelColors) + ? JSON.parse(labelColors) + : labelColors; + Object.keys(colorMap).forEach(label => { + categoricalNamespace.setColor(label, colorMap[label]); + }); +} + // updates partially changed dashboard info export function dashboardInfoChanged(newInfo: { metadata: any }) { const { metadata } = newInfo; @@ -33,14 +48,12 @@ export function dashboardInfoChanged(newInfo: { metadata: any }) { categoricalNamespace.resetColors(); + if (metadata?.shared_label_colors) { + updateColorSchema(metadata, metadata?.shared_label_colors); + } + if (metadata?.label_colors) { - const labelColors = metadata.label_colors; - const colorMap = isString(labelColors) - ? JSON.parse(labelColors) - : labelColors; - Object.keys(colorMap).forEach(label => { - categoricalNamespace.setColor(label, colorMap[label]); - }); + updateColorSchema(metadata, metadata?.label_colors); } return { type: DASHBOARD_INFO_UPDATED, newInfo }; diff --git a/superset-frontend/src/dashboard/actions/dashboardLayout.js b/superset-frontend/src/dashboard/actions/dashboardLayout.js index 1fe988849d627..e0cbe7aa00c77 100644 --- a/superset-frontend/src/dashboard/actions/dashboardLayout.js +++ b/superset-frontend/src/dashboard/actions/dashboardLayout.js @@ -47,17 +47,19 @@ function setUnsavedChangesAfterAction(action) { dispatch(result); } + const { dashboardLayout, dashboardState } = getState(); + const isComponentLevelEvent = result.type === UPDATE_COMPONENTS && result.payload && result.payload.nextComponents; // trigger dashboardFilters state update if dashboard layout is changed. if (!isComponentLevelEvent) { - const components = getState().dashboardLayout.present; + const components = dashboardLayout.present; dispatch(updateLayoutComponents(components)); } - if (!getState().dashboardState.hasUnsavedChanges) { + if (!dashboardState.hasUnsavedChanges) { dispatch(setUnsavedChanges(true)); } }; diff --git a/superset-frontend/src/dashboard/actions/dashboardState.js b/superset-frontend/src/dashboard/actions/dashboardState.js index 0afe42e063f7f..839b5feb7a153 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.js @@ -18,7 +18,12 @@ */ /* eslint camelcase: 0 */ import { ActionCreators as UndoActionCreators } from 'redux-undo'; -import { ensureIsArray, t, SupersetClient } from '@superset-ui/core'; +import { + ensureIsArray, + t, + SupersetClient, + getSharedLabelColor, +} from '@superset-ui/core'; import { addChart, removeChart, @@ -67,6 +72,11 @@ export function removeSlice(sliceId) { return { type: REMOVE_SLICE, sliceId }; } +export const RESET_SLICE = 'RESET_SLICE'; +export function resetSlice() { + return { type: RESET_SLICE }; +} + const FAVESTAR_BASE_URL = '/superset/favstar/Dashboard'; export const TOGGLE_FAVE_STAR = 'TOGGLE_FAVE_STAR'; export function toggleFaveStar(isStarred) { @@ -232,6 +242,7 @@ export function saveDashboardRequest(data, id, saveType) { color_scheme: data.metadata?.color_scheme || '', expanded_slices: data.metadata?.expanded_slices || {}, label_colors: data.metadata?.label_colors || {}, + shared_label_colors: data.metadata?.shared_label_colors || {}, refresh_frequency: data.metadata?.refresh_frequency || 0, timed_refresh_immune_slices: data.metadata?.timed_refresh_immune_slices || [], @@ -495,6 +506,28 @@ export function addSliceToDashboard(id, component) { }; } +export function postAddSliceFromDashboard() { + return (dispatch, getState) => { + const { + dashboardInfo: { metadata }, + dashboardState, + } = getState(); + + if (dashboardState?.updateSlice && dashboardState?.editMode) { + metadata.shared_label_colors = getSharedLabelColor().getColorMap( + metadata?.color_namespace, + metadata?.color_scheme, + ); + dispatch( + dashboardInfoChanged({ + metadata, + }), + ); + dispatch(resetSlice()); + } + }; +} + export function removeSliceFromDashboard(id) { return (dispatch, getState) => { const sliceEntity = getState().sliceEntities.slices[id]; @@ -504,6 +537,20 @@ export function removeSliceFromDashboard(id) { dispatch(removeSlice(id)); dispatch(removeChart(id)); + + const { + dashboardInfo: { metadata }, + } = getState(); + getSharedLabelColor().removeSlice(id); + metadata.shared_label_colors = getSharedLabelColor().getColorMap( + metadata?.color_namespace, + metadata?.color_scheme, + ); + dispatch( + dashboardInfoChanged({ + metadata, + }), + ); }; } diff --git a/superset-frontend/src/dashboard/actions/dashboardState.test.js b/superset-frontend/src/dashboard/actions/dashboardState.test.js index 295dc8cfc1189..f5fa60c08d56d 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.test.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.test.js @@ -39,7 +39,11 @@ describe('dashboardState actions', () => { sliceIds: [filterId], hasUnsavedChanges: true, }, - dashboardInfo: {}, + dashboardInfo: { + metadata: { + color_scheme: 'supersetColors', + }, + }, sliceEntities, dashboardFilters: emptyFilters, dashboardLayout: { @@ -116,6 +120,6 @@ describe('dashboardState actions', () => { const removeFilter = dispatch.getCall(0).args[0]; removeFilter(dispatch, getState); - expect(dispatch.getCall(3).args[0].type).toBe(REMOVE_FILTER); + expect(dispatch.getCall(4).args[0].type).toBe(REMOVE_FILTER); }); }); diff --git a/superset-frontend/src/dashboard/actions/hydrate.js b/superset-frontend/src/dashboard/actions/hydrate.js index 4c8a978e96389..f02d6f26484aa 100644 --- a/superset-frontend/src/dashboard/actions/hydrate.js +++ b/superset-frontend/src/dashboard/actions/hydrate.js @@ -17,12 +17,7 @@ * under the License. */ /* eslint-disable camelcase */ -import { isString } from 'lodash'; -import { - Behavior, - CategoricalColorNamespace, - getChartMetadataRegistry, -} from '@superset-ui/core'; +import { Behavior, getChartMetadataRegistry } from '@superset-ui/core'; import { chart } from 'src/components/Chart/chartReducer'; import { initSliceEntities } from 'src/dashboard/reducers/sliceEntities'; @@ -59,6 +54,7 @@ import { FILTER_BOX_MIGRATION_STATES } from 'src/explore/constants'; import { FeatureFlag, isFeatureEnabled } from '../../featureFlags'; import extractUrlParams from '../util/extractUrlParams'; import getNativeFilterConfig from '../util/filterboxMigrationHelper'; +import { updateColorSchema } from './dashboardInfo'; export const HYDRATE_DASHBOARD = 'HYDRATE_DASHBOARD'; @@ -92,19 +88,14 @@ export const hydrateDashboard = // } + if (metadata?.shared_label_colors) { + updateColorSchema(metadata, metadata?.shared_label_colors); + } + // Priming the color palette with user's label-color mapping provided in // the dashboard's JSON metadata if (metadata?.label_colors) { - const namespace = metadata.color_namespace; - const colorMap = isString(metadata.label_colors) - ? JSON.parse(metadata.label_colors) - : metadata.label_colors; - const categoricalNamespace = - CategoricalColorNamespace.getNamespace(namespace); - - Object.keys(colorMap).forEach(label => { - categoricalNamespace.setColor(label, colorMap[label]); - }); + updateColorSchema(metadata, metadata?.label_colors); } // dashboard layout diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index a67061832ddaf..89b1b9bee673c 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -20,7 +20,7 @@ import moment from 'moment'; import React from 'react'; import PropTypes from 'prop-types'; -import { styled, t } from '@superset-ui/core'; +import { styled, t, getSharedLabelColor } from '@superset-ui/core'; import ButtonGroup from 'src/components/ButtonGroup'; import { @@ -356,6 +356,15 @@ class Header extends React.PureComponent { ? currentRefreshFrequency : dashboardInfo.metadata?.refresh_frequency; + const currentColorScheme = + dashboardInfo?.metadata?.color_scheme || colorScheme; + const currentColorNamespace = + dashboardInfo?.metadata?.color_namespace || colorNamespace; + const currentSharedLabelColors = getSharedLabelColor().getColorMap( + currentColorNamespace, + currentColorScheme, + ); + const data = { certified_by: dashboardInfo.certified_by, certification_details: dashboardInfo.certification_details, @@ -367,11 +376,11 @@ class Header extends React.PureComponent { slug, metadata: { ...dashboardInfo?.metadata, - color_namespace: - dashboardInfo?.metadata?.color_namespace || colorNamespace, - color_scheme: dashboardInfo?.metadata?.color_scheme || colorScheme, + color_namespace: currentColorNamespace, + color_scheme: currentColorScheme, positions, refresh_frequency: refreshFrequency, + shared_label_colors: currentSharedLabelColors, }, }; diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index a18cb40ead87b..67c86cb1fc7de 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx @@ -29,6 +29,7 @@ import { SupersetClient, getCategoricalSchemeRegistry, ensureIsArray, + getSharedLabelColor, } from '@superset-ui/core'; import Modal from 'src/components/Modal'; @@ -169,7 +170,11 @@ const PropertiesModal = ({ if (metadata?.positions) { delete metadata.positions; } - setJsonMetadata(metadata ? jsonStringify(metadata) : ''); + const metaDataCopy = { ...metadata }; + if (metaDataCopy?.shared_label_colors) { + delete metaDataCopy.shared_label_colors; + } + setJsonMetadata(metaDataCopy ? jsonStringify(metaDataCopy) : ''); }, [form], ); @@ -282,12 +287,25 @@ const PropertiesModal = ({ form.getFieldsValue(); let currentColorScheme = colorScheme; let colorNamespace = ''; + let currentJsonMetadata = jsonMetadata; // color scheme in json metadata has precedence over selection - if (jsonMetadata?.length) { - const metadata = JSON.parse(jsonMetadata); + if (currentJsonMetadata?.length) { + const metadata = JSON.parse(currentJsonMetadata); currentColorScheme = metadata?.color_scheme || colorScheme; colorNamespace = metadata?.color_namespace || ''; + + // filter shared_label_color from user input + if (metadata?.shared_label_colors) { + delete metadata.shared_label_colors; + } + const colorMap = getSharedLabelColor().getColorMap( + colorNamespace, + currentColorScheme, + true, + ); + metadata.shared_label_colors = colorMap; + currentJsonMetadata = jsonStringify(metadata); } onColorSchemeChange(currentColorScheme, { @@ -304,7 +322,7 @@ const PropertiesModal = ({ id: dashboardId, title, slug, - jsonMetadata, + jsonMetadata: currentJsonMetadata, owners, colorScheme: currentColorScheme, colorNamespace, @@ -323,7 +341,7 @@ const PropertiesModal = ({ body: JSON.stringify({ dashboard_title: title, slug: slug || null, - json_metadata: jsonMetadata || null, + json_metadata: currentJsonMetadata || null, owners: (owners || []).map(o => o.id), certified_by: certifiedBy || null, certification_details: diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index 239bb508deb06..821b311b48fd2 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -56,6 +56,7 @@ const propTypes = { chart: chartPropShape.isRequired, formData: PropTypes.object.isRequired, labelColors: PropTypes.object, + sharedLabelColors: PropTypes.object, datasource: PropTypes.object, slice: slicePropShape.isRequired, sliceName: PropTypes.string.isRequired, @@ -81,6 +82,7 @@ const propTypes = { addDangerToast: PropTypes.func.isRequired, ownState: PropTypes.object, filterState: PropTypes.object, + postTransformProps: PropTypes.func, }; const defaultProps = { @@ -319,6 +321,7 @@ export default class Chart extends React.Component { filters, formData, labelColors, + sharedLabelColors, updateSliceName, sliceName, toggleExpandSlice, @@ -334,6 +337,7 @@ export default class Chart extends React.Component { handleToggleFullSize, isFullSize, filterboxMigrationState, + postTransformProps, } = this.props; const { width } = this.state; @@ -449,6 +453,7 @@ export default class Chart extends React.Component { initialValues={initialValues} formData={formData} labelColors={labelColors} + sharedLabelColors={sharedLabelColors} ownState={ownState} filterState={filterState} queriesResponse={chart.queriesResponse} @@ -457,6 +462,7 @@ export default class Chart extends React.Component { vizType={slice.viz_type} isDeactivatedViz={isDeactivatedViz} filterboxMigrationState={filterboxMigrationState} + postTransformProps={postTransformProps} /> diff --git a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.jsx b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.jsx index 465d646e7bd16..95fb967c777b2 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.jsx @@ -69,6 +69,7 @@ const propTypes = { updateComponents: PropTypes.func.isRequired, handleComponentDrop: PropTypes.func.isRequired, setFullSizeChartId: PropTypes.func.isRequired, + postAddSliceFromDashboard: PropTypes.func, }; const defaultProps = { @@ -197,6 +198,7 @@ class ChartHolder extends React.Component { this.handleDeleteComponent = this.handleDeleteComponent.bind(this); this.handleUpdateSliceName = this.handleUpdateSliceName.bind(this); this.handleToggleFullSize = this.handleToggleFullSize.bind(this); + this.handlePostTransformProps = this.handlePostTransformProps.bind(this); } componentDidMount() { @@ -251,6 +253,11 @@ class ChartHolder extends React.Component { setFullSizeChartId(isFullSize ? null : chartId); } + handlePostTransformProps(props) { + this.props.postAddSliceFromDashboard(); + return props; + } + render() { const { isFocused } = this.state; const { @@ -364,6 +371,7 @@ class ChartHolder extends React.Component { isComponentVisible={isComponentVisible} handleToggleFullSize={this.handleToggleFullSize} isFullSize={isFullSize} + postTransformProps={this.handlePostTransformProps} /> {editMode && ( diff --git a/superset-frontend/src/dashboard/containers/Chart.jsx b/superset-frontend/src/dashboard/containers/Chart.jsx index 06d3f56e34fae..96e053e8ed60f 100644 --- a/superset-frontend/src/dashboard/containers/Chart.jsx +++ b/superset-frontend/src/dashboard/containers/Chart.jsx @@ -62,6 +62,7 @@ function mapStateToProps( PLACEHOLDER_DATASOURCE; const { colorScheme, colorNamespace } = dashboardState; const labelColors = dashboardInfo?.metadata?.label_colors || {}; + const sharedLabelColors = dashboardInfo?.metadata?.shared_label_colors || {}; // note: this method caches filters if possible to prevent render cascades const formData = getFormDataWithExtraFilters({ layout: dashboardLayout.present, @@ -76,6 +77,7 @@ function mapStateToProps( nativeFilters, dataMask, labelColors, + sharedLabelColors, }); formData.dashboardId = dashboardInfo.id; @@ -84,6 +86,7 @@ function mapStateToProps( chart, datasource, labelColors, + sharedLabelColors, slice: sliceEntities.slices[id], timeout: dashboardInfo.common.conf.SUPERSET_WEBSERVER_TIMEOUT, filters: getActiveFilters() || EMPTY_OBJECT, diff --git a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx index 08b7ed9f82d90..23298d8bf96a4 100644 --- a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx +++ b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx @@ -37,6 +37,7 @@ import { setDirectPathToChild, setActiveTabs, setFullSizeChartId, + postAddSliceFromDashboard, } from 'src/dashboard/actions/dashboardState'; const propTypes = { @@ -111,6 +112,7 @@ function mapDispatchToProps(dispatch) { setFullSizeChartId, setActiveTabs, logEvent, + postAddSliceFromDashboard, }, dispatch, ); diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.tsx index e5fff328724d5..cd6772527509e 100644 --- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx +++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx @@ -17,7 +17,14 @@ * under the License. */ import React, { FC, useRef, useEffect, useState } from 'react'; -import { FeatureFlag, isFeatureEnabled, t, useTheme } from '@superset-ui/core'; +import { + CategoricalColorNamespace, + FeatureFlag, + getSharedLabelColor, + isFeatureEnabled, + t, + useTheme, +} from '@superset-ui/core'; import { useDispatch, useSelector } from 'react-redux'; import { Global } from '@emotion/react'; import { useParams } from 'react-router-dom'; @@ -222,6 +229,18 @@ const DashboardPage: FC = () => { return () => {}; }, [css]); + useEffect( + () => () => { + // clean up label color + const categoricalNamespace = CategoricalColorNamespace.getNamespace( + metadata?.color_namespace, + ); + categoricalNamespace.resetColors(); + getSharedLabelColor().clear(); + }, + [metadata?.color_namespace], + ); + useEffect(() => { if (datasetsApiError) { addDangerToast( diff --git a/superset-frontend/src/dashboard/reducers/dashboardState.js b/superset-frontend/src/dashboard/reducers/dashboardState.js index 64c794af93318..21d70b4f53bb9 100644 --- a/superset-frontend/src/dashboard/reducers/dashboardState.js +++ b/superset-frontend/src/dashboard/reducers/dashboardState.js @@ -39,6 +39,7 @@ import { UNSET_FOCUSED_FILTER_FIELD, SET_ACTIVE_TABS, SET_FULL_SIZE_CHART_ID, + RESET_SLICE, ON_FILTERS_REFRESH, ON_FILTERS_REFRESH_SUCCESS, } from '../actions/dashboardState'; @@ -58,6 +59,7 @@ export default function dashboardStateReducer(state = {}, action) { return { ...state, sliceIds: Array.from(updatedSliceIds), + updateSlice: true, }; }, [REMOVE_SLICE]() { @@ -70,6 +72,12 @@ export default function dashboardStateReducer(state = {}, action) { sliceIds: Array.from(updatedSliceIds), }; }, + [RESET_SLICE]() { + return { + ...state, + updateSlice: false, + }; + }, [TOGGLE_FAVE_STAR]() { return { ...state, isStarred: action.isStarred }; }, @@ -116,6 +124,7 @@ export default function dashboardStateReducer(state = {}, action) { maxUndoHistoryExceeded: false, editMode: false, updatedColorScheme: false, + updateSlice: false, // server-side returns last_modified_time for latest change lastModifiedTime: action.lastModifiedTime, }; diff --git a/superset-frontend/src/dashboard/reducers/dashboardState.test.js b/superset-frontend/src/dashboard/reducers/dashboardState.test.js index 39798ecf139e5..de3ecf72ff3ec 100644 --- a/superset-frontend/src/dashboard/reducers/dashboardState.test.js +++ b/superset-frontend/src/dashboard/reducers/dashboardState.test.js @@ -28,6 +28,7 @@ import { TOGGLE_EXPAND_SLICE, TOGGLE_FAVE_STAR, UNSET_FOCUSED_FILTER_FIELD, + RESET_SLICE, } from 'src/dashboard/actions/dashboardState'; import dashboardStateReducer from 'src/dashboard/reducers/dashboardState'; @@ -43,7 +44,7 @@ describe('dashboardState reducer', () => { { sliceIds: [1] }, { type: ADD_SLICE, slice: { slice_id: 2 } }, ), - ).toEqual({ sliceIds: [1, 2] }); + ).toEqual({ sliceIds: [1, 2], updateSlice: true }); }); it('should remove a slice', () => { @@ -55,6 +56,12 @@ describe('dashboardState reducer', () => { ).toEqual({ sliceIds: [1], filters: {} }); }); + it('should reset updateSlice', () => { + expect( + dashboardStateReducer({ updateSlice: true }, { type: RESET_SLICE }), + ).toEqual({ updateSlice: false }); + }); + it('should toggle fav star', () => { expect( dashboardStateReducer( diff --git a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts index 90022a9dce396..54e0417b27718 100644 --- a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts +++ b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts @@ -46,6 +46,7 @@ export interface GetFormDataWithExtraFiltersArguments { dataMask: DataMaskStateWithId; nativeFilters: NativeFiltersState; labelColors?: Record; + sharedLabelColors?: Record; } // this function merge chart's formData with dashboard filters value, @@ -63,6 +64,7 @@ export default function getFormDataWithExtraFilters({ layout, dataMask, labelColors, + sharedLabelColors, }: GetFormDataWithExtraFiltersArguments) { // if dashboard metadata + filters have not changed, use cache if possible const cachedFormData = cachedFormdataByChart[sliceId]; @@ -77,6 +79,9 @@ export default function getFormDataWithExtraFilters({ areObjectsEqual(cachedFormData?.label_colors, labelColors, { ignoreUndefined: true, }) && + areObjectsEqual(cachedFormData?.shared_label_colors, sharedLabelColors, { + ignoreUndefined: true, + }) && !!cachedFormData && areObjectsEqual(cachedFormData?.dataMask, dataMask, { ignoreUndefined: true, @@ -108,6 +113,7 @@ export default function getFormDataWithExtraFilters({ const formData = { ...chart.formData, label_colors: labelColors, + shared_label_colors: sharedLabelColors, ...(colorScheme && { color_scheme: colorScheme }), extra_filters: getEffectiveExtraFilters(filters), ...extraData, diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index bb1fde9c30e73..21605c553dfcd 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -156,13 +156,23 @@ export class ExploreChartHeader extends React.PureComponent { if (dashboard && dashboard.json_metadata) { // setting the chart to use the dashboard custom label colors if any - const labelColors = - JSON.parse(dashboard.json_metadata).label_colors || {}; + const metadata = JSON.parse(dashboard.json_metadata); + const sharedLabelColors = metadata.shared_label_colors || {}; + const customLabelColors = metadata.label_colors || {}; + const mergedLabelColors = { + ...sharedLabelColors, + ...customLabelColors, + }; + const categoricalNamespace = CategoricalColorNamespace.getNamespace(); - Object.keys(labelColors).forEach(label => { - categoricalNamespace.setColor(label, labelColors[label]); + Object.keys(mergedLabelColors).forEach(label => { + categoricalNamespace.setColor( + label, + mergedLabelColors[label], + metadata.color_scheme, + ); }); } } diff --git a/superset/dashboards/dao.py b/superset/dashboards/dao.py index 0443763cb370f..ce6e30f8d4beb 100644 --- a/superset/dashboards/dao.py +++ b/superset/dashboards/dao.py @@ -265,6 +265,7 @@ def set_dash_metadata( # pylint: disable=too-many-locals md["refresh_frequency"] = data.get("refresh_frequency", 0) md["color_scheme"] = data.get("color_scheme", "") md["label_colors"] = data.get("label_colors", {}) + md["shared_label_colors"] = data.get("shared_label_colors", {}) dashboard.json_metadata = json.dumps(md) diff --git a/superset/dashboards/schemas.py b/superset/dashboards/schemas.py index b1831fdcbbe70..661c61e1c2483 100644 --- a/superset/dashboards/schemas.py +++ b/superset/dashboards/schemas.py @@ -128,6 +128,7 @@ class DashboardJSONMetadataSchema(Schema): color_namespace = fields.Str(allow_none=True) positions = fields.Dict(allow_none=True) label_colors = fields.Dict() + shared_label_colors = fields.Dict() # used for v0 import/export import_time = fields.Integer() remote_id = fields.Integer() diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 2ed627a257247..8669da99f4a9e 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -72,7 +72,7 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi "slug": "slug1_changed", "position_json": '{"b": "B"}', "css": "css_changed", - "json_metadata": '{"refresh_frequency": 30, "timed_refresh_immune_slices": [], "expanded_slices": {}, "color_scheme": "", "label_colors": {}}', + "json_metadata": '{"refresh_frequency": 30, "timed_refresh_immune_slices": [], "expanded_slices": {}, "color_scheme": "", "label_colors": {}, "shared_label_colors": {}}', "published": False, } From 783168e13fd06794507807f782956b7a41e2ab8f Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Mon, 21 Mar 2022 13:09:38 +0000 Subject: [PATCH 5/9] chore: remove deprecated celery cli (#19273) * chore: remove deprecated celery cli * remove configs and UPDATING --- UPDATING.md | 1 + superset/cli/celery.py | 80 ------------------------------------------ superset/config.py | 2 -- 3 files changed, 1 insertion(+), 82 deletions(-) delete mode 100755 superset/cli/celery.py diff --git a/UPDATING.md b/UPDATING.md index 953d2c34c400c..64e22761335be 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -29,6 +29,7 @@ assists people when migrating to a new version. ### Breaking Changes +- [19273](https://github.com/apache/superset/pull/19273): The `SUPERSET_CELERY_WORKERS` and `SUPERSET_WORKERS` config keys has been removed. Configure celery directly using `CELERY_CONFIG` on Superset - [19231](https://github.com/apache/superset/pull/19231): The `ENABLE_REACT_CRUD_VIEWS` feature flag has been removed (permanently enabled). Any deployments which had set this flag to false will need to verify that the React views support their use case. - [17556](https://github.com/apache/superset/pull/17556): Bumps mysqlclient from v1 to v2 - [19113](https://github.com/apache/superset/pull/19113): The `ENABLE_JAVASCRIPT_CONTROLS` setting has moved from app config to a feature flag. Any deployments who overrode this setting will now need to override the feature flag from here onward. diff --git a/superset/cli/celery.py b/superset/cli/celery.py deleted file mode 100755 index a0373573e8825..0000000000000 --- a/superset/cli/celery.py +++ /dev/null @@ -1,80 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. -import logging -from subprocess import Popen - -import click -from colorama import Fore -from flask.cli import with_appcontext - -from superset import app -from superset.extensions import celery_app - -logger = logging.getLogger(__name__) - - -@click.command() -@with_appcontext -@click.option( - "--workers", "-w", type=int, help="Number of celery server workers to fire up", -) -def worker(workers: int) -> None: - """Starts a Superset worker for async SQL query execution.""" - logger.info( - "The 'superset worker' command is deprecated. Please use the 'celery " - "worker' command instead." - ) - if workers: - celery_app.conf.update(CELERYD_CONCURRENCY=workers) - elif app.config["SUPERSET_CELERY_WORKERS"]: - celery_app.conf.update( - CELERYD_CONCURRENCY=app.config["SUPERSET_CELERY_WORKERS"] - ) - - local_worker = celery_app.Worker(optimization="fair") - local_worker.start() - - -@click.command() -@with_appcontext -@click.option( - "-p", "--port", default="5555", help="Port on which to start the Flower process", -) -@click.option( - "-a", "--address", default="localhost", help="Address on which to run the service", -) -def flower(port: int, address: str) -> None: - """Runs a Celery Flower web server - - Celery Flower is a UI to monitor the Celery operation on a given - broker""" - broker_url = celery_app.conf.BROKER_URL - cmd = ( - "celery flower " - f"--broker={broker_url} " - f"--port={port} " - f"--address={address} " - ) - logger.info( - "The 'superset flower' command is deprecated. Please use the 'celery " - "flower' command instead." - ) - print(Fore.GREEN + "Starting a Celery Flower instance") - print(Fore.BLUE + "-=" * 40) - print(Fore.YELLOW + cmd) - print(Fore.BLUE + "-=" * 40) - Popen(cmd, shell=True).wait() # pylint: disable=consider-using-with diff --git a/superset/config.py b/superset/config.py index 86e35be718702..c8c0a0053141b 100644 --- a/superset/config.py +++ b/superset/config.py @@ -140,8 +140,6 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: SAMPLES_ROW_LIMIT = 1000 # max rows retrieved by filter select auto complete FILTER_SELECT_ROW_LIMIT = 10000 -SUPERSET_WORKERS = 2 # deprecated -SUPERSET_CELERY_WORKERS = 32 # deprecated SUPERSET_WEBSERVER_PROTOCOL = "http" SUPERSET_WEBSERVER_ADDRESS = "0.0.0.0" From d215cbcdc8df85e5c5600cffc8bc7c337f45dee9 Mon Sep 17 00:00:00 2001 From: prassanna-helixsense-com <78132595+prassanna-helixsense-com@users.noreply.github.com> Date: Mon, 21 Mar 2022 19:14:29 +0530 Subject: [PATCH 6/9] Update README.md (#19270) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3ecc921e58a4b..40ef7bb77bc2e 100644 --- a/README.md +++ b/README.md @@ -47,7 +47,7 @@ A modern, enterprise-ready business intelligence web application. ## Why Superset? -Superset is a modern data exploration and data visualization platform. Superset can replace or augment proprietary business intelligence tools for many teams. +Superset is a modern data exploration and data visualization platform. Superset can replace or augment proprietary business intelligence tools for many teams. Superset integrates well with a variety of data sources. Superset provides: From af91a136701401ab52e68fa5946d618f0d78e530 Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Mon, 21 Mar 2022 14:57:38 +0000 Subject: [PATCH 7/9] chore: remove PUBLIC_ROLE_LIKE_GAMMA deprecated config key (#19274) --- UPDATING.md | 1 + superset/security/manager.py | 6 ------ superset/views/core.py | 2 +- 3 files changed, 2 insertions(+), 7 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 64e22761335be..f9ffadfae7185 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -29,6 +29,7 @@ assists people when migrating to a new version. ### Breaking Changes +- [19274](https://github.com/apache/superset/pull/19274): The `PUBLIC_ROLE_LIKE_GAMMA` config key has been removed, set `PUBLIC_ROLE_LIKE` = "Gamma" to have the same functionality. - [19273](https://github.com/apache/superset/pull/19273): The `SUPERSET_CELERY_WORKERS` and `SUPERSET_WORKERS` config keys has been removed. Configure celery directly using `CELERY_CONFIG` on Superset - [19231](https://github.com/apache/superset/pull/19231): The `ENABLE_REACT_CRUD_VIEWS` feature flag has been removed (permanently enabled). Any deployments which had set this flag to false will need to verify that the React views support their use case. - [17556](https://github.com/apache/superset/pull/17556): Bumps mysqlclient from v1 to v2 diff --git a/superset/security/manager.py b/superset/security/manager.py index eb068c81fbb11..275c77a41cd20 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -726,12 +726,6 @@ def sync_role_definitions(self) -> None: self.auth_role_public, merge=True, ) - if current_app.config.get("PUBLIC_ROLE_LIKE_GAMMA", False): - logger.warning( - "The config `PUBLIC_ROLE_LIKE_GAMMA` is deprecated and will be removed " - "in Superset 1.0. Please use `PUBLIC_ROLE_LIKE` instead." - ) - self.copy_role("Gamma", self.auth_role_public, merge=True) self.create_missing_perms() diff --git a/superset/views/core.py b/superset/views/core.py index 50a56569c547c..5959584f1c2d3 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -2792,7 +2792,7 @@ def show_traceback(self) -> FlaskResponse: # pylint: disable=no-self-use def welcome(self) -> FlaskResponse: """Personalized welcome page""" if not g.user or not g.user.get_id(): - if conf.get("PUBLIC_ROLE_LIKE_GAMMA", False) or conf["PUBLIC_ROLE_LIKE"]: + if conf["PUBLIC_ROLE_LIKE"]: return self.render_template("superset/public_welcome.html") return redirect(appbuilder.get_url_for_login) From c07a707eaba3bb4db3061728a3bcc8636e43ffb1 Mon Sep 17 00:00:00 2001 From: PApostol <50751110+PApostol@users.noreply.github.com> Date: Mon, 21 Mar 2022 16:01:57 +0000 Subject: [PATCH 8/9] Various docstring fixes (#18221) --- superset/utils/mock_data.py | 3 ++- superset/views/utils.py | 1 + tests/integration_tests/fixtures/query_context.py | 2 -- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/superset/utils/mock_data.py b/superset/utils/mock_data.py index e51bb39e20ff2..1c6515804b7fa 100644 --- a/superset/utils/mock_data.py +++ b/superset/utils/mock_data.py @@ -176,7 +176,7 @@ def add_data( If the table already exists `columns` can be `None`. :param Optional[List[ColumnInfo]] columns: list of column names and types to create - :param int run_nows: how many rows to generate and insert + :param int num_rows: how many rows to generate and insert :param str table_name: name of table, will be created if it doesn't exist :param bool append: if the table already exists, append data or replace? """ @@ -239,6 +239,7 @@ def add_sample_rows( ) -> Iterator[Model]: """ Add entities of a given model. + :param Session session: an SQLAlchemy session :param Model model: a Superset/FAB model :param int count: how many entities to generate and insert """ diff --git a/superset/views/utils.py b/superset/views/utils.py index 62639174f647e..19c9a2eaf05af 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -279,6 +279,7 @@ def apply_display_max_row_limit( metadata. :param sql_results: The results of a sql query from sql_lab.get_sql_results + :param rows: The number of rows to apply a limit to :returns: The mutated sql_results structure """ diff --git a/tests/integration_tests/fixtures/query_context.py b/tests/integration_tests/fixtures/query_context.py index e8a3118bf5db0..00a3036e01c25 100644 --- a/tests/integration_tests/fixtures/query_context.py +++ b/tests/integration_tests/fixtures/query_context.py @@ -37,8 +37,6 @@ def get_query_context( generated by the "Boy Name Cloud" chart in the examples. :param query_name: name of an example query, which is always in the format of `datasource_name[:test_case_name]`, where `:test_case_name` is optional. - :param datasource_id: id of datasource to query. - :param datasource_type: type of datasource to query. :param add_postprocessing_operations: Add post-processing operations to QueryObject :param add_time_offsets: Add time offsets to QueryObject(advanced analytics) :param form_data: chart metadata From 88029e21b6068f845d806cfc10d478a5d972ffa5 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Mon, 21 Mar 2022 09:43:51 -0700 Subject: [PATCH 9/9] fix dataset update table (#19269) --- superset/connectors/sqla/models.py | 272 +++++++++++++---------- tests/unit_tests/datasets/test_models.py | 81 ++++++- 2 files changed, 225 insertions(+), 128 deletions(-) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 62ae8c9ebaf95..bbd1b5d84dad7 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1863,11 +1863,20 @@ def update_table( # pylint: disable=unused-argument session.execute(update(SqlaTable).where(SqlaTable.id == target.table.id)) - # update ``Column`` model as well dataset = ( - session.query(NewDataset).filter_by(sqlatable_id=target.table.id).one() + session.query(NewDataset) + .filter_by(sqlatable_id=target.table.id) + .one_or_none() ) + if not dataset: + # if dataset is not found create a new copy + # of the dataset instead of updating the existing + + SqlaTable.write_shadow_dataset(target.table, database, session) + return + + # update ``Column`` model as well if isinstance(target, TableColumn): columns = [ column @@ -1923,7 +1932,7 @@ def update_table( # pylint: disable=unused-argument column.extra_json = json.dumps(extra_json) if extra_json else None @staticmethod - def after_insert( # pylint: disable=too-many-locals + def after_insert( mapper: Mapper, connection: Connection, target: "SqlaTable", ) -> None: """ @@ -1938,135 +1947,18 @@ def after_insert( # pylint: disable=too-many-locals For more context: https://github.com/apache/superset/issues/14909 """ + session = inspect(target).session # set permissions security_manager.set_perm(mapper, connection, target) - session = inspect(target).session - # get DB-specific conditional quoter for expressions that point to columns or # table names database = ( target.database or session.query(Database).filter_by(id=target.database_id).one() ) - engine = database.get_sqla_engine(schema=target.schema) - conditional_quote = engine.dialect.identifier_preparer.quote - - # create columns - columns = [] - for column in target.columns: - # ``is_active`` might be ``None`` at this point, but it defaults to ``True``. - if column.is_active is False: - continue - - extra_json = json.loads(column.extra or "{}") - for attr in {"groupby", "filterable", "verbose_name", "python_date_format"}: - value = getattr(column, attr) - if value: - extra_json[attr] = value - - columns.append( - NewColumn( - name=column.column_name, - type=column.type or "Unknown", - expression=column.expression - or conditional_quote(column.column_name), - description=column.description, - is_temporal=column.is_dttm, - is_aggregation=False, - is_physical=column.expression is None, - is_spatial=False, - is_partition=False, - is_increase_desired=True, - extra_json=json.dumps(extra_json) if extra_json else None, - is_managed_externally=target.is_managed_externally, - external_url=target.external_url, - ), - ) - - # create metrics - for metric in target.metrics: - extra_json = json.loads(metric.extra or "{}") - for attr in {"verbose_name", "metric_type", "d3format"}: - value = getattr(metric, attr) - if value: - extra_json[attr] = value - - is_additive = ( - metric.metric_type - and metric.metric_type.lower() in ADDITIVE_METRIC_TYPES - ) - - columns.append( - NewColumn( - name=metric.metric_name, - type="Unknown", # figuring this out would require a type inferrer - expression=metric.expression, - warning_text=metric.warning_text, - description=metric.description, - is_aggregation=True, - is_additive=is_additive, - is_physical=False, - is_spatial=False, - is_partition=False, - is_increase_desired=True, - extra_json=json.dumps(extra_json) if extra_json else None, - is_managed_externally=target.is_managed_externally, - external_url=target.external_url, - ), - ) - - # physical dataset - tables = [] - if target.sql is None: - physical_columns = [column for column in columns if column.is_physical] - - # create table - table = NewTable( - name=target.table_name, - schema=target.schema, - catalog=None, # currently not supported - database_id=target.database_id, - columns=physical_columns, - is_managed_externally=target.is_managed_externally, - external_url=target.external_url, - ) - tables.append(table) - # virtual dataset - else: - # mark all columns as virtual (not physical) - for column in columns: - column.is_physical = False - - # find referenced tables - parsed = ParsedQuery(target.sql) - referenced_tables = parsed.tables - - # predicate for finding the referenced tables - predicate = or_( - *[ - and_( - NewTable.schema == (table.schema or target.schema), - NewTable.name == table.table, - ) - for table in referenced_tables - ] - ) - tables = session.query(NewTable).filter(predicate).all() - - # create the new dataset - dataset = NewDataset( - sqlatable_id=target.id, - name=target.table_name, - expression=target.sql or conditional_quote(target.table_name), - tables=tables, - columns=columns, - is_physical=target.sql is None, - is_managed_externally=target.is_managed_externally, - external_url=target.external_url, - ) - session.add(dataset) + SqlaTable.write_shadow_dataset(target, database, session) @staticmethod def after_delete( # pylint: disable=unused-argument @@ -2301,6 +2193,142 @@ def after_update( # pylint: disable=too-many-branches, too-many-locals, too-man dataset.expression = target.sql or conditional_quote(target.table_name) dataset.is_physical = target.sql is None + @staticmethod + def write_shadow_dataset( # pylint: disable=too-many-locals + dataset: "SqlaTable", database: Database, session: Session + ) -> None: + """ + Shadow write the dataset to new models. + + The ``SqlaTable`` model is currently being migrated to two new models, ``Table`` + and ``Dataset``. In the first phase of the migration the new models are populated + whenever ``SqlaTable`` is modified (created, updated, or deleted). + + In the second phase of the migration reads will be done from the new models. + Finally, in the third phase of the migration the old models will be removed. + + For more context: https://github.com/apache/superset/issues/14909 + """ + + engine = database.get_sqla_engine(schema=dataset.schema) + conditional_quote = engine.dialect.identifier_preparer.quote + + # create columns + columns = [] + for column in dataset.columns: + # ``is_active`` might be ``None`` at this point, but it defaults to ``True``. + if column.is_active is False: + continue + + extra_json = json.loads(column.extra or "{}") + for attr in {"groupby", "filterable", "verbose_name", "python_date_format"}: + value = getattr(column, attr) + if value: + extra_json[attr] = value + + columns.append( + NewColumn( + name=column.column_name, + type=column.type or "Unknown", + expression=column.expression + or conditional_quote(column.column_name), + description=column.description, + is_temporal=column.is_dttm, + is_aggregation=False, + is_physical=column.expression is None, + is_spatial=False, + is_partition=False, + is_increase_desired=True, + extra_json=json.dumps(extra_json) if extra_json else None, + is_managed_externally=dataset.is_managed_externally, + external_url=dataset.external_url, + ), + ) + + # create metrics + for metric in dataset.metrics: + extra_json = json.loads(metric.extra or "{}") + for attr in {"verbose_name", "metric_type", "d3format"}: + value = getattr(metric, attr) + if value: + extra_json[attr] = value + + is_additive = ( + metric.metric_type + and metric.metric_type.lower() in ADDITIVE_METRIC_TYPES + ) + + columns.append( + NewColumn( + name=metric.metric_name, + type="Unknown", # figuring this out would require a type inferrer + expression=metric.expression, + warning_text=metric.warning_text, + description=metric.description, + is_aggregation=True, + is_additive=is_additive, + is_physical=False, + is_spatial=False, + is_partition=False, + is_increase_desired=True, + extra_json=json.dumps(extra_json) if extra_json else None, + is_managed_externally=dataset.is_managed_externally, + external_url=dataset.external_url, + ), + ) + + # physical dataset + tables = [] + if dataset.sql is None: + physical_columns = [column for column in columns if column.is_physical] + + # create table + table = NewTable( + name=dataset.table_name, + schema=dataset.schema, + catalog=None, # currently not supported + database_id=dataset.database_id, + columns=physical_columns, + is_managed_externally=dataset.is_managed_externally, + external_url=dataset.external_url, + ) + tables.append(table) + + # virtual dataset + else: + # mark all columns as virtual (not physical) + for column in columns: + column.is_physical = False + + # find referenced tables + parsed = ParsedQuery(dataset.sql) + referenced_tables = parsed.tables + + # predicate for finding the referenced tables + predicate = or_( + *[ + and_( + NewTable.schema == (table.schema or dataset.schema), + NewTable.name == table.table, + ) + for table in referenced_tables + ] + ) + tables = session.query(NewTable).filter(predicate).all() + + # create the new dataset + new_dataset = NewDataset( + sqlatable_id=dataset.id, + name=dataset.table_name, + expression=dataset.sql or conditional_quote(dataset.table_name), + tables=tables, + columns=columns, + is_physical=dataset.sql is None, + is_managed_externally=dataset.is_managed_externally, + external_url=dataset.external_url, + ) + session.add(new_dataset) + sa.event.listen(SqlaTable, "before_update", SqlaTable.before_update) sa.event.listen(SqlaTable, "after_insert", SqlaTable.after_insert) diff --git a/tests/unit_tests/datasets/test_models.py b/tests/unit_tests/datasets/test_models.py index eab0a8aa28288..095b502760912 100644 --- a/tests/unit_tests/datasets/test_models.py +++ b/tests/unit_tests/datasets/test_models.py @@ -980,9 +980,9 @@ def test_update_sqlatable_schema( sqla_table.schema = "new_schema" session.flush() - dataset = session.query(Dataset).one() - assert dataset.tables[0].schema == "new_schema" - assert dataset.tables[0].id == 2 + new_dataset = session.query(Dataset).one() + assert new_dataset.tables[0].schema == "new_schema" + assert new_dataset.tables[0].id == 2 def test_update_sqlatable_metric( @@ -1098,9 +1098,9 @@ def test_update_virtual_sqlatable_references( session.flush() # check that new dataset has both tables - dataset = session.query(Dataset).one() - assert dataset.tables == [table1, table2] - assert dataset.expression == "SELECT a, b FROM table_a JOIN table_b" + new_dataset = session.query(Dataset).one() + assert new_dataset.tables == [table1, table2] + assert new_dataset.expression == "SELECT a, b FROM table_a JOIN table_b" def test_quote_expressions(app_context: None, session: Session) -> None: @@ -1242,3 +1242,72 @@ def test_update_physical_sqlatable( # check that dataset points to the original table assert dataset.tables[0].database_id == 1 + + +def test_update_physical_sqlatable_no_dataset( + mocker: MockFixture, app_context: None, session: Session +) -> None: + """ + Test updating the table on a physical dataset that it creates + a new dataset if one didn't already exist. + + When updating the table on a physical dataset by pointing it somewhere else (change + in database ID, schema, or table name) we should point the ``Dataset`` to an + existing ``Table`` if possible, and create a new one otherwise. + """ + # patch session + mocker.patch( + "superset.security.SupersetSecurityManager.get_session", return_value=session + ) + mocker.patch("superset.datasets.dao.db.session", session) + + from superset.columns.models import Column + from superset.connectors.sqla.models import SqlaTable, TableColumn + from superset.datasets.models import Dataset + from superset.models.core import Database + from superset.tables.models import Table + from superset.tables.schemas import TableSchema + + engine = session.get_bind() + Dataset.metadata.create_all(engine) # pylint: disable=no-member + + columns = [ + TableColumn(column_name="a", type="INTEGER"), + ] + + sqla_table = SqlaTable( + table_name="old_dataset", + columns=columns, + metrics=[], + database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"), + ) + session.add(sqla_table) + session.flush() + + # check that the table was created + table = session.query(Table).one() + assert table.id == 1 + + dataset = session.query(Dataset).one() + assert dataset.tables == [table] + + # point ``SqlaTable`` to a different database + new_database = Database( + database_name="my_other_database", sqlalchemy_uri="sqlite://" + ) + session.add(new_database) + session.flush() + sqla_table.database = new_database + session.flush() + + new_dataset = session.query(Dataset).one() + + # check that dataset now points to the new table + assert new_dataset.tables[0].database_id == 2 + + # point ``SqlaTable`` back + sqla_table.database_id = 1 + session.flush() + + # check that dataset points to the original table + assert new_dataset.tables[0].database_id == 1