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

[SIP-97] Import Option to Overwrite Cascading Assets of Existing Item #24660

Closed
2 tasks
unnyns-307 opened this issue Jul 11, 2023 · 18 comments
Closed
2 tasks

[SIP-97] Import Option to Overwrite Cascading Assets of Existing Item #24660

unnyns-307 opened this issue Jul 11, 2023 · 18 comments
Assignees
Labels
sip Superset Improvement Proposal

Comments

@unnyns-307
Copy link

unnyns-307 commented Jul 11, 2023

[SIP-97] Import Option to Overwrite Cascading Assets of Existing Item

Motivation

Currently, when importing an item into Superset, such as a dashboard or chart, any cascading items, such as associated charts, datasets, or databases, are not automatically overwritten. This limitation often results in time-consuming manual updates to ensure data consistency and accuracy across the platform. This capability would greatly enhance the efficiency and ease of maintaining dashboards and charts, users can easily manage and maintain their data infrastructure by updating all relevant dependencies in one go which enabling users to seamlessly update their visualizations with new data sources or modified configurations.

Proposed Change

1.) When users initiate an import of an item, such as a dashboard or chart, they will be presented with an additional checkbox or toggle named "Overwrite Cascading Items." By selecting this option, the user indicates their intention to update all associated dependencies and ensure complete consistency between the imported item and the existing data infrastructure.

2.) Additional checkbox or toggle for overwrite each cascading group of items individually
For example, to Importing dashboard will show option to

  • "Overwrite Cascading Charts"
  • "Overwrite Cascading Datasets"

New or Changed Public Interfaces

image

Adding promt window show additional checkbox for confirm overwriting cascading items

New dependencies

None

Rejected Alternatives

Manually importing items by each level consume much effort and also risk of manual errors or inconsistencies when modifying cascading items.

@pdhdol
Copy link

pdhdol commented Jul 12, 2023

I also found this issue still happening. we can discuss this further.
#22506

@EinavDanielDX
Copy link

This will be of great value for us, as we struggle with failure while importing dashboards on a daily basis!
Also - please make this toggle available through cli commands such as

superset import-dashboards --overwrite-cascading-charts --overwrite-cascading-datasets

@pdhdol
Copy link

pdhdol commented Jul 12, 2023

I also found this issue still happening. we can discuss this further. #22506

I added this logic Naveench131@1bd019b
and also commented out filters.extend([getattr(cls, k) == dict_rep.get(k) for k in parent_refs.keys()])
in /superset/superset/models/helpers.py

Our test case

  • swap dataset, change database for dataset
  • Adjusted /rename each object
    and export the dashboard from one environment and import to another environment and it does overwrite each object successfully

@rusackas rusackas changed the title [SIP] Import Option to Overwrite Cascading Assets of Existing Item [SIP-97] Import Option to Overwrite Cascading Assets of Existing Item Jul 17, 2023
@rusackas
Copy link
Member

rusackas commented Jul 17, 2023

I'm numbering this as SIP-97. Please start a [DISCUSS] thread on the Apache dev mailing list, so we can start steering this toward a consensus vote. Let me know here or on Superset Slack if you'd like any assistance with that process.

@villebro
Copy link
Member

I've often needed this, too, so +1 from me.

@john-bodley
Copy link
Member

I was wondering whether there are any potential security issues, and/or a requirement that the importer is the owner of the underlying assets? My fear with regards to a checkbox is people will likely check it without fully accounting for or potentially grokking the impact of said action.

Have we considered the potential risks with this approach? I think that would be valuable to enumerate in the SIP.

@rusackas
Copy link
Member

The SIP not only proposes the UI to allow choice in the cascade of overrides, but introduces a place to check for or add ownership/RBAC constraints. Can we consider/discuss what those limitations might be, e.g. you can't overwrite a dataset that you don't have write permissions on?

I'm also generally unsure what security concerns might be worth considering here. If users are allowed to overwrite a datasource on Superset Instance A with a datasource from Superset Instance B, what sort of disruptions might people cause?

@villebro
Copy link
Member

I think ideally we would keep the full operation in a transaction that's rolled back if any overwritten element is not overwritable by the importing user (preferably raising a clear message about which objects failed).

@MarcinZegar
Copy link

Native support would be very helpful. As an FYI, the preset-cli / superset-cli enables this using the superset sync native command linked here: https://github.com/preset-io/backend-sdk#synchronizing-from-exports

Useful for anyone looking for an interim solution

@mdeshmu
Copy link
Contributor

mdeshmu commented Jul 28, 2023

+1

@rusackas
Copy link
Member

Sounds like this is ready for a vote... I'll set that up today.

@bkyryliuk
Copy link
Member

+1, great proposal.
Let's make sure that safeguards are also implemented as mentioned in the comments:

  1. permissions
  2. warnings about potential blast radius e.g. # of chats / dashboards affected
  3. ability to roll back if import goes wrong e.g. unable to update one of the assets

@michael-s-molina
Copy link
Member

By selecting this option, the user indicates their intention to update all associated dependencies and ensure complete consistency between the imported item and the existing data infrastructure.

One concern that I have apart from the security ones, is about data consistency. Specifically about data that is not automatically checked by database constraints such as JSON fields (json_metadata, query_context, etc). I'm worried that users will be able to import assets successfully even if these fields contain inconsistencies which might result in many bug reports that are hard to reproduce. This can also negatively impact the users perception of Superset because they will not know that the errors are because of corrupted data. Is there any plans regarding the validation of imported assets to avoid this situation?

@rusackas
Copy link
Member

The SIP has passed from a procedural voting standpoint, but I think the concerns regarding security still need to be addressed here, as well as on the PRs as they come in (including tests/docs, etc). Thank you!

@unnyns-307
Copy link
Author

I agree that only the owner should have the ability to import and overwrite items. While anyone can export items, when attempting to import them on the destination server, we can implement a warning message. This message will kindly inform users about the list of items, including any cascading items, that they cannot overwrite due to a lack of ownership on the destination server. They will only see changes that apply to items they own.

If a user wishes to overwrite all these items, we encourage them to request ownership from the owner before proceeding with the import. We believe this approach promotes transparency and collaboration.

Additionally, for cases where users need to revert to a previous version, we are considering another development. This development involves automatically exporting dashboard files every time they are saved and storing these files on the server. We would also maintain logs in a database repository, linking them to the file paths. Please note that implementing this feature may require further development to fully support item version control in Superset. Your input and any concerns are highly valued as we continue to refine these processes.

@rusackas
Copy link
Member

rusackas commented May 7, 2024

@kasiazjc have you given this a bit of a design review yet? I just want to make sure we're reusing appropriate components/layouts/patterns.

@kasiazjc
Copy link
Contributor

kasiazjc commented Jul 30, 2024

@kasiazjc have you given this a bit of a design review yet? I just want to make sure we're reusing appropriate components/layouts/patterns.

Thanks @rusackas for tagging!

I have two comments which are -

  • let's use sentence case instead of title case in the checkboxes
  • it would be great to add a tooltip to better explain what cascading charts/datasets is, so that people can understand what items this will affect

@rusackas
Copy link
Member

Thanks @kasiazjc! @unnyns-307 just let us know when your team might have a target timeline for this, and we'll go from there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sip Superset Improvement Proposal
Projects
Development

No branches or pull requests