Skip to content

Sharing embeds with safe parameters#3495

Merged
rauchy merged 61 commits into
masterfrom
allow-parameters-on-embeds
Apr 2, 2019
Merged

Sharing embeds with safe parameters#3495
rauchy merged 61 commits into
masterfrom
allow-parameters-on-embeds

Conversation

@rauchy

@rauchy rauchy commented Feb 26, 2019

Copy link
Copy Markdown
Contributor

What type of PR is this? (check all applicable)

  • Feature

Description

This PR points embeds are the textless endpoint, resulting in the ability to execute queries with safe parameters in embedded visualizations.

Related Tickets & Documents

Towards #3284

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

feb-27-2019 14-15-05

@ghost ghost assigned rauchy Feb 26, 2019
@ghost ghost added the in progress label Feb 26, 2019
@rauchy
rauchy changed the base branch from master to be-more-permissive-when-parameters-are-safe February 26, 2019 13:14
Comment thread client/app/components/queries/visualization-embed.js Outdated
@rauchy
rauchy changed the base branch from be-more-permissive-when-parameters-are-safe to master February 26, 2019 18:50
Comment thread redash/handlers/query_results.py Outdated
Comment thread redash/handlers/query_results.py Outdated
Comment thread client/app/components/queries/embed-code-dialog.js Outdated
@rauchy
rauchy marked this pull request as ready for review March 4, 2019 07:56
Comment thread client/app/pages/home/home.html Outdated
@rauchy rauchy mentioned this pull request Mar 7, 2019
1 task
@rauchy rauchy changed the title Parameters in embeds Sharing embeds with safe parameters Mar 18, 2019
@rauchy
rauchy force-pushed the allow-parameters-on-embeds branch from 1c35c1c to 1649f42 Compare March 20, 2019 07:45
…cts that require access, instead of their groups
@rauchy
rauchy changed the base branch from master to change-has-access-signature March 20, 2019 08:04
@rauchy
rauchy force-pushed the allow-parameters-on-embeds branch from 1649f42 to e8dfde3 Compare March 20, 2019 10:01
@rauchy

rauchy commented Mar 31, 2019

Copy link
Copy Markdown
Contributor Author

Why is { force: true } needed?

@ranbena - the anchor tag is not actionable at that point (it's hidden away), so { force: true } clicks it anyway.

@rauchy
rauchy requested a review from arikfr April 1, 2019 07:12

@arikfr arikfr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good.

  1. See the unicode comment.
  2. The Percy snapshots look empty - why?

Comment thread redash/models/parameterized_query.py Outdated
@rauchy
rauchy merged commit dd477d4 into master Apr 2, 2019
@rauchy rauchy removed the in progress label Apr 2, 2019
@arikfr

arikfr commented Apr 2, 2019

Copy link
Copy Markdown
Member

🚀

@arikfr
arikfr deleted the allow-parameters-on-embeds branch April 2, 2019 08:54
@HenroRitchie

Copy link
Copy Markdown

Hi Guys - thank you for the merge. What is the quickest way to get this into production? I usually pull the docker images from docker hub, but I guess this will not be there yet. Should I pull the preview tag?

@arikfr

arikfr commented Apr 2, 2019

Copy link
Copy Markdown
Member

@HenroRitchie you can use the redash/preview Docker repo and use a specific tag from there (once there is a master build with this PR).

@HenroRitchie

Copy link
Copy Markdown

@arikfr Thank you. Will just wait for the master build with this PR.

@HenroRitchie

Copy link
Copy Markdown

Hi Guys - I have attempted to embed a query into another page but I get an error response It seems like we encountered an error. Try refreshing this page or contact your administrator.

When embedding a dashboard it still works fine, without the parameters being available. Looking at the developer console in chrome the response from Redash is a 400 Bad Request. Should I continue the discussion here, or somewhere else?

@rauchy

rauchy commented Apr 5, 2019

Copy link
Copy Markdown
Contributor Author

@HenroRitchie generally we'd prefer support discussions at the forums but since this is a new feature it may be related directly to this ticket.

A 400 here could probably mean an incorrect parameter type passed in - could you check and also send over the response message that came along with that 400?

@HenroRitchie

HenroRitchie commented Apr 5, 2019

Copy link
Copy Markdown

@rauchy I will continue here until we are sure my problem and this PR are unrelated.

I used the embed Embed elsewhere link to generate the iframe code. When I try to embed a query without any parameters this is the iframe code generated:

<iframe src="{{ $ctrl.embedUrl }}" width="720" height="391"></iframe>

This is clearly wrong. When I try to embed a query with parameters and use the Embed elsewhere link this is the result:

<iframe src="https://secret.url/embed/query/1/visualization/2?api_key=xd8wfBdBxsW8rB1jGxIgDySQhkQGBvLKfpiMPx9J&p_fridge_tag=fridge_1_top&p_date_range.start=2019-02-21%2000%3A00%3A00&p_date_range.end=2019-02-26%2000%3A00%3A00" width="720" height="391"></iframe>

If I just use the generated URI I get the same error response posted earlier.

The response message is: "message": "The following parameter values are incompatible with their definitions: date_range.start, date_range.end"}. I thus removed the date range parameter and now the embed works. However, when I select a different option in the embed the chart is not updated correctly. The chart defaults back to the initial setup, i.e. x-axis from -1 to 6 and y-axis from -1 to 4 without any data. Is the query automatically refreshed/run when the parameter value changed? Is looks like this is not the case. I first must run the query directly in Redash with the corresponding parameter selected before in will show correctly in the embed.

@rauchy

rauchy commented Apr 7, 2019

Copy link
Copy Markdown
Contributor Author

@HenroRitchie thanks for the details. You've helped me find 2 bugs which are fixed here and here. I'm assuming they will get merged today and I'll be looking to hear if they solved your problem.

@HenroRitchie

HenroRitchie commented Apr 8, 2019

Copy link
Copy Markdown

@rauchy Thank you for the updates and commits. I am happy to report that I can now embed data parameters successfully. However, the queries are still not refreshed when any of the parameters change. In my test, I have one drop-down parameter and one date range parameter. I followed the steps below to test.

In Redash select the first parameter and the date range and run the query. I manually run the query every time. Check the results in Redash. Embed the query in the secondary portal as an Iframe. This works and provides the same results as Redash directly. If I now select a different parameter from the dropdown or change the date range the chart display defaults to the generic chart. Furthermore, if I attempt to download the CSV file it fails with a server error.

I monitored the Celery Status page - and it does seem that something is happening as soon as I change the parameter, but the page and chart are never updated. I don't know if the page is not updated with the latest info or if takes too long. I have a large number of data to display, and changing the first parameter might trigger the query but it takes such a long time that it never gets updated on the display. Visual feedback to show the query is being refreshed might help here.

Changing the refresh schedule to every 1 minute does not make a difference in the selected parameter set's displayed chart. I have found some strange behavior regarding the automatic refresh. If the refresh is set to 1 minute the selected parameter set and the corresponding chart does not get updated, but if the drop down parameter changes the new set's chart is correctly displayed. If you then select the previous set (which were not updated) it now shows the correct chart. It is as if the query is run, but the chart displayed is not updated unless reloaded/refreshed. Refreshing the browser does not work as it then loads the parameter as set in the query window in Redash directly.

How does Redash handle parameters with an automatic refresh schedule? Are queries run for all the different drop down options, or only for the active parameter set?

Lastly, am I approaching this correctly? Should the parameter change a variable in the SQL query, or should the variable filter the results? The difference is the amount of data sent from Postgres to Redash. Running the same query every time might be better for Redash, or will it not make a difference?

@HenroRitchie

Copy link
Copy Markdown

@rauchy Update

If parameter 1 option 1 is selected with a specific date range nothing happens.
If parameter 1 option 2 is selected with the same date range nothing happens.
If parameter 1 option 1 is selected with the same date range the correct chart is now available.
If parameter 1 option 2 is selected with the same date range the correct chart is available.
If parameter 1 option 3 is selected with the same date range nothing happens.
If parameter 1 option 1 is selected, and then option 3 again the chart is available.

@ghoshsanjoy78

Copy link
Copy Markdown

Does this PR also cover embedding dashboards with parameters. I am now able to embed visualizations with params but dashboards with params are still not supported?

harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
* change has_access and require_access signatures to work with the objects that require access, instead of their groups

* change has_access and require_access signatures to work with the objects that require access, instead of their groups

* use the textless endpoint (/api/queries/:id/results) for pristine
queriest

* Revert "use the textless endpoint (/api/queries/:id/results) for pristine"

This reverts commit cd2cee7.

* go to textless /api/queries/:id/results by default

* change `run_query`'s signature to accept a ParameterizedQuery instead of
constructing it inside

* raise HTTP 400 when receiving invalid parameter values. Fixes getredash#3394

* support querystring params

* extract coercing of numbers to function, along with a friendlier
implementation

* wire embeds to textless endpoint

* allow users with view_only permissions to execute queries on the
textless endpoint, as it only allows safe queries to run

* enqueue jobs for ApiUsers

* add parameters component for embeds

* include existing parameters in embed code

* fetch correct values for json requests

* remove previous embed parameter code

* rename `id` to `user_id`

* support executing queries using Query api_keys by instantiating an ApiUser that would be able to execute the specific query

* bring back ALLOW_PARAMETERS_IN_EMBEDS (with link on deprecation coming up)

* show deprecation messages for ALLOW_PARAMETERS_IN_EMBEDS. Also, move
other message (email not verified) to use the same mechanism

* add link to forum message on setting deprecation

* rephrase deprecation message

* add link to forum message regarding embed deprecation

* change API to /api/queries/:id/dropdowns/:dropdown_id

* split to 2 different dropdown endpoints and implement the second

* add test cases for /api/queries/:id/dropdowns/:id

* use new /dropdowns endpoint in frontend

* first e2e test for sharing embeds

* Pleasing the CodeClimate overlords

* All glory to CodeClimate

* change has_access and require_access signatures to work with the objects that require access, instead of their groups

* split has_access between normal users and ApiKey users

* remove residues from bad rebase

* allow access to safe queries via api keys

* rename `object` to `obj`

* support both objects and group dicts in `has_access` and `require_access`

* simplify permission tests once `has_access` accepts groups

* change has_access and require_access signatures to work with the objects that require access, instead of their groups

* rename `object` to `obj`

* support both objects and group dicts in `has_access` and `require_access`

* simplify permission tests once `has_access` accepts groups

* fix bad rebase

* send embed parameters through POST data

* no need to log `is_api_key`

* move query fetching by api_key to within the Query model

* fetch user by adding a get_by_id function on the User model

* pass parameters as POST data (fixes test failure introduced by switching
from query string parameters to POST data)

* test the right thing - queries with safe parameters should be embeddable

* introduce cy.clickThrough

* add another Cypress test to make sure unsafe queries cannot be embedded

* serialize Parameters into query string

* set is_api_key as the last parameter to (hopefully) avoid
backward-dependency problems

* Update redash/models/parameterized_query.py

Co-Authored-By: rauchy <[email protected]>

* attempt to fix empty percy snapshots

* snap percies after DOM is fully loaded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants