Skip to content

Commit

Permalink
fix(explore): Fix chart standalone URL for report/thumbnail generation (
Browse files Browse the repository at this point in the history
apache#20673)

* Update explore URLs.

* More URL fixes.

* Make frontend accept true/false query params case-insensitively.

* Fix URL mistake.

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
  • Loading branch information
codyml and michael-s-molina committed Jul 19, 2022
1 parent e1fd906 commit 84d4302
Show file tree
Hide file tree
Showing 16 changed files with 63 additions and 57 deletions.
2 changes: 1 addition & 1 deletion docs/docs/installation/sql-templating.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ Here's a concrete example:

It's possible to query physical and virtual datasets using the `dataset` macro. This is useful if you've defined computed columns and metrics on your datasets, and want to reuse the definition in adhoc SQL Lab queries.

To use the macro, first you need to find the ID of the dataset. This can be done by going to the view showing all the datasets, hovering over the dataset you're interested in, and looking at its URL. For example, if the URL for a dataset is https://superset.example.org/superset/explore/table/42/ its ID is 42.
To use the macro, first you need to find the ID of the dataset. This can be done by going to the view showing all the datasets, hovering over the dataset you're interested in, and looking at its URL. For example, if the URL for a dataset is https://superset.example.org/explore/?dataset_type=table&dataset_id=42 its ID is 42.

Once you have the ID you can query it as if it were a table:

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const MAX_URL_LENGTH = 8000;

export function getURIDirectory(formData, endpointType = 'base') {
// Building the directory part of the URI
let directory = '/superset/explore/';
let directory = '/explore/';
if (['json', 'csv', 'query', 'results', 'samples'].includes(endpointType)) {
directory = '/superset/explore_json/';
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const MAX_URL_LENGTH = 8000;

export function getURIDirectory(formData, endpointType = 'base') {
// Building the directory part of the URI
let directory = '/superset/explore/';
let directory = '/explore/';
if (['json', 'csv', 'query', 'results', 'samples'].includes(endpointType)) {
directory = '/superset/explore_json/';
}
Expand Down
22 changes: 11 additions & 11 deletions superset-frontend/spec/fixtures/mockSliceEntities.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const sliceEntitiesForChart = {
slices: {
[sliceId]: {
slice_id: sliceId,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%2018%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%2018%7D',
slice_name: 'Genders',
form_data: {
slice_id: sliceId,
Expand Down Expand Up @@ -62,7 +62,7 @@ export const sliceEntitiesForDashboard = {
slices: {
[filterId]: {
slice_id: filterId,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20127%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20127%7D',
slice_name: 'Region Filter',
form_data: {
instant_filtering: true,
Expand All @@ -86,7 +86,7 @@ export const sliceEntitiesForDashboard = {
},
128: {
slice_id: 128,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20128%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20128%7D',
slice_name: "World's Population",
form_data: {},
viz_type: 'big_number',
Expand All @@ -98,7 +98,7 @@ export const sliceEntitiesForDashboard = {
},
129: {
slice_id: 129,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20129%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20129%7D',
slice_name: 'Most Populated Countries',
form_data: {},
viz_type: 'table',
Expand All @@ -110,7 +110,7 @@ export const sliceEntitiesForDashboard = {
},
130: {
slice_id: 130,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20130%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20130%7D',
slice_name: 'Growth Rate',
form_data: {},
viz_type: 'line',
Expand All @@ -122,7 +122,7 @@ export const sliceEntitiesForDashboard = {
},
131: {
slice_id: 131,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20131%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20131%7D',
slice_name: '% Rural',
form_data: {},
viz_type: 'world_map',
Expand All @@ -134,7 +134,7 @@ export const sliceEntitiesForDashboard = {
},
132: {
slice_id: 132,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20132%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20132%7D',
slice_name: 'Life Expectancy VS Rural %',
form_data: {},
viz_type: 'bubble',
Expand All @@ -146,7 +146,7 @@ export const sliceEntitiesForDashboard = {
},
133: {
slice_id: 133,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20133%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20133%7D',
slice_name: 'Rural Breakdown',
form_data: {},
viz_type: 'sunburst',
Expand All @@ -158,7 +158,7 @@ export const sliceEntitiesForDashboard = {
},
134: {
slice_id: 134,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20134%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20134%7D',
slice_name: "World's Pop Growth",
form_data: {},
viz_type: 'area',
Expand All @@ -170,7 +170,7 @@ export const sliceEntitiesForDashboard = {
},
135: {
slice_id: 135,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20135%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20135%7D',
slice_name: 'Box plot',
form_data: {},
viz_type: 'box_plot',
Expand All @@ -182,7 +182,7 @@ export const sliceEntitiesForDashboard = {
},
136: {
slice_id: 136,
slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20136%7D',
slice_url: '/explore/?form_data=%7B%22slice_id%22%3A%20136%7D',
slice_name: 'Treemap',
form_data: {},
viz_type: 'treemap',
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/src/utils/urlUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ export function getUrlParam({ name, type }: UrlParam): unknown {
if (!urlParam) {
return null;
}
if (urlParam === 'true') {
if (urlParam.toLowerCase() === 'true') {
return 1;
}
if (urlParam === 'false') {
if (urlParam.toLowerCase() === 'false') {
return 0;
}
if (!Number.isNaN(Number(urlParam))) {
Expand All @@ -71,7 +71,7 @@ export function getUrlParam({ name, type }: UrlParam): unknown {
if (!urlParam) {
return null;
}
return urlParam !== 'false' && urlParam !== '0';
return urlParam.toLowerCase() !== 'false' && urlParam !== '0';
case 'rison':
if (!urlParam) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const mockdatasets = [...new Array(3)].map((_, i) => ({
changed_by: 'user',
changed_on: new Date().toISOString(),
database_name: `db ${i}`,
explore_url: `/explore/table/${i}`,
explore_url: `/explore/?dataset_type=table&dataset_id=${i}`,
id: i,
schema: `schema ${i}`,
table_name: `coolest table ${i}`,
Expand Down
2 changes: 1 addition & 1 deletion superset/connectors/sqla/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ def edit(self, pk: str) -> FlaskResponse:
resp = super().edit(pk)
if isinstance(resp, str):
return resp
return redirect("/superset/explore/table/{}/".format(pk))
return redirect("/explore/?dataset_type=table&dataset_id={}".format(pk))

@expose("/list/")
@has_access
Expand Down
2 changes: 1 addition & 1 deletion superset/models/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ def get_sqla_engine(
if not source and request and request.referrer:
if "/superset/dashboard/" in request.referrer:
source = utils.QuerySource.DASHBOARD
elif "/superset/explore/" in request.referrer:
elif "/explore/" in request.referrer:
source = utils.QuerySource.CHART
elif "/superset/sqllab/" in request.referrer:
source = utils.QuerySource.SQL_LAB
Expand Down
2 changes: 1 addition & 1 deletion superset/reports/commands/execute.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ def _get_url(
force=force,
)
return get_url_path(
"Superset.explore",
"ExploreView.root",
user_friendly=user_friendly,
form_data=json.dumps({"slice_id": self._report_schedule.chart_id}),
standalone="true",
Expand Down
2 changes: 1 addition & 1 deletion superset/templates/email/role_extended.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Dear {{ user.username }},
<br>
<a href={{ url_for('Superset.profile', username=granter.username, _external=True) }}>
{{ granter.username }}</a> has extended the role {{ role.name }} to include
<a href={{ url_for('Superset.explore', datasource_type=datasource.type, datasource_id=datasource.id, _external=True) }}>
<a href={{ url_for('ExploreView.root', datasource_type=datasource.type, datasource_id=datasource.id, _external=True) }}>
{{datasource.full_name}}</a> and granted you access to it.
<br>
<br>
Expand Down
2 changes: 1 addition & 1 deletion superset/templates/email/role_granted.txt
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Dear {{ user.username }},
<a href={{ url_for('Superset.profile', username=granter.username, _external=True) }}>
{{ granter.username }}</a> has granted you the role {{ role.name }}
that gives access to the
<a href={{ url_for('Superset.explore', datasource_type=datasource.type, datasource_id=datasource.id, _external=True) }}>
<a href={{ url_for('ExploreView.root', datasource_type=datasource.type, datasource_id=datasource.id, _external=True) }}>
{{datasource.full_name}}</a>
<br>
<br>
Expand Down
4 changes: 2 additions & 2 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -427,13 +427,13 @@ def slice(self, slice_id: int) -> FlaskResponse: # pylint: disable=no-self-use
_, slc = get_form_data(slice_id, use_slice_data=True)
if not slc:
abort(404)
endpoint = "/superset/explore/?form_data={}".format(
endpoint = "/explore/?form_data={}".format(
parse.quote(json.dumps({"slice_id": slice_id}))
)

is_standalone_mode = ReservedUrlParameters.is_standalone_mode()
if is_standalone_mode:
endpoint += f"&{ReservedUrlParameters.STANDALONE}={is_standalone_mode}"
endpoint += f"&{ReservedUrlParameters.STANDALONE}=true"
return redirect(endpoint)

def get_query_string_response(self, viz_obj: BaseViz) -> FlaskResponse:
Expand Down
39 changes: 27 additions & 12 deletions superset/views/redirects.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,25 +34,40 @@ class R(BaseSupersetView): # pylint: disable=invalid-name
"""used for short urls"""

@staticmethod
def _validate_url(url: Optional[str] = None) -> bool:
if url and (
url.startswith("//superset/dashboard/")
or url.startswith("//superset/explore/")
):
return True
return False
def _validate_explore_url(url: str) -> Optional[str]:
if url.startswith("//superset/explore/p/"):
return url

if url.startswith("//superset/explore"):
return "/" + url[10:] # Remove /superset from old Explore URLs

if url.startswith("//explore"):
return url

return None

@staticmethod
def _validate_dashboard_url(url: str) -> Optional[str]:
if url.startswith("//superset/dashboard/"):
return url

return None

@event_logger.log_this
@expose("/<int:url_id>")
def index(self, url_id: int) -> FlaskResponse:
url = db.session.query(models.Url).get(url_id)
if url and url.url:
explore_url = "//superset/explore/?"
if url.url.startswith(explore_url):
explore_url += f"r={url_id}"
explore_url = self._validate_explore_url(url.url)
if explore_url:
if explore_url.startswith("//explore/?"):
explore_url = f"//explore/?r={url_id}"
return redirect(explore_url[1:])
if self._validate_url(url.url):
return redirect(url.url[1:])

dashboard_url = self._validate_dashboard_url(url.url)
if dashboard_url:
return redirect(dashboard_url[1:])

return redirect("/")

flash("URL to nowhere...", "danger")
Expand Down
17 changes: 4 additions & 13 deletions tests/integration_tests/core_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,15 +123,6 @@ def test_dashboard_endpoint(self):
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
def test_slice_endpoint(self):
self.login(username="admin")
slc = self.get_slice("Girls", db.session)
resp = self.get_resp("/superset/slice/{}/".format(slc.id))
assert "Original value" in resp
assert "List Roles" in resp

# Testing overrides
resp = self.get_resp("/superset/slice/{}/?standalone=true".format(slc.id))
assert '<div class="navbar' not in resp

resp = self.client.get("/superset/slice/-1/")
assert resp.status_code == 404

Expand Down Expand Up @@ -881,7 +872,7 @@ def test_user_activity_access(self, username="gamma"):

@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
def test_slice_id_is_always_logged_correctly_on_web_request(self):
# superset/explore case
# explore case
self.login("admin")
slc = db.session.query(Slice).filter_by(slice_name="Girls").one()
qry = db.session.query(models.Log).filter_by(slice_id=slc.id)
Expand Down Expand Up @@ -1424,7 +1415,7 @@ def test_feature_flag_serialization(self):
"/superset/welcome",
f"/superset/dashboard/{dash_id}/",
"/superset/profile/admin/",
f"/superset/explore/table/{tbl_id}",
f"/explore/?dataset_type=table&dataset_id={tbl_id}",
]
for url in urls:
data = self.get_resp(url)
Expand Down Expand Up @@ -1566,7 +1557,7 @@ def test_explore_injected_exceptions(self, mock_db_connection_mutator):
exception = SupersetException("Error message")
mock_db_connection_mutator.side_effect = exception
slice = db.session.query(Slice).first()
url = f"/superset/explore/?form_data=%7B%22slice_id%22%3A%20{slice.id}%7D"
url = f"/explore/?form_data=%7B%22slice_id%22%3A%20{slice.id}%7D"

self.login()
data = self.get_resp(url)
Expand All @@ -1576,7 +1567,7 @@ def test_explore_injected_exceptions(self, mock_db_connection_mutator):
exception = SQLAlchemyError("Error message")
mock_db_connection_mutator.side_effect = exception
slice = db.session.query(Slice).first()
url = f"/superset/explore/?form_data=%7B%22slice_id%22%3A%20{slice.id}%7D"
url = f"/explore/?form_data=%7B%22slice_id%22%3A%20{slice.id}%7D"

self.login()
data = self.get_resp(url)
Expand Down
10 changes: 5 additions & 5 deletions tests/integration_tests/reports/commands_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,7 @@ def test_email_chart_report_schedule(
)
# assert that the link sent is correct
assert (
'<a href="http://0.0.0.0:8080/superset/explore/?'
'<a href="http://0.0.0.0:8080/explore/?'
"form_data=%7B%22slice_id%22%3A%20"
f"{create_report_email_chart.chart.id}%7D&"
'standalone=0&force=false">Explore in Superset</a>'
Expand Down Expand Up @@ -769,7 +769,7 @@ def test_email_chart_report_schedule_force_screenshot(
)
# assert that the link sent is correct
assert (
'<a href="http://0.0.0.0:8080/superset/explore/?'
'<a href="http://0.0.0.0:8080/explore/?'
"form_data=%7B%22slice_id%22%3A%20"
f"{create_report_email_chart_force_screenshot.chart.id}%7D&"
'standalone=0&force=true">Explore in Superset</a>'
Expand Down Expand Up @@ -808,7 +808,7 @@ def test_email_chart_alert_schedule(
notification_targets = get_target_from_report_schedule(create_alert_email_chart)
# assert that the link sent is correct
assert (
'<a href="http://0.0.0.0:8080/superset/explore/?'
'<a href="http://0.0.0.0:8080/explore/?'
"form_data=%7B%22slice_id%22%3A%20"
f"{create_alert_email_chart.chart.id}%7D&"
'standalone=0&force=true">Explore in Superset</a>'
Expand Down Expand Up @@ -882,7 +882,7 @@ def test_email_chart_report_schedule_with_csv(
)
# assert that the link sent is correct
assert (
'<a href="http://0.0.0.0:8080/superset/explore/?'
'<a href="http://0.0.0.0:8080/explore/?'
"form_data=%7B%22slice_id%22%3A%20"
f"{create_report_email_chart_with_csv.chart.id}%7D&"
'standalone=0&force=false">Explore in Superset</a>'
Expand Down Expand Up @@ -1260,7 +1260,7 @@ def test_slack_chart_report_schedule_with_text(
| 1 | c21 | c22 | c23 |"""
assert table_markdown in post_message_mock.call_args[1]["text"]
assert (
f"<http://0.0.0.0:8080/superset/explore/?form_data=%7B%22slice_id%22%3A%20{create_report_slack_chart_with_text.chart.id}%7D&standalone=0&force=false|Explore in Superset>"
f"<http://0.0.0.0:8080/explore/?form_data=%7B%22slice_id%22%3A%20{create_report_slack_chart_with_text.chart.id}%7D&standalone=0&force=false|Explore in Superset>"
in post_message_mock.call_args[1]["text"]
)

Expand Down
4 changes: 2 additions & 2 deletions tests/unit_tests/utils/urls_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
# under the License.
from superset.utils.urls import modify_url_query

EXPLORE_CHART_LINK = "http://localhost:9000/superset/explore/?form_data=%7B%22slice_id%22%3A+76%7D&standalone=true&force=false"
EXPLORE_CHART_LINK = "http://localhost:9000/explore/?form_data=%7B%22slice_id%22%3A+76%7D&standalone=true&force=false"

EXPLORE_DASHBOARD_LINK = "http://localhost:9000/superset/dashboard/3/?standalone=3"

Expand All @@ -26,7 +26,7 @@ def test_convert_chart_link() -> None:
test_url = modify_url_query(EXPLORE_CHART_LINK, standalone="0")
assert (
test_url
== "http://localhost:9000/superset/explore/?form_data=%7B%22slice_id%22%3A%2076%7D&standalone=0&force=false"
== "http://localhost:9000/explore/?form_data=%7B%22slice_id%22%3A%2076%7D&standalone=0&force=false"
)


Expand Down

0 comments on commit 84d4302

Please sign in to comment.