From 72cd9dffa3257b8f5a26dc4bbf25c8b62328e42c Mon Sep 17 00:00:00 2001 From: Vitor Avila <96086495+Vitor-Avila@users.noreply.github.com> Date: Thu, 8 May 2025 15:22:25 -0300 Subject: [PATCH 1/5] fix: Persist catalog change during dataset update + validation fixes (#33384) --- .../components/Datasource/DatasourceModal.tsx | 142 ++++--- .../src/features/datasets/types.ts | 1 + superset/commands/dataset/exceptions.py | 21 +- superset/commands/dataset/update.py | 89 +++-- tests/integration_tests/datasets/api_tests.py | 377 ++++++++++++++---- .../{test_update.py => update_test.py} | 157 +++++--- 6 files changed, 542 insertions(+), 245 deletions(-) rename tests/unit_tests/commands/dataset/{test_update.py => update_test.py} (72%) diff --git a/superset-frontend/src/components/Datasource/DatasourceModal.tsx b/superset-frontend/src/components/Datasource/DatasourceModal.tsx index 1055e51f444..51a60e11bef 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal.tsx @@ -120,71 +120,83 @@ const DatasourceModal: FunctionComponent = ({ const [isEditing, setIsEditing] = useState(false); const dialog = useRef(null); const [modal, contextHolder] = Modal.useModal(); - const buildPayload = (datasource: Record) => ({ - table_name: datasource.table_name, - database_id: datasource.database?.id, - sql: datasource.sql, - filter_select_enabled: datasource.filter_select_enabled, - fetch_values_predicate: datasource.fetch_values_predicate, - schema: - datasource.tableSelector?.schema || - datasource.databaseSelector?.schema || - datasource.schema, - description: datasource.description, - main_dttm_col: datasource.main_dttm_col, - normalize_columns: datasource.normalize_columns, - always_filter_main_dttm: datasource.always_filter_main_dttm, - offset: datasource.offset, - default_endpoint: datasource.default_endpoint, - cache_timeout: - datasource.cache_timeout === '' ? null : datasource.cache_timeout, - is_sqllab_view: datasource.is_sqllab_view, - template_params: datasource.template_params, - extra: datasource.extra, - is_managed_externally: datasource.is_managed_externally, - external_url: datasource.external_url, - metrics: datasource?.metrics?.map((metric: DatasetObject['metrics'][0]) => { - const metricBody: any = { - expression: metric.expression, - description: metric.description, - metric_name: metric.metric_name, - metric_type: metric.metric_type, - d3format: metric.d3format || null, - currency: !isDefined(metric.currency) - ? null - : JSON.stringify(metric.currency), - verbose_name: metric.verbose_name, - warning_text: metric.warning_text, - uuid: metric.uuid, - extra: buildExtraJsonObject(metric), - }; - if (!Number.isNaN(Number(metric.id))) { - metricBody.id = metric.id; - } - return metricBody; - }), - columns: datasource?.columns?.map( - (column: DatasetObject['columns'][0]) => ({ - id: typeof column.id === 'number' ? column.id : undefined, - column_name: column.column_name, - type: column.type, - advanced_data_type: column.advanced_data_type, - verbose_name: column.verbose_name, - description: column.description, - expression: column.expression, - filterable: column.filterable, - groupby: column.groupby, - is_active: column.is_active, - is_dttm: column.is_dttm, - python_date_format: column.python_date_format || null, - uuid: column.uuid, - extra: buildExtraJsonObject(column), - }), - ), - owners: datasource.owners.map( - (o: Record) => o.value || o.id, - ), - }); + const buildPayload = (datasource: Record) => { + const payload: Record = { + table_name: datasource.table_name, + database_id: datasource.database?.id, + sql: datasource.sql, + filter_select_enabled: datasource.filter_select_enabled, + fetch_values_predicate: datasource.fetch_values_predicate, + schema: + datasource.tableSelector?.schema || + datasource.databaseSelector?.schema || + datasource.schema, + description: datasource.description, + main_dttm_col: datasource.main_dttm_col, + normalize_columns: datasource.normalize_columns, + always_filter_main_dttm: datasource.always_filter_main_dttm, + offset: datasource.offset, + default_endpoint: datasource.default_endpoint, + cache_timeout: + datasource.cache_timeout === '' ? null : datasource.cache_timeout, + is_sqllab_view: datasource.is_sqllab_view, + template_params: datasource.template_params, + extra: datasource.extra, + is_managed_externally: datasource.is_managed_externally, + external_url: datasource.external_url, + metrics: datasource?.metrics?.map( + (metric: DatasetObject['metrics'][0]) => { + const metricBody: any = { + expression: metric.expression, + description: metric.description, + metric_name: metric.metric_name, + metric_type: metric.metric_type, + d3format: metric.d3format || null, + currency: !isDefined(metric.currency) + ? null + : JSON.stringify(metric.currency), + verbose_name: metric.verbose_name, + warning_text: metric.warning_text, + uuid: metric.uuid, + extra: buildExtraJsonObject(metric), + }; + if (!Number.isNaN(Number(metric.id))) { + metricBody.id = metric.id; + } + return metricBody; + }, + ), + columns: datasource?.columns?.map( + (column: DatasetObject['columns'][0]) => ({ + id: typeof column.id === 'number' ? column.id : undefined, + column_name: column.column_name, + type: column.type, + advanced_data_type: column.advanced_data_type, + verbose_name: column.verbose_name, + description: column.description, + expression: column.expression, + filterable: column.filterable, + groupby: column.groupby, + is_active: column.is_active, + is_dttm: column.is_dttm, + python_date_format: column.python_date_format || null, + uuid: column.uuid, + extra: buildExtraJsonObject(column), + }), + ), + owners: datasource.owners.map( + (o: Record) => o.value || o.id, + ), + }; + // Handle catalog based on database's allow_multi_catalog setting + // If multi-catalog is disabled, don't include catalog in payload + // The backend will use the default catalog + // If multi-catalog is enabled, include the selected catalog + if (datasource.database?.allow_multi_catalog) { + payload.catalog = datasource.catalog; + } + return payload; + }; const onConfirmSave = async () => { // Pull out extra fields into the extra object setIsSaving(true); diff --git a/superset-frontend/src/features/datasets/types.ts b/superset-frontend/src/features/datasets/types.ts index d343ad153dc..3e91e8effa7 100644 --- a/superset-frontend/src/features/datasets/types.ts +++ b/superset-frontend/src/features/datasets/types.ts @@ -62,6 +62,7 @@ export type DatasetObject = { filter_select_enabled?: boolean; fetch_values_predicate?: string; schema?: string; + catalog?: string; description: string | null; main_dttm_col: string; offset?: number; diff --git a/superset/commands/dataset/exceptions.py b/superset/commands/dataset/exceptions.py index 04afc4fc9b0..be82b7c88f9 100644 --- a/superset/commands/dataset/exceptions.py +++ b/superset/commands/dataset/exceptions.py @@ -33,6 +33,18 @@ def get_dataset_exist_error_msg(table: Table) -> str: return _("Dataset %(table)s already exists", table=table) +class MultiCatalogDisabledValidationError(ValidationError): + """ + Validation error for using a non-default catalog when multi-catalog is disabled + """ + + def __init__(self) -> None: + super().__init__( + [_("Only the default catalog is supported for this connection")], + field_name="catalog", + ) + + class DatabaseNotFoundValidationError(ValidationError): """ Marshmallow validation error for database does not exist @@ -42,15 +54,6 @@ class DatabaseNotFoundValidationError(ValidationError): super().__init__([_("Database does not exist")], field_name="database") -class DatabaseChangeValidationError(ValidationError): - """ - Marshmallow validation error database changes are not allowed on update - """ - - def __init__(self) -> None: - super().__init__([_("Database not allowed to change")], field_name="database") - - class DatasetExistsValidationError(ValidationError): """ Marshmallow validation error for dataset already exists diff --git a/superset/commands/dataset/update.py b/superset/commands/dataset/update.py index 7f6134d20a0..0f0e8f30ecc 100644 --- a/superset/commands/dataset/update.py +++ b/superset/commands/dataset/update.py @@ -14,6 +14,8 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from __future__ import annotations + import logging from collections import Counter from functools import partial @@ -26,7 +28,7 @@ from sqlalchemy.exc import SQLAlchemyError from superset import is_feature_enabled, security_manager from superset.commands.base import BaseCommand, UpdateMixin from superset.commands.dataset.exceptions import ( - DatabaseChangeValidationError, + DatabaseNotFoundValidationError, DatasetColumnNotFoundValidationError, DatasetColumnsDuplicateValidationError, DatasetColumnsExistsValidationError, @@ -38,11 +40,13 @@ from superset.commands.dataset.exceptions import ( DatasetMetricsNotFoundValidationError, DatasetNotFoundError, DatasetUpdateFailedError, + MultiCatalogDisabledValidationError, ) from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn from superset.daos.dataset import DatasetDAO from superset.datasets.schemas import FolderSchema from superset.exceptions import SupersetSecurityException +from superset.models.core import Database from superset.sql_parse import Table from superset.utils.decorators import on_error, transaction @@ -86,38 +90,12 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand): if not self._model: raise DatasetNotFoundError() - # Check ownership + # Check permission to update the dataset try: security_manager.raise_for_ownership(self._model) except SupersetSecurityException as ex: raise DatasetForbiddenError() from ex - database_id = self._properties.get("database") - - catalog = self._properties.get("catalog") - if not catalog: - catalog = self._properties["catalog"] = ( - self._model.database.get_default_catalog() - ) - - table = Table( - self._properties.get("table_name"), # type: ignore - self._properties.get("schema"), - catalog, - ) - - # Validate uniqueness - if not DatasetDAO.validate_update_uniqueness( - self._model.database, - table, - self._model_id, - ): - exceptions.append(DatasetExistsValidationError(table)) - - # Validate/Populate database not allowed to change - if database_id and database_id != self._model: - exceptions.append(DatabaseChangeValidationError()) - # Validate/Populate owner try: owners = self.compute_owners( @@ -128,15 +106,68 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand): except ValidationError as ex: exceptions.append(ex) + self._validate_dataset_source(exceptions) self._validate_semantics(exceptions) if exceptions: raise DatasetInvalidError(exceptions=exceptions) + def _validate_dataset_source(self, exceptions: list[ValidationError]) -> None: + # we know we have a valid model + self._model = cast(SqlaTable, self._model) + database_id = self._properties.pop("database_id", None) + catalog = self._properties.get("catalog") + new_db_connection: Database | None = None + + if database_id and database_id != self._model.database.id: + if new_db_connection := DatasetDAO.get_database_by_id(database_id): + self._properties["database"] = new_db_connection + else: + exceptions.append(DatabaseNotFoundValidationError()) + db = new_db_connection or self._model.database + default_catalog = db.get_default_catalog() + + # If multi-catalog is disabled, and catalog provided is not + # the default one, fail + if ( + "catalog" in self._properties + and catalog != default_catalog + and not db.allow_multi_catalog + ): + exceptions.append(MultiCatalogDisabledValidationError()) + + # If the DB connection does not support multi-catalog, + # use the default catalog + elif not db.allow_multi_catalog: + catalog = self._properties["catalog"] = default_catalog + + # Fallback to using the previous value if not provided + elif "catalog" not in self._properties: + catalog = self._model.catalog + + schema = ( + self._properties["schema"] + if "schema" in self._properties + else self._model.schema + ) + + table = Table( + self._properties.get("table_name", self._model.table_name), + schema, + catalog, + ) + + # Validate uniqueness + if not DatasetDAO.validate_update_uniqueness( + db, + table, + self._model_id, + ): + exceptions.append(DatasetExistsValidationError(table)) + def _validate_semantics(self, exceptions: list[ValidationError]) -> None: # we know we have a valid model self._model = cast(SqlaTable, self._model) - if columns := self._properties.get("columns"): self._validate_columns(columns, exceptions) diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index 9990110befe..8ed7ec216ba 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -14,11 +14,10 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""Unit tests for Superset""" +from __future__ import annotations import unittest from io import BytesIO -from typing import Optional from unittest.mock import ANY, patch from zipfile import is_zipfile, ZipFile @@ -70,14 +69,26 @@ from tests.integration_tests.fixtures.importexport import ( class TestDatasetApi(SupersetTestCase): fixture_tables_names = ("ab_permission", "ab_permission_view", "ab_view_menu") fixture_virtual_table_names = ("sql_virtual_dataset_1", "sql_virtual_dataset_2") + items_to_delete: list[SqlaTable | Database | TableColumn] = [] + + def setUp(self): + self.items_to_delete = [] + + def tearDown(self): + for item in self.items_to_delete: + db.session.delete(item) + db.session.commit() + super().tearDown() @staticmethod def insert_dataset( table_name: str, owners: list[int], database: Database, - sql: Optional[str] = None, - schema: Optional[str] = None, + sql: str | None = None, + schema: str | None = None, + catalog: str | None = None, + fetch_metadata: bool = True, ) -> SqlaTable: obj_owners = list() # noqa: C408 for owner in owners: @@ -89,10 +100,12 @@ class TestDatasetApi(SupersetTestCase): owners=obj_owners, database=database, sql=sql, + catalog=catalog, ) db.session.add(table) db.session.commit() - table.fetch_metadata() + if fetch_metadata: + table.fetch_metadata() return table def insert_default_dataset(self): @@ -100,6 +113,16 @@ class TestDatasetApi(SupersetTestCase): "ab_permission", [self.get_user("admin").id], get_main_database() ) + def insert_database(self, name: str, allow_multi_catalog: bool = False) -> Database: + db_connection = Database( + database_name=name, + sqlalchemy_uri=get_example_database().sqlalchemy_uri, + extra=('{"allow_multi_catalog": true}' if allow_multi_catalog else "{}"), + ) + db.session.add(db_connection) + db.session.commit() + return db_connection + def get_fixture_datasets(self) -> list[SqlaTable]: return ( db.session.query(SqlaTable) @@ -315,8 +338,7 @@ class TestDatasetApi(SupersetTestCase): # revert gamma permission gamma_role.permissions.remove(main_db_pvm) - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_get_dataset_related_database_gamma(self): """ @@ -480,8 +502,7 @@ class TestDatasetApi(SupersetTestCase): ], } - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_get_dataset_render_jinja_exceptions(self): """ @@ -547,8 +568,7 @@ class TestDatasetApi(SupersetTestCase): == "Unable to render expression from dataset calculated column." ) - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_get_dataset_distinct_schema(self): """ @@ -618,9 +638,7 @@ class TestDatasetApi(SupersetTestCase): }, ) - for dataset in datasets: - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = datasets def test_get_dataset_distinct_not_allowed(self): """ @@ -647,8 +665,7 @@ class TestDatasetApi(SupersetTestCase): assert response["count"] == 0 assert response["result"] == [] - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_get_dataset_info(self): """ @@ -722,8 +739,7 @@ class TestDatasetApi(SupersetTestCase): ) assert columns[0].expression == "COUNT(*)" - db.session.delete(model) - db.session.commit() + self.items_to_delete = [model] def test_create_dataset_item_normalize(self): """ @@ -749,8 +765,7 @@ class TestDatasetApi(SupersetTestCase): assert model.database_id == table_data["database"] assert model.normalize_columns is True - db.session.delete(model) - db.session.commit() + self.items_to_delete = [model] def test_create_dataset_item_gamma(self): """ @@ -791,8 +806,7 @@ class TestDatasetApi(SupersetTestCase): model = db.session.query(SqlaTable).get(data.get("id")) assert admin in model.owners assert alpha in model.owners - db.session.delete(model) - db.session.commit() + self.items_to_delete = [model] def test_create_dataset_item_owners_invalid(self): """ @@ -839,8 +853,7 @@ class TestDatasetApi(SupersetTestCase): model = db.session.query(SqlaTable).get(data.get("id")) assert admin in model.owners assert alpha in model.owners - db.session.delete(model) - db.session.commit() + self.items_to_delete = [model] @unittest.skip("test is failing stochastically") def test_create_dataset_same_name_different_schema(self): @@ -991,8 +1004,7 @@ class TestDatasetApi(SupersetTestCase): model = db.session.query(SqlaTable).get(dataset.id) assert model.owners == current_owners - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_clear_owner_list(self): """ @@ -1008,8 +1020,7 @@ class TestDatasetApi(SupersetTestCase): model = db.session.query(SqlaTable).get(dataset.id) assert model.owners == [] - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_populate_owner(self): """ @@ -1026,8 +1037,7 @@ class TestDatasetApi(SupersetTestCase): model = db.session.query(SqlaTable).get(dataset.id) assert model.owners == [gamma] - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_item(self): """ @@ -1045,8 +1055,7 @@ class TestDatasetApi(SupersetTestCase): assert model.description == dataset_data["description"] assert model.owners == current_owners - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_item_w_override_columns(self): """ @@ -1082,8 +1091,7 @@ class TestDatasetApi(SupersetTestCase): col.advanced_data_type for col in columns ] - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_item_w_override_columns_same_columns(self): """ @@ -1130,8 +1138,7 @@ class TestDatasetApi(SupersetTestCase): columns = db.session.query(TableColumn).filter_by(table_id=dataset.id).all() assert len(columns) != prev_col_len assert len(columns) == 3 - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_create_column_and_metric(self): """ @@ -1226,8 +1233,7 @@ class TestDatasetApi(SupersetTestCase): assert metrics[1].warning_text == new_metric_data["warning_text"] assert str(metrics[1].uuid) == new_metric_data["uuid"] - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_delete_column(self): """ @@ -1276,8 +1282,7 @@ class TestDatasetApi(SupersetTestCase): assert columns[1].column_name == "name" assert len(columns) == 2 - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_update_column(self): """ @@ -1313,8 +1318,7 @@ class TestDatasetApi(SupersetTestCase): assert columns[0].groupby is False assert columns[0].filterable is False - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_delete_metric(self): """ @@ -1357,8 +1361,7 @@ class TestDatasetApi(SupersetTestCase): metrics = metrics_query.all() assert len(metrics) == 1 - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_update_column_uniqueness(self): """ @@ -1378,8 +1381,7 @@ class TestDatasetApi(SupersetTestCase): "message": {"columns": ["One or more columns already exist"]} } assert data == expected_result - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_update_metric_uniqueness(self): """ @@ -1399,8 +1401,7 @@ class TestDatasetApi(SupersetTestCase): "message": {"metrics": ["One or more metrics already exist"]} } assert data == expected_result - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_update_column_duplicate(self): """ @@ -1425,8 +1426,7 @@ class TestDatasetApi(SupersetTestCase): "message": {"columns": ["One or more columns are duplicated"]} } assert data == expected_result - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_update_metric_duplicate(self): """ @@ -1451,8 +1451,7 @@ class TestDatasetApi(SupersetTestCase): "message": {"metrics": ["One or more metrics are duplicated"]} } assert data == expected_result - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_item_gamma(self): """ @@ -1465,8 +1464,7 @@ class TestDatasetApi(SupersetTestCase): uri = f"api/v1/dataset/{dataset.id}" rv = self.client.put(uri, json=table_data) assert rv.status_code == 403 - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_dataset_get_list_no_username(self): """ @@ -1491,8 +1489,7 @@ class TestDatasetApi(SupersetTestCase): assert current_dataset["description"] == "changed_description" assert "username" not in current_dataset["changed_by"].keys() - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_dataset_get_no_username(self): """ @@ -1512,8 +1509,7 @@ class TestDatasetApi(SupersetTestCase): assert res["description"] == "changed_description" assert "username" not in res["changed_by"].keys() - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_item_not_owned(self): """ @@ -1526,8 +1522,7 @@ class TestDatasetApi(SupersetTestCase): uri = f"api/v1/dataset/{dataset.id}" rv = self.put_assert_metric(uri, table_data, "put") assert rv.status_code == 403 - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_update_dataset_item_owners_invalid(self): """ @@ -1540,8 +1535,7 @@ class TestDatasetApi(SupersetTestCase): uri = f"api/v1/dataset/{dataset.id}" rv = self.put_assert_metric(uri, table_data, "put") assert rv.status_code == 422 - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] @patch("superset.daos.dataset.DatasetDAO.update") def test_update_dataset_sqlalchemy_error(self, mock_dao_update): @@ -1560,8 +1554,7 @@ class TestDatasetApi(SupersetTestCase): assert rv.status_code == 422 assert data == {"message": "Dataset could not be updated."} - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] @with_feature_flags(DATASET_FOLDERS=True) def test_update_dataset_add_folders(self): @@ -1607,7 +1600,6 @@ class TestDatasetApi(SupersetTestCase): uri = f"api/v1/dataset/{dataset.id}" rv = self.put_assert_metric(uri, dataset_data, "put") - print(rv.data.decode("utf-8")) assert rv.status_code == 200 model = db.session.query(SqlaTable).get(dataset.id) @@ -1643,8 +1635,229 @@ class TestDatasetApi(SupersetTestCase): }, ] - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] + + def test_update_dataset_change_db_connection_multi_catalog_disabled(self): + """ + Dataset API: Test changing the DB connection powering the dataset + to a connection with multi-catalog disabled. + """ + self.login(ADMIN_USERNAME) + + db_connection = self.insert_database("db_connection") + new_db_connection = self.insert_database("new_db_connection") + dataset = self.insert_dataset( + table_name="test_dataset", + owners=[], + database=db_connection, + sql="select 1 as one", + schema="test_schema", + catalog="old_default_catalog", + fetch_metadata=False, + ) + + with patch.object( + new_db_connection, "get_default_catalog", return_value="new_default_catalog" + ): + payload = {"database_id": new_db_connection.id} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.put_assert_metric(uri, payload, "put") + assert rv.status_code == 200 + + model = db.session.query(SqlaTable).get(dataset.id) + assert model.database == new_db_connection + # Catalog should have been updated to new connection's default catalog + assert model.catalog == "new_default_catalog" + + self.items_to_delete = [dataset, db_connection, new_db_connection] + + def test_update_dataset_change_db_connection_multi_catalog_enabled(self): + """ + Dataset API: Test changing the DB connection powering the dataset + to a connection with multi-catalog enabled. + """ + self.login(ADMIN_USERNAME) + + db_connection = self.insert_database("db_connection") + new_db_connection = self.insert_database( + "new_db_connection", allow_multi_catalog=True + ) + dataset = self.insert_dataset( + table_name="test_dataset", + owners=[], + database=db_connection, + sql="select 1 as one", + schema="test_schema", + catalog="old_default_catalog", + fetch_metadata=False, + ) + + with patch.object( + new_db_connection, "get_default_catalog", return_value="default" + ): + payload = {"database_id": new_db_connection.id} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.put_assert_metric(uri, payload, "put") + assert rv.status_code == 200 + + model = db.session.query(SqlaTable).get(dataset.id) + assert model.database == new_db_connection + # Catalog was not changed as not provided and multi-catalog is enabled + assert model.catalog == "old_default_catalog" + + self.items_to_delete = [dataset, db_connection, new_db_connection] + + def test_update_dataset_change_db_connection_not_found(self): + """ + Dataset API: Test changing the DB connection powering the dataset + to an invalid DB connection. + """ + self.login(ADMIN_USERNAME) + + dataset = self.insert_default_dataset() + + payload = {"database_id": 1500} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.put_assert_metric(uri, payload, "put") + response = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 422 + assert response["message"] == {"database": ["Database does not exist"]} + + self.items_to_delete = [dataset] + + def test_update_dataset_change_catalog(self): + """ + Dataset API: Test changing the catalog associated with the dataset. + """ + self.login(ADMIN_USERNAME) + + db_connection = self.insert_database("db_connection", allow_multi_catalog=True) + dataset = self.insert_dataset( + table_name="test_dataset", + owners=[], + database=db_connection, + sql="select 1 as one", + schema="test_schema", + catalog="test_catalog", + fetch_metadata=False, + ) + + with patch.object(db_connection, "get_default_catalog", return_value="default"): + payload = {"catalog": "other_catalog"} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.put_assert_metric(uri, payload, "put") + assert rv.status_code == 200 + + model = db.session.query(SqlaTable).get(dataset.id) + assert model.catalog == "other_catalog" + + self.items_to_delete = [dataset, db_connection] + + def test_update_dataset_change_catalog_not_allowed(self): + """ + Dataset API: Test changing the catalog associated with the dataset fails + when multi-catalog is disabled on the DB connection. + """ + self.login(ADMIN_USERNAME) + + db_connection = self.insert_database("db_connection") + dataset = self.insert_dataset( + table_name="test_dataset", + owners=[], + database=db_connection, + sql="select 1 as one", + schema="test_schema", + catalog="test_catalog", + fetch_metadata=False, + ) + + with patch.object(db_connection, "get_default_catalog", return_value="default"): + payload = {"catalog": "other_catalog"} + uri = f"api/v1/dataset/{dataset.id}" + rv = self.put_assert_metric(uri, payload, "put") + response = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 422 + assert response["message"] == { + "catalog": ["Only the default catalog is supported for this connection"] + } + + self.items_to_delete = [dataset, db_connection] + + def test_update_dataset_validate_uniqueness(self): + """ + Dataset API: Test the dataset uniqueness validation takes into + consideration the new database connection. + """ + test_db = get_main_database() + if test_db.backend == "sqlite": + # Skip this test for SQLite as it doesn't support multiple + # schemas. + return + + self.login(ADMIN_USERNAME) + + db_connection = self.insert_database("db_connection") + new_db_connection = self.insert_database("new_db_connection") + first_schema_dataset = self.insert_dataset( + table_name="test_dataset", + owners=[], + database=db_connection, + sql="select 1 as one", + schema="first_schema", + fetch_metadata=False, + ) + second_schema_dataset = self.insert_dataset( + table_name="test_dataset", + owners=[], + database=db_connection, + sql="select 1 as one", + schema="second_schema", + fetch_metadata=False, + ) + new_db_conn_dataset = self.insert_dataset( + table_name="test_dataset", + owners=[], + database=new_db_connection, + sql="select 1 as one", + schema="first_schema", + fetch_metadata=False, + ) + + with patch.object( + db_connection, + "get_default_catalog", + return_value=None, + ): + payload = {"schema": "second_schema"} + uri = f"api/v1/dataset/{first_schema_dataset.id}" + rv = self.put_assert_metric(uri, payload, "put") + response = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 422 + assert response["message"] == { + "table": ["Dataset second_schema.test_dataset already exists"] + } + + with patch.object( + new_db_connection, + "get_default_catalog", + return_value=None, + ): + payload["database_id"] = new_db_connection.id + uri = f"api/v1/dataset/{first_schema_dataset.id}" + rv = self.put_assert_metric(uri, payload, "put") + assert rv.status_code == 200 + + model = db.session.query(SqlaTable).get(first_schema_dataset.id) + assert model.database == new_db_connection + assert model.schema == "second_schema" + + self.items_to_delete = [ + first_schema_dataset, + second_schema_dataset, + new_db_conn_dataset, + new_db_connection, + db_connection, + ] def test_delete_dataset_item(self): """ @@ -1674,8 +1887,7 @@ class TestDatasetApi(SupersetTestCase): uri = f"api/v1/dataset/{dataset.id}" rv = self.delete_assert_metric(uri, "delete") assert rv.status_code == 403 - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_delete_dataset_item_not_authorized(self): """ @@ -1687,8 +1899,7 @@ class TestDatasetApi(SupersetTestCase): uri = f"api/v1/dataset/{dataset.id}" rv = self.client.delete(uri) assert rv.status_code == 403 - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] @patch("superset.daos.dataset.DatasetDAO.delete") def test_delete_dataset_sqlalchemy_error(self, mock_dao_delete): @@ -1705,8 +1916,7 @@ class TestDatasetApi(SupersetTestCase): data = json.loads(rv.data.decode("utf-8")) assert rv.status_code == 422 assert data == {"message": "Datasets could not be deleted."} - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] @pytest.mark.usefixtures("create_datasets") def test_delete_dataset_column(self): @@ -1947,8 +2157,7 @@ class TestDatasetApi(SupersetTestCase): .filter_by(table_id=dataset.id, column_name="id") .one() ) - db.session.delete(id_column) - db.session.commit() + self.items_to_delete = [id_column] self.login(ADMIN_USERNAME) uri = f"api/v1/dataset/{dataset.id}/refresh" @@ -1961,8 +2170,7 @@ class TestDatasetApi(SupersetTestCase): .one() ) assert id_column is not None - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] def test_dataset_item_refresh_not_found(self): """ @@ -1987,8 +2195,7 @@ class TestDatasetApi(SupersetTestCase): rv = self.put_assert_metric(uri, {}, "refresh") assert rv.status_code == 403 - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] @unittest.skip("test is failing stochastically") def test_export_dataset(self): @@ -2250,8 +2457,7 @@ class TestDatasetApi(SupersetTestCase): dataset = ( db.session.query(SqlaTable).filter_by(table_name="birth_names_2").one() ) - db.session.delete(dataset) - db.session.commit() + self.items_to_delete = [dataset] @patch("superset.commands.database.importers.v1.utils.add_permissions") def test_import_dataset_overwrite(self, mock_add_permissions): @@ -2447,8 +2653,7 @@ class TestDatasetApi(SupersetTestCase): response = json.loads(rv.data.decode("utf-8")) assert response.get("count") == 1 - db.session.delete(table_w_certification) - db.session.commit() + self.items_to_delete = [table_w_certification] @pytest.mark.usefixtures("create_virtual_datasets") def test_duplicate_virtual_dataset(self): @@ -2473,8 +2678,7 @@ class TestDatasetApi(SupersetTestCase): assert len(new_dataset.columns) == 2 assert new_dataset.columns[0].column_name == "id" assert new_dataset.columns[1].column_name == "name" - db.session.delete(new_dataset) - db.session.commit() + self.items_to_delete = [new_dataset] @pytest.mark.usefixtures("create_datasets") def test_duplicate_physical_dataset(self): @@ -2604,8 +2808,7 @@ class TestDatasetApi(SupersetTestCase): assert table.template_params == '{"param": 1}' assert table.normalize_columns is False - db.session.delete(table) - db.session.commit() + self.items_to_delete = [table] with examples_db.get_sqla_engine() as engine: engine.execute("DROP TABLE test_create_sqla_table_api") diff --git a/tests/unit_tests/commands/dataset/test_update.py b/tests/unit_tests/commands/dataset/update_test.py similarity index 72% rename from tests/unit_tests/commands/dataset/test_update.py rename to tests/unit_tests/commands/dataset/update_test.py index fa2026b533b..45a1a4160d8 100644 --- a/tests/unit_tests/commands/dataset/test_update.py +++ b/tests/unit_tests/commands/dataset/update_test.py @@ -15,78 +15,125 @@ # specific language governing permissions and limitations # under the License. -from typing import cast -from unittest.mock import MagicMock +from typing import Any, cast import pytest from marshmallow import ValidationError from pytest_mock import MockerFixture -from superset import db -from superset.commands.dataset.exceptions import DatasetInvalidError +from superset.commands.dataset.exceptions import ( + DatabaseNotFoundValidationError, + DatasetExistsValidationError, + DatasetForbiddenError, + DatasetInvalidError, + DatasetNotFoundError, + MultiCatalogDisabledValidationError, +) from superset.commands.dataset.update import UpdateDatasetCommand, validate_folders -from superset.connectors.sqla.models import SqlaTable +from superset.commands.exceptions import OwnersNotFoundValidationError from superset.datasets.schemas import FolderSchema -from superset.models.core import Database +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import SupersetSecurityException from tests.unit_tests.conftest import with_feature_flags -@pytest.mark.usefixture("session") -def test_update_uniqueness_error(mocker: MockerFixture) -> None: +def test_update_dataset_not_found(mocker: MockerFixture) -> None: """ - Test uniqueness validation in dataset update command. + Test updating an unexisting ID raises a `DatasetNotFoundError`. """ - SqlaTable.metadata.create_all(db.session.get_bind()) + mock_dataset_dao = mocker.patch("superset.commands.dataset.update.DatasetDAO") + mock_dataset_dao.find_by_id.return_value = None - # First, make sure session is clean - db.session.rollback() + with pytest.raises(DatasetNotFoundError): + UpdateDatasetCommand(1, {"name": "test"}).run() - try: - # Set up test data - database = Database(database_name="my_db", sqlalchemy_uri="sqlite://") - bar = SqlaTable(table_name="bar", schema="foo", database=database) - baz = SqlaTable(table_name="baz", schema="qux", database=database) - db.session.add_all([database, bar, baz]) - db.session.commit() - # Set up mocks - mock_g = mocker.patch("superset.security.manager.g") - mock_g.user = MagicMock() - mocker.patch( - "superset.views.base.security_manager.can_access_all_datasources", - return_value=True, - ) - mocker.patch( - "superset.commands.dataset.update.security_manager.raise_for_ownership", - return_value=None, - ) - mocker.patch.object(UpdateDatasetCommand, "compute_owners", return_value=[]) +def test_update_dataset_forbidden(mocker: MockerFixture) -> None: + """ + Test try updating a dataset without permission raises a `DatasetForbiddenError`. + """ + mock_dataset_dao = mocker.patch("superset.commands.dataset.update.DatasetDAO") + mock_dataset_dao.find_by_id.return_value = mocker.MagicMock() - # Run the test that should fail - with pytest.raises(DatasetInvalidError): - UpdateDatasetCommand( - bar.id, - { - "table_name": "baz", - "schema": "qux", - }, - ).run() - except Exception: - db.session.rollback() - raise - finally: - # Clean up - this will run even if the test fails - try: - db.session.query(SqlaTable).filter( - SqlaTable.table_name.in_(["bar", "baz"]), - SqlaTable.schema.in_(["foo", "qux"]), - ).delete(synchronize_session=False) - db.session.query(Database).filter(Database.database_name == "my_db").delete( - synchronize_session=False + mocker.patch( + "superset.commands.dataset.update.security_manager.raise_for_ownership", + side_effect=SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.MISSING_OWNERSHIP_ERROR, + message="Sample message", + level=ErrorLevel.ERROR, ) - db.session.commit() - except Exception: - db.session.rollback() + ), + ) + + with pytest.raises(DatasetForbiddenError): + UpdateDatasetCommand(1, {"name": "test"}).run() + + +@pytest.mark.parametrize( + ("payload, exception, error_msg"), + [ + ( + {"database_id": 2}, + DatabaseNotFoundValidationError, + "Database does not exist", + ), + ( + {"catalog": "test"}, + MultiCatalogDisabledValidationError, + "Only the default catalog is supported for this connection", + ), + ( + {"table_name": "table", "schema": "schema"}, + DatasetExistsValidationError, + "Dataset catalog.schema.table already exists", + ), + ( + {"owners": [1]}, + OwnersNotFoundValidationError, + "Owners are invalid", + ), + ], +) +def test_update_validation_errors( + payload: dict[str, Any], + exception: Exception, + error_msg: str, + mocker: MockerFixture, +) -> None: + """ + Test validation errors for the `UpdateDatasetCommand`. + """ + mock_dataset_dao = mocker.patch("superset.commands.dataset.update.DatasetDAO") + mocker.patch( + "superset.commands.dataset.update.security_manager.raise_for_ownership", + ) + mocker.patch("superset.commands.utils.security_manager.is_admin", return_value=True) + mocker.patch( + "superset.commands.utils.security_manager.get_user_by_id", return_value=None + ) + mock_database = mocker.MagicMock() + mock_database.id = 1 + mock_database.get_default_catalog.return_value = "catalog" + mock_database.allow_multi_catalog = False + mock_dataset = mocker.MagicMock() + mock_dataset.database = mock_database + mock_dataset.catalog = "catalog" + mock_dataset_dao.find_by_id.return_value = mock_dataset + + if exception == DatabaseNotFoundValidationError: + mock_dataset_dao.get_database_by_id.return_value = None + else: + mock_dataset_dao.get_database_by_id.return_value = mock_database + + if exception == DatasetExistsValidationError: + mock_dataset_dao.validate_update_uniqueness.return_value = False + else: + mock_dataset_dao.validate_update_uniqueness.return_value = True + + with pytest.raises(DatasetInvalidError) as excinfo: + UpdateDatasetCommand(1, payload).run() + assert any(error_msg in str(exc) for exc in excinfo.value._exceptions) @with_feature_flags(DATASET_FOLDERS=True) From a391ebecca893510defd4df871394f4f9fcb2917 Mon Sep 17 00:00:00 2001 From: Rafael Benitez Date: Fri, 9 May 2025 11:35:59 -0400 Subject: [PATCH 2/5] feat: Run SQL on DataSourceEditor implementation (#33340) --- .../Datasource/DatasourceEditor.jsx | 91 ++++++++++++++++++- .../src/components/Datasource/Field.test.tsx | 15 ++- .../src/components/Datasource/Field.tsx | 63 +++++++++---- .../src/components/Icons/AntdEnhanced.tsx | 2 + superset-frontend/src/database/actions.ts | 68 ++++++++++++++ superset-frontend/src/database/reducers.ts | 56 ++++++++++++ superset-frontend/src/database/types.ts | 57 ++++++++++++ superset-frontend/src/views/store.ts | 3 +- 8 files changed, 331 insertions(+), 24 deletions(-) create mode 100644 superset-frontend/src/database/actions.ts create mode 100644 superset-frontend/src/database/reducers.ts create mode 100644 superset-frontend/src/database/types.ts diff --git a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx index dce962a6ce9..4f21e7ae609 100644 --- a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx @@ -34,6 +34,7 @@ import { t, withTheme, getClientErrorObject, + getExtensionsRegistry, } from '@superset-ui/core'; import { Select, AsyncSelect, Row, Col } from 'src/components'; import { FormLabel } from 'src/components/Form'; @@ -53,10 +54,15 @@ import SpatialControl from 'src/explore/components/controls/SpatialControl'; import withToasts from 'src/components/MessageToasts/withToasts'; import { Icons } from 'src/components/Icons'; import CurrencyControl from 'src/explore/components/controls/CurrencyControl'; +import { executeQuery, resetDatabaseState } from 'src/database/actions'; +import { connect } from 'react-redux'; import CollectionTable from './CollectionTable'; import Fieldset from './Fieldset'; import Field from './Field'; import { fetchSyncedColumns, updateColumns } from './utils'; +import FilterableTable from '../FilterableTable'; + +const extensionsRegistry = getExtensionsRegistry(); const DatasourceContainer = styled.div` .change-warning { @@ -586,6 +592,8 @@ function OwnersSelector({ datasource, onChange }) { /> ); } +const ResultTable = + extensionsRegistry.get('sqleditor.extension.resultTable') ?? FilterableTable; class DatasourceEditor extends PureComponent { constructor(props) { @@ -698,6 +706,23 @@ class DatasourceEditor extends PureComponent { this.validate(this.onChange); } + async onQueryRun() { + this.props.runQuery({ + client_id: this.props.clientId, + database_id: this.state.datasource.database.id, + json: true, + runAsync: false, + catalog: this.state.datasource.catalog, + schema: this.state.datasource.schema, + sql: this.state.datasource.sql, + tmp_table_name: '', + select_as_cta: false, + ctas_method: 'TABLE', + queryLimit: 25, + expand_data: true, + }); + } + tableChangeAndSyncMetadata() { this.validate(() => { this.syncMetadata(); @@ -1078,14 +1103,62 @@ class DatasourceEditor extends PureComponent { } + additionalControl={ +
+ +
+ } + errorMessage={ + this.props.database?.error && t('Error executing query.') + } /> + {this.props.database?.queryResult && ( + col.column_name, + )} + height={100} + expandedColumns={ + this.props.database.queryResult.expandedColumns + } + allowHTML + /> + )} )} @@ -1466,6 +1539,10 @@ class DatasourceEditor extends PureComponent { ); } + + componentWillUnmount() { + this.props.resetQuery(); + } } DatasourceEditor.defaultProps = defaultProps; @@ -1473,4 +1550,14 @@ DatasourceEditor.propTypes = propTypes; const DataSourceComponent = withTheme(DatasourceEditor); -export default withToasts(DataSourceComponent); +const mapDispatchToProps = dispatch => ({ + runQuery: payload => dispatch(executeQuery(payload)), + resetQuery: () => dispatch(resetDatabaseState()), +}); +const mapStateToProps = state => ({ + test: state.queryApi, + database: state.database, +}); +export default withToasts( + connect(mapStateToProps, mapDispatchToProps)(DataSourceComponent), +); diff --git a/superset-frontend/src/components/Datasource/Field.test.tsx b/superset-frontend/src/components/Datasource/Field.test.tsx index 2e696dca8cd..cab68b0c2fc 100644 --- a/superset-frontend/src/components/Datasource/Field.test.tsx +++ b/superset-frontend/src/components/Datasource/Field.test.tsx @@ -29,13 +29,20 @@ const defaultProps = { onChange: jest.fn(), compact: false, inline: false, + additionalControl: ( + + ), }; test('should render', () => { const { container } = render(); expect(container).toBeInTheDocument(); }); - +test('should render with aditional control', () => { + const { getByTestId } = render(); + const additionalControl = getByTestId('mock-text-aditional-control'); + expect(additionalControl).toBeInTheDocument(); +}); test('should call onChange', () => { const { getByTestId } = render(); const textArea = getByTestId('mock-text-control'); @@ -47,3 +54,9 @@ test('should render compact', () => { render(); expect(screen.queryByText(defaultProps.description)).not.toBeInTheDocument(); }); +test('shiuld render error message', () => { + const { getByText } = render( + , + ); + expect(getByText('error message')).toBeInTheDocument(); +}); diff --git a/superset-frontend/src/components/Datasource/Field.tsx b/superset-frontend/src/components/Datasource/Field.tsx index 1232f0208c1..61404d401b4 100644 --- a/superset-frontend/src/components/Datasource/Field.tsx +++ b/superset-frontend/src/components/Datasource/Field.tsx @@ -21,6 +21,7 @@ import { useCallback, ReactNode, ReactElement, cloneElement } from 'react'; import { css, SupersetTheme } from '@superset-ui/core'; import { Tooltip } from 'src/components/Tooltip'; import { FormItem, FormLabel } from 'src/components/Form'; +import { Icons } from 'src/components/Icons'; const formItemInlineCss = css` .ant-form-item-control-input-content { @@ -28,16 +29,17 @@ const formItemInlineCss = css` flex-direction: row; } `; - interface FieldProps { fieldKey: string; value?: V; label: string; description?: ReactNode; control: ReactElement; + additionalControl?: ReactElement; onChange: (fieldKey: string, newValue: V) => void; compact: boolean; inline: boolean; + errorMessage?: string; } export default function Field({ @@ -46,9 +48,11 @@ export default function Field({ label, description = null, control, + additionalControl, onChange = () => {}, compact = false, inline, + errorMessage, }: FieldProps) { const onControlChange = useCallback( newValue => { @@ -62,32 +66,51 @@ export default function Field({ onChange: onControlChange, }); return ( - - {label || fieldKey} - {compact && description && ( - - {/* TODO: Remove fa-icon */} - {/* eslint-disable-next-line icons/no-fa-icons-usage */} - - - )} - +
- {hookedControl} - {!compact && description && ( + {additionalControl} + + {label || fieldKey} + {compact && description && ( + + + + )} + + } + css={inline && formItemInlineCss} + > + {hookedControl} + {!compact && description && ( +
({ + color: theme.colors.grayscale.base, + [inline ? 'marginLeft' : 'marginTop']: theme.gridUnit, + })} + > + {description} +
+ )} + + {errorMessage && (
({ - color: theme.colors.grayscale.base, - [inline ? 'marginLeft' : 'marginTop']: theme.gridUnit, + color: theme.colors.error.base, + marginTop: -16, + fontSize: theme.typography.sizes.s, })} > - {description} + {errorMessage}
)} - +
); } diff --git a/superset-frontend/src/components/Icons/AntdEnhanced.tsx b/superset-frontend/src/components/Icons/AntdEnhanced.tsx index 57eb41c61d6..5b836324bd4 100644 --- a/superset-frontend/src/components/Icons/AntdEnhanced.tsx +++ b/superset-frontend/src/components/Icons/AntdEnhanced.tsx @@ -34,6 +34,7 @@ import { CaretDownOutlined, CaretLeftOutlined, CaretRightOutlined, + CaretRightFilled, CalendarOutlined, CheckOutlined, CheckCircleOutlined, @@ -134,6 +135,7 @@ const AntdIcons = { CaretDownOutlined, CaretLeftOutlined, CaretRightOutlined, + CaretRightFilled, CalendarOutlined, CheckOutlined, CheckCircleOutlined, diff --git a/superset-frontend/src/database/actions.ts b/superset-frontend/src/database/actions.ts new file mode 100644 index 00000000000..ba98d0cd49c --- /dev/null +++ b/superset-frontend/src/database/actions.ts @@ -0,0 +1,68 @@ +/** + * 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 { makeApi } from '@superset-ui/core'; +import { ThunkDispatch } from 'redux-thunk'; +import { AnyAction } from 'redux'; +import { QueryExecutePayload, QueryExecuteResponse } from './types'; + +export const executeQueryApi = makeApi< + QueryExecutePayload, + QueryExecuteResponse +>({ + method: 'POST', + endpoint: '/api/v1/sqllab/execute', +}); + +export function setQueryIsLoading(isLoading: boolean) { + return { + type: 'SET_QUERY_IS_LOADING', + payload: isLoading, + }; +} +export function setQueryResult(queryResult: QueryExecuteResponse) { + return { + type: 'SET_QUERY_RESULT', + payload: queryResult, + }; +} +export function resetDatabaseState() { + return { + type: 'RESET_DATABASE_STATE', + }; +} +export function setQueryError(error: string) { + return { + type: 'SET_QUERY_ERROR', + payload: error, + }; +} +export function executeQuery(payload: QueryExecutePayload) { + return async function (dispatch: ThunkDispatch) { + try { + dispatch(setQueryIsLoading(true)); + const result = await executeQueryApi(payload); + dispatch(setQueryResult(result as QueryExecuteResponse)); + } catch (error) { + dispatch(setQueryError(error.message)); + } finally { + dispatch(setQueryIsLoading(false)); + } + }; +} diff --git a/superset-frontend/src/database/reducers.ts b/superset-frontend/src/database/reducers.ts new file mode 100644 index 00000000000..7541494b839 --- /dev/null +++ b/superset-frontend/src/database/reducers.ts @@ -0,0 +1,56 @@ +/** + * 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 type { QueryAdhocState } from './types'; + +const initialState: QueryAdhocState = { + isLoading: null, + sql: null, + queryResult: null, + error: null, +}; + +export default function databaseReducer( + state: QueryAdhocState = initialState, + action: any, +): QueryAdhocState { + switch (action.type) { + case 'SET_QUERY_IS_LOADING': + return { + ...state, + isLoading: action.payload, + }; + case 'SET_QUERY_RESULT': + return { + ...state, + sql: action.payload.query.sql ?? '', + queryResult: action.payload, + error: null, + }; + case 'SET_QUERY_ERROR': + return { + ...initialState, + error: action.payload, + }; + case 'RESET_DATABASE_STATE': + return initialState; + default: + return state; + } +} diff --git a/superset-frontend/src/database/types.ts b/superset-frontend/src/database/types.ts new file mode 100644 index 00000000000..8abfeca5d68 --- /dev/null +++ b/superset-frontend/src/database/types.ts @@ -0,0 +1,57 @@ +/** + * 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. + */ + +export interface QueryExecutePayload { + client_id: string; + database_id: number; + json: boolean; + runAsync: boolean; + catalog: string | null; + schema: string; + sql: string; + tmp_table_name: string; + select_as_cta: boolean; + ctas_method: string; + queryLimit: number; + expand_data: boolean; +} +export interface Column { + name: string; + type: string; + is_dttm: boolean; + type_generic: number; + is_hidden: boolean; + column_name: string; +} +export interface QueryExecuteResponse { + status: string; + query_id: string; + data: any[]; + columns: Column[]; + selected_columns: Column[]; + expanded_columns: Column[]; + query: any; +} + +export interface QueryAdhocState { + isLoading: boolean | null; + sql: string | null; + queryResult: QueryExecuteResponse | null; + error: string | null; +} diff --git a/superset-frontend/src/views/store.ts b/superset-frontend/src/views/store.ts index 9fcdf53e9d2..6498d4116e7 100644 --- a/superset-frontend/src/views/store.ts +++ b/superset-frontend/src/views/store.ts @@ -38,7 +38,6 @@ import logger from 'src/middleware/loggerMiddleware'; import saveModal from 'src/explore/reducers/saveModalReducer'; import explore from 'src/explore/reducers/exploreReducer'; import exploreDatasources from 'src/explore/reducers/datasourcesReducer'; - import { persistSqlLabStateEnhancer } from 'src/SqlLab/middlewares/persistSqlLabStateEnhancer'; import sqlLabReducer from 'src/SqlLab/reducers/sqlLab'; import getInitialState from 'src/SqlLab/reducers/getInitialState'; @@ -57,6 +56,7 @@ import { AnyDatasourcesAction } from 'src/explore/actions/datasourcesActions'; import { HydrateExplore } from 'src/explore/actions/hydrateExplore'; import getBootstrapData from 'src/utils/getBootstrapData'; import { Dataset } from '@superset-ui/chart-controls'; +import databaseReducer from 'src/database/reducers'; // Some reducers don't do anything, and redux is just used to reference the initial "state". // This may change later, as the client application takes on more responsibilities. @@ -139,6 +139,7 @@ const reducers = { reports, saveModal, explore, + database: databaseReducer, }; /* In some cases the jinja template injects two separate React apps into basic.html From 9e38a0cc29e4026c361f8891e9e3d2d1096c5c9f Mon Sep 17 00:00:00 2001 From: VED PRAKASH KASHYAP Date: Fri, 9 May 2025 22:42:23 +0530 Subject: [PATCH 3/5] docs: fix for role sync issues in case of custom OAuth2 configuration (#30878) --- docs/docs/configuration/configuring-superset.mdx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/docs/configuration/configuring-superset.mdx b/docs/docs/configuration/configuring-superset.mdx index 0fb208d4f8c..1a453adfe32 100644 --- a/docs/docs/configuration/configuring-superset.mdx +++ b/docs/docs/configuration/configuring-superset.mdx @@ -302,6 +302,15 @@ AUTH_USER_REGISTRATION = True AUTH_USER_REGISTRATION_ROLE = "Public" ``` +In case you want to assign the `Admin` role on new user registration, it can be assigned as follows: +```python +AUTH_USER_REGISTRATION_ROLE = "Admin" +``` +If you encounter the [issue](https://github.com/apache/superset/issues/13243) of not being able to list users from the Superset main page settings, although a newly registered user has an `Admin` role, please re-run `superset init` to sync the required permissions. Below is the command to re-run `superset init` using docker compose. +``` +docker-compose exec superset superset init +``` + Then, create a `CustomSsoSecurityManager` that extends `SupersetSecurityManager` and overrides `oauth_user_info`: From 22475e787e3e8a9c9cfcaf82eebce75ff9e63151 Mon Sep 17 00:00:00 2001 From: amaannawab923 Date: Fri, 9 May 2025 22:57:31 +0530 Subject: [PATCH 4/5] feat(Table Chart): Row limit Increase , Backend Sorting , Backend Search , Excel/CSV Improvements (#33357) Co-authored-by: Amaan Nawab --- .../e2e/explore/visualizations/table.test.ts | 211 ++++++++++++++++++ .../src/constants.ts | 1 + .../superset-ui-core/src/validator/index.ts | 1 + .../src/validator/validateServerPagination.ts | 30 +++ .../validateServerPagination.test.ts | 46 ++++ .../src/DataTable/DataTable.tsx | 116 +++++++++- .../src/DataTable/components/GlobalFilter.tsx | 62 ++++- .../components/SearchSelectDropdown.tsx | 53 +++++ .../src/DataTable/types/react-table.d.ts | 8 + .../src/DataTable/utils/externalAPIs.ts | 9 + .../plugin-chart-table/src/TableChart.tsx | 135 +++++++++-- .../plugin-chart-table/src/buildQuery.ts | 70 ++++-- .../plugin-chart-table/src/controlPanel.tsx | 64 +++++- .../plugin-chart-table/src/transformProps.ts | 42 +++- .../plugins/plugin-chart-table/src/types.ts | 26 ++- .../src/components/Chart/ChartRenderer.jsx | 5 + .../src/explore/reducers/exploreReducer.js | 47 ++++ superset/common/query_context_factory.py | 13 +- superset/common/query_object_factory.py | 25 ++- superset/config.py | 4 + superset/utils/core.py | 28 ++- superset/views/base.py | 1 + .../common/test_query_object_factory.py | 14 +- 23 files changed, 934 insertions(+), 77 deletions(-) create mode 100644 superset-frontend/packages/superset-ui-core/src/validator/validateServerPagination.ts create mode 100644 superset-frontend/packages/superset-ui-core/test/validator/validateServerPagination.test.ts create mode 100644 superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx diff --git a/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/table.test.ts b/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/table.test.ts index 603cc7f2c8d..319938b95e7 100644 --- a/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/table.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/table.test.ts @@ -252,4 +252,215 @@ describe('Visualization > Table', () => { }); cy.get('td').contains(/\d*%/); }); + + it('Test row limit with server pagination toggle', () => { + cy.visitChartByParams({ + ...VIZ_DEFAULTS, + metrics: ['count'], + row_limit: 100, + }); + + // Enable server pagination + cy.get('[data-test="server_pagination-header"] div.pull-left').click(); + + // Click row limit control and select high value (200k) + cy.get('div[aria-label="Row limit"]').click(); + + // Type 200000 and press enter to select the option + cy.get('div[aria-label="Row limit"]') + .find('.ant-select-selection-search-input:visible') + .type('200000{enter}'); + + // Verify that there is no error tooltip when server pagination is enabled + cy.get('[data-test="error-tooltip"]').should('not.exist'); + + // Disable server pagination + cy.get('[data-test="server_pagination-header"] div.pull-left').click(); + + // Verify error tooltip appears + cy.get('[data-test="error-tooltip"]').should('be.visible'); + + // Trigger mouseover and verify tooltip text + cy.get('[data-test="error-tooltip"]').trigger('mouseover'); + + // Verify tooltip content + cy.get('.antd5-tooltip-inner').should('be.visible'); + cy.get('.antd5-tooltip-inner').should( + 'contain', + 'Server pagination needs to be enabled for values over', + ); + + // Hide the tooltip by adding display:none style + cy.get('.antd5-tooltip').invoke('attr', 'style', 'display: none'); + + // Enable server pagination again + cy.get('[data-test="server_pagination-header"] div.pull-left').click(); + + cy.get('[data-test="error-tooltip"]').should('not.exist'); + + cy.get('div[aria-label="Row limit"]').click(); + + // Type 1000000 + cy.get('div[aria-label="Row limit"]') + .find('.ant-select-selection-search-input:visible') + .type('1000000'); + + // Wait for 1 second + cy.wait(1000); + + // Press enter + cy.get('div[aria-label="Row limit"]') + .find('.ant-select-selection-search-input:visible') + .type('{enter}'); + + // Wait for error tooltip to appear and verify its content + cy.get('[data-test="error-tooltip"]') + .should('be.visible') + .trigger('mouseover'); + + // Wait for tooltip content and verify + cy.get('.antd5-tooltip-inner').should('exist'); + cy.get('.antd5-tooltip-inner').should('be.visible'); + + // Verify tooltip content separately + cy.get('.antd5-tooltip-inner').should('contain', 'Value cannot exceed'); + }); + + it('Test sorting with server pagination enabled', () => { + cy.visitChartByParams({ + ...VIZ_DEFAULTS, + metrics: ['count'], + groupby: ['name'], + row_limit: 100000, + server_pagination: true, // Enable server pagination + }); + + // Wait for the initial data load + cy.wait('@chartData'); + + // Get the first column header (name) + cy.get('.chart-container th').contains('name').as('nameHeader'); + + // Click to sort ascending + cy.get('@nameHeader').click(); + cy.wait('@chartData'); + + // Verify first row starts with 'A' + cy.get('.chart-container td:first').invoke('text').should('match', /^[Aa]/); + + // Click again to sort descending + cy.get('@nameHeader').click(); + cy.wait('@chartData'); + + // Verify first row starts with 'Z' + cy.get('.chart-container td:first').invoke('text').should('match', /^[Zz]/); + + // Test numeric sorting + cy.get('.chart-container th').contains('COUNT').as('countHeader'); + + // Click to sort ascending by count + cy.get('@countHeader').click(); + cy.wait('@chartData'); + + // Get first two count values and verify ascending order + cy.get('.chart-container td:nth-child(2)').then($cells => { + const first = parseFloat($cells[0].textContent || '0'); + const second = parseFloat($cells[1].textContent || '0'); + expect(first).to.be.at.most(second); + }); + + // Click again to sort descending + cy.get('@countHeader').click(); + cy.wait('@chartData'); + + // Get first two count values and verify descending order + cy.get('.chart-container td:nth-child(2)').then($cells => { + const first = parseFloat($cells[0].textContent || '0'); + const second = parseFloat($cells[1].textContent || '0'); + expect(first).to.be.at.least(second); + }); + }); + + it('Test search with server pagination enabled', () => { + cy.visitChartByParams({ + ...VIZ_DEFAULTS, + metrics: ['count'], + groupby: ['name', 'state'], + row_limit: 100000, + server_pagination: true, + include_search: true, + }); + + cy.wait('@chartData'); + + // Basic search test + cy.get('span.dt-global-filter input.form-control.input-sm').should( + 'be.visible', + ); + + cy.get('span.dt-global-filter input.form-control.input-sm').type('John'); + + cy.wait('@chartData'); + + cy.get('.chart-container tbody tr').each($row => { + cy.wrap($row).contains(/John/i); + }); + + // Clear and test case-insensitive search + cy.get('span.dt-global-filter input.form-control.input-sm').clear(); + + cy.wait('@chartData'); + + cy.get('span.dt-global-filter input.form-control.input-sm').type('mary'); + + cy.wait('@chartData'); + + cy.get('.chart-container tbody tr').each($row => { + cy.wrap($row).contains(/Mary/i); + }); + + // Test special characters + cy.get('span.dt-global-filter input.form-control.input-sm').clear(); + + cy.get('span.dt-global-filter input.form-control.input-sm').type('Nicole'); + + cy.wait('@chartData'); + + cy.get('.chart-container tbody tr').each($row => { + cy.wrap($row).contains(/Nicole/i); + }); + + // Test no results + cy.get('span.dt-global-filter input.form-control.input-sm').clear(); + + cy.get('span.dt-global-filter input.form-control.input-sm').type('XYZ123'); + + cy.wait('@chartData'); + + cy.get('.chart-container').contains('No records found'); + + // Test column-specific search + cy.get('.search-select').should('be.visible'); + + cy.get('.search-select').click(); + + cy.get('.ant-select-dropdown').should('be.visible'); + + cy.get('.ant-select-item-option').contains('state').should('be.visible'); + + cy.get('.ant-select-item-option').contains('state').click(); + + cy.get('span.dt-global-filter input.form-control.input-sm').clear(); + + cy.get('span.dt-global-filter input.form-control.input-sm').type('CA'); + + cy.wait('@chartData'); + cy.wait(1000); + + cy.get('td[aria-labelledby="header-state"]').should('be.visible'); + + cy.get('td[aria-labelledby="header-state"]') + .first() + .should('contain', 'CA'); + }); }); diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/constants.ts b/superset-frontend/packages/superset-ui-chart-controls/src/constants.ts index c9e8bf9db7f..394c2c7f309 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/constants.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/constants.ts @@ -26,6 +26,7 @@ import { import { ColumnMeta, SortSeriesData, SortSeriesType } from './types'; export const DEFAULT_MAX_ROW = 100000; +export const DEFAULT_MAX_ROW_TABLE_SERVER = 500000; // eslint-disable-next-line import/prefer-default-export export const TIME_FILTER_LABELS = { diff --git a/superset-frontend/packages/superset-ui-core/src/validator/index.ts b/superset-frontend/packages/superset-ui-core/src/validator/index.ts index 1198c4e0a5b..df8dc10a70f 100644 --- a/superset-frontend/packages/superset-ui-core/src/validator/index.ts +++ b/superset-frontend/packages/superset-ui-core/src/validator/index.ts @@ -25,3 +25,4 @@ export { default as validateNonEmpty } from './validateNonEmpty'; export { default as validateMaxValue } from './validateMaxValue'; export { default as validateMapboxStylesUrl } from './validateMapboxStylesUrl'; export { default as validateTimeComparisonRangeValues } from './validateTimeComparisonRangeValues'; +export { default as validateServerPagination } from './validateServerPagination'; diff --git a/superset-frontend/packages/superset-ui-core/src/validator/validateServerPagination.ts b/superset-frontend/packages/superset-ui-core/src/validator/validateServerPagination.ts new file mode 100644 index 00000000000..02db3585a1a --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/src/validator/validateServerPagination.ts @@ -0,0 +1,30 @@ +/** + * 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 { t } from '../translation'; + +export default function validateServerPagination( + v: unknown, + serverPagination: boolean, + max: number, +) { + if (Number(v) > +max && !serverPagination) { + return t('Server pagination needs to be enabled for values over %s', max); + } + return false; +} diff --git a/superset-frontend/packages/superset-ui-core/test/validator/validateServerPagination.test.ts b/superset-frontend/packages/superset-ui-core/test/validator/validateServerPagination.test.ts new file mode 100644 index 00000000000..5a638e5142f --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/test/validator/validateServerPagination.test.ts @@ -0,0 +1,46 @@ +/** + * 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 { validateServerPagination } from '@superset-ui/core'; +import './setup'; + +test('validateServerPagination returns warning message when server pagination is disabled and value exceeds max', () => { + expect(validateServerPagination(100001, false, 100000)).toBeTruthy(); + expect(validateServerPagination('150000', false, 100000)).toBeTruthy(); + expect(validateServerPagination(200000, false, 100000)).toBeTruthy(); +}); + +test('validateServerPagination returns false when server pagination is enabled', () => { + expect(validateServerPagination(100001, true, 100000)).toBeFalsy(); + expect(validateServerPagination(150000, true, 100000)).toBeFalsy(); + expect(validateServerPagination('200000', true, 100000)).toBeFalsy(); +}); + +test('validateServerPagination returns false when value is below max', () => { + expect(validateServerPagination(50000, false, 100000)).toBeFalsy(); + expect(validateServerPagination('75000', false, 100000)).toBeFalsy(); + expect(validateServerPagination(99999, false, 100000)).toBeFalsy(); +}); + +test('validateServerPagination handles edge cases', () => { + expect(validateServerPagination(undefined, false, 100000)).toBeFalsy(); + expect(validateServerPagination(null, false, 100000)).toBeFalsy(); + expect(validateServerPagination(NaN, false, 100000)).toBeFalsy(); + expect(validateServerPagination('invalid', false, 100000)).toBeFalsy(); +}); diff --git a/superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx b/superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx index 21ce6541cf2..3aacefb2f30 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ +/* eslint-disable import/no-extraneous-dependencies */ import { useCallback, useRef, @@ -24,6 +25,7 @@ import { MutableRefObject, CSSProperties, DragEvent, + useEffect, } from 'react'; import { @@ -39,8 +41,9 @@ import { Row, } from 'react-table'; import { matchSorter, rankings } from 'match-sorter'; -import { typedMemo, usePrevious } from '@superset-ui/core'; +import { styled, typedMemo, usePrevious } from '@superset-ui/core'; import { isEqual } from 'lodash'; +import { Space } from 'antd'; import GlobalFilter, { GlobalFilterProps } from './components/GlobalFilter'; import SelectPageSize, { SelectPageSizeProps, @@ -50,6 +53,8 @@ import SimplePagination from './components/Pagination'; import useSticky from './hooks/useSticky'; import { PAGE_SIZE_OPTIONS } from '../consts'; import { sortAlphanumericCaseInsensitive } from './utils/sortAlphanumericCaseInsensitive'; +import { SearchOption, SortByItem } from '../types'; +import SearchSelectDropdown from './components/SearchSelectDropdown'; export interface DataTableProps extends TableOptions { tableClassName?: string; @@ -62,7 +67,12 @@ export interface DataTableProps extends TableOptions { height?: string | number; serverPagination?: boolean; onServerPaginationChange: (pageNumber: number, pageSize: number) => void; - serverPaginationData: { pageSize?: number; currentPage?: number }; + serverPaginationData: { + pageSize?: number; + currentPage?: number; + sortBy?: SortByItem[]; + searchColumn?: string; + }; pageSize?: number; noResults?: string | ((filterString: string) => ReactNode); sticky?: boolean; @@ -71,6 +81,14 @@ export interface DataTableProps extends TableOptions { onColumnOrderChange: () => void; renderGroupingHeaders?: () => JSX.Element; renderTimeComparisonDropdown?: () => JSX.Element; + handleSortByChange: (sortBy: SortByItem[]) => void; + sortByFromParent: SortByItem[]; + manualSearch?: boolean; + onSearchChange?: (searchText: string) => void; + initialSearchText?: string; + searchInputId?: string; + onSearchColChange: (searchCol: string) => void; + searchOptions: SearchOption[]; } export interface RenderHTMLCellProps extends HTMLProps { @@ -81,6 +99,20 @@ const sortTypes = { alphanumeric: sortAlphanumericCaseInsensitive, }; +const StyledSpace = styled(Space)` + display: flex; + justify-content: flex-end; + + .search-select-container { + display: flex; + } + + .search-by-label { + align-self: center; + margin-right: 4px; + } +`; + // Be sure to pass our updateMyData and the skipReset option export default typedMemo(function DataTable({ tableClassName, @@ -105,6 +137,14 @@ export default typedMemo(function DataTable({ onColumnOrderChange, renderGroupingHeaders, renderTimeComparisonDropdown, + handleSortByChange, + sortByFromParent = [], + manualSearch = false, + onSearchChange, + initialSearchText, + searchInputId, + onSearchColChange, + searchOptions, ...moreUseTableOptions }: DataTableProps): JSX.Element { const tableHooks: PluginHook[] = [ @@ -115,6 +155,7 @@ export default typedMemo(function DataTable({ doSticky ? useSticky : [], hooks || [], ].flat(); + const columnNames = Object.keys(data?.[0] || {}); const previousColumnNames = usePrevious(columnNames); const resultsSize = serverPagination ? rowCount : data.length; @@ -127,7 +168,8 @@ export default typedMemo(function DataTable({ ...initialState_, // zero length means all pages, the `usePagination` plugin does not // understand pageSize = 0 - sortBy: sortByRef.current, + // sortBy: sortByRef.current, + sortBy: serverPagination ? sortByFromParent : sortByRef.current, pageSize: initialPageSize > 0 ? initialPageSize : resultsSize || 10, }; const defaultWrapperRef = useRef(null); @@ -188,7 +230,13 @@ export default typedMemo(function DataTable({ wrapStickyTable, setColumnOrder, allColumns, - state: { pageIndex, pageSize, globalFilter: filterValue, sticky = {} }, + state: { + pageIndex, + pageSize, + globalFilter: filterValue, + sticky = {}, + sortBy, + }, } = useTable( { columns, @@ -198,10 +246,46 @@ export default typedMemo(function DataTable({ globalFilter: defaultGlobalFilter, sortTypes, autoResetSortBy: !isEqual(columnNames, previousColumnNames), + manualSortBy: !!serverPagination, ...moreUseTableOptions, }, ...tableHooks, ); + + const handleSearchChange = useCallback( + (query: string) => { + if (manualSearch && onSearchChange) { + onSearchChange(query); + } else { + setGlobalFilter(query); + } + }, + [manualSearch, onSearchChange, setGlobalFilter], + ); + + // updating the sort by to the own State of table viz + useEffect(() => { + const serverSortBy = serverPaginationData?.sortBy || []; + + if (serverPagination && !isEqual(sortBy, serverSortBy)) { + if (Array.isArray(sortBy) && sortBy.length > 0) { + const [sortByItem] = sortBy; + const matchingColumn = columns.find(col => col?.id === sortByItem?.id); + + if (matchingColumn && 'columnKey' in matchingColumn) { + const sortByWithColumnKey: SortByItem = { + ...sortByItem, + key: (matchingColumn as { columnKey: string }).columnKey, + }; + + handleSortByChange([sortByWithColumnKey]); + } + } else { + handleSortByChange([]); + } + } + }, [sortBy]); + // make setPageSize accept 0 const setPageSize = (size: number) => { if (serverPagination) { @@ -355,6 +439,7 @@ export default typedMemo(function DataTable({ resultOnPageChange = (pageNumber: number) => onServerPaginationChange(pageNumber, serverPageSize); } + return (
({ ) : null}
{searchInput ? ( -
+ + {serverPagination && ( +
+ Search by: + +
+ )} searchInput={ typeof searchInput === 'boolean' ? undefined : searchInput } preGlobalFilteredRows={preGlobalFilteredRows} - setGlobalFilter={setGlobalFilter} - filterValue={filterValue} + setGlobalFilter={ + manualSearch ? handleSearchChange : setGlobalFilter + } + filterValue={manualSearch ? initialSearchText : filterValue} + id={searchInputId} + serverPagination={!!serverPagination} + rowCount={rowCount} /> -
+ ) : null} {renderTimeComparisonDropdown ? (
; + onBlur?: () => void; + inputRef?: React.RefObject; } +const isSearchFocused = new Map(); + export interface GlobalFilterProps { preGlobalFilteredRows: Row[]; // filter value cannot be `undefined` otherwise React will report component @@ -33,17 +43,28 @@ export interface GlobalFilterProps { filterValue: string; setGlobalFilter: (filterValue: FilterValue) => void; searchInput?: ComponentType; + id?: string; + serverPagination: boolean; + rowCount: number; } -function DefaultSearchInput({ count, value, onChange }: SearchInputProps) { +function DefaultSearchInput({ + count, + value, + onChange, + onBlur, + inputRef, +}: SearchInputProps) { return ( Search{' '} ); @@ -56,8 +77,13 @@ export default (memo as (fn: T) => T)(function GlobalFilter< filterValue = '', searchInput, setGlobalFilter, + id = '', + serverPagination, + rowCount, }: GlobalFilterProps) { - const count = preGlobalFilteredRows.length; + const count = serverPagination ? rowCount : preGlobalFilteredRows.length; + const inputRef = useRef(null); + const [value, setValue] = useAsyncState( filterValue, (newValue: string) => { @@ -66,17 +92,37 @@ export default (memo as (fn: T) => T)(function GlobalFilter< 200, ); + // Preserve focus during server-side filtering to maintain a better user experience + useEffect(() => { + if ( + serverPagination && + isSearchFocused.get(id) && + document.activeElement !== inputRef.current + ) { + inputRef.current?.focus(); + } + }, [value, serverPagination]); + + const handleChange = (e: React.ChangeEvent) => { + const target = e.target as HTMLInputElement; + e.preventDefault(); + isSearchFocused.set(id, true); + setValue(target.value); + }; + + const handleBlur = () => { + isSearchFocused.set(id, false); + }; + const SearchInput = searchInput || DefaultSearchInput; return ( { - const target = e.target as HTMLInputElement; - e.preventDefault(); - setValue(target.value); - }} + inputRef={inputRef} + onChange={handleChange} + onBlur={handleBlur} /> ); }); diff --git a/superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx b/superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx new file mode 100644 index 00000000000..b58cf2e8a22 --- /dev/null +++ b/superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx @@ -0,0 +1,53 @@ +/** + * 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. + */ +/* eslint-disable import/no-extraneous-dependencies */ +import { styled } from '@superset-ui/core'; +import { Select } from 'antd'; +import { SearchOption } from '../../types'; + +const StyledSelect = styled(Select)` + width: 120px; + margin-right: 8px; +`; + +interface SearchSelectDropdownProps { + /** The currently selected search column value */ + value?: string; + /** Callback triggered when a new search column is selected */ + onChange: (searchCol: string) => void; + /** Available search column options to populate the dropdown */ + searchOptions: SearchOption[]; +} + +function SearchSelectDropdown({ + value, + onChange, + searchOptions, +}: SearchSelectDropdownProps) { + return ( + + ); +} + +export default SearchSelectDropdown; diff --git a/superset-frontend/plugins/plugin-chart-table/src/DataTable/types/react-table.d.ts b/superset-frontend/plugins/plugin-chart-table/src/DataTable/types/react-table.d.ts index 6c319f76845..9d591a08182 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/DataTable/types/react-table.d.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/DataTable/types/react-table.d.ts @@ -115,3 +115,11 @@ declare module 'react-table' { extends UseTableHooks, UseSortByHooks {} } + +interface TableOwnState { + currentPage?: number; + pageSize?: number; + sortColumn?: string; + sortOrder?: 'asc' | 'desc'; + searchText?: string; +} diff --git a/superset-frontend/plugins/plugin-chart-table/src/DataTable/utils/externalAPIs.ts b/superset-frontend/plugins/plugin-chart-table/src/DataTable/utils/externalAPIs.ts index b703d66d971..8a5d20ed5bc 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/DataTable/utils/externalAPIs.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/DataTable/utils/externalAPIs.ts @@ -18,6 +18,7 @@ */ import { SetDataMaskHook } from '@superset-ui/core'; +import { TableOwnState } from '../types/react-table'; export const updateExternalFormData = ( setDataMask: SetDataMaskHook = () => {}, @@ -30,3 +31,11 @@ export const updateExternalFormData = ( pageSize, }, }); + +export const updateTableOwnState = ( + setDataMask: SetDataMaskHook = () => {}, + modifiedOwnState: TableOwnState, +) => + setDataMask({ + ownState: modifiedOwnState, + }); diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index 88a326544e6..475056c7503 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -24,6 +24,7 @@ import { useState, MouseEvent, KeyboardEvent as ReactKeyboardEvent, + useEffect, } from 'react'; import { @@ -61,10 +62,12 @@ import { PlusCircleOutlined, TableOutlined, } from '@ant-design/icons'; -import { isEmpty } from 'lodash'; +import { debounce, isEmpty, isEqual } from 'lodash'; import { ColorSchemeEnum, DataColumnMeta, + SearchOption, + SortByItem, TableChartTransformedProps, } from './types'; import DataTable, { @@ -77,7 +80,7 @@ import DataTable, { import Styles from './Styles'; import { formatColumnValue } from './utils/formatValue'; import { PAGE_SIZE_OPTIONS } from './consts'; -import { updateExternalFormData } from './DataTable/utils/externalAPIs'; +import { updateTableOwnState } from './DataTable/utils/externalAPIs'; import getScrollBarSize from './DataTable/utils/getScrollBarSize'; type ValueRange = [number, number]; @@ -176,20 +179,26 @@ function SortIcon({ column }: { column: ColumnInstance }) { return sortIcon; } -function SearchInput({ count, value, onChange }: SearchInputProps) { - return ( - - {t('Search')}{' '} - - - ); -} +const SearchInput = ({ + count, + value, + onChange, + onBlur, + inputRef, +}: SearchInputProps) => ( + + {t('Search')}{' '} + + +); function SelectPageSize({ options, @@ -267,6 +276,9 @@ export default function TableChart( isUsingTimeComparison, basicColorFormatters, basicColorColumnFormatters, + hasServerPageLengthChanged, + serverPageLength, + slice_id, } = props; const comparisonColumns = [ { key: 'all', label: t('Display all') }, @@ -679,7 +691,12 @@ export default function TableChart( ); const getColumnConfigs = useCallback( - (column: DataColumnMeta, i: number): ColumnWithLooseAccessor => { + ( + column: DataColumnMeta, + i: number, + ): ColumnWithLooseAccessor & { + columnKey: string; + } => { const { key, label: originalLabel, @@ -766,6 +783,7 @@ export default function TableChart( // must use custom accessor to allow `.` in column names // typing is incorrect in current version of `@types/react-table` // so we ask TS not to check. + columnKey: key, accessor: ((datum: D) => datum[key]) as never, Cell: ({ value, row }: { value: DataRecordValue; row: Row }) => { const [isHtml, text] = formatColumnValue(column, value); @@ -1058,13 +1076,50 @@ export default function TableChart( [visibleColumnsMeta, getColumnConfigs], ); + const [searchOptions, setSearchOptions] = useState([]); + + useEffect(() => { + const options = ( + columns as unknown as ColumnWithLooseAccessor & + { + columnKey: string; + sortType?: string; + }[] + ) + .filter(col => col?.sortType === 'alphanumeric') + .map(column => ({ + value: column.columnKey, + label: column.columnKey, + })); + + if (!isEqual(options, searchOptions)) { + setSearchOptions(options || []); + } + }, [columns]); + const handleServerPaginationChange = useCallback( (pageNumber: number, pageSize: number) => { - updateExternalFormData(setDataMask, pageNumber, pageSize); + const modifiedOwnState = { + ...serverPaginationData, + currentPage: pageNumber, + pageSize, + }; + updateTableOwnState(setDataMask, modifiedOwnState); }, [setDataMask], ); + useEffect(() => { + if (hasServerPageLengthChanged) { + const modifiedOwnState = { + ...serverPaginationData, + currentPage: 0, + pageSize: serverPageLength, + }; + updateTableOwnState(setDataMask, modifiedOwnState); + } + }, []); + const handleSizeChange = useCallback( ({ width, height }: { width: number; height: number }) => { setTableSize({ width, height }); @@ -1100,6 +1155,42 @@ export default function TableChart( const { width: widthFromState, height: heightFromState } = tableSize; + const handleSortByChange = useCallback( + (sortBy: SortByItem[]) => { + if (!serverPagination) return; + const modifiedOwnState = { + ...serverPaginationData, + sortBy, + }; + updateTableOwnState(setDataMask, modifiedOwnState); + }, + [setDataMask, serverPagination], + ); + + const handleSearch = (searchText: string) => { + const modifiedOwnState = { + ...(serverPaginationData || {}), + searchColumn: + serverPaginationData?.searchColumn || searchOptions[0]?.value, + searchText, + currentPage: 0, // Reset to first page when searching + }; + updateTableOwnState(setDataMask, modifiedOwnState); + }; + + const debouncedSearch = debounce(handleSearch, 800); + + const handleChangeSearchCol = (searchCol: string) => { + if (!isEqual(searchCol, serverPaginationData?.searchColumn)) { + const modifiedOwnState = { + ...(serverPaginationData || {}), + searchColumn: searchCol, + searchText: '', + }; + updateTableOwnState(setDataMask, modifiedOwnState); + } + }; + return ( @@ -1115,6 +1206,9 @@ export default function TableChart( serverPagination={serverPagination} onServerPaginationChange={handleServerPaginationChange} onColumnOrderChange={() => setColumnOrderToggle(!columnOrderToggle)} + initialSearchText={serverPaginationData?.searchText || ''} + sortByFromParent={serverPaginationData?.sortBy || []} + searchInputId={`${slice_id}-search`} // 9 page items in > 340px works well even for 100+ pages maxPageItemCount={width > 340 ? 9 : 7} noResults={getNoResultsMessage} @@ -1128,6 +1222,11 @@ export default function TableChart( renderTimeComparisonDropdown={ isUsingTimeComparison ? renderTimeComparisonDropdown : undefined } + handleSortByChange={handleSortByChange} + onSearchColChange={handleChangeSearchCol} + manualSearch={serverPagination} + onSearchChange={debouncedSearch} + searchOptions={searchOptions} /> ); diff --git a/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts index 5b9cb684ed4..439df369f5a 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts @@ -22,6 +22,7 @@ import { ensureIsArray, getMetricLabel, isPhysicalColumn, + QueryFormOrderBy, QueryMode, QueryObject, removeDuplicates, @@ -34,7 +35,7 @@ import { } from '@superset-ui/chart-controls'; import { isEmpty } from 'lodash'; import { TableChartFormData } from './types'; -import { updateExternalFormData } from './DataTable/utils/externalAPIs'; +import { updateTableOwnState } from './DataTable/utils/externalAPIs'; /** * Infer query mode from form data. If `all_columns` is set, then raw records mode, @@ -191,18 +192,40 @@ const buildQuery: BuildQuery = ( const moreProps: Partial = {}; const ownState = options?.ownState ?? {}; - if (formDataCopy.server_pagination) { - moreProps.row_limit = - ownState.pageSize ?? formDataCopy.server_page_length; - moreProps.row_offset = - (ownState.currentPage ?? 0) * (ownState.pageSize ?? 0); + // Build Query flag to check if its for either download as csv, excel or json + const isDownloadQuery = + ['csv', 'xlsx'].includes(formData?.result_format || '') || + (formData?.result_format === 'json' && + formData?.result_type === 'results'); + + if (isDownloadQuery) { + moreProps.row_limit = Number(formDataCopy.row_limit) || 0; + moreProps.row_offset = 0; + } + + if (!isDownloadQuery && formDataCopy.server_pagination) { + const pageSize = ownState.pageSize ?? formDataCopy.server_page_length; + const currentPage = ownState.currentPage ?? 0; + + moreProps.row_limit = pageSize; + moreProps.row_offset = currentPage * pageSize; + } + + // getting sort by in case of server pagination from own state + let sortByFromOwnState: QueryFormOrderBy[] | undefined; + if (Array.isArray(ownState?.sortBy) && ownState?.sortBy.length > 0) { + const sortByItem = ownState?.sortBy[0]; + sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]]; } let queryObject = { ...baseQueryObject, columns, extras, - orderby, + orderby: + formData.server_pagination && sortByFromOwnState + ? sortByFromOwnState + : orderby, metrics, post_processing: postProcessing, time_offsets: timeOffsets, @@ -216,11 +239,12 @@ const buildQuery: BuildQuery = ( JSON.stringify(queryObject.filters) ) { queryObject = { ...queryObject, row_offset: 0 }; - updateExternalFormData( - options?.hooks?.setDataMask, - 0, - queryObject.row_limit ?? 0, - ); + const modifiedOwnState = { + ...(options?.ownState || {}), + currentPage: 0, + pageSize: queryObject.row_limit ?? 0, + }; + updateTableOwnState(options?.hooks?.setDataMask, modifiedOwnState); } // Because we use same buildQuery for all table on the page we need split them by id options?.hooks?.setCachedChanges({ @@ -252,12 +276,32 @@ const buildQuery: BuildQuery = ( } if (formData.server_pagination) { + // Add search filter if search text exists + if (ownState.searchText && ownState?.searchColumn) { + queryObject = { + ...queryObject, + filters: [ + ...(queryObject.filters || []), + { + col: ownState?.searchColumn, + op: 'ILIKE', + val: `${ownState.searchText}%`, + }, + ], + }; + } + } + + // Now since row limit control is always visible even + // in case of server pagination + // we must use row limit from form data + if (formData.server_pagination && !isDownloadQuery) { return [ { ...queryObject }, { ...queryObject, time_offsets: [], - row_limit: 0, + row_limit: Number(formData?.row_limit) ?? 0, row_offset: 0, post_processing: [], is_rowcount: true, diff --git a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx index f6798b79848..aab66f800dd 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx @@ -28,7 +28,10 @@ import { ControlStateMapping, D3_TIME_FORMAT_OPTIONS, Dataset, + DEFAULT_MAX_ROW, + DEFAULT_MAX_ROW_TABLE_SERVER, defineSavedMetrics, + formatSelectOptions, getStandardizedControls, QueryModeLabel, sections, @@ -40,11 +43,14 @@ import { getMetricLabel, isAdhocColumn, isPhysicalColumn, + legacyValidateInteger, QueryFormColumn, QueryFormMetric, QueryMode, SMART_DATE_ID, t, + validateMaxValue, + validateServerPagination, } from '@superset-ui/core'; import { isEmpty, last } from 'lodash'; @@ -188,6 +194,15 @@ const processComparisonColumns = (columns: any[], suffix: string) => }) .flat(); +/* +Options for row limit control +*/ + +export const ROW_LIMIT_OPTIONS_TABLE = [ + 10, 50, 100, 250, 500, 1000, 5000, 10000, 50000, 100000, 150000, 200000, + 250000, 300000, 350000, 400000, 450000, 500000, +]; + const config: ControlPanelConfig = { controlPanelSections: [ { @@ -342,14 +357,6 @@ const config: ControlPanelConfig = { }, ], [ - { - name: 'row_limit', - override: { - default: 1000, - visibility: ({ controls }: ControlPanelsContainerProps) => - !controls?.server_pagination?.value, - }, - }, { name: 'server_page_length', config: { @@ -364,6 +371,47 @@ const config: ControlPanelConfig = { }, }, ], + [ + { + name: 'row_limit', + config: { + type: 'SelectControl', + freeForm: true, + label: t('Row limit'), + clearable: false, + mapStateToProps: state => ({ + maxValue: state?.common?.conf?.TABLE_VIZ_MAX_ROW_SERVER, + server_pagination: state?.form_data?.server_pagination, + maxValueWithoutServerPagination: + state?.common?.conf?.SQL_MAX_ROW, + }), + validators: [ + legacyValidateInteger, + (v, state) => + validateMaxValue( + v, + state?.maxValue || DEFAULT_MAX_ROW_TABLE_SERVER, + ), + (v, state) => + validateServerPagination( + v, + state?.server_pagination, + state?.maxValueWithoutServerPagination || DEFAULT_MAX_ROW, + ), + ], + // Re run the validations when this control value + validationDependancies: ['server_pagination'], + default: 10000, + choices: formatSelectOptions(ROW_LIMIT_OPTIONS_TABLE), + description: t( + 'Limits the number of the rows that are computed in the query that is the source of the data used for this chart.', + ), + }, + override: { + default: 1000, + }, + }, + ], [ { name: 'order_desc', diff --git a/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts b/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts index d62d9cb92c9..86d4efed8f4 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts @@ -90,6 +90,15 @@ const processDataRecords = memoizeOne(function processDataRecords( return data; }); +// Create a map to store cached values per slice +const sliceCache = new Map< + number, + { + cachedServerLength: number; + passedColumns?: DataColumnMeta[]; + } +>(); + const calculateDifferences = ( originalValue: number, comparisonValue: number, @@ -480,6 +489,7 @@ const transformProps = ( comparison_color_enabled: comparisonColorEnabled = false, comparison_color_scheme: comparisonColorScheme = ColorSchemeEnum.Green, comparison_type, + slice_id, } = formData; const isUsingTimeComparison = !isEmpty(time_compare) && @@ -675,6 +685,26 @@ const transformProps = ( conditionalFormatting, ); + // Get cached values for this slice + const cachedValues = sliceCache.get(slice_id); + let hasServerPageLengthChanged = false; + + if ( + cachedValues?.cachedServerLength !== undefined && + cachedValues.cachedServerLength !== serverPageLength + ) { + hasServerPageLengthChanged = true; + } + + // Update cache with new values + sliceCache.set(slice_id, { + cachedServerLength: serverPageLength, + passedColumns: + Array.isArray(passedColumns) && passedColumns?.length > 0 + ? passedColumns + : cachedValues?.passedColumns, + }); + const startDateOffset = chartProps.rawFormData?.start_date_offset; return { height, @@ -682,7 +712,10 @@ const transformProps = ( isRawRecords: queryMode === QueryMode.Raw, data: passedData, totals, - columns: passedColumns, + columns: + Array.isArray(passedColumns) && passedColumns?.length > 0 + ? passedColumns + : cachedValues?.passedColumns || [], serverPagination, metrics, percentMetrics, @@ -697,7 +730,9 @@ const transformProps = ( includeSearch, rowCount, pageSize: serverPagination - ? serverPageLength + ? serverPaginationData?.pageSize + ? serverPaginationData?.pageSize + : serverPageLength : getPageSize(pageLength, data.length, columns.length), filters: filterState.filters, emitCrossFilters, @@ -711,6 +746,9 @@ const transformProps = ( basicColorFormatters, startDateOffset, basicColorColumnFormatters, + hasServerPageLengthChanged, + serverPageLength, + slice_id, }; }; diff --git a/superset-frontend/plugins/plugin-chart-table/src/types.ts b/superset-frontend/plugins/plugin-chart-table/src/types.ts index 7460a27c461..1b7cfd45904 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/types.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/types.ts @@ -114,13 +114,32 @@ export type BasicColorFormatterType = { mainArrow: string; }; +export type SortByItem = { + id: string; + key: string; + desc?: boolean; +}; + +export type SearchOption = { + value: string; + label: string; +}; + +export interface ServerPaginationData { + pageSize?: number; + currentPage?: number; + sortBy?: SortByItem[]; + searchText?: string; + searchColumn?: string; +} + export interface TableChartTransformedProps { timeGrain?: TimeGranularity; height: number; width: number; rowCount?: number; serverPagination: boolean; - serverPaginationData: { pageSize?: number; currentPage?: number }; + serverPaginationData: ServerPaginationData; setDataMask: SetDataMaskHook; isRawRecords?: boolean; data: D[]; @@ -152,6 +171,11 @@ export interface TableChartTransformedProps { basicColorFormatters?: { [Key: string]: BasicColorFormatterType }[]; basicColorColumnFormatters?: { [Key: string]: BasicColorFormatterType }[]; startDateOffset?: string; + // For explore page to reset the server Pagination data + // if server page length is changed from control panel + hasServerPageLengthChanged: boolean; + serverPageLength: number; + slice_id: number; } export enum ColorSchemeEnum { diff --git a/superset-frontend/src/components/Chart/ChartRenderer.jsx b/superset-frontend/src/components/Chart/ChartRenderer.jsx index 25fc045d210..7b641d0eaec 100644 --- a/superset-frontend/src/components/Chart/ChartRenderer.jsx +++ b/superset-frontend/src/components/Chart/ChartRenderer.jsx @@ -327,6 +327,10 @@ class ChartRenderer extends Component { ?.behaviors.find(behavior => behavior === Behavior.DrillToDetail) ? { inContextMenu: this.state.inContextMenu } : {}; + // By pass no result component when server pagination is enabled & the table has a backend search query + const bypassNoResult = !( + formData?.server_pagination && (ownState?.searchText?.length || 0) > 0 + ); return ( <> @@ -367,6 +371,7 @@ class ChartRenderer extends Component { postTransformProps={postTransformProps} emitCrossFilters={emitCrossFilters} legendState={this.state.legendState} + enableNoResults={bypassNoResult} {...drillToDetailProps} />
diff --git a/superset-frontend/src/explore/reducers/exploreReducer.js b/superset-frontend/src/explore/reducers/exploreReducer.js index 2677c1e3ff5..156459353e8 100644 --- a/superset-frontend/src/explore/reducers/exploreReducer.js +++ b/superset-frontend/src/explore/reducers/exploreReducer.js @@ -214,6 +214,52 @@ export default function exploreReducer(state = {}, action) { currentControlsState = transformed.controlsState; } + const dependantControls = Object.entries(state.controls) + .filter( + ([, item]) => + Array.isArray(item?.validationDependancies) && + item.validationDependancies.includes(controlName), + ) + .map(([key, item]) => ({ + controlState: item, + dependantControlName: key, + })); + + let updatedControlStates = {}; + if (dependantControls.length > 0) { + const updatedControls = dependantControls.map( + ({ controlState, dependantControlName }) => { + // overwrite state form data with current control value as the redux state will not + // have latest action value + const overWrittenState = { + ...state, + form_data: { + ...state.form_data, + [controlName]: action.value, + }, + }; + + return { + // Re run validation for dependant controls + controlState: getControlStateFromControlConfig( + controlState, + overWrittenState, + controlState?.value, + ), + dependantControlName, + }; + }, + ); + + updatedControlStates = updatedControls.reduce( + (acc, { controlState, dependantControlName }) => { + acc[dependantControlName] = { ...controlState }; + return acc; + }, + {}, + ); + } + return { ...state, form_data: new_form_data, @@ -227,6 +273,7 @@ export default function exploreReducer(state = {}, action) { }, }), ...rerenderedControls, + ...updatedControlStates, }, }; }, diff --git a/superset/common/query_context_factory.py b/superset/common/query_context_factory.py index 3e3fe291394..7a9e9e15028 100644 --- a/superset/common/query_context_factory.py +++ b/superset/common/query_context_factory.py @@ -65,12 +65,23 @@ class QueryContextFactory: # pylint: disable=too-few-public-methods result_type = result_type or ChartDataResultType.FULL result_format = result_format or ChartDataResultFormat.JSON + + # The server pagination var is extracted from form data as the + # row limit for server pagination is more + # This particular flag server_pagination only exists for table viz type + server_pagination = ( + bool(form_data.get("server_pagination")) if form_data else False + ) + queries_ = [ self._process_query_object( datasource_model_instance, form_data, self._query_object_factory.create( - result_type, datasource=datasource, **query_obj + result_type, + datasource=datasource, + server_pagination=server_pagination, + **query_obj, ), ) for query_obj in queries diff --git a/superset/common/query_object_factory.py b/superset/common/query_object_factory.py index 63d1ef09665..063ba023481 100644 --- a/superset/common/query_object_factory.py +++ b/superset/common/query_object_factory.py @@ -57,6 +57,7 @@ class QueryObjectFactory: # pylint: disable=too-few-public-methods row_limit: int | None = None, time_range: str | None = None, time_shift: str | None = None, + server_pagination: bool | None = None, **kwargs: Any, ) -> QueryObject: datasource_model_instance = None @@ -64,7 +65,12 @@ class QueryObjectFactory: # pylint: disable=too-few-public-methods datasource_model_instance = self._convert_to_model(datasource) processed_extras = self._process_extras(extras) result_type = kwargs.setdefault("result_type", parent_result_type) - row_limit = self._process_row_limit(row_limit, result_type) + + # Process row limit taking server pagination into account + row_limit = self._process_row_limit( + row_limit, result_type, server_pagination=server_pagination + ) + processed_time_range = self._process_time_range( time_range, kwargs.get("filters"), kwargs.get("columns") ) @@ -96,14 +102,27 @@ class QueryObjectFactory: # pylint: disable=too-few-public-methods return extras def _process_row_limit( - self, row_limit: int | None, result_type: ChartDataResultType + self, + row_limit: int | None, + result_type: ChartDataResultType, + server_pagination: bool | None = None, ) -> int: + """Process row limit taking into account server pagination. + + :param row_limit: The requested row limit + :param result_type: The type of result being processed + :param server_pagination: Whether server-side pagination is enabled + :return: The processed row limit + """ default_row_limit = ( self._config["SAMPLES_ROW_LIMIT"] if result_type == ChartDataResultType.SAMPLES else self._config["ROW_LIMIT"] ) - return apply_max_row_limit(row_limit or default_row_limit) + return apply_max_row_limit( + row_limit or default_row_limit, + server_pagination=server_pagination, + ) @staticmethod def _process_time_range( diff --git a/superset/config.py b/superset/config.py index a82c080b429..a01c380a7e9 100644 --- a/superset/config.py +++ b/superset/config.py @@ -969,6 +969,10 @@ MAPBOX_API_KEY = os.environ.get("MAPBOX_API_KEY", "") # Maximum number of rows returned for any analytical database query SQL_MAX_ROW = 100000 +# Maximum number of rows for any query with Server Pagination in Table Viz type +TABLE_VIZ_MAX_ROW_SERVER = 500000 + + # Maximum number of rows displayed in SQL Lab UI # Is set to avoid out of memory/localstorage issues in browsers. Does not affect # exported CSVs diff --git a/superset/utils/core.py b/superset/utils/core.py index 2b80c89f612..f1792ac47a3 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -1741,24 +1741,30 @@ def parse_boolean_string(bool_str: str | None) -> bool: def apply_max_row_limit( limit: int, - max_limit: int | None = None, + server_pagination: bool | None = None, ) -> int: """ - Override row limit if max global limit is defined + Override row limit based on server pagination setting :param limit: requested row limit - :param max_limit: Maximum allowed row limit + :param server_pagination: whether server-side pagination + is enabled, defaults to None :return: Capped row limit - >>> apply_max_row_limit(100000, 10) - 10 - >>> apply_max_row_limit(10, 100000) - 10 - >>> apply_max_row_limit(0, 10000) - 10000 + >>> apply_max_row_limit(600000, server_pagination=True) # Server pagination + 500000 + >>> apply_max_row_limit(600000, server_pagination=False) # No pagination + 50000 + >>> apply_max_row_limit(5000) # No server_pagination specified + 5000 + >>> apply_max_row_limit(0) # Zero returns default max limit + 50000 """ - if max_limit is None: - max_limit = current_app.config["SQL_MAX_ROW"] + max_limit = ( + current_app.config["TABLE_VIZ_MAX_ROW_SERVER"] + if server_pagination + else current_app.config["SQL_MAX_ROW"] + ) if limit != 0: return min(max_limit, limit) return max_limit diff --git a/superset/views/base.py b/superset/views/base.py index a598994f19f..43b4f776718 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -109,6 +109,7 @@ FRONTEND_CONF_KEYS = ( "JWT_ACCESS_CSRF_COOKIE_NAME", "SQLLAB_QUERY_RESULT_TIMEOUT", "SYNC_DB_PERMISSIONS_IN_ASYNC_MODE", + "TABLE_VIZ_MAX_ROW_SERVER", ) logger = logging.getLogger(__name__) diff --git a/tests/unit_tests/common/test_query_object_factory.py b/tests/unit_tests/common/test_query_object_factory.py index 8ff362dec36..379137a34c4 100644 --- a/tests/unit_tests/common/test_query_object_factory.py +++ b/tests/unit_tests/common/test_query_object_factory.py @@ -14,7 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from typing import Any, Optional +from typing import Any from unittest.mock import Mock from pytest import fixture # noqa: PT013 @@ -45,9 +45,15 @@ def connector_registry() -> Mock: return mock -def apply_max_row_limit(limit: int, max_limit: Optional[int] = None) -> int: - if max_limit is None: - max_limit = create_app_config()["SQL_MAX_ROW"] +def apply_max_row_limit( + limit: int, + server_pagination: bool | None = None, +) -> int: + max_limit = ( + create_app_config()["TABLE_VIZ_MAX_ROW_SERVER"] + if server_pagination + else create_app_config()["SQL_MAX_ROW"] + ) if limit != 0: return min(max_limit, limit) return max_limit From 73701b729563d796d4aae628eff24f7163e7671c Mon Sep 17 00:00:00 2001 From: irodriguez-nebustream <81891746+irodriguez-nebustream@users.noreply.github.com> Date: Fri, 9 May 2025 17:25:40 -0500 Subject: [PATCH 5/5] fix(embedded): handle SUPERSET_APP_ROOT in embedded dashboard URLs (#33356) Co-authored-by: Irving Rodriguez --- superset-frontend/src/embedded/index.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/embedded/index.tsx b/superset-frontend/src/embedded/index.tsx index deddab121e9..1932ac005d3 100644 --- a/superset-frontend/src/embedded/index.tsx +++ b/superset-frontend/src/embedded/index.tsx @@ -23,7 +23,7 @@ import ReactDOM from 'react-dom'; import { BrowserRouter as Router, Route } from 'react-router-dom'; import { makeApi, t, logging } from '@superset-ui/core'; import Switchboard from '@superset-ui/switchboard'; -import getBootstrapData from 'src/utils/getBootstrapData'; +import getBootstrapData, { applicationRoot } from 'src/utils/getBootstrapData'; import setupClient from 'src/setup/setupClient'; import setupPlugins from 'src/setup/setupPlugins'; import { useUiConfig } from 'src/components/UiConfigContext'; @@ -94,7 +94,7 @@ const EmbeddedRoute = () => ( ); const EmbeddedApp = () => ( - + {/* todo (embedded) remove this line after uuids are deployed */} @@ -187,6 +187,7 @@ function start() { */ function setupGuestClient(guestToken: string) { setupClient({ + appRoot: applicationRoot(), guestToken, guestTokenHeaderName: bootstrapData.config?.GUEST_TOKEN_HEADER_NAME, unauthorizedHandler: guestUnauthorizedHandler,