Skip to content
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(explore): Fix chart standalone URL for report/thumbnail generation #20673

Merged
merged 4 commits into from
Jul 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the boolean type can be implicitly converted to string.

Python 3.8.6 (default, Nov 11 2020, 23:52:15)
[Clang 12.0.0 (clang-1200.0.32.21)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> str(True)
'True'
>>> str(False)
'False'
>>>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this one the implicit conversion to True was part of why the URL wasn't working: the frontend expected it to be lowercase, so I wanted to make that explicit. But, I did update the frontend so if others use implicit conversion from Python bool it should work!

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is false now, what's the current value? Do we still have a standalone url without the nav?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that now that Explore is in the SPA, the response doc just contains the SPA wrapper. I was assuming that get_resp doesn't download/execute the JS bundle, so I think these tests were only still passing by accident:

  • "Original value" and "List Roles" are contained in the embedded bootstrap data but I don't think that bootstrap data is used by Explore anymore (it uses API endpoints now)
  • <div class="navbar' isn't present in either standalone or non-standalone mode anymore

So, it seemed like these tests don't test anything meaningful anymore, with the exception of the 404 test below. I looked around for examples of backend tests for Dashboard, but didn't see much and assumed they had also been removed when Dashboard was brought into the SPA.


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