Update Worksheet database names in UI for new simplified db names #1063
Conversation
|
The PR looks good just 2 things to call out:
|
|
Hi @dlpzx , @noah-paige , I agree with @noah-paige on both the points. If possible it would be great to display the database name ( old or new ) which is actually present in the account where it is shared. For the second point, I agree that we should not have a drop down to select the team and only show the databases and tables which are accessible to the team which created the worksheet and also because the querying is happening with the worksheet admin-group's role and not the role of the team selected in dropdown ( please correct me if I am wrong ) |
This reverts commit 82e4ed3.
|
Tested this PR locally:
|
| return None | ||
| old_shared_db_name = (source.GlueDatabaseName + '_shared_' + source.shareUri)[:254] | ||
| with context.engine.scoped_session() as session: | ||
| share = ShareObjectService.get_share_object_in_environment(uri=source.environmentUri, shareUri=source.shareUri) |
There was a problem hiding this comment.
Why did we get the share item from this new method instead of getting it from get_share_object() ? Did we just want to confirm if the user has the permissions to access that env ?
There was a problem hiding this comment.
believe it is exactly for that reason because of decorator @has_resource_permission(GET_ENVIRONMENT) but will leave it open for @dlpzx to confirm
There was a problem hiding this comment.
Exactly, I wanted to keep the permissions checking of the environment
There was a problem hiding this comment.
Thanks @noah-paige , @dlpzx for the clarification
| environmentUri: d.environmentUri | ||
| })); | ||
| // Remove duplicates based on GlueDatabaseName | ||
| sharedWithDatabases = sharedWithDatabases.filter( |
There was a problem hiding this comment.
Could we potentially remove this duplication in the query itself ? dataall.modules.dataset_sharing.db.share_object_repositories.ShareObjectRepository.paginate_shared_datasets in here
There was a problem hiding this comment.
Not sure if we want to remove duplication of DB names on the backend because this query searchEnvironmentDataItems is also used at frontend/src/modules/Environments/components/EnvironmentSharedDatasets.js where we show each share item that is shared with the given environment and for what respective team in that environment (in this case we would want to show all shares duplicate dbs or not)
Another note (out of scope for this PR): We currently do not handle shared items of type S3Bucket for the table mentioned above ...
There was a problem hiding this comment.
Oh okay I get it. Thanks @noah-paige .
I didn't quite get your what you meant by the note and which table you are mentioning
There was a problem hiding this comment.
There are 2 reasons not to remove the duplicates, one is that the query is used in other places as Noah pointed out (in the Environment view > Datasets tab > Shared items).
The second reason was that it is difficult to predict which shares are using the old method and the new method, and that is retrieved in the resolver that is a field of the type. Once we deprecate the old type of share names I think we can redesign the remove-duplicates and the separation between plot the shared items in environment and list the shared databases in Worksheets.
I tried implementing a new resolver (check the commits) to separate the searchSharedDataItems used in environments to what we want to achieve in Worksheets, but I found it a bit complex to fulfill the purpose of this PR = show correct glue database names
There was a problem hiding this comment.
Yeah makes sense. Thanks @dlpzx
Feature or Bugfix
Detail
After implementing #1016 the names displayed for the databases in Worksheets won't contain the unique identifier.
In addition this PR solves #805 by removing duplicates also in FE.
Here is an screenshot of a local test:
Update:
Because there will be a mix of old shares with Glue database names ending in
shared_URIand shares with database names suffixed bysharedonly, this PR introduces a new field in the GraphQL type returned by the searchDataItems query. This field resolves the name of the shared Glue database. @noah-paige @TejasRGitHub in this commitAt first I tried implementing a separate resolver for the Worksheets, but I think we can fix step by step and first focus on the database name and then on the group of the Worksheet vs the group chosen inside the Worksheet. In any case I left the commit to have some reference.
Relates
Security
Please answer the questions below briefly where applicable, or write
N/A. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
evalor similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.