-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Rockset Query Runner: Fetch collections when api key is access limited #6661
base: master
Are you sure you want to change the base?
Rockset Query Runner: Fetch collections when api key is access limited #6661
Conversation
When the rockset api key does not have full access to all workspaces the collection fetching fails.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6661 +/- ##
==========================================
+ Coverage 62.57% 62.75% +0.18%
==========================================
Files 161 162 +1
Lines 13184 13264 +80
Branches 1797 1807 +10
==========================================
+ Hits 8250 8324 +74
- Misses 4649 4654 +5
- Partials 285 286 +1
|
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.
Sorry, I didn't submit my review. I left a comment. Could you check it?
"columns": self.api.collection_columns(workspace, collection), | ||
} | ||
columns = self.api.collection_columns(workspace, collection) | ||
if columns: |
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.
Do we need to give up showing a table name as well if columns
is None?
Doesn't this work?
schema[name] = {"name": name, "columns": []}
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.
I personally feel it doesn't make sense to show collections & tables where the user does not have access to the data?
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.
@MaxBer I see... Can you run a query with your API key?
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.
Calling list_workspaces & list_collections requires an access level within Rockset which yields all the workspaces available (even ones I do not have "data read" access to), and the request does not seem to provide me with any indication if I have access or not.
In this case the role has access to read data in a specific workspace but not the other ones, and when it attempts to list a collection where I do not have access I do not get a result.
Example of output from collection_columns() where my key does not have access:
{'message': 'Your role does not have sufficient privileges to query data in workspace
commons
.', 'message_key': None, 'type': 'FORBIDDEN', 'line': None, 'column': None...
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.
@MaxBer Thanks for your explanation. I've understood the situation. The Rockset API specification looks a little strange, but it's not something we can change, so that's okay from my end.
I checked the test coverage in the previous commit since some uncovered code existed. Most of it wasn't covered from last time. So, I approve this PR.
https://app.codecov.io/gh/getredash/redash/commit/a8c6dd0043c06b3e90533bcc9eed8b5a4c9bf65c/blob/redash/query_runner/rockset.py
Please wait to finish the reviews for a few days.
@guidopetri @konnectr Could you check this if you have some time?
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.
This looks reasonable to me too, though I'd prefer a test being added. @MaxBer any interest in adding a test?
@guidopetri Sorry just saw this. We ended up going with BigQuery and shut down our Rockset instances, so.I don't really have the means to develop this further unfortunately. |
When the rockset api key does not have full access to all workspaces the collection fetching fails. This PR checks for a result when fetching collection columns.
What type of PR is this?
Description
When the rockset api key does not have full access to all workspaces the collection fetching fails. This PR checks for a result when fetching collection columns.
Expected compatibility: Rockset production db.
How is this tested?
Refresh works although our API key does not have read access to all collections (across workspaces)
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)