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/01 16:43:07 UTC
(superset) branch 3.0 updated (c739ce786f -> a31331c1f0)
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 c739ce786f fix(time-series table): Can't compare from the beginning of the time range (#26814)
new fb64100043 feat(sqlparse): improve table parsing (#26476)
new c816142a5e fix(deck.gl Multiple Layer Chart): Add Contour and Heatmap Layer as options (#25923)
new e453059413 fix: prevent guest user from modifying metrics (#26749)
new 89527c6c97 fix: Bar charts horizontal margin adjustment error (#26817)
new af079fb13b fix(pinot): typo in the name for epoch_ms_to_dttm (#26906)
new efae3e7ce5 fix: handle CRLF endings causing sqlglot failure (#26911)
new 9133837501 fix: dashboard import validation (#26887)
new e82b815296 fix: Allow exporting saved queries without schema information (#26889)
new a31331c1f0 fix(cache): remove unused webserver config & handle trailing slashes (#22849)
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:
requirements/base.txt | 2 +
requirements/testing.txt | 6 +-
setup.py | 1 +
.../legacy-preset-chart-deckgl/src/index.js | 2 +
.../src/layers/Arc/Arc.jsx | 8 +-
.../src/layers/Geojson/Geojson.jsx | 4 +-
.../src/layers/Heatmap/Heatmap.tsx | 2 +-
.../src/layers/Path/Path.jsx | 2 +-
.../src/layers/Polygon/Polygon.jsx | 10 +-
.../src/layers/Scatter/Scatter.jsx | 10 +-
.../src/layers/Screengrid/Screengrid.jsx | 4 +-
.../legacy-preset-chart-deckgl/src/layers/index.js | 4 +
.../src/Timeseries/transformProps.ts | 1 +
.../src/Timeseries/transformers.ts | 42 ++--
.../plugin-chart-echarts/src/utils/series.ts | 14 ++
.../plugin-chart-echarts/test/utils/series.test.ts | 32 +++
superset/common/query_object.py | 2 +-
superset/config.py | 4 -
superset/connectors/sqla/models.py | 2 +-
superset/connectors/sqla/utils.py | 2 +-
superset/dashboards/commands/importers/v1/utils.py | 8 +-
superset/datasets/commands/duplicate.py | 5 +-
superset/db_engine_specs/base.py | 12 +-
superset/db_engine_specs/bigquery.py | 2 +-
superset/db_engine_specs/pinot.py | 2 +-
superset/models/helpers.py | 2 +-
superset/models/sql_lab.py | 6 +-
superset/queries/saved_queries/commands/export.py | 11 +-
superset/security/manager.py | 31 ++-
superset/sql_lab.py | 11 +-
superset/sql_parse.py | 247 ++++++++++++++-------
superset/sql_validators/presto_db.py | 4 +-
superset/sqllab/commands/export.py | 5 +-
superset/sqllab/query_render.py | 6 +-
superset/tasks/cache.py | 4 +-
.../db_engine_specs/pinot_tests.py | 14 ++
.../security/guest_token_security_tests.py | 15 ++
tests/integration_tests/superset_test_config.py | 1 -
.../superset_test_config_thumbnails.py | 1 -
tests/integration_tests/tasks/test_cache.py | 58 +++++
.../commands/importers/v1/import_test.py | 110 ++++++++-
tests/unit_tests/security/manager_test.py | 76 +++++++
tests/unit_tests/sql_parse_tests.py | 55 ++++-
43 files changed, 663 insertions(+), 177 deletions(-)
create mode 100644 tests/integration_tests/tasks/test_cache.py
(superset) 02/09: fix(deck.gl Multiple Layer Chart): Add Contour and Heatmap Layer as options (#25923)
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 c816142a5ea08ba95fe5944ef2d590e60e17bbaa
Author: Matthew Chiang <36...@users.noreply.github.com>
AuthorDate: Mon Jan 29 05:10:11 2024 -0800
fix(deck.gl Multiple Layer Chart): Add Contour and Heatmap Layer as options (#25923)
---
.../plugins/legacy-preset-chart-deckgl/src/index.js | 2 ++
.../plugins/legacy-preset-chart-deckgl/src/layers/Arc/Arc.jsx | 8 ++++----
.../legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.jsx | 4 ++--
.../legacy-preset-chart-deckgl/src/layers/Heatmap/Heatmap.tsx | 2 +-
.../legacy-preset-chart-deckgl/src/layers/Path/Path.jsx | 2 +-
.../legacy-preset-chart-deckgl/src/layers/Polygon/Polygon.jsx | 10 +++++-----
.../legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.jsx | 10 +++++-----
.../src/layers/Screengrid/Screengrid.jsx | 4 ++--
.../plugins/legacy-preset-chart-deckgl/src/layers/index.js | 4 ++++
9 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/index.js b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/index.js
index 819964173e..fc4aa7fca0 100644
--- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/index.js
+++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/index.js
@@ -26,3 +26,5 @@ export { default as PathChartPlugin } from './layers/Path';
export { default as PolygonChartPlugin } from './layers/Polygon';
export { default as ScatterChartPlugin } from './layers/Scatter';
export { default as ScreengridChartPlugin } from './layers/Screengrid';
+export { default as ContourChartPlugin } from './layers/Contour';
+export { default as HeatmapChartPlugin } from './layers/Heatmap';
diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Arc/Arc.jsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Arc/Arc.jsx
index 75cf8d09a1..07f3b55b8f 100644
--- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Arc/Arc.jsx
+++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Arc/Arc.jsx
@@ -38,16 +38,16 @@ function setTooltipContent(formData) {
<div className="deckgl-tooltip">
<TooltipRow
label={t('Start (Longitude, Latitude): ')}
- value={`${o.object.sourcePosition[0]}, ${o.object.sourcePosition[1]}`}
+ value={`${o.object?.sourcePosition?.[0]}, ${o.object?.sourcePosition?.[1]}`}
/>
<TooltipRow
label={t('End (Longitude, Latitude): ')}
- value={`${o.object.targetPosition[0]}, ${o.object.targetPosition[1]}`}
+ value={`${o.object?.targetPosition?.[0]}, ${o.object?.targetPosition?.[1]}`}
/>
{formData.dimension && (
<TooltipRow
- label={`${formData.dimension}: `}
- value={`${o.object.cat_color}`}
+ label={`${formData?.dimension}: `}
+ value={`${o.object?.cat_color}`}
/>
)}
</div>
diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.jsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.jsx
index 263b576ec9..07e6972ea9 100644
--- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.jsx
+++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/Geojson.jsx
@@ -80,13 +80,13 @@ const recurseGeoJson = (node, propOverrides, extraProps) => {
function setTooltipContent(o) {
return (
- o.object.extraProps && (
+ o.object?.extraProps && (
<div className="deckgl-tooltip">
{Object.keys(o.object.extraProps).map((prop, index) => (
<TooltipRow
key={`prop-${index}`}
label={`${prop}: `}
- value={`${o.object.extraProps[prop]}`}
+ value={`${o.object.extraProps?.[prop]}`}
/>
))}
</div>
diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Heatmap/Heatmap.tsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Heatmap/Heatmap.tsx
index 2bd1f63ce7..72d27b2ba7 100644
--- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Heatmap/Heatmap.tsx
+++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Heatmap/Heatmap.tsx
@@ -30,7 +30,7 @@ function setTooltipContent(o: any) {
<div className="deckgl-tooltip">
<TooltipRow
label={t('Centroid (Longitude and Latitude): ')}
- value={`(${o.coordinate[0]}, ${o.coordinate[1]})`}
+ value={`(${o?.coordinate[0]}, ${o?.coordinate[1]})`}
/>
</div>
);
diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/Path.jsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/Path.jsx
index 0cea73e2b7..db47eb023d 100644
--- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/Path.jsx
+++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/Path.jsx
@@ -26,7 +26,7 @@ import TooltipRow from '../../TooltipRow';
function setTooltipContent(o) {
return (
- o.object.extraProps && (
+ o.object?.extraProps && (
<div className="deckgl-tooltip">
{Object.keys(o.object.extraProps).map((prop, index) => (
<TooltipRow
diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/Polygon.jsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/Polygon.jsx
index 81df4384f9..f43009c9e0 100644
--- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/Polygon.jsx
+++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/Polygon.jsx
@@ -53,27 +53,27 @@ function getElevation(d, colorScaler) {
function setTooltipContent(formData) {
return o => {
- const metricLabel = formData.metric.label || formData.metric;
+ const metricLabel = formData?.metric?.label || formData?.metric;
return (
<div className="deckgl-tooltip">
- {o.object.name && (
+ {o.object?.name && (
<TooltipRow
// eslint-disable-next-line prefer-template
label={t('name') + ': '}
value={`${o.object.name}`}
/>
)}
- {o.object[formData.line_column] && (
+ {o.object?.[formData?.line_column] && (
<TooltipRow
label={`${formData.line_column}: `}
value={`${o.object[formData.line_column]}`}
/>
)}
- {formData.metric && (
+ {formData?.metric && (
<TooltipRow
label={`${metricLabel}: `}
- value={`${o.object[metricLabel]}`}
+ value={`${o.object?.[metricLabel]}`}
/>
)}
</div>
diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.jsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.jsx
index 5237523067..a08a3819f9 100644
--- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.jsx
+++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.jsx
@@ -38,17 +38,17 @@ function setTooltipContent(formData, verboseMap) {
<TooltipRow
// eslint-disable-next-line prefer-template
label={t('Longitude and Latitude') + ': '}
- value={`${o.object.position[0]}, ${o.object.position[1]}`}
+ value={`${o.object?.position?.[0]}, ${o.object?.position?.[1]}`}
/>
- {o.object.cat_color && (
+ {o.object?.cat_color && (
<TooltipRow
// eslint-disable-next-line prefer-template
label={t('Category') + ': '}
- value={`${o.object.cat_color}`}
+ value={`${o.object?.cat_color}`}
/>
)}
- {o.object.metric && (
- <TooltipRow label={`${label}: `} value={`${o.object.metric}`} />
+ {o.object?.metric && (
+ <TooltipRow label={`${label}: `} value={`${o.object?.metric}`} />
)}
</div>
);
diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Screengrid/Screengrid.jsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Screengrid/Screengrid.jsx
index 7883dda17e..0d5ad72ac7 100644
--- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Screengrid/Screengrid.jsx
+++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Screengrid/Screengrid.jsx
@@ -42,12 +42,12 @@ function setTooltipContent(o) {
<TooltipRow
// eslint-disable-next-line prefer-template
label={t('Longitude and Latitude') + ': '}
- value={`${o.coordinate[0]}, ${o.coordinate[1]}`}
+ value={`${o?.coordinate?.[0]}, ${o?.coordinate?.[1]}`}
/>
<TooltipRow
// eslint-disable-next-line prefer-template
label={t('Weight') + ': '}
- value={`${o.object.cellWeight}`}
+ value={`${o.object?.cellWeight}`}
/>
</div>
);
diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/index.js b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/index.js
index b77d5bd12c..9747a50b1e 100644
--- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/index.js
+++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/index.js
@@ -25,6 +25,8 @@ import { getLayer as deck_scatter } from './Scatter/Scatter';
import { getLayer as deck_geojson } from './Geojson/Geojson';
import { getLayer as deck_arc } from './Arc/Arc';
import { getLayer as deck_polygon } from './Polygon/Polygon';
+import { getLayer as deck_heatmap } from './Heatmap/Heatmap';
+import { getLayer as deck_contour } from './Contour/Contour';
const layerGenerators = {
deck_grid,
@@ -35,6 +37,8 @@ const layerGenerators = {
deck_geojson,
deck_arc,
deck_polygon,
+ deck_heatmap,
+ deck_contour,
};
export default layerGenerators;
(superset) 05/09: fix(pinot): typo in the name for epoch_ms_to_dttm (#26906)
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 af079fb13b8f1ee84f20cc2d3cb30f060265028b
Author: Erich <13...@users.noreply.github.com>
AuthorDate: Tue Jan 30 23:49:55 2024 -0500
fix(pinot): typo in the name for epoch_ms_to_dttm (#26906)
(cherry picked from commit 484901f4832b64845931f728db3e367f7f7c562c)
---
superset/db_engine_specs/pinot.py | 2 +-
tests/integration_tests/db_engine_specs/pinot_tests.py | 14 ++++++++++++++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/superset/db_engine_specs/pinot.py b/superset/db_engine_specs/pinot.py
index 2cafd5ecb0..0f53cfa77b 100644
--- a/superset/db_engine_specs/pinot.py
+++ b/superset/db_engine_specs/pinot.py
@@ -62,7 +62,7 @@ class PinotEngineSpec(BaseEngineSpec):
)
@classmethod
- def epoch_ms_to_dttm_(cls) -> str:
+ def epoch_ms_to_dttm(cls) -> str:
return (
"DATETIMECONVERT({col}, '1:MILLISECONDS:EPOCH', "
+ "'1:MILLISECONDS:EPOCH', '1:MILLISECONDS')"
diff --git a/tests/integration_tests/db_engine_specs/pinot_tests.py b/tests/integration_tests/db_engine_specs/pinot_tests.py
index 3998d20940..c8deef6fc4 100755
--- a/tests/integration_tests/db_engine_specs/pinot_tests.py
+++ b/tests/integration_tests/db_engine_specs/pinot_tests.py
@@ -84,6 +84,20 @@ class TestPinotDbEngineSpec(TestDbEngineSpec):
expected,
)
+ def test_pinot_time_expression_millisec_one_1m_grain(self):
+ col = column("tstamp")
+ expr = PinotEngineSpec.get_timestamp_expr(col, "epoch_ms", "P1M")
+ result = str(expr.compile())
+ expected = (
+ "CAST(DATE_TRUNC('month', CAST("
+ + "DATETIMECONVERT(tstamp, '1:MILLISECONDS:EPOCH', "
+ + "'1:MILLISECONDS:EPOCH', '1:MILLISECONDS') AS TIMESTAMP)) AS TIMESTAMP)"
+ )
+ self.assertEqual(
+ result,
+ expected,
+ )
+
def test_invalid_get_time_expression_arguments(self):
with self.assertRaises(NotImplementedError):
PinotEngineSpec.get_timestamp_expr(column("tstamp"), None, "P0.25Y")
(superset) 03/09: fix: prevent guest user from modifying metrics (#26749)
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 e453059413ee14c8eb1200c3855ffa9a2722b040
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Mon Jan 29 12:59:33 2024 -0500
fix: prevent guest user from modifying metrics (#26749)
(cherry picked from commit fade4806ceebde32a775c04d86a46c7e93bc371f)
---
superset/common/query_object.py | 2 +-
superset/security/manager.py | 26 +++++++-
.../security/guest_token_security_tests.py | 15 +++++
tests/unit_tests/security/manager_test.py | 76 ++++++++++++++++++++++
4 files changed, 117 insertions(+), 2 deletions(-)
diff --git a/superset/common/query_object.py b/superset/common/query_object.py
index 1e826761ec..ea56a9072b 100644
--- a/superset/common/query_object.py
+++ b/superset/common/query_object.py
@@ -125,7 +125,7 @@ class QueryObject: # pylint: disable=too-many-instance-attributes
order_desc: bool = True,
orderby: list[OrderBy] | None = None,
post_processing: list[dict[str, Any] | None] | None = None,
- row_limit: int | None,
+ row_limit: int | None = None,
row_offset: int | None = None,
series_columns: list[Column] | None = None,
series_limit: int = 0,
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 5c0833fdf9..f7f8662b5d 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1797,7 +1797,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
return []
def raise_for_access(
- # pylint: disable=too-many-arguments,too-many-branches,too-many-locals
+ # pylint: disable=too-many-arguments,too-many-branches,too-many-locals,too-many-statements
self,
dashboard: Optional["Dashboard"] = None,
database: Optional["Database"] = None,
@@ -1887,6 +1887,30 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
self.get_table_access_error_object(denied)
)
+ if self.is_guest_user() and query_context:
+ # Guest users MUST not modify the payload so it's requesting a different
+ # chart or different ad-hoc metrics from what's saved.
+ form_data = query_context.form_data
+ stored_chart = query_context.slice_
+
+ if (
+ form_data is None
+ or stored_chart is None
+ or form_data.get("slice_id") != stored_chart.id
+ or form_data.get("metrics", []) != stored_chart.params_dict["metrics"]
+ or any(
+ query.metrics != stored_chart.params_dict["metrics"]
+ for query in query_context.queries
+ )
+ ):
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR,
+ message=_("Guest user cannot modify chart payload"),
+ level=ErrorLevel.ERROR,
+ )
+ )
+
if datasource or query_context or viz:
form_data = None
diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py
index 4cd8a77f76..b812929433 100644
--- a/tests/integration_tests/security/guest_token_security_tests.py
+++ b/tests/integration_tests/security/guest_token_security_tests.py
@@ -288,7 +288,10 @@ class TestGuestUserDatasourceAccess(SupersetTestCase):
form_data={
"dashboardId": self.dash.id,
"slice_id": self.chart.id,
+ "metrics": self.chart.params_dict["metrics"],
},
+ slice_=self.chart,
+ queries=[],
)
}
)
@@ -304,7 +307,11 @@ class TestGuestUserDatasourceAccess(SupersetTestCase):
"dashboardId": self.dash.id,
"native_filter_id": "NATIVE_FILTER-ABCDEFGH",
"type": "NATIVE_FILTER",
+ "slice_id": self.chart.id,
+ "metrics": self.chart.params_dict["metrics"],
},
+ slice_=self.chart,
+ queries=[],
)
}
)
@@ -382,7 +389,11 @@ class TestGuestUserDatasourceAccess(SupersetTestCase):
form_data={
"dashboardId": self.dash.id,
"type": "NATIVE_FILTER",
+ "slice_id": self.chart.id,
+ "metrics": self.chart.params_dict["metrics"],
},
+ slice_=self.chart,
+ queries=[],
)
}
)
@@ -399,7 +410,11 @@ class TestGuestUserDatasourceAccess(SupersetTestCase):
"dashboardId": self.dash.id,
"native_filter_id": "NATIVE_FILTER-ABCDEFGH",
"type": "NATIVE_FILTER",
+ "slice_id": self.chart.id,
+ "metrics": self.chart.params_dict["metrics"],
},
+ slice_=self.chart,
+ queries=[],
)
}
)
diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py
index 1843e7261c..ad6e53e993 100644
--- a/tests/unit_tests/security/manager_test.py
+++ b/tests/unit_tests/security/manager_test.py
@@ -18,6 +18,7 @@
import pytest
from pytest_mock import MockFixture
+from superset.common.query_object import QueryObject
from superset.exceptions import SupersetSecurityException
from superset.extensions import appbuilder
from superset.security.manager import SupersetSecurityManager
@@ -31,6 +32,81 @@ def test_security_manager(app_context: None) -> None:
assert sm
+def test_raise_for_access_guest_user(
+ mocker: MockFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that guest user can't modify chart payload.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ mocker.patch.object(sm, "is_guest_user", return_value=True)
+ mocker.patch.object(sm, "can_access", return_value=True)
+
+ query_context = mocker.MagicMock()
+ query_context.slice_.id = 42
+ stored_metrics = [
+ {
+ "aggregate": None,
+ "column": None,
+ "datasourceWarning": False,
+ "expressionType": "SQL",
+ "hasCustomLabel": False,
+ "label": "COUNT(*) + 1",
+ "optionName": "metric_ssa1gwimio_cxpyjc7vj3s",
+ "sqlExpression": "COUNT(*) + 1",
+ }
+ ]
+ query_context.slice_.params_dict = {
+ "metrics": stored_metrics,
+ }
+
+ # normal request
+ query_context.form_data = {
+ "slice_id": 42,
+ "metrics": stored_metrics,
+ }
+ query_context.queries = [QueryObject(metrics=stored_metrics)] # type: ignore
+ sm.raise_for_access(query_context=query_context)
+
+ # tampered requests
+ query_context.form_data = {
+ "slice_id": 43,
+ "metrics": stored_metrics,
+ }
+ query_context.queries = [QueryObject(metrics=stored_metrics)] # type: ignore
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(query_context=query_context)
+
+ tampered_metrics = [
+ {
+ "aggregate": None,
+ "column": None,
+ "datasourceWarning": False,
+ "expressionType": "SQL",
+ "hasCustomLabel": False,
+ "label": "COUNT(*) + 2",
+ "optionName": "metric_ssa1gwimio_cxpyjc7vj3s",
+ "sqlExpression": "COUNT(*) + 2",
+ }
+ ]
+
+ query_context.form_data = {
+ "slice_id": 42,
+ "metrics": tampered_metrics,
+ }
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(query_context=query_context)
+
+ query_context.form_data = {
+ "slice_id": 42,
+ "metrics": stored_metrics,
+ }
+ query_context.queries = [QueryObject(metrics=tampered_metrics)] # type: ignore
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(query_context=query_context)
+
+
def test_raise_for_access_query_default_schema(
mocker: MockFixture,
app_context: None,
(superset) 07/09: fix: dashboard import validation (#26887)
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 9133837501f77fe5f7cda5e81fb7b2d2ab2d86b9
Author: Daniel Vaz Gaspar <da...@gmail.com>
AuthorDate: Thu Feb 1 10:09:59 2024 +0000
fix: dashboard import validation (#26887)
---
superset/dashboards/commands/importers/v1/utils.py | 8 +-
.../commands/importers/v1/import_test.py | 110 +++++++++++++++++++--
2 files changed, 111 insertions(+), 7 deletions(-)
diff --git a/superset/dashboards/commands/importers/v1/utils.py b/superset/dashboards/commands/importers/v1/utils.py
index 1deb44949a..712161bc16 100644
--- a/superset/dashboards/commands/importers/v1/utils.py
+++ b/superset/dashboards/commands/importers/v1/utils.py
@@ -157,7 +157,13 @@ def import_dashboard(
)
existing = session.query(Dashboard).filter_by(uuid=config["uuid"]).first()
if existing:
- if not overwrite or not can_write:
+ if overwrite and can_write and hasattr(g, "user") and g.user:
+ if not security_manager.can_access_dashboard(existing):
+ raise ImportFailedError(
+ "A dashboard already exists and user doesn't "
+ "have permissions to overwrite it"
+ )
+ elif not overwrite or not can_write:
return existing
config["id"] = existing.id
elif not can_write:
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 e07a23f6bf..95920683b7 100644
--- a/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/dashboards/commands/importers/v1/import_test.py
@@ -23,6 +23,7 @@ from pytest_mock import MockFixture
from sqlalchemy.orm.session import Session
from superset.commands.exceptions import ImportFailedError
+from superset.utils.core import override_user
def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
@@ -30,9 +31,7 @@ def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
Test importing a dashboard.
"""
from superset import security_manager
- from superset.connectors.sqla.models import SqlaTable
from superset.dashboards.commands.importers.v1.utils import import_dashboard
- from superset.models.core import Database
from superset.models.slice import Slice
from tests.integration_tests.fixtures.importexport import dashboard_config
@@ -48,6 +47,8 @@ def test_import_dashboard(mocker: MockFixture, session: Session) -> None:
assert dashboard.description is None
assert dashboard.is_managed_externally is False
assert dashboard.external_url is None
+ # Assert that the can write to dashboard was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
def test_import_dashboard_managed_externally(
@@ -58,9 +59,7 @@ def test_import_dashboard_managed_externally(
Test importing a dashboard that is managed externally.
"""
from superset import security_manager
- from superset.connectors.sqla.models import SqlaTable
from superset.dashboards.commands.importers.v1.utils import import_dashboard
- from superset.models.core import Database
from superset.models.slice import Slice
from tests.integration_tests.fixtures.importexport import dashboard_config
@@ -77,6 +76,9 @@ def test_import_dashboard_managed_externally(
assert dashboard.is_managed_externally is True
assert dashboard.external_url == "https://example.org/my_dashboard"
+ # Assert that the can write to dashboard was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
+
def test_import_dashboard_without_permission(
mocker: MockFixture,
@@ -86,9 +88,7 @@ def test_import_dashboard_without_permission(
Test importing a dashboard when a user doesn't have permissions to create.
"""
from superset import security_manager
- from superset.connectors.sqla.models import SqlaTable
from superset.dashboards.commands.importers.v1.utils import import_dashboard
- from superset.models.core import Database
from superset.models.slice import Slice
from tests.integration_tests.fixtures.importexport import dashboard_config
@@ -105,3 +105,101 @@ def test_import_dashboard_without_permission(
str(excinfo.value)
== "Dashboard doesn't exist and user doesn't have permission to create dashboards"
)
+
+ # Assert that the can write to dashboard was checked
+ security_manager.can_access.assert_called_once_with("can_write", "Dashboard")
+
+
+def test_import_existing_dashboard_without_permission(
+ mocker: MockFixture,
+ session: 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",
+ )
+ 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"
+ )
+
+ # 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)
+
+
+def test_import_existing_dashboard_with_permission(
+ mocker: MockFixture,
+ session: Session,
+) -> None:
+ """
+ Test importing a dashboard when a user doesn't have permissions to create.
+ """
+ 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",
+ email="adoe@example.org",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ dashboard_obj = Dashboard(
+ id=100,
+ dashboard_title="Test dash",
+ slug=None,
+ slices=[],
+ published=True,
+ uuid="c4b28c4e-a1fe-4cf8-a5ac-d6f11d6fdd51",
+ )
+ session.add(dashboard_obj)
+ session.flush()
+ config = copy.deepcopy(dashboard_config)
+
+ with override_user(admin):
+ import_dashboard(session, 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)
(superset) 08/09: fix: Allow exporting saved queries without schema information (#26889)
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 e82b81529639a3cedeba32fcaa98e682033fa9a3
Author: Sebastian Bernauer <se...@stackable.de>
AuthorDate: Thu Feb 1 16:12:29 2024 +0100
fix: Allow exporting saved queries without schema information (#26889)
(cherry picked from commit 4c5176eea82e3b168c5d11f130387d5913b33efa)
---
superset/queries/saved_queries/commands/export.py | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/superset/queries/saved_queries/commands/export.py b/superset/queries/saved_queries/commands/export.py
index 1b85cda796..9fcd90b5f2 100644
--- a/superset/queries/saved_queries/commands/export.py
+++ b/superset/queries/saved_queries/commands/export.py
@@ -40,11 +40,16 @@ class ExportSavedQueriesCommand(ExportModelsCommand):
def _export(
model: SavedQuery, export_related: bool = True
) -> Iterator[tuple[str, str]]:
- # build filename based on database, optional schema, and label
+ # build filename based on database, optional schema, and label.
+ # we call secure_filename() multiple times and join the directories afterwards,
+ # as secure_filename() replaces "/" with "_".
database_slug = secure_filename(model.database.database_name)
- schema_slug = secure_filename(model.schema)
query_slug = secure_filename(model.label) or str(model.uuid)
- file_name = f"queries/{database_slug}/{schema_slug}/{query_slug}.yaml"
+ if model.schema is None:
+ file_name = f"queries/{database_slug}/{query_slug}.yaml"
+ else:
+ schema_slug = secure_filename(model.schema)
+ file_name = f"queries/{database_slug}/{schema_slug}/{query_slug}.yaml"
payload = model.export_to_dict(
recursive=False,
(superset) 09/09: fix(cache): remove unused webserver config & handle trailing slashes (#22849)
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 a31331c1f00c7d0d4586d8e2d889f8673d8ef432
Author: Usiel Riedl <us...@automattic.com>
AuthorDate: Thu Feb 1 07:34:07 2024 -0800
fix(cache): remove unused webserver config & handle trailing slashes (#22849)
---
superset/config.py | 4 --
superset/tasks/cache.py | 4 +-
tests/integration_tests/superset_test_config.py | 1 -
.../superset_test_config_thumbnails.py | 1 -
tests/integration_tests/tasks/test_cache.py | 58 ++++++++++++++++++++++
5 files changed, 60 insertions(+), 8 deletions(-)
diff --git a/superset/config.py b/superset/config.py
index 6454ba6b7d..21e61586ec 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -156,10 +156,6 @@ FILTER_SELECT_ROW_LIMIT = 10000
# values may be "Last day", "Last week", "<ISO date> : now", etc.
DEFAULT_TIME_FILTER = NO_TIME_RANGE
-SUPERSET_WEBSERVER_PROTOCOL = "http"
-SUPERSET_WEBSERVER_ADDRESS = "0.0.0.0"
-SUPERSET_WEBSERVER_PORT = 8088
-
# This is an important setting, and should be lower than your
# [load balancer / proxy / envoy / kong / ...] timeout settings.
# You should also make sure to configure your WSGI server
diff --git a/superset/tasks/cache.py b/superset/tasks/cache.py
index 01f6351919..1f60a5cd84 100644
--- a/superset/tasks/cache.py
+++ b/superset/tasks/cache.py
@@ -32,6 +32,7 @@ from superset.models.slice import Slice
from superset.tags.models import Tag, TaggedObject
from superset.utils.date_parser import parse_human_datetime
from superset.utils.machine_auth import MachineAuthProvider
+from superset.utils.urls import get_url_path
logger = get_task_logger(__name__)
logger.setLevel(logging.INFO)
@@ -233,8 +234,7 @@ def fetch_url(data: str, headers: dict[str, str]) -> dict[str, str]:
"""
result = {}
try:
- baseurl = "{WEBDRIVER_BASEURL}".format(**app.config)
- url = f"{baseurl}api/v1/chart/warm_up_cache"
+ url = get_url_path("Superset.warm_up_cache")
logger.info("Fetching %s with payload %s", url, data)
req = request.Request(
url, data=bytes(data, "utf-8"), headers=headers, method="PUT"
diff --git a/tests/integration_tests/superset_test_config.py b/tests/integration_tests/superset_test_config.py
index fd0da9f65f..42e4721ffe 100644
--- a/tests/integration_tests/superset_test_config.py
+++ b/tests/integration_tests/superset_test_config.py
@@ -36,7 +36,6 @@ SQLALCHEMY_DATABASE_URI = "sqlite:///" + os.path.join(
DATA_DIR, "unittests.integration_tests.db"
)
DEBUG = False
-SUPERSET_WEBSERVER_PORT = 8081
SILENCE_FAB = False
# Allowing SQLALCHEMY_DATABASE_URI and SQLALCHEMY_EXAMPLES_URI to be defined as an env vars for
# continuous integration
diff --git a/tests/integration_tests/superset_test_config_thumbnails.py b/tests/integration_tests/superset_test_config_thumbnails.py
index 5bd02e7b0f..29efab53f4 100644
--- a/tests/integration_tests/superset_test_config_thumbnails.py
+++ b/tests/integration_tests/superset_test_config_thumbnails.py
@@ -24,7 +24,6 @@ SQLALCHEMY_DATABASE_URI = "sqlite:///" + os.path.join(
DATA_DIR, "unittests.integration_tests.db"
)
DEBUG = True
-SUPERSET_WEBSERVER_PORT = 8081
# Allowing SQLALCHEMY_DATABASE_URI to be defined as an env var for
# continuous integration
diff --git a/tests/integration_tests/tasks/test_cache.py b/tests/integration_tests/tasks/test_cache.py
new file mode 100644
index 0000000000..943b444f76
--- /dev/null
+++ b/tests/integration_tests/tasks/test_cache.py
@@ -0,0 +1,58 @@
+# 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.
+from unittest import mock
+
+import pytest
+
+from tests.integration_tests.test_app import app
+
+
+@pytest.mark.parametrize(
+ "base_url",
+ [
+ "http://base-url",
+ "http://base-url/",
+ ],
+ ids=["Without trailing slash", "With trailing slash"],
+)
+@mock.patch("superset.tasks.cache.request.Request")
+@mock.patch("superset.tasks.cache.request.urlopen")
+def test_fetch_url(mock_urlopen, mock_request_cls, base_url):
+ from superset.tasks.cache import fetch_url
+
+ mock_request = mock.MagicMock()
+ mock_request_cls.return_value = mock_request
+
+ mock_urlopen.return_value = mock.MagicMock()
+ mock_urlopen.return_value.code = 200
+
+ app.config["WEBDRIVER_BASEURL"] = base_url
+ headers = {"key": "value"}
+ data = "data"
+ data_encoded = b"data"
+
+ result = fetch_url(data, headers)
+
+ assert data == result["success"]
+ mock_request_cls.assert_called_once_with(
+ "http://base-url/superset/warm_up_cache/",
+ data=data_encoded,
+ headers=headers,
+ method="PUT",
+ )
+ # assert the same Request object is used
+ mock_urlopen.assert_called_once_with(mock_request, timeout=mock.ANY)
(superset) 06/09: fix: handle CRLF endings causing sqlglot failure (#26911)
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 efae3e7ce53d17146169f03a29738c6b5c4f4d57
Author: mapledan <ma...@gmail.com>
AuthorDate: Thu Feb 1 10:07:43 2024 +0800
fix: handle CRLF endings causing sqlglot failure (#26911)
(cherry picked from commit f2bf9f72e4f17604f5db80f25815525236a7269a)
---
superset/sql_parse.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/superset/sql_parse.py b/superset/sql_parse.py
index 080572eba1..771ca32928 100644
--- a/superset/sql_parse.py
+++ b/superset/sql_parse.py
@@ -283,7 +283,7 @@ class ParsedQuery:
Note: this uses sqlglot, since it's better at catching more edge cases.
"""
try:
- statements = parse(self.sql, dialect=self._dialect)
+ statements = parse(self.stripped(), dialect=self._dialect)
except ParseError:
logger.warning("Unable to parse SQL (%s): %s", self._dialect, self.sql)
return set()
@@ -493,7 +493,7 @@ class ParsedQuery:
return self._parsed[0].get_type() == "UNKNOWN"
def stripped(self) -> str:
- return self.sql.strip(" \t\n;")
+ return self.sql.strip(" \t\r\n;")
def strip_comments(self) -> str:
return sqlparse.format(self.stripped(), strip_comments=True)
(superset) 01/09: feat(sqlparse): improve table parsing (#26476)
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 fb6410004369c73bae12bb60c5b5ca7981c6cf49
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Mon Jan 22 11:16:50 2024 -0500
feat(sqlparse): improve table parsing (#26476)
---
requirements/base.txt | 2 +
requirements/testing.txt | 6 +-
setup.py | 1 +
superset/connectors/sqla/models.py | 2 +-
superset/connectors/sqla/utils.py | 2 +-
superset/datasets/commands/duplicate.py | 5 +-
superset/db_engine_specs/base.py | 12 +-
superset/db_engine_specs/bigquery.py | 2 +-
superset/models/helpers.py | 2 +-
superset/models/sql_lab.py | 6 +-
superset/security/manager.py | 5 +-
superset/sql_lab.py | 11 +-
superset/sql_parse.py | 245 +++++++++++++++++++++-----------
superset/sql_validators/presto_db.py | 4 +-
superset/sqllab/commands/export.py | 5 +-
superset/sqllab/query_render.py | 6 +-
tests/unit_tests/sql_parse_tests.py | 55 +++++--
17 files changed, 253 insertions(+), 118 deletions(-)
diff --git a/requirements/base.txt b/requirements/base.txt
index c65c7ba77e..a34df1905d 100644
--- a/requirements/base.txt
+++ b/requirements/base.txt
@@ -283,6 +283,8 @@ sqlalchemy-utils==0.38.3
# via
# apache-superset
# flask-appbuilder
+sqlglot==20.8.0
+ # via apache-superset
sqlparse==0.4.4
# via apache-superset
sshtunnel==0.4.0
diff --git a/requirements/testing.txt b/requirements/testing.txt
index 0a4d826c8c..e41ac7d96f 100644
--- a/requirements/testing.txt
+++ b/requirements/testing.txt
@@ -24,10 +24,6 @@ db-dtypes==1.1.1
# via pandas-gbq
docker==6.1.1
# via -r requirements/testing.in
-ephem==4.1.4
- # via lunarcalendar
-exceptiongroup==1.1.1
- # via pytest
flask-testing==0.8.1
# via -r requirements/testing.in
fonttools==4.39.4
@@ -119,7 +115,7 @@ pyee==9.0.4
# via playwright
pyfakefs==5.2.2
# via -r requirements/testing.in
-pyhive[presto]==0.6.5
+pyhive[presto]==0.7.0
# via apache-superset
pytest==7.3.1
# via
diff --git a/setup.py b/setup.py
index 211f8c5a80..e428797094 100644
--- a/setup.py
+++ b/setup.py
@@ -121,6 +121,7 @@ setup(
"slack_sdk>=3.19.0, <4",
"sqlalchemy>=1.4, <2",
"sqlalchemy-utils>=0.38.3, <0.39",
+ "sqlglot>=20,<21",
"sqlparse>=0.4.4, <0.5",
"tabulate>=0.8.9, <0.9",
"typing-extensions>=4, <5",
diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py
index 5edc724b23..4bcf3fc344 100644
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -848,7 +848,7 @@ class SqlaTable(
return self.get_sqla_table(), None
from_sql = self.get_rendered_sql(template_processor)
- parsed_query = ParsedQuery(from_sql)
+ parsed_query = ParsedQuery(from_sql, engine=self.db_engine_spec.engine)
if not (
parsed_query.is_unknown()
or self.db_engine_spec.is_readonly_query(parsed_query)
diff --git a/superset/connectors/sqla/utils.py b/superset/connectors/sqla/utils.py
index c8a5f9f260..3b64bec807 100644
--- a/superset/connectors/sqla/utils.py
+++ b/superset/connectors/sqla/utils.py
@@ -111,7 +111,7 @@ def get_virtual_table_metadata(dataset: SqlaTable) -> list[ResultSetColumnType]:
sql = dataset.get_template_processor().process_template(
dataset.sql, **dataset.template_params_dict
)
- parsed_query = ParsedQuery(sql)
+ parsed_query = ParsedQuery(sql, engine=db_engine_spec.engine)
if not db_engine_spec.is_readonly_query(parsed_query):
raise SupersetSecurityException(
SupersetError(
diff --git a/superset/datasets/commands/duplicate.py b/superset/datasets/commands/duplicate.py
index 9fc05c0960..238f9a2391 100644
--- a/superset/datasets/commands/duplicate.py
+++ b/superset/datasets/commands/duplicate.py
@@ -69,7 +69,10 @@ class DuplicateDatasetCommand(CreateMixin, BaseCommand):
table.template_params = self._base_model.template_params
table.normalize_columns = self._base_model.normalize_columns
table.is_sqllab_view = True
- table.sql = ParsedQuery(self._base_model.sql).stripped()
+ table.sql = ParsedQuery(
+ self._base_model.sql,
+ engine=database.db_engine_spec.engine,
+ ).stripped()
db.session.add(table)
cols = []
for config_ in self._base_model.columns:
diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py
index c086ce27b7..be0b55b3e9 100644
--- a/superset/db_engine_specs/base.py
+++ b/superset/db_engine_specs/base.py
@@ -874,7 +874,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
return database.compile_sqla_query(qry)
if cls.limit_method == LimitMethod.FORCE_LIMIT:
- parsed_query = sql_parse.ParsedQuery(sql)
+ parsed_query = sql_parse.ParsedQuery(sql, engine=cls.engine)
sql = parsed_query.set_or_update_query_limit(limit, force=force)
return sql
@@ -955,7 +955,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
:param sql: SQL query
:return: Value of limit clause in query
"""
- parsed_query = sql_parse.ParsedQuery(sql)
+ parsed_query = sql_parse.ParsedQuery(sql, engine=cls.engine)
return parsed_query.limit
@classmethod
@@ -967,7 +967,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
:param limit: New limit to insert/replace into query
:return: Query with new limit
"""
- parsed_query = sql_parse.ParsedQuery(sql)
+ parsed_query = sql_parse.ParsedQuery(sql, engine=cls.engine)
return parsed_query.set_or_update_query_limit(limit)
@classmethod
@@ -1450,7 +1450,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
:param database: Database instance
:return: Dictionary with different costs
"""
- parsed_query = ParsedQuery(statement)
+ parsed_query = ParsedQuery(statement, engine=cls.engine)
sql = parsed_query.stripped()
sql_query_mutator = current_app.config["SQL_QUERY_MUTATOR"]
mutate_after_split = current_app.config["MUTATE_AFTER_SPLIT"]
@@ -1483,7 +1483,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
if not cls.get_allow_cost_estimate(extra):
raise Exception("Database does not support cost estimation")
- parsed_query = sql_parse.ParsedQuery(sql)
+ parsed_query = sql_parse.ParsedQuery(sql, engine=cls.engine)
statements = parsed_query.get_statements()
costs = []
@@ -1544,7 +1544,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
:return:
"""
if not cls.allows_sql_comments:
- query = sql_parse.strip_comments_from_sql(query)
+ query = sql_parse.strip_comments_from_sql(query, engine=cls.engine)
if cls.arraysize:
cursor.arraysize = cls.arraysize
diff --git a/superset/db_engine_specs/bigquery.py b/superset/db_engine_specs/bigquery.py
index 73b7b18d36..125760ec7f 100644
--- a/superset/db_engine_specs/bigquery.py
+++ b/superset/db_engine_specs/bigquery.py
@@ -436,7 +436,7 @@ class BigQueryEngineSpec(BaseEngineSpec): # pylint: disable=too-many-public-met
if not cls.get_allow_cost_estimate(extra):
raise SupersetException("Database does not support cost estimation")
- parsed_query = sql_parse.ParsedQuery(sql)
+ parsed_query = sql_parse.ParsedQuery(sql, engine=cls.engine)
statements = parsed_query.get_statements()
costs = []
for statement in statements:
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index bf33451e34..f9b30c3d42 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -1085,7 +1085,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
"""
from_sql = self.get_rendered_sql(template_processor)
- parsed_query = ParsedQuery(from_sql)
+ parsed_query = ParsedQuery(from_sql, engine=self.db_engine_spec.engine)
if not (
parsed_query.is_unknown()
or self.db_engine_spec.is_readonly_query(parsed_query)
diff --git a/superset/models/sql_lab.py b/superset/models/sql_lab.py
index 20df535ad3..156add2529 100644
--- a/superset/models/sql_lab.py
+++ b/superset/models/sql_lab.py
@@ -183,7 +183,7 @@ class Query(
@property
def sql_tables(self) -> list[Table]:
- return list(ParsedQuery(self.sql).tables)
+ return list(ParsedQuery(self.sql, engine=self.db_engine_spec.engine).tables)
@property
def columns(self) -> list["TableColumn"]:
@@ -428,7 +428,9 @@ class SavedQuery(Model, AuditMixinNullable, ExtraJSONMixin, ImportExportMixin):
@property
def sql_tables(self) -> list[Table]:
- return list(ParsedQuery(self.sql).tables)
+ return list(
+ ParsedQuery(self.sql, engine=self.database.db_engine_spec.engine).tables
+ )
@property
def last_run_humanized(self) -> str:
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 88657e8f21..5c0833fdf9 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1855,7 +1855,10 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
default_schema = database.get_default_schema_for_query(query)
tables = {
Table(table_.table, table_.schema or default_schema)
- for table_ in sql_parse.ParsedQuery(query.sql).tables
+ for table_ in sql_parse.ParsedQuery(
+ query.sql,
+ engine=database.db_engine_spec.engine,
+ ).tables
}
elif table:
tables = {table}
diff --git a/superset/sql_lab.py b/superset/sql_lab.py
index ca157b3240..6da1125458 100644
--- a/superset/sql_lab.py
+++ b/superset/sql_lab.py
@@ -203,7 +203,7 @@ def execute_sql_statement( # pylint: disable=too-many-arguments
database: Database = query.database
db_engine_spec = database.db_engine_spec
- parsed_query = ParsedQuery(sql_statement)
+ parsed_query = ParsedQuery(sql_statement, engine=db_engine_spec.engine)
if is_feature_enabled("RLS_IN_SQLLAB"):
# Insert any applicable RLS predicates
parsed_query = ParsedQuery(
@@ -213,7 +213,8 @@ def execute_sql_statement( # pylint: disable=too-many-arguments
database.id,
query.schema,
)
- )
+ ),
+ engine=db_engine_spec.engine,
)
sql = parsed_query.stripped()
@@ -404,7 +405,11 @@ def execute_sql_statements(
)
# Breaking down into multiple statements
- parsed_query = ParsedQuery(rendered_query, strip_comments=True)
+ parsed_query = ParsedQuery(
+ rendered_query,
+ strip_comments=True,
+ engine=db_engine_spec.engine,
+ )
if not db_engine_spec.run_multiple_statements_as_one:
statements = parsed_query.get_statements()
logger.info(
diff --git a/superset/sql_parse.py b/superset/sql_parse.py
index c196fdabfa..080572eba1 100644
--- a/superset/sql_parse.py
+++ b/superset/sql_parse.py
@@ -14,15 +14,20 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+
import logging
import re
-from collections.abc import Iterator
+import urllib.parse
+from collections.abc import Iterable, Iterator
from dataclasses import dataclass
from typing import Any, cast, Optional
-from urllib import parse
import sqlparse
from sqlalchemy import and_
+from sqlglot import exp, parse, parse_one
+from sqlglot.dialects import Dialects
+from sqlglot.errors import ParseError
+from sqlglot.optimizer.scope import Scope, ScopeType, traverse_scope
from sqlparse import keywords
from sqlparse.lexer import Lexer
from sqlparse.sql import (
@@ -52,7 +57,7 @@ from superset.utils.backports import StrEnum
try:
from sqloxide import parse_sql as sqloxide_parse
-except: # pylint: disable=bare-except
+except (ImportError, ModuleNotFoundError):
sqloxide_parse = None
RESULT_OPERATIONS = {"UNION", "INTERSECT", "EXCEPT", "SELECT"}
@@ -71,6 +76,59 @@ sqlparser_sql_regex.insert(25, (r"'(''|\\\\|\\|[^'])*'", sqlparse.tokens.String.
lex.set_SQL_REGEX(sqlparser_sql_regex)
+# mapping between DB engine specs and sqlglot dialects
+SQLGLOT_DIALECTS = {
+ "ascend": Dialects.HIVE,
+ "awsathena": Dialects.PRESTO,
+ "bigquery": Dialects.BIGQUERY,
+ "clickhouse": Dialects.CLICKHOUSE,
+ "clickhousedb": Dialects.CLICKHOUSE,
+ "cockroachdb": Dialects.POSTGRES,
+ # "crate": ???
+ # "databend": ???
+ "databricks": Dialects.DATABRICKS,
+ # "db2": ???
+ # "dremio": ???
+ "drill": Dialects.DRILL,
+ # "druid": ???
+ "duckdb": Dialects.DUCKDB,
+ # "dynamodb": ???
+ # "elasticsearch": ???
+ # "exa": ???
+ # "firebird": ???
+ # "firebolt": ???
+ "gsheets": Dialects.SQLITE,
+ "hana": Dialects.POSTGRES,
+ "hive": Dialects.HIVE,
+ # "ibmi": ???
+ # "impala": ???
+ # "kustokql": ???
+ # "kylin": ???
+ # "mssql": ???
+ "mysql": Dialects.MYSQL,
+ "netezza": Dialects.POSTGRES,
+ # "ocient": ???
+ # "odelasticsearch": ???
+ "oracle": Dialects.ORACLE,
+ # "pinot": ???
+ "postgresql": Dialects.POSTGRES,
+ "presto": Dialects.PRESTO,
+ "pydoris": Dialects.DORIS,
+ "redshift": Dialects.REDSHIFT,
+ # "risingwave": ???
+ # "rockset": ???
+ "shillelagh": Dialects.SQLITE,
+ "snowflake": Dialects.SNOWFLAKE,
+ # "solr": ???
+ "sqlite": Dialects.SQLITE,
+ "starrocks": Dialects.STARROCKS,
+ "superset": Dialects.SQLITE,
+ "teradatasql": Dialects.TERADATA,
+ "trino": Dialects.TRINO,
+ "vertica": Dialects.POSTGRES,
+}
+
+
class CtasMethod(StrEnum):
TABLE = "TABLE"
VIEW = "VIEW"
@@ -149,7 +207,7 @@ def get_cte_remainder_query(sql: str) -> tuple[Optional[str], str]:
return cte, remainder
-def strip_comments_from_sql(statement: str) -> str:
+def strip_comments_from_sql(statement: str, engine: Optional[str] = None) -> str:
"""
Strips comments from a SQL statement, does a simple test first
to avoid always instantiating the expensive ParsedQuery constructor
@@ -159,7 +217,11 @@ def strip_comments_from_sql(statement: str) -> str:
:param statement: A string with the SQL statement
:return: SQL statement without comments
"""
- return ParsedQuery(statement).strip_comments() if "--" in statement else statement
+ return (
+ ParsedQuery(statement, engine=engine).strip_comments()
+ if "--" in statement
+ else statement
+ )
@dataclass(eq=True, frozen=True)
@@ -178,7 +240,7 @@ class Table:
"""
return ".".join(
- parse.quote(part, safe="").replace(".", "%2E")
+ urllib.parse.quote(part, safe="").replace(".", "%2E")
for part in [self.catalog, self.schema, self.table]
if part
)
@@ -188,11 +250,17 @@ class Table:
class ParsedQuery:
- def __init__(self, sql_statement: str, strip_comments: bool = False):
+ def __init__(
+ self,
+ sql_statement: str,
+ strip_comments: bool = False,
+ engine: Optional[str] = None,
+ ):
if strip_comments:
sql_statement = sqlparse.format(sql_statement, strip_comments=True)
self.sql: str = sql_statement
+ self._dialect = SQLGLOT_DIALECTS.get(engine) if engine else None
self._tables: set[Table] = set()
self._alias_names: set[str] = set()
self._limit: Optional[int] = None
@@ -205,14 +273,95 @@ class ParsedQuery:
@property
def tables(self) -> set[Table]:
if not self._tables:
- for statement in self._parsed:
- self._extract_from_token(statement)
-
- self._tables = {
- table for table in self._tables if str(table) not in self._alias_names
- }
+ self._tables = self._extract_tables_from_sql()
return self._tables
+ def _extract_tables_from_sql(self) -> set[Table]:
+ """
+ Extract all table references in a query.
+
+ Note: this uses sqlglot, since it's better at catching more edge cases.
+ """
+ try:
+ statements = parse(self.sql, dialect=self._dialect)
+ except ParseError:
+ logger.warning("Unable to parse SQL (%s): %s", self._dialect, self.sql)
+ return set()
+
+ return {
+ table
+ for statement in statements
+ for table in self._extract_tables_from_statement(statement)
+ if statement
+ }
+
+ def _extract_tables_from_statement(self, statement: exp.Expression) -> set[Table]:
+ """
+ Extract all table references in a single statement.
+
+ Please not that this is not trivial; consider the following queries:
+
+ DESCRIBE some_table;
+ SHOW PARTITIONS FROM some_table;
+ WITH masked_name AS (SELECT * FROM some_table) SELECT * FROM masked_name;
+
+ See the unit tests for other tricky cases.
+ """
+ sources: Iterable[exp.Table]
+
+ if isinstance(statement, exp.Describe):
+ # A `DESCRIBE` query has no sources in sqlglot, so we need to explicitly
+ # query for all tables.
+ sources = statement.find_all(exp.Table)
+ elif isinstance(statement, exp.Command):
+ # Commands, like `SHOW COLUMNS FROM foo`, have to be converted into a
+ # `SELECT` statetement in order to extract tables.
+ literal = statement.find(exp.Literal)
+ if not literal:
+ return set()
+
+ pseudo_query = parse_one(f"SELECT {literal.this}", dialect=self._dialect)
+ sources = pseudo_query.find_all(exp.Table)
+ else:
+ sources = [
+ source
+ for scope in traverse_scope(statement)
+ for source in scope.sources.values()
+ if isinstance(source, exp.Table) and not self._is_cte(source, scope)
+ ]
+
+ return {
+ Table(
+ source.name,
+ source.db if source.db != "" else None,
+ source.catalog if source.catalog != "" else None,
+ )
+ for source in sources
+ }
+
+ # pylint: disable=no-self-use
+ def _is_cte(self, source: exp.Table, scope: Scope) -> bool:
+ """
+ Is the source a CTE?
+
+ CTEs in the parent scope look like tables (and are represented by
+ exp.Table objects), but should not be considered as such;
+ otherwise a user with access to table `foo` could access any table
+ with a query like this:
+
+ WITH foo AS (SELECT * FROM target_table) SELECT * FROM foo
+
+ """
+ parent_sources = scope.parent.sources if scope.parent else {}
+ ctes_in_scope = {
+ name
+ for name, parent_scope in parent_sources.items()
+ if isinstance(parent_scope, Scope)
+ and parent_scope.scope_type == ScopeType.CTE
+ }
+
+ return source.name in ctes_in_scope
+
@property
def limit(self) -> Optional[int]:
return self._limit
@@ -393,28 +542,6 @@ class ParsedQuery:
def _is_identifier(token: Token) -> bool:
return isinstance(token, (IdentifierList, Identifier))
- def _process_tokenlist(self, token_list: TokenList) -> None:
- """
- Add table names to table set
-
- :param token_list: TokenList to be processed
- """
- # exclude subselects
- if "(" not in str(token_list):
- table = self.get_table(token_list)
- if table and not table.table.startswith(CTE_PREFIX):
- self._tables.add(table)
- return
-
- # store aliases
- if token_list.has_alias():
- self._alias_names.add(token_list.get_alias())
-
- # some aliases are not parsed properly
- if token_list.tokens[0].ttype == Name:
- self._alias_names.add(token_list.tokens[0].value)
- self._extract_from_token(token_list)
-
def as_create_table(
self,
table_name: str,
@@ -441,50 +568,6 @@ class ParsedQuery:
exec_sql += f"CREATE {method} {full_table_name} AS \n{sql}"
return exec_sql
- def _extract_from_token(self, token: Token) -> None:
- """
- <Identifier> store a list of subtokens and <IdentifierList> store lists of
- subtoken list.
-
- It extracts <IdentifierList> and <Identifier> from :param token: and loops
- through all subtokens recursively. It finds table_name_preceding_token and
- passes <IdentifierList> and <Identifier> to self._process_tokenlist to populate
- self._tables.
-
- :param token: instance of Token or child class, e.g. TokenList, to be processed
- """
- if not hasattr(token, "tokens"):
- return
-
- table_name_preceding_token = False
-
- for item in token.tokens:
- if item.is_group and (
- not self._is_identifier(item) or isinstance(item.tokens[0], Parenthesis)
- ):
- self._extract_from_token(item)
-
- if item.ttype in Keyword and (
- item.normalized in PRECEDES_TABLE_NAME
- or item.normalized.endswith(" JOIN")
- ):
- table_name_preceding_token = True
- continue
-
- if item.ttype in Keyword:
- table_name_preceding_token = False
- continue
- if table_name_preceding_token:
- if isinstance(item, Identifier):
- self._process_tokenlist(item)
- elif isinstance(item, IdentifierList):
- for token2 in item.get_identifiers():
- if isinstance(token2, TokenList):
- self._process_tokenlist(token2)
- elif isinstance(item, IdentifierList):
- if any(not self._is_identifier(token2) for token2 in item.tokens):
- self._extract_from_token(item)
-
def set_or_update_query_limit(self, new_limit: int, force: bool = False) -> str:
"""Returns the query with the specified limit.
@@ -779,7 +862,7 @@ def insert_rls(
# mapping between sqloxide and SQLAlchemy dialects
-SQLOXITE_DIALECTS = {
+SQLOXIDE_DIALECTS = {
"ansi": {"trino", "trinonative", "presto"},
"hive": {"hive", "databricks"},
"ms": {"mssql"},
@@ -812,7 +895,7 @@ def extract_table_references(
tree = None
if sqloxide_parse:
- for dialect, sqla_dialects in SQLOXITE_DIALECTS.items():
+ for dialect, sqla_dialects in SQLOXIDE_DIALECTS.items():
if sqla_dialect in sqla_dialects:
break
sql_text = RE_JINJA_BLOCK.sub(" ", sql_text)
diff --git a/superset/sql_validators/presto_db.py b/superset/sql_validators/presto_db.py
index 9d3e7641a6..20b9a8eb98 100644
--- a/superset/sql_validators/presto_db.py
+++ b/superset/sql_validators/presto_db.py
@@ -50,7 +50,7 @@ class PrestoDBSQLValidator(BaseSQLValidator):
) -> Optional[SQLValidationAnnotation]:
# pylint: disable=too-many-locals
db_engine_spec = database.db_engine_spec
- parsed_query = ParsedQuery(statement)
+ parsed_query = ParsedQuery(statement, engine=db_engine_spec.engine)
sql = parsed_query.stripped()
# Hook to allow environment-specific mutation (usually comments) to the SQL
@@ -156,7 +156,7 @@ class PrestoDBSQLValidator(BaseSQLValidator):
For example, "SELECT 1 FROM default.mytable" becomes "EXPLAIN (TYPE
VALIDATE) SELECT 1 FROM default.mytable.
"""
- parsed_query = ParsedQuery(sql)
+ parsed_query = ParsedQuery(sql, engine=database.db_engine_spec.engine)
statements = parsed_query.get_statements()
logger.info("Validating %i statement(s)", len(statements))
diff --git a/superset/sqllab/commands/export.py b/superset/sqllab/commands/export.py
index 1b9b0e0344..aa6050f27f 100644
--- a/superset/sqllab/commands/export.py
+++ b/superset/sqllab/commands/export.py
@@ -115,7 +115,10 @@ class SqlResultExportCommand(BaseCommand):
limit = None
else:
sql = self._query.executed_sql
- limit = ParsedQuery(sql).limit
+ limit = ParsedQuery(
+ sql,
+ engine=self._query.database.db_engine_spec.engine,
+ ).limit
if limit is not None and self._query.limiting_factor in {
LimitingFactor.QUERY,
LimitingFactor.DROPDOWN,
diff --git a/superset/sqllab/query_render.py b/superset/sqllab/query_render.py
index 95111276fe..5f846ef3b0 100644
--- a/superset/sqllab/query_render.py
+++ b/superset/sqllab/query_render.py
@@ -58,7 +58,11 @@ class SqlQueryRenderImpl(SqlQueryRender):
database=query_model.database, query=query_model
)
- parsed_query = ParsedQuery(query_model.sql, strip_comments=True)
+ parsed_query = ParsedQuery(
+ query_model.sql,
+ strip_comments=True,
+ engine=query_model.database.db_engine_spec.engine,
+ )
rendered_query = sql_template_processor.process_template(
parsed_query.stripped(), **execution_context.template_params
)
diff --git a/tests/unit_tests/sql_parse_tests.py b/tests/unit_tests/sql_parse_tests.py
index 341ba9d789..52f0597ce3 100644
--- a/tests/unit_tests/sql_parse_tests.py
+++ b/tests/unit_tests/sql_parse_tests.py
@@ -39,11 +39,11 @@ from superset.sql_parse import (
)
-def extract_tables(query: str) -> set[Table]:
+def extract_tables(query: str, engine: Optional[str] = None) -> set[Table]:
"""
Helper function to extract tables referenced in a query.
"""
- return ParsedQuery(query).tables
+ return ParsedQuery(query, engine=engine).tables
def test_table() -> None:
@@ -95,8 +95,13 @@ def test_extract_tables() -> None:
Table("left_table")
}
- # reverse select
- assert extract_tables("FROM t1 SELECT field") == {Table("t1")}
+ assert extract_tables(
+ "SELECT FROM (SELECT FROM forbidden_table) AS forbidden_table;"
+ ) == {Table("forbidden_table")}
+
+ assert extract_tables(
+ "select * from (select * from forbidden_table) forbidden_table"
+ ) == {Table("forbidden_table")}
def test_extract_tables_subselect() -> None:
@@ -262,14 +267,16 @@ def test_extract_tables_illdefined() -> None:
assert extract_tables("SELECT * FROM schemaname.") == set()
assert extract_tables("SELECT * FROM catalogname.schemaname.") == set()
assert extract_tables("SELECT * FROM catalogname..") == set()
- assert extract_tables("SELECT * FROM catalogname..tbname") == set()
+ assert extract_tables("SELECT * FROM catalogname..tbname") == {
+ Table(table="tbname", schema=None, catalog="catalogname")
+ }
def test_extract_tables_show_tables_from() -> None:
"""
Test ``SHOW TABLES FROM``.
"""
- assert extract_tables("SHOW TABLES FROM s1 like '%order%'") == set()
+ assert extract_tables("SHOW TABLES FROM s1 like '%order%'", "mysql") == set()
def test_extract_tables_show_columns_from() -> None:
@@ -310,7 +317,7 @@ WHERE regionkey IN (SELECT regionkey FROM t2)
"""
SELECT name
FROM t1
-WHERE regionkey EXISTS (SELECT regionkey FROM t2)
+WHERE EXISTS (SELECT 1 FROM t2 WHERE t1.regionkey = t2.regionkey);
"""
)
== {Table("t1"), Table("t2")}
@@ -525,6 +532,18 @@ select * from (select key from q1) a
== {Table("src")}
)
+ # weird query with circular dependency
+ assert (
+ extract_tables(
+ """
+with src as ( select key from q2 where key = '5'),
+q2 as ( select key from src where key = '5')
+select * from (select key from src) a
+"""
+ )
+ == set()
+ )
+
def test_extract_tables_multistatement() -> None:
"""
@@ -664,7 +683,8 @@ def test_extract_tables_nested_select() -> None:
select (extractvalue(1,concat(0x7e,(select GROUP_CONCAT(TABLE_NAME)
from INFORMATION_SCHEMA.COLUMNS
WHERE TABLE_SCHEMA like "%bi%"),0x7e)));
-"""
+""",
+ "mysql",
)
== {Table("COLUMNS", "INFORMATION_SCHEMA")}
)
@@ -675,7 +695,8 @@ WHERE TABLE_SCHEMA like "%bi%"),0x7e)));
select (extractvalue(1,concat(0x7e,(select GROUP_CONCAT(COLUMN_NAME)
from INFORMATION_SCHEMA.COLUMNS
WHERE TABLE_NAME="bi_achievement_daily"),0x7e)));
-"""
+""",
+ "mysql",
)
== {Table("COLUMNS", "INFORMATION_SCHEMA")}
)
@@ -1305,6 +1326,14 @@ def test_sqlparse_issue_652():
"(SELECT table_name FROM /**/ information_schema.tables WHERE table_name LIKE '%user%' LIMIT 1)",
True,
),
+ (
+ "SELECT FROM (SELECT FROM forbidden_table) AS forbidden_table;",
+ True,
+ ),
+ (
+ "SELECT * FROM (SELECT * FROM forbidden_table) forbidden_table",
+ True,
+ ),
],
)
def test_has_table_query(sql: str, expected: bool) -> None:
@@ -1607,13 +1636,17 @@ def test_extract_table_references(mocker: MockerFixture) -> None:
assert extract_table_references(
sql,
"trino",
- ) == {Table(table="other_table", schema=None, catalog=None)}
+ ) == {
+ Table(table="table", schema=None, catalog=None),
+ Table(table="other_table", schema=None, catalog=None),
+ }
logger.warning.assert_called_once()
logger = mocker.patch("superset.migrations.shared.utils.logger")
sql = "SELECT * FROM table UNION ALL SELECT * FROM other_table"
assert extract_table_references(sql, "trino", show_warning=False) == {
- Table(table="other_table", schema=None, catalog=None)
+ Table(table="table", schema=None, catalog=None),
+ Table(table="other_table", schema=None, catalog=None),
}
logger.warning.assert_not_called()
(superset) 04/09: fix: Bar charts horizontal margin adjustment error (#26817)
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 89527c6c977d3ccdaa6848a99edb8c57850117b3
Author: Michael S. Molina <70...@users.noreply.github.com>
AuthorDate: Mon Jan 29 15:25:16 2024 -0500
fix: Bar charts horizontal margin adjustment error (#26817)
(cherry picked from commit 84c48d11d8b3bef244823643804f5fd3d6e3ca86)
---
.../src/Timeseries/transformProps.ts | 1 +
.../src/Timeseries/transformers.ts | 42 +++++++++++++---------
.../plugin-chart-echarts/src/utils/series.ts | 14 ++++++++
.../plugin-chart-echarts/test/utils/series.test.ts | 32 +++++++++++++++++
4 files changed, 72 insertions(+), 17 deletions(-)
diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
index 519323a62e..0f91fb4a00 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
@@ -435,6 +435,7 @@ export default function transformProps(
yAxisTitlePosition,
convertInteger(yAxisTitleMargin),
convertInteger(xAxisTitleMargin),
+ isHorizontal,
);
const legendData = rawSeries
diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
index 0bcc5baf8d..1e274ca875 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
@@ -554,6 +554,7 @@ export function getPadding(
yAxisTitlePosition?: string,
yAxisTitleMargin?: number,
xAxisTitleMargin?: number,
+ isHorizontal?: boolean,
): {
bottom: number;
left: number;
@@ -564,23 +565,30 @@ export function getPadding(
? TIMESERIES_CONSTANTS.yAxisLabelTopOffset
: 0;
const xAxisOffset = addXAxisTitleOffset ? Number(xAxisTitleMargin) || 0 : 0;
- return getChartPadding(showLegend, legendOrientation, margin, {
- top:
- yAxisTitlePosition && yAxisTitlePosition === 'Top'
- ? TIMESERIES_CONSTANTS.gridOffsetTop + (Number(yAxisTitleMargin) || 0)
- : TIMESERIES_CONSTANTS.gridOffsetTop + yAxisOffset,
- bottom: zoomable
- ? TIMESERIES_CONSTANTS.gridOffsetBottomZoomable + xAxisOffset
- : TIMESERIES_CONSTANTS.gridOffsetBottom + xAxisOffset,
- left:
- yAxisTitlePosition === 'Left'
- ? TIMESERIES_CONSTANTS.gridOffsetLeft + (Number(yAxisTitleMargin) || 0)
- : TIMESERIES_CONSTANTS.gridOffsetLeft,
- right:
- showLegend && legendOrientation === LegendOrientation.Right
- ? 0
- : TIMESERIES_CONSTANTS.gridOffsetRight,
- });
+ return getChartPadding(
+ showLegend,
+ legendOrientation,
+ margin,
+ {
+ top:
+ yAxisTitlePosition && yAxisTitlePosition === 'Top'
+ ? TIMESERIES_CONSTANTS.gridOffsetTop + (Number(yAxisTitleMargin) || 0)
+ : TIMESERIES_CONSTANTS.gridOffsetTop + yAxisOffset,
+ bottom: zoomable
+ ? TIMESERIES_CONSTANTS.gridOffsetBottomZoomable + xAxisOffset
+ : TIMESERIES_CONSTANTS.gridOffsetBottom + xAxisOffset,
+ left:
+ yAxisTitlePosition === 'Left'
+ ? TIMESERIES_CONSTANTS.gridOffsetLeft +
+ (Number(yAxisTitleMargin) || 0)
+ : TIMESERIES_CONSTANTS.gridOffsetLeft,
+ right:
+ showLegend && legendOrientation === LegendOrientation.Right
+ ? 0
+ : TIMESERIES_CONSTANTS.gridOffsetRight,
+ },
+ isHorizontal,
+ );
}
export function getTooltipTimeFormatter(
diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
index 6a51e7cbf7..8c87bf4333 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
@@ -466,6 +466,7 @@ export function getChartPadding(
orientation: LegendOrientation,
margin?: string | number | null,
padding?: { top?: number; bottom?: number; left?: number; right?: number },
+ isHorizontal?: boolean,
): {
bottom: number;
left: number;
@@ -486,6 +487,19 @@ export function getChartPadding(
}
const { bottom = 0, left = 0, right = 0, top = 0 } = padding || {};
+
+ if (isHorizontal) {
+ return {
+ left:
+ left + (orientation === LegendOrientation.Bottom ? legendMargin : 0),
+ right:
+ right + (orientation === LegendOrientation.Right ? legendMargin : 0),
+ top: top + (orientation === LegendOrientation.Top ? legendMargin : 0),
+ bottom:
+ bottom + (orientation === LegendOrientation.Left ? legendMargin : 0),
+ };
+ }
+
return {
left: left + (orientation === LegendOrientation.Left ? legendMargin : 0),
right: right + (orientation === LegendOrientation.Right ? legendMargin : 0),
diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts
index 3d7d21c8d0..10768a47f8 100644
--- a/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts
+++ b/superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts
@@ -795,6 +795,14 @@ describe('getChartPadding', () => {
right: 0,
top: 0,
});
+ expect(
+ getChartPadding(true, LegendOrientation.Left, 100, undefined, true),
+ ).toEqual({
+ bottom: 100,
+ left: 0,
+ right: 0,
+ top: 0,
+ });
});
it('should return the correct padding for right orientation', () => {
@@ -804,6 +812,14 @@ describe('getChartPadding', () => {
right: 50,
top: 0,
});
+ expect(
+ getChartPadding(true, LegendOrientation.Right, 50, undefined, true),
+ ).toEqual({
+ bottom: 0,
+ left: 0,
+ right: 50,
+ top: 0,
+ });
});
it('should return the correct padding for top orientation', () => {
@@ -813,6 +829,14 @@ describe('getChartPadding', () => {
right: 0,
top: 20,
});
+ expect(
+ getChartPadding(true, LegendOrientation.Top, 20, undefined, true),
+ ).toEqual({
+ bottom: 0,
+ left: 0,
+ right: 0,
+ top: 20,
+ });
});
it('should return the correct padding for bottom orientation', () => {
@@ -822,6 +846,14 @@ describe('getChartPadding', () => {
right: 0,
top: 0,
});
+ expect(
+ getChartPadding(true, LegendOrientation.Bottom, 10, undefined, true),
+ ).toEqual({
+ bottom: 0,
+ left: 10,
+ right: 0,
+ top: 0,
+ });
});
});