You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2024/02/12 22:21:06 UTC
(superset) branch 3.0 updated (05042facd3 -> c029475f60)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a change to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
from 05042facd3 fix(cache): remove unused webserver config & handle trailing slashes (#22849)
new d1dad1e63d fix(plugin-chart-table): Prevent misalignment of totals and headers when scrollbar is visible (#26964)
new 383b4d949f fix: column values with NaN (#26946)
new e3da64593f fix(security manager): Users should not have access to all draft dashboards (#27015)
new 42a6846f42 fix: safer error message in alerts (#27019)
new 648e0a08d6 fix(explore): allow free-form d3 format on custom column formatting (#27023)
new db56b7fc36 fix(plugins): missing currency on small number format in table chart (#27041)
new b0d0d77bac fix: Filters sidebar stretching dashboard height (#27069)
new e772915bb8 fix(drill): no rows returned (#27073)
new c029475f60 fix: chart import validation (#26993)
The 9 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
setup.py | 2 +-
.../superset-ui-chart-controls/src/types.ts | 1 +
.../src/DataTable/hooks/useSticky.tsx | 4 +
.../plugin-chart-table/src/utils/formatValue.ts | 6 +
.../plugin-chart-table/test/TableChart.test.tsx | 42 ++++++
.../DashboardBuilder/DashboardWrapper.tsx | 1 +
.../controls/ColumnConfigControl/constants.tsx | 1 +
superset/charts/commands/importers/v1/utils.py | 13 +-
superset/dashboards/commands/importers/v1/utils.py | 8 +-
superset/datasets/commands/importers/v1/utils.py | 7 +-
superset/db_engine_specs/drill.py | 37 ++++-
superset/errors.py | 1 +
superset/jinja_context.py | 9 +-
superset/models/helpers.py | 7 +-
superset/reports/commands/alert.py | 9 +-
superset/security/manager.py | 72 +++++++--
superset/utils/core.py | 9 ++
tests/integration_tests/charts/commands_tests.py | 2 +-
tests/integration_tests/conftest.py | 21 +--
.../integration_tests/dashboards/commands_tests.py | 2 +-
.../dashboards/security/security_rbac_tests.py | 24 +--
tests/integration_tests/datasets/commands_tests.py | 2 +-
tests/integration_tests/datasource/api_tests.py | 10 ++
tests/integration_tests/datasource_tests.py | 9 +-
.../charts/commands/importers/v1/import_test.py | 164 ++++++++++++++++-----
.../commands/importers/v1/import_test.py | 152 ++++++++-----------
tests/unit_tests/security/manager_test.py | 140 ++++++++++++++++++
27 files changed, 564 insertions(+), 191 deletions(-)
(superset) 04/09: fix: safer error message in alerts (#27019)
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 42a6846f424782e1f367360805f5187015d93f69
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Wed Feb 7 14:58:51 2024 -0500
fix: safer error message in alerts (#27019)
(cherry picked from commit 686ce33ea5017aad4cca18a6409c00f6b366dcf4)
---
superset/reports/commands/alert.py | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/superset/reports/commands/alert.py b/superset/reports/commands/alert.py
index 41163dc064..6a5c5cdf71 100644
--- a/superset/reports/commands/alert.py
+++ b/superset/reports/commands/alert.py
@@ -151,7 +151,7 @@ class AlertCommand(BaseCommand):
rendered_sql, ALERT_SQL_LIMIT
)
- _, username = get_executor(
+ executor, username = get_executor( # pylint: disable=unused-variable
executor_types=app.config["ALERT_REPORTS_EXECUTE_AS"],
model=self._report_schedule,
)
@@ -170,7 +170,12 @@ class AlertCommand(BaseCommand):
logger.warning("A timeout occurred while executing the alert query: %s", ex)
raise AlertQueryTimeout() from ex
except Exception as ex:
- raise AlertQueryError(message=str(ex)) from ex
+ logger.exception("An error occurred when running alert query")
+ # The exception message here can reveal to much information to malicious
+ # users, so we raise a generic message.
+ raise AlertQueryError(
+ message=_("An error occurred when running alert query")
+ ) from ex
def validate(self) -> None:
"""
(superset) 07/09: fix: Filters sidebar stretching dashboard height (#27069)
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit b0d0d77bac02d5a731cb44a11c36bb7d0fcdee90
Author: Kamil Gabryjelski <ka...@gmail.com>
AuthorDate: Fri Feb 9 18:16:39 2024 +0100
fix: Filters sidebar stretching dashboard height (#27069)
(cherry picked from commit 3f91bdb40d76539e953dd9205481459f6b2ae082)
---
.../src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx | 1 +
1 file changed, 1 insertion(+)
diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx
index f39c7ed630..5bb193de1b 100644
--- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx
+++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx
@@ -25,6 +25,7 @@ import classNames from 'classnames';
const StyledDiv = styled.div`
${({ theme }) => css`
+ position: relative;
display: grid;
grid-template-columns: auto 1fr;
grid-template-rows: auto 1fr;
(superset) 09/09: fix: chart import validation (#26993)
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit c029475f60c0a2cfda5a82202bda82fe4df8bc5c
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Tue Feb 6 12:14:02 2024 +0000
fix: chart import validation (#26993)
---
superset/charts/commands/importers/v1/utils.py | 13 +-
superset/dashboards/commands/importers/v1/utils.py | 8 +-
superset/datasets/commands/importers/v1/utils.py | 7 +-
superset/errors.py | 1 +
superset/jinja_context.py | 9 +-
superset/security/manager.py | 41 ++++++
superset/utils/core.py | 9 ++
tests/integration_tests/charts/commands_tests.py | 2 +-
.../integration_tests/dashboards/commands_tests.py | 2 +-
tests/integration_tests/datasets/commands_tests.py | 2 +-
.../charts/commands/importers/v1/import_test.py | 164 ++++++++++++++++-----
.../commands/importers/v1/import_test.py | 152 ++++++++-----------
tests/unit_tests/security/manager_test.py | 140 ++++++++++++++++++
13 files changed, 404 insertions(+), 146 deletions(-)
diff --git a/superset/charts/commands/importers/v1/utils.py b/superset/charts/commands/importers/v1/utils.py
index 91ec58406e..7b8ed643fc 100644
--- a/superset/charts/commands/importers/v1/utils.py
+++ b/superset/charts/commands/importers/v1/utils.py
@@ -20,7 +20,6 @@ import json
from inspect import isclass
from typing import Any
-from flask import g
from sqlalchemy.orm import Session
from superset import security_manager
@@ -28,7 +27,7 @@ from superset.commands.exceptions import ImportFailedError
from superset.migrations.shared.migrate_viz import processors
from superset.migrations.shared.migrate_viz.base import MigrateViz
from superset.models.slice import Slice
-from superset.utils.core import AnnotationType
+from superset.utils.core import AnnotationType, get_user
def filter_chart_annotations(chart_config: dict[str, Any]) -> None:
@@ -55,6 +54,12 @@ def import_chart(
can_write = ignore_permissions or security_manager.can_access("can_write", "Chart")
existing = session.query(Slice).filter_by(uuid=config["uuid"]).first()
if existing:
+ if overwrite and can_write and get_user():
+ if not security_manager.can_access_chart(existing):
+ raise ImportFailedError(
+ "A chart already exists and user doesn't "
+ "have permissions to overwrite it"
+ )
if not overwrite or not can_write:
return existing
config["id"] = existing.id
@@ -77,8 +82,8 @@ def import_chart(
if chart.id is None:
session.flush()
- if hasattr(g, "user") and g.user:
- chart.owners.append(g.user)
+ if user := get_user():
+ chart.owners.append(user)
return chart
diff --git a/superset/dashboards/commands/importers/v1/utils.py b/superset/dashboards/commands/importers/v1/utils.py
index 712161bc16..b8ac3144db 100644
--- a/superset/dashboards/commands/importers/v1/utils.py
+++ b/superset/dashboards/commands/importers/v1/utils.py
@@ -19,12 +19,12 @@ import json
import logging
from typing import Any
-from flask import g
from sqlalchemy.orm import Session
from superset import security_manager
from superset.commands.exceptions import ImportFailedError
from superset.models.dashboard import Dashboard
+from superset.utils.core import get_user
logger = logging.getLogger(__name__)
@@ -157,7 +157,7 @@ def import_dashboard(
)
existing = session.query(Dashboard).filter_by(uuid=config["uuid"]).first()
if existing:
- if overwrite and can_write and hasattr(g, "user") and g.user:
+ if overwrite and can_write and get_user():
if not security_manager.can_access_dashboard(existing):
raise ImportFailedError(
"A dashboard already exists and user doesn't "
@@ -191,7 +191,7 @@ def import_dashboard(
if dashboard.id is None:
session.flush()
- if hasattr(g, "user") and g.user:
- dashboard.owners.append(g.user)
+ if user := get_user():
+ dashboard.owners.append(user)
return dashboard
diff --git a/superset/datasets/commands/importers/v1/utils.py b/superset/datasets/commands/importers/v1/utils.py
index ae47fc411a..9107ebfef5 100644
--- a/superset/datasets/commands/importers/v1/utils.py
+++ b/superset/datasets/commands/importers/v1/utils.py
@@ -22,7 +22,7 @@ from typing import Any
from urllib import request
import pandas as pd
-from flask import current_app, g
+from flask import current_app
from sqlalchemy import BigInteger, Boolean, Date, DateTime, Float, String, Text
from sqlalchemy.orm import Session
from sqlalchemy.orm.exc import MultipleResultsFound
@@ -33,6 +33,7 @@ from superset.commands.exceptions import ImportFailedError
from superset.connectors.sqla.models import SqlaTable
from superset.datasets.commands.exceptions import DatasetForbiddenDataURI
from superset.models.core import Database
+from superset.utils.core import get_user
logger = logging.getLogger(__name__)
@@ -174,8 +175,8 @@ def import_dataset(
if data_uri and (not table_exists or force_data):
load_data(data_uri, dataset, dataset.database, session)
- if hasattr(g, "user") and g.user:
- dataset.owners.append(g.user)
+ if user := get_user():
+ dataset.owners.append(user)
return dataset
diff --git a/superset/errors.py b/superset/errors.py
index 167ec2d3b4..b78437521e 100644
--- a/superset/errors.py
+++ b/superset/errors.py
@@ -66,6 +66,7 @@ class SupersetErrorType(StrEnum):
MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR"
USER_ACTIVITY_SECURITY_ACCESS_ERROR = "USER_ACTIVITY_SECURITY_ACCESS_ERROR"
DASHBOARD_SECURITY_ACCESS_ERROR = "DASHBOARD_SECURITY_ACCESS_ERROR"
+ CHART_SECURITY_ACCESS_ERROR = "CHART_SECURITY_ACCESS_ERROR"
# Other errors
BACKEND_TIMEOUT_ERROR = "BACKEND_TIMEOUT_ERROR"
diff --git a/superset/jinja_context.py b/superset/jinja_context.py
index a736b9278e..6309d726fb 100644
--- a/superset/jinja_context.py
+++ b/superset/jinja_context.py
@@ -35,7 +35,7 @@ from superset.exceptions import SupersetTemplateException
from superset.extensions import feature_flag_manager
from superset.utils.core import (
convert_legacy_filters_into_adhoc,
- get_user_id,
+ get_user,
merge_extra_filters,
)
@@ -109,11 +109,10 @@ class ExtraCache:
:returns: The user ID
"""
- if hasattr(g, "user") and g.user:
- id_ = get_user_id()
+ if user := get_user():
if add_to_cache_keys:
- self.cache_key_wrapper(id_)
- return id_
+ self.cache_key_wrapper(user.id)
+ return user.id
return None
def current_username(self, add_to_cache_keys: bool = True) -> Optional[str]:
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 58d48112e2..e6658cddcb 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -82,6 +82,7 @@ if TYPE_CHECKING:
from superset.connectors.sqla.models import RowLevelSecurityFilter, SqlaTable
from superset.models.core import Database
from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
from superset.models.sql_lab import Query
from superset.sql_parse import Table
from superset.viz import BaseViz
@@ -420,6 +421,19 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
return True
+ def can_access_chart(self, chart: "Slice") -> bool:
+ """
+ Return True if the user can access the specified chart, False otherwise.
+ :param chart: The chart
+ :return: Whether the user can access the chart
+ """
+ try:
+ self.raise_for_access(chart=chart)
+ except SupersetSecurityException:
+ return False
+
+ return True
+
# pylint: disable=no-self-use
def get_dashboard_access_error_object( # pylint: disable=invalid-name
self,
@@ -438,6 +452,23 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
level=ErrorLevel.ERROR,
)
+ def get_chart_access_error_object(
+ self,
+ dashboard: "Dashboard", # pylint: disable=unused-argument
+ ) -> SupersetError:
+ """
+ Return the error object for the denied Superset dashboard.
+
+ :param dashboard: The denied Superset dashboard
+ :returns: The error object
+ """
+
+ return SupersetError(
+ error_type=SupersetErrorType.CHART_SECURITY_ACCESS_ERROR,
+ message="You don't have access to this chart.",
+ level=ErrorLevel.ERROR,
+ )
+
@staticmethod
def get_datasource_access_error_msg(datasource: "BaseDatasource") -> str:
"""
@@ -1800,6 +1831,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
# pylint: disable=too-many-arguments,too-many-branches,too-many-locals,too-many-statements
self,
dashboard: Optional["Dashboard"] = None,
+ chart: Optional["Slice"] = None,
database: Optional["Database"] = None,
datasource: Optional["BaseDatasource"] = None,
query: Optional["Query"] = None,
@@ -2025,6 +2057,15 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
self.get_dashboard_access_error_object(dashboard)
)
+ if chart:
+ if self.is_admin() or self.is_owner(chart):
+ return
+
+ if chart.datasource and self.can_access_datasource(chart.datasource):
+ return
+
+ raise SupersetSecurityException(self.get_chart_access_error_object(chart))
+
def get_user_by_username(
self, username: str, session: Session = None
) -> Optional[User]:
diff --git a/superset/utils/core.py b/superset/utils/core.py
index 9e3592d22b..abcb2e3203 100644
--- a/superset/utils/core.py
+++ b/superset/utils/core.py
@@ -1415,6 +1415,15 @@ def split_adhoc_filters_into_base_filters( # pylint: disable=invalid-name
form_data["filters"] = simple_where_filters
+def get_user() -> User | None:
+ """
+ Get the current user (if defined).
+
+ :returns: The current user
+ """
+ return g.user if hasattr(g, "user") else None
+
+
def get_username() -> str | None:
"""
Get username (if defined) associated with the current user.
diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py
index 5d29bfe817..43276b02dc 100644
--- a/tests/integration_tests/charts/commands_tests.py
+++ b/tests/integration_tests/charts/commands_tests.py
@@ -171,7 +171,7 @@ class TestExportChartsCommand(SupersetTestCase):
class TestImportChartsCommand(SupersetTestCase):
- @patch("superset.charts.commands.importers.v1.utils.g")
+ @patch("superset.utils.core.g")
@patch("superset.security.manager.g")
def test_import_v1_chart(self, sm_g, utils_g):
"""Test that we can import a chart"""
diff --git a/tests/integration_tests/dashboards/commands_tests.py b/tests/integration_tests/dashboards/commands_tests.py
index 14b5a1cd88..9934dd04ef 100644
--- a/tests/integration_tests/dashboards/commands_tests.py
+++ b/tests/integration_tests/dashboards/commands_tests.py
@@ -480,7 +480,7 @@ class TestImportDashboardsCommand(SupersetTestCase):
db.session.delete(dataset)
db.session.commit()
- @patch("superset.dashboards.commands.importers.v1.utils.g")
+ @patch("superset.utils.core.g")
@patch("superset.security.manager.g")
def test_import_v1_dashboard(self, sm_g, utils_g):
"""Test that we can import a dashboard"""
diff --git a/tests/integration_tests/datasets/commands_tests.py b/tests/integration_tests/datasets/commands_tests.py
index a02c6c7076..1cbc0fc02b 100644
--- a/tests/integration_tests/datasets/commands_tests.py
+++ b/tests/integration_tests/datasets/commands_tests.py
@@ -337,7 +337,7 @@ class TestImportDatasetsCommand(SupersetTestCase):
db.session.delete(dataset)
db.session.commit()
- @patch("superset.datasets.commands.importers.v1.utils.g")
+ @patch("superset.utils.core.g")
@patch("superset.security.manager.g")
@pytest.mark.usefixtures("load_energy_table_with_slice")
def test_import_v1_dataset(self, sm_g, utils_g):
diff --git a/tests/unit_tests/charts/commands/importers/v1/import_test.py b/tests/unit_tests/charts/commands/importers/v1/import_test.py
index f2c4773417..b22abb1111 100644
--- a/tests/unit_tests/charts/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/charts/commands/importers/v1/import_test.py
@@ -17,104 +17,130 @@
# pylint: disable=unused-argument, import-outside-toplevel, unused-import, invalid-name
import copy
+from collections.abc import Generator
import pytest
+from flask_appbuilder.security.sqla.models import Role, User
from pytest_mock import MockFixture
from sqlalchemy.orm.session import Session
+from superset import security_manager
+from superset.charts.commands.importers.v1.utils import import_chart
from superset.commands.exceptions import ImportFailedError
+from superset.connectors.sqla.models import Database, SqlaTable
+from superset.models.slice import Slice
+from superset.utils.core import override_user
+from tests.integration_tests.fixtures.importexport import chart_config
-def test_import_chart(mocker: MockFixture, session: Session) -> None:
+@pytest.fixture
+def session_with_data(session: Session) -> Generator[Session, None, None]:
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ dataset = SqlaTable(
+ table_name="test_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+ slice = Slice(
+ id=1,
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="tmp_perm_table",
+ slice_name="slice_name",
+ uuid=chart_config["uuid"],
+ )
+ session.add(slice)
+ session.flush()
+
+ yield session
+ session.rollback()
+
+
+@pytest.fixture
+def session_with_schema(session: Session) -> Generator[Session, None, None]:
+ from superset.connectors.sqla.models import SqlaTable
+
+ engine = session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ yield session
+
+
+def test_import_chart(mocker: MockFixture, session_with_schema: Session) -> None:
"""
Test importing a chart.
"""
- from superset import security_manager
- from superset.charts.commands.importers.v1.utils import import_chart
- from superset.connectors.sqla.models import SqlaTable
- from superset.models.core import Database
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import chart_config
mocker.patch.object(security_manager, "can_access", return_value=True)
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
-
config = copy.deepcopy(chart_config)
config["datasource_id"] = 1
config["datasource_type"] = "table"
- chart = import_chart(session, config)
+ chart = import_chart(session_with_schema, config)
assert chart.slice_name == "Deck Path"
assert chart.viz_type == "deck_path"
assert chart.is_managed_externally is False
assert chart.external_url is None
+ # Assert that the can write to chart was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Chart")
-def test_import_chart_managed_externally(mocker: MockFixture, session: Session) -> None:
+
+def test_import_chart_managed_externally(
+ mocker: MockFixture, session_with_schema: Session
+) -> None:
"""
Test importing a chart that is managed externally.
"""
- from superset import security_manager
- from superset.charts.commands.importers.v1.utils import import_chart
- from superset.connectors.sqla.models import SqlaTable
- from superset.models.core import Database
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import chart_config
-
mocker.patch.object(security_manager, "can_access", return_value=True)
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
-
config = copy.deepcopy(chart_config)
config["datasource_id"] = 1
config["datasource_type"] = "table"
config["is_managed_externally"] = True
config["external_url"] = "https://example.org/my_chart"
- chart = import_chart(session, config)
+ chart = import_chart(session_with_schema, config)
assert chart.is_managed_externally is True
assert chart.external_url == "https://example.org/my_chart"
+ # Assert that the can write to chart was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Chart")
+
def test_import_chart_without_permission(
mocker: MockFixture,
- session: Session,
+ session_with_schema: Session,
) -> None:
"""
Test importing a chart when a user doesn't have permissions to create.
"""
- from superset import security_manager
- from superset.charts.commands.importers.v1.utils import import_chart
- from superset.connectors.sqla.models import SqlaTable
- from superset.models.core import Database
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import chart_config
-
mocker.patch.object(security_manager, "can_access", return_value=False)
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
-
config = copy.deepcopy(chart_config)
config["datasource_id"] = 1
config["datasource_type"] = "table"
with pytest.raises(ImportFailedError) as excinfo:
- import_chart(session, config)
+ import_chart(session_with_schema, config)
assert (
str(excinfo.value)
== "Chart doesn't exist and user doesn't have permission to create charts"
)
+ # Assert that the can write to chart was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Chart")
-def test_filter_chart_annotations(mocker: MockFixture, session: Session) -> None:
+def test_filter_chart_annotations(session: Session) -> None:
"""
Test importing a chart.
"""
- from superset import security_manager
from superset.charts.commands.importers.v1.utils import filter_chart_annotations
from tests.integration_tests.fixtures.importexport import (
chart_config_with_mixed_annotations,
@@ -127,3 +153,67 @@ def test_filter_chart_annotations(mocker: MockFixture, session: Session) -> None
assert len(annotation_layers) == 1
assert all([al["annotationType"] == "FORMULA" for al in annotation_layers])
+
+
+def test_import_existing_chart_without_permission(
+ mocker: MockFixture,
+ session_with_data: Session,
+) -> None:
+ """
+ Test importing a chart when a user doesn't have permissions to modify.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "can_access_chart", return_value=False)
+
+ slice = (
+ session_with_data.query(Slice)
+ .filter(Slice.uuid == chart_config["uuid"])
+ .one_or_none()
+ )
+
+ with override_user("admin"):
+ with pytest.raises(ImportFailedError) as excinfo:
+ import_chart(session_with_data, chart_config, overwrite=True)
+ assert (
+ str(excinfo.value)
+ == "A chart already exists and user doesn't have permissions to overwrite it"
+ )
+
+ # Assert that the can write to chart was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Chart")
+ security_manager.can_access_chart.assert_called_once_with(slice)
+
+
+def test_import_existing_chart_with_permission(
+ mocker: MockFixture,
+ session_with_data: Session,
+) -> None:
+ """
+ Test importing a chart that exists when a user has access permission to that chart.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "can_access_chart", return_value=True)
+
+ admin = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="adoe@example.org",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ config = copy.deepcopy(chart_config)
+ config["datasource_id"] = 1
+ config["datasource_type"] = "table"
+
+ slice = (
+ session_with_data.query(Slice)
+ .filter(Slice.uuid == config["uuid"])
+ .one_or_none()
+ )
+
+ with override_user(admin):
+ import_chart(session_with_data, config, overwrite=True)
+ # Assert that the can write to chart was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Chart")
+ security_manager.can_access_chart.assert_called_once_with(slice)
diff --git a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py b/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
index 95920683b7..6777d7c848 100644
--- a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
@@ -17,32 +17,57 @@
# pylint: disable=unused-argument, import-outside-toplevel, unused-import, invalid-name
import copy
+from collections.abc import Generator
import pytest
+from flask_appbuilder.security.sqla.models import Role, User
from pytest_mock import MockFixture
from sqlalchemy.orm.session import Session
+from superset import security_manager
from superset.commands.exceptions import ImportFailedError
+from superset.dashboards.commands.importers.v1.utils import import_dashboard
+from superset.models.dashboard import Dashboard
from superset.utils.core import override_user
+from tests.integration_tests.fixtures.importexport import dashboard_config
-def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
- """
- Test importing a dashboard.
- """
- from superset import security_manager
- from superset.dashboards.commands.importers.v1.utils import import_dashboard
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import dashboard_config
+@pytest.fixture
+def session_with_data(session: Session) -> Generator[Session, None, None]:
+ engine = session.get_bind()
+ Dashboard.metadata.create_all(engine) # pylint: disable=no-member
+
+ dashboard = Dashboard(
+ id=100,
+ dashboard_title="Test dash",
+ slug=None,
+ slices=[],
+ published=True,
+ uuid=dashboard_config["uuid"],
+ )
+
+ session.add(dashboard)
+ session.flush()
+ yield session
+ session.rollback()
- mocker.patch.object(security_manager, "can_access", return_value=True)
+@pytest.fixture
+def session_with_schema(session: Session) -> Generator[Session, None, None]:
engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
+ Dashboard.metadata.create_all(engine) # pylint: disable=no-member
- config = copy.deepcopy(dashboard_config)
+ yield session
+ session.rollback()
+
+
+def test_import_dashboard(mocker: MockFixture, session_with_schema: Session) -> None:
+ """
+ Test importing a dashboard.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
- dashboard = import_dashboard(session, config)
+ dashboard = import_dashboard(session_with_schema, dashboard_config)
assert dashboard.dashboard_title == "Test dash"
assert dashboard.description is None
assert dashboard.is_managed_externally is False
@@ -53,26 +78,18 @@ def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
def test_import_dashboard_managed_externally(
mocker: MockFixture,
- session: Session,
+ session_with_schema: Session,
) -> None:
"""
Test importing a dashboard that is managed externally.
"""
- from superset import security_manager
- from superset.dashboards.commands.importers.v1.utils import import_dashboard
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import dashboard_config
-
mocker.patch.object(security_manager, "can_access", return_value=True)
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
-
config = copy.deepcopy(dashboard_config)
config["is_managed_externally"] = True
config["external_url"] = "https://example.org/my_dashboard"
- dashboard = import_dashboard(session, config)
+ dashboard = import_dashboard(session_with_schema, config)
assert dashboard.is_managed_externally is True
assert dashboard.external_url == "https://example.org/my_dashboard"
@@ -82,25 +99,15 @@ def test_import_dashboard_managed_externally(
def test_import_dashboard_without_permission(
mocker: MockFixture,
- session: Session,
+ session_with_schema: Session,
) -> None:
"""
Test importing a dashboard when a user doesn't have permissions to create.
"""
- from superset import security_manager
- from superset.dashboards.commands.importers.v1.utils import import_dashboard
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import dashboard_config
-
mocker.patch.object(security_manager, "can_access", return_value=False)
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
-
- config = copy.deepcopy(dashboard_config)
-
with pytest.raises(ImportFailedError) as excinfo:
- import_dashboard(session, config)
+ import_dashboard(session_with_schema, dashboard_config)
assert (
str(excinfo.value)
== "Dashboard doesn't exist and user doesn't have permission to create dashboards"
@@ -112,72 +119,43 @@ def test_import_dashboard_without_permission(
def test_import_existing_dashboard_without_permission(
mocker: MockFixture,
- session: Session,
+ session_with_data: Session,
) -> None:
"""
Test importing a dashboard when a user doesn't have permissions to create.
"""
- from superset import security_manager
- from superset.dashboards.commands.importers.v1.utils import g, import_dashboard
- from superset.models.dashboard import Dashboard
- from superset.models.slice import Slice
- from tests.integration_tests.fixtures.importexport import dashboard_config
-
mocker.patch.object(security_manager, "can_access", return_value=True)
mocker.patch.object(security_manager, "can_access_dashboard", return_value=False)
- mock_g = mocker.patch(
- "superset.dashboards.commands.importers.v1.utils.g"
- ) # Replace with the actual path to g
- mock_g.user = mocker.MagicMock(return_value=True)
-
- engine = session.get_bind()
- Slice.metadata.create_all(engine) # pylint: disable=no-member
- Dashboard.metadata.create_all(engine) # pylint: disable=no-member
- dashboard_obj = Dashboard(
- id=100,
- dashboard_title="Test dash",
- slug=None,
- slices=[],
- published=True,
- uuid="c4b28c4e-a1fe-4cf8-a5ac-d6f11d6fdd51",
+ dashboard = (
+ session_with_data.query(Dashboard)
+ .filter(Dashboard.uuid == dashboard_config["uuid"])
+ .one_or_none()
)
- session.add(dashboard_obj)
- session.flush()
- config = copy.deepcopy(dashboard_config)
- with pytest.raises(ImportFailedError) as excinfo:
- import_dashboard(session, config, overwrite=True)
- assert (
- str(excinfo.value)
- == "A dashboard already exists and user doesn't have permissions to overwrite it"
- )
+ with override_user("admin"):
+ with pytest.raises(ImportFailedError) as excinfo:
+ import_dashboard(session_with_data, dashboard_config, overwrite=True)
+ assert (
+ str(excinfo.value)
+ == "A dashboard already exists and user doesn't have permissions to overwrite it"
+ )
# Assert that the can write to dashboard was checked
security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
- security_manager.can_access_dashboard.assert_called_once_with(dashboard_obj)
+ security_manager.can_access_dashboard.assert_called_once_with(dashboard)
def test_import_existing_dashboard_with_permission(
mocker: MockFixture,
- session: Session,
+ session_with_data: Session,
) -> None:
"""
- Test importing a dashboard when a user doesn't have permissions to create.
+ Test importing a dashboard that exists when a user has access permission to that dashboard.
"""
- from flask_appbuilder.security.sqla.models import Role, User
-
- from superset import security_manager
- from superset.dashboards.commands.importers.v1.utils import import_dashboard
- from superset.models.dashboard import Dashboard
- from tests.integration_tests.fixtures.importexport import dashboard_config
-
mocker.patch.object(security_manager, "can_access", return_value=True)
mocker.patch.object(security_manager, "can_access_dashboard", return_value=True)
- engine = session.get_bind()
- Dashboard.metadata.create_all(engine) # pylint: disable=no-member
-
admin = User(
first_name="Alice",
last_name="Doe",
@@ -186,20 +164,14 @@ def test_import_existing_dashboard_with_permission(
roles=[Role(name="Admin")],
)
- dashboard_obj = Dashboard(
- id=100,
- dashboard_title="Test dash",
- slug=None,
- slices=[],
- published=True,
- uuid="c4b28c4e-a1fe-4cf8-a5ac-d6f11d6fdd51",
+ dashboard = (
+ session_with_data.query(Dashboard)
+ .filter(Dashboard.uuid == dashboard_config["uuid"])
+ .one_or_none()
)
- session.add(dashboard_obj)
- session.flush()
- config = copy.deepcopy(dashboard_config)
with override_user(admin):
- import_dashboard(session, config, overwrite=True)
+ import_dashboard(session_with_data, dashboard_config, overwrite=True)
# Assert that the can write to dashboard was checked
security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
- security_manager.can_access_dashboard.assert_called_once_with(dashboard_obj)
+ security_manager.can_access_dashboard.assert_called_once_with(dashboard)
diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py
index ad6e53e993..7d2b9153a3 100644
--- a/tests/unit_tests/security/manager_test.py
+++ b/tests/unit_tests/security/manager_test.py
@@ -16,12 +16,16 @@
# under the License.
import pytest
+from flask_appbuilder.security.sqla.models import Role, User
from pytest_mock import MockFixture
from superset.common.query_object import QueryObject
+from superset.connectors.sqla.models import Database, SqlaTable
from superset.exceptions import SupersetSecurityException
from superset.extensions import appbuilder
+from superset.models.slice import Slice
from superset.security.manager import SupersetSecurityManager
+from superset.utils.core import override_user
def test_security_manager(app_context: None) -> None:
@@ -164,3 +168,139 @@ def test_raise_for_access_query_default_schema(
== """You need access to the following tables: `public.ab_user`,
`all_database_access` or `all_datasource_access` permission"""
)
+
+
+def test_raise_for_access_chart_for_datasource_permission(
+ mocker: MockFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that the security manager can raise an exception for chart access,
+ when the user does not have access to the chart datasource
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ session = sm.get_session
+
+ engine = session.get_bind()
+ Slice.metadata.create_all(engine) # pylint: disable=no-member
+
+ alpha = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="adoe@example.org",
+ username="admin",
+ roles=[Role(name="Alpha")],
+ )
+
+ dataset = SqlaTable(
+ table_name="test_table",
+ metrics=[],
+ main_dttm_col=None,
+ database=Database(database_name="my_database", sqlalchemy_uri="sqlite://"),
+ )
+ session.add(dataset)
+ session.flush()
+
+ slice = Slice(
+ id=1,
+ datasource_id=dataset.id,
+ datasource_type="table",
+ datasource_name="tmp_perm_table",
+ slice_name="slice_name",
+ )
+ session.add(slice)
+ session.flush()
+
+ mocker.patch.object(sm, "can_access_datasource", return_value=False)
+ with override_user(alpha):
+ with pytest.raises(SupersetSecurityException) as excinfo:
+ sm.raise_for_access(
+ chart=slice,
+ )
+ assert str(excinfo.value) == "You don't have access to this chart."
+
+ mocker.patch.object(sm, "can_access_datasource", return_value=True)
+ with override_user(alpha):
+ sm.raise_for_access(
+ chart=slice,
+ )
+
+
+def test_raise_for_access_chart_on_admin(
+ app_context: None,
+) -> None:
+ """
+ Test that the security manager can raise an exception for chart access,
+ when the user does not have access to the chart datasource
+ """
+ from flask_appbuilder.security.sqla.models import Role, User
+
+ from superset.models.slice import Slice
+ from superset.utils.core import override_user
+
+ sm = SupersetSecurityManager(appbuilder)
+ session = sm.get_session
+
+ engine = session.get_bind()
+ Slice.metadata.create_all(engine) # pylint: disable=no-member
+
+ admin = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="adoe@example.org",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ slice = Slice(
+ id=1,
+ datasource_id=1,
+ datasource_type="table",
+ datasource_name="tmp_perm_table",
+ slice_name="slice_name",
+ )
+ session.add(slice)
+ session.flush()
+
+ with override_user(admin):
+ sm.raise_for_access(
+ chart=slice,
+ )
+
+
+def test_raise_for_access_chart_owner(
+ app_context: None,
+) -> None:
+ """
+ Test that the security manager can raise an exception for chart access,
+ when the user does not have access to the chart datasource
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ session = sm.get_session
+
+ engine = session.get_bind()
+ Slice.metadata.create_all(engine) # pylint: disable=no-member
+
+ alpha = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="adoe@example.org",
+ username="admin",
+ roles=[Role(name="Alpha")],
+ )
+
+ slice = Slice(
+ id=1,
+ datasource_id=1,
+ datasource_type="table",
+ datasource_name="tmp_perm_table",
+ slice_name="slice_name",
+ owners=[alpha],
+ )
+ session.add(slice)
+ session.flush()
+
+ with override_user(alpha):
+ sm.raise_for_access(
+ chart=slice,
+ )
(superset) 05/09: fix(explore): allow free-form d3 format on custom column formatting (#27023)
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 648e0a08d6a662ebccde875fbe64da8d08835944
Author: JUST.in DO IT <ju...@airbnb.com>
AuthorDate: Thu Feb 8 09:27:59 2024 -0800
fix(explore): allow free-form d3 format on custom column formatting (#27023)
(cherry picked from commit fd06ff3745b0ce96ef2506e18b6d5f27d3eee045)
---
superset-frontend/packages/superset-ui-chart-controls/src/types.ts | 1 +
.../src/explore/components/controls/ColumnConfigControl/constants.tsx | 1 +
2 files changed, 2 insertions(+)
diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts
index bbfaf904fb..ac5d421242 100644
--- a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts
+++ b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts
@@ -518,6 +518,7 @@ export type ControlFormItemSpec<T extends ControlType = ControlType> = {
debounceDelay?: number;
} & (T extends 'Select'
? {
+ allowNewOptions?: boolean;
options: any;
value?: string;
defaultValue?: string;
diff --git a/superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx b/superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx
index 684d8faf14..985fc2f47c 100644
--- a/superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx
+++ b/superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx
@@ -42,6 +42,7 @@ export type SharedColumnConfigProp =
| 'currencyFormat';
const d3NumberFormat: ControlFormItemSpec<'Select'> = {
+ allowNewOptions: true,
controlType: 'Select',
label: t('D3 format'),
description: D3_FORMAT_DOCS,
(superset) 03/09: fix(security manager): Users should not have access to all draft dashboards (#27015)
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit e3da64593fcb83e78986cbc03c51ba2c42405fcc
Author: Vitor Avila <96...@users.noreply.github.com>
AuthorDate: Wed Feb 7 13:20:41 2024 -0300
fix(security manager): Users should not have access to all draft dashboards (#27015)
(cherry picked from commit 01e2f8ace31950ca337a6a8d7348d37c59cf8126)
---
superset/security/manager.py | 31 ++++++++++++----------
.../dashboards/security/security_rbac_tests.py | 24 ++++++++++-------
2 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index f7f8662b5d..58d48112e2 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1996,25 +1996,28 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
if self.is_admin() or self.is_owner(dashboard):
return
- # RBAC and legacy (datasource inferred) access controls.
+ # TODO: Once a better sharing flow is in place, we should move the
+ # dashboard.published check here so that it's applied to both
+ # regular RBAC and DASHBOARD_RBAC
+
+ # DASHBOARD_RBAC logic - Manage dashboard access through roles.
+ # Only applicable in case the dashboard has roles set.
if is_feature_enabled("DASHBOARD_RBAC") and dashboard.roles:
if dashboard.published and {role.id for role in dashboard.roles} & {
role.id for role in self.get_user_roles()
}:
return
- elif (
- # To understand why we rely on status and give access to draft dashboards
- # without roles take a look at:
- #
- # - https://github.com/apache/superset/pull/24350#discussion_r1225061550
- # - https://github.com/apache/superset/pull/17511#issuecomment-975870169
- #
- not dashboard.published
- or not dashboard.datasources
- or any(
- self.can_access_datasource(datasource)
- for datasource in dashboard.datasources
- )
+
+ # REGULAR RBAC logic
+ # User can only acess the dashboard in case:
+ # It doesn't have any datasets; OR
+ # They have access to at least one dataset used.
+ # We currently don't check if the dashboard is published,
+ # to allow creators to share a WIP dashboard with a viewer
+ # to collect feedback.
+ elif not dashboard.datasources or any(
+ self.can_access_datasource(datasource)
+ for datasource in dashboard.datasources
):
return
diff --git a/tests/integration_tests/dashboards/security/security_rbac_tests.py b/tests/integration_tests/dashboards/security/security_rbac_tests.py
index 8b7f2ad1ef..ab8da1e3b7 100644
--- a/tests/integration_tests/dashboards/security/security_rbac_tests.py
+++ b/tests/integration_tests/dashboards/security/security_rbac_tests.py
@@ -404,22 +404,28 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
for dash in published_dashboards + draft_dashboards:
revoke_access_to_dashboard(dash, "Public")
- def test_get_draft_dashboard_without_roles_by_uuid(self):
+ def test_cannot_get_draft_dashboard_without_roles_by_uuid(self):
"""
Dashboard API: Test get draft dashboard without roles by uuid
"""
admin = self.get_user("admin")
- dashboard = self.insert_dashboard("title", "slug1", [admin.id])
- assert not dashboard.published
- assert dashboard.roles == []
+
+ database = create_database_to_db(name="test_db_rbac")
+ table = create_datasource_table_to_db(
+ name="test_datasource_rbac", db_id=database.id, owners=[admin]
+ )
+ dashboard_to_access = create_dashboard_to_db(
+ dashboard_title="test_dashboard_rbac",
+ owners=[admin],
+ slices=[create_slice_to_db(datasource_id=table.id)],
+ )
+ assert not dashboard_to_access.published
+ assert dashboard_to_access.roles == []
self.login(username="gamma")
- uri = f"api/v1/dashboard/{dashboard.uuid}"
+ uri = f"api/v1/dashboard/{dashboard_to_access.uuid}"
rv = self.client.get(uri)
- assert rv.status_code == 200
- # rollback changes
- db.session.delete(dashboard)
- db.session.commit()
+ assert rv.status_code == 403
def test_cannot_get_draft_dashboard_with_roles_by_uuid(self):
"""
(superset) 02/09: fix: column values with NaN (#26946)
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 383b4d949fdffb6e86e5e3ab53fcaeeb9eb27301
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Fri Feb 2 12:56:02 2024 -0500
fix: column values with NaN (#26946)
(cherry picked from commit d8a98475036a4fba28b3d3eb508b3d1f3f5072aa)
---
superset/models/helpers.py | 7 ++++++-
tests/integration_tests/conftest.py | 21 +++++++++++----------
tests/integration_tests/datasource/api_tests.py | 10 ++++++++++
tests/integration_tests/datasource_tests.py | 9 ++++++++-
4 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index f9b30c3d42..849f3db176 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -1332,7 +1332,10 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
return and_(*l)
def values_for_column(
- self, column_name: str, limit: int = 10000, denormalize_column: bool = False
+ self,
+ column_name: str,
+ limit: int = 10000,
+ denormalize_column: bool = False,
) -> list[Any]:
# denormalize column name before querying for values
# unless disabled in the dataset configuration
@@ -1370,6 +1373,8 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
sql = self.mutate_query_from_config(sql)
df = pd.read_sql_query(sql=sql, con=engine)
+ # replace NaN with None to ensure it can be serialized to JSON
+ df = df.replace({np.nan: None})
return df["column_values"].to_list()
def get_timestamp_expression(
diff --git a/tests/integration_tests/conftest.py b/tests/integration_tests/conftest.py
index 28da7b7913..4134514af3 100644
--- a/tests/integration_tests/conftest.py
+++ b/tests/integration_tests/conftest.py
@@ -296,25 +296,25 @@ def virtual_dataset():
dataset = SqlaTable(
table_name="virtual_dataset",
sql=(
- "SELECT 0 as col1, 'a' as col2, 1.0 as col3, NULL as col4, '2000-01-01 00:00:00' as col5 "
+ "SELECT 0 as col1, 'a' as col2, 1.0 as col3, NULL as col4, '2000-01-01 00:00:00' as col5, 1 as col6 "
"UNION ALL "
- "SELECT 1, 'b', 1.1, NULL, '2000-01-02 00:00:00' "
+ "SELECT 1, 'b', 1.1, NULL, '2000-01-02 00:00:00', NULL "
"UNION ALL "
- "SELECT 2 as col1, 'c' as col2, 1.2, NULL, '2000-01-03 00:00:00' "
+ "SELECT 2 as col1, 'c' as col2, 1.2, NULL, '2000-01-03 00:00:00', 3 "
"UNION ALL "
- "SELECT 3 as col1, 'd' as col2, 1.3, NULL, '2000-01-04 00:00:00' "
+ "SELECT 3 as col1, 'd' as col2, 1.3, NULL, '2000-01-04 00:00:00', 4 "
"UNION ALL "
- "SELECT 4 as col1, 'e' as col2, 1.4, NULL, '2000-01-05 00:00:00' "
+ "SELECT 4 as col1, 'e' as col2, 1.4, NULL, '2000-01-05 00:00:00', 5 "
"UNION ALL "
- "SELECT 5 as col1, 'f' as col2, 1.5, NULL, '2000-01-06 00:00:00' "
+ "SELECT 5 as col1, 'f' as col2, 1.5, NULL, '2000-01-06 00:00:00', 6 "
"UNION ALL "
- "SELECT 6 as col1, 'g' as col2, 1.6, NULL, '2000-01-07 00:00:00' "
+ "SELECT 6 as col1, 'g' as col2, 1.6, NULL, '2000-01-07 00:00:00', 7 "
"UNION ALL "
- "SELECT 7 as col1, 'h' as col2, 1.7, NULL, '2000-01-08 00:00:00' "
+ "SELECT 7 as col1, 'h' as col2, 1.7, NULL, '2000-01-08 00:00:00', 8 "
"UNION ALL "
- "SELECT 8 as col1, 'i' as col2, 1.8, NULL, '2000-01-09 00:00:00' "
+ "SELECT 8 as col1, 'i' as col2, 1.8, NULL, '2000-01-09 00:00:00', 9 "
"UNION ALL "
- "SELECT 9 as col1, 'j' as col2, 1.9, NULL, '2000-01-10 00:00:00' "
+ "SELECT 9 as col1, 'j' as col2, 1.9, NULL, '2000-01-10 00:00:00', 10"
),
database=get_example_database(),
)
@@ -324,6 +324,7 @@ def virtual_dataset():
TableColumn(column_name="col4", type="VARCHAR(255)", table=dataset)
# Different database dialect datetime type is not consistent, so temporarily use varchar
TableColumn(column_name="col5", type="VARCHAR(255)", table=dataset)
+ TableColumn(column_name="col6", type="INTEGER", table=dataset)
SqlMetric(metric_name="count", expression="count(*)", table=dataset)
db.session.merge(dataset)
diff --git a/tests/integration_tests/datasource/api_tests.py b/tests/integration_tests/datasource/api_tests.py
index b6a6af105d..2605ec399f 100644
--- a/tests/integration_tests/datasource/api_tests.py
+++ b/tests/integration_tests/datasource/api_tests.py
@@ -72,6 +72,16 @@ class TestDatasourceApi(SupersetTestCase):
response = json.loads(rv.data.decode("utf-8"))
self.assertEqual(response["result"], [None])
+ @pytest.mark.usefixtures("app_context", "virtual_dataset")
+ def test_get_column_values_integers_with_nulls(self):
+ self.login(username="admin")
+ table = self.get_virtual_dataset()
+ rv = self.client.get(f"api/v1/datasource/table/{table.id}/column/col6/values/")
+ self.assertEqual(rv.status_code, 200)
+ response = json.loads(rv.data.decode("utf-8"))
+ for val in [1, None, 3, 4, 5, 6, 7, 8, 9, 10]:
+ assert val in response["result"]
+
@pytest.mark.usefixtures("app_context", "virtual_dataset")
def test_get_column_values_invalid_datasource_type(self):
self.login(username="admin")
diff --git a/tests/integration_tests/datasource_tests.py b/tests/integration_tests/datasource_tests.py
index 12293325d1..59ed9117e7 100644
--- a/tests/integration_tests/datasource_tests.py
+++ b/tests/integration_tests/datasource_tests.py
@@ -545,7 +545,14 @@ def test_get_samples_with_filters(test_client, login_as_admin, virtual_dataset):
},
)
assert rv.status_code == 200
- assert rv.json["result"]["colnames"] == ["col1", "col2", "col3", "col4", "col5"]
+ assert rv.json["result"]["colnames"] == [
+ "col1",
+ "col2",
+ "col3",
+ "col4",
+ "col5",
+ "col6",
+ ]
assert rv.json["result"]["rowcount"] == 1
# empty results
(superset) 08/09: fix(drill): no rows returned (#27073)
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit e772915bb8288adad51720f0f5e0d01f050d3e16
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Mon Feb 12 12:11:06 2024 -0500
fix(drill): no rows returned (#27073)
(cherry picked from commit 0950bb7b7dd4658a112cc90e2d813267836ae002)
---
setup.py | 2 +-
superset/db_engine_specs/drill.py | 37 ++++++++++++++++++++++++++++++-------
2 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/setup.py b/setup.py
index e428797094..c29e224500 100644
--- a/setup.py
+++ b/setup.py
@@ -149,7 +149,7 @@ setup(
],
"db2": ["ibm-db-sa>0.3.8, <=0.4.0"],
"dremio": ["sqlalchemy-dremio>=1.1.5, <1.3"],
- "drill": ["sqlalchemy-drill==0.1.dev"],
+ "drill": ["sqlalchemy-drill>=1.1.4, <2"],
"druid": ["pydruid>=0.6.5,<0.7"],
"dynamodb": ["pydynamodb>=0.4.2"],
"solr": ["sqlalchemy-solr >= 0.2.0"],
diff --git a/superset/db_engine_specs/drill.py b/superset/db_engine_specs/drill.py
index fb42409b4e..276ff5b185 100644
--- a/superset/db_engine_specs/drill.py
+++ b/superset/db_engine_specs/drill.py
@@ -14,8 +14,11 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+
+from __future__ import annotations
+
from datetime import datetime
-from typing import Any, Optional
+from typing import Any
from urllib import parse
from sqlalchemy import types
@@ -60,8 +63,8 @@ class DrillEngineSpec(BaseEngineSpec):
@classmethod
def convert_dttm(
- cls, target_type: str, dttm: datetime, db_extra: Optional[dict[str, Any]] = None
- ) -> Optional[str]:
+ cls, target_type: str, dttm: datetime, db_extra: dict[str, Any] | None = None
+ ) -> str | None:
sqla_type = cls.get_sqla_column_type(target_type)
if isinstance(sqla_type, types.Date):
@@ -76,8 +79,8 @@ class DrillEngineSpec(BaseEngineSpec):
cls,
uri: URL,
connect_args: dict[str, Any],
- catalog: Optional[str] = None,
- schema: Optional[str] = None,
+ catalog: str | None = None,
+ schema: str | None = None,
) -> tuple[URL, dict[str, Any]]:
if schema:
uri = uri.set(database=parse.quote(schema.replace(".", "/"), safe=""))
@@ -89,7 +92,7 @@ class DrillEngineSpec(BaseEngineSpec):
cls,
sqlalchemy_uri: URL,
connect_args: dict[str, Any],
- ) -> Optional[str]:
+ ) -> str | None:
"""
Return the configured schema.
"""
@@ -97,7 +100,7 @@ class DrillEngineSpec(BaseEngineSpec):
@classmethod
def get_url_for_impersonation(
- cls, url: URL, impersonate_user: bool, username: Optional[str]
+ cls, url: URL, impersonate_user: bool, username: str | None
) -> URL:
"""
Return a modified URL with the username set.
@@ -117,3 +120,23 @@ class DrillEngineSpec(BaseEngineSpec):
)
return url
+
+ @classmethod
+ def fetch_data(
+ cls,
+ cursor: Any,
+ limit: int | None = None,
+ ) -> list[tuple[Any, ...]]:
+ """
+ Custom `fetch_data` for Drill.
+
+ When no rows are returned, Drill raises a `RuntimeError` with the message
+ "generator raised StopIteration". This method catches the exception and
+ returns an empty list instead.
+ """
+ try:
+ return super().fetch_data(cursor, limit)
+ except RuntimeError as ex:
+ if str(ex) == "generator raised StopIteration":
+ return []
+ raise
(superset) 06/09: fix(plugins): missing currency on small number format in table chart (#27041)
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit db56b7fc367a0f51d66c417d651fed3b5b816254
Author: JUST.in DO IT <ju...@airbnb.com>
AuthorDate: Thu Feb 8 15:24:24 2024 -0800
fix(plugins): missing currency on small number format in table chart (#27041)
(cherry picked from commit 6f402991e54ae6ab0c6c98613d7e831c7f847f54)
---
.../plugin-chart-table/src/utils/formatValue.ts | 6 ++++
.../plugin-chart-table/test/TableChart.test.tsx | 42 ++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts b/superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts
index 139f92336c..043fa6b59d 100644
--- a/superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts
+++ b/superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts
@@ -17,6 +17,7 @@
* under the License.
*/
import {
+ CurrencyFormatter,
DataRecordValue,
GenericDataType,
getNumberFormatter,
@@ -64,6 +65,11 @@ export function formatColumnValue(
const smallNumberFormatter =
config.d3SmallNumberFormat === undefined
? formatter
+ : config.currencyFormat
+ ? new CurrencyFormatter({
+ d3Format: config.d3SmallNumberFormat,
+ currency: config.currencyFormat,
+ })
: getNumberFormatter(config.d3SmallNumberFormat);
return formatValue(
isNumber && typeof value === 'number' && Math.abs(value) < 1
diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
index d6998476ba..52cfab1662 100644
--- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
+++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
@@ -166,6 +166,48 @@ describe('plugin-chart-table', () => {
expect(cells[2]).toHaveTextContent('$ 0');
});
+ it('render small formatted data with currencies', () => {
+ const props = transformProps({
+ ...testData.raw,
+ rawFormData: {
+ ...testData.raw.rawFormData,
+ column_config: {
+ num: {
+ d3SmallNumberFormat: '.2r',
+ currencyFormat: { symbol: 'USD', symbolPosition: 'prefix' },
+ },
+ },
+ },
+ queriesData: [
+ {
+ ...testData.raw.queriesData[0],
+ data: [
+ {
+ num: 1234,
+ },
+ {
+ num: 0.5,
+ },
+ {
+ num: 0.61234,
+ },
+ ],
+ },
+ ],
+ });
+ render(
+ ProviderWrapper({
+ children: <TableChart {...props} sticky={false} />,
+ }),
+ );
+ const cells = document.querySelectorAll('td');
+
+ expect(document.querySelectorAll('th')[0]).toHaveTextContent('num');
+ expect(cells[0]).toHaveTextContent('$ 1.23k');
+ expect(cells[1]).toHaveTextContent('$ 0.50');
+ expect(cells[2]).toHaveTextContent('$ 0.61');
+ });
+
it('render empty data', () => {
wrap.setProps({ ...transformProps(testData.empty), sticky: false });
tree = wrap.render();
(superset) 01/09: fix(plugin-chart-table): Prevent misalignment of totals and headers when scrollbar is visible (#26964)
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit d1dad1e63d7646b290331fa28f9104f9e46c549b
Author: Kamil Gabryjelski <ka...@gmail.com>
AuthorDate: Thu Feb 1 18:27:22 2024 +0100
fix(plugin-chart-table): Prevent misalignment of totals and headers when scrollbar is visible (#26964)
(cherry picked from commit e6d2fb6fdfa4d741de16b322bdc4bd01fb559413)
---
.../plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx b/superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx
index 067d071ee1..e154a521f6 100644
--- a/superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx
+++ b/superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx
@@ -226,6 +226,7 @@ function StickyWrap({
height: maxHeight,
overflow: 'auto',
visibility: 'hidden',
+ scrollbarGutter: 'stable',
}}
>
{React.cloneElement(table, {}, theadWithRef, tbody, tfootWithRef)}
@@ -252,6 +253,7 @@ function StickyWrap({
ref={scrollHeaderRef}
style={{
overflow: 'hidden',
+ scrollbarGutter: 'stable',
}}
>
{React.cloneElement(
@@ -270,6 +272,7 @@ function StickyWrap({
ref={scrollFooterRef}
style={{
overflow: 'hidden',
+ scrollbarGutter: 'stable',
}}
>
{React.cloneElement(
@@ -297,6 +300,7 @@ function StickyWrap({
style={{
height: bodyHeight,
overflow: 'auto',
+ scrollbarGutter: 'stable',
}}
onScroll={sticky.hasHorizontalScroll ? onScroll : undefined}
>