-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
1c35c1c
to
1649f42
Compare
…cts that require access, instead of their groups
1649f42
to
e8dfde3
Compare
…cts that require access, instead of their groups
…tine" This reverts commit cd2cee7.
constructing it inside
textless endpoint, as it only allows safe queries to run
@ranbena - the anchor tag is not actionable at that point (it's hidden away), so |
backward-dependency problems
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.
Looks good.
- See the unicode comment.
- The Percy snapshots look empty - why?
Co-Authored-By: rauchy <omer@rauchy.net>
…ash into allow-parameters-on-embeds
🚀 |
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? |
@HenroRitchie you can use the |
@arikfr Thank you. Will just wait for the master build with this PR. |
Hi Guys - I have attempted to embed a query into another page but I get an error response 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? |
@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? |
@rauchy I will continue here until we are sure my problem and this PR are unrelated. I used the embed
This is clearly wrong. When I try to embed a query with parameters and use the
If I just use the generated URI I get the same error response posted earlier. The response message is: |
@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. |
@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? |
@rauchy Update If parameter 1 option 1 is selected with a specific date range nothing happens. |
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? |
* 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
What type of PR is this? (check all applicable)
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)