-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Ch31968query context #17600
fix: Ch31968query context #17600
Changes from all commits
9c52557
bfaab74
c4a1205
66c5844
6fa51cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ | |
|
||
from superset import is_feature_enabled, security_manager | ||
from superset.constants import NULL_STRING | ||
from superset.datasets.commands.exceptions import DatasetNotFoundError | ||
from superset.models.helpers import AuditMixinNullable, ImportExportMixin, QueryResult | ||
from superset.models.slice import Slice | ||
from superset.typing import FilterValue, FilterValues, QueryObjectDict | ||
|
@@ -319,8 +320,13 @@ def data_for_slices( # pylint: disable=too-many-locals | |
if "column" in filter_config | ||
) | ||
|
||
# for legacy dashboard imports which have the wrong query_context in them | ||
try: | ||
query_context = slc.get_query_context() | ||
except DatasetNotFoundError: | ||
query_context = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AAfghahi and I also talked about maybe writing a script/migration as a follow up that will remove any existing query_context fields from charts if they contain an invalid dataset id. Either way, it seems that if query context is failing in any way here, that it would be just as fine not to use it. |
||
|
||
# legacy charts don't have query_context charts | ||
query_context = slc.get_query_context() | ||
if query_context: | ||
column_names.update( | ||
[ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,14 +79,14 @@ def test_export_chart_command(self, mock_g): | |
"slice_name": "Energy Sankey", | ||
"viz_type": "sankey", | ||
}, | ||
"query_context": None, | ||
"cache_timeout": None, | ||
"dataset_uuid": str(example_chart.table.uuid), | ||
"uuid": str(example_chart.uuid), | ||
"version": "1.0.0", | ||
} | ||
|
||
@patch("superset.security.manager.g") | ||
@pytest.mark.usefixtures("load_energy_table_with_slice") | ||
def test_export_chart_command_no_access(self, mock_g): | ||
"""Test that users can't export datasets they don't have access to""" | ||
mock_g.user = security_manager.find_user("gamma") | ||
|
@@ -125,7 +125,6 @@ def test_export_chart_command_key_order(self, mock_g): | |
"slice_name", | ||
"viz_type", | ||
"params", | ||
"query_context", | ||
"cache_timeout", | ||
"uuid", | ||
"version", | ||
|
@@ -191,34 +190,6 @@ def test_import_v1_chart(self, mock_g): | |
) | ||
assert dataset.table_name == "imported_dataset" | ||
assert chart.table == dataset | ||
assert json.loads(chart.query_context) == { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also @AAfghahi is going to fast-follow with a test for these changes. |
||
"datasource": {"id": dataset.id, "type": "table"}, | ||
"force": False, | ||
"queries": [ | ||
{ | ||
"time_range": " : ", | ||
"filters": [], | ||
"extras": { | ||
"time_grain_sqla": None, | ||
"having": "", | ||
"having_druid": [], | ||
"where": "", | ||
}, | ||
"applied_time_extras": {}, | ||
"columns": [], | ||
"metrics": [], | ||
"annotation_layers": [], | ||
"row_limit": 5000, | ||
"timeseries_limit": 0, | ||
"order_desc": True, | ||
"url_params": {}, | ||
"custom_params": {}, | ||
"custom_form_data": {}, | ||
} | ||
], | ||
"result_format": "json", | ||
"result_type": "full", | ||
} | ||
|
||
database = ( | ||
db.session.query(Database).filter_by(uuid=database_config["uuid"]).one() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to delete query_context if it exists:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better if this was