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

Sharing embeds with safe parameters #3495

Merged
merged 61 commits into from
Apr 2, 2019
Merged

Conversation

rauchy
Copy link
Contributor

@rauchy rauchy commented Feb 26, 2019

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
@rauchy rauchy changed the base branch from be-more-permissive-when-parameters-are-safe to master February 26, 2019 18:50
redash/handlers/query_results.py Outdated Show resolved Hide resolved
client/app/components/queries/embed-code-dialog.js Outdated Show resolved Hide resolved
@rauchy rauchy marked this pull request as ready for review March 4, 2019 07:56
@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
Copy link
Contributor Author

rauchy commented Mar 31, 2019

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

@arikfr arikfr left a comment

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?

redash/models/parameterized_query.py Outdated Show resolved Hide resolved
@rauchy rauchy merged commit dd477d4 into master Apr 2, 2019
@rauchy rauchy removed the in progress label Apr 2, 2019
@arikfr
Copy link
Member

arikfr commented Apr 2, 2019

🚀

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

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

arikfr commented Apr 2, 2019

@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

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

@HenroRitchie
Copy link

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
Copy link
Contributor Author

rauchy commented Apr 5, 2019

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

HenroRitchie commented Apr 5, 2019

@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
Copy link
Contributor Author

rauchy commented Apr 7, 2019

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

HenroRitchie commented Apr 8, 2019

@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

@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

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 <omer@rauchy.net>

* 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