You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/07/30 07:53:13 UTC
[GitHub] [incubator-superset] stuarthu opened a new pull request #10480: fix: dashboard slug is cleared on import (#10479)
stuarthu opened a new pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480
Signed-off-by: Stuart Hu <sh...@improbable.io>
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
willbarrett commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-668211673
In order to avoid introducing destructive changes, how about this as an alternative?
1. Attempt to insert with the given slug
2. If the slug exists (unique key error), add a random postfix to the slug and re-attempt the insert
That would effectively attempt to retain the slug, but fall back to a behavior that preserves existing data.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu edited a comment on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu edited a comment on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-668329081
> In order to avoid introducing destructive changes, how about this as an alternative?
>
> 1. Attempt to insert with the given slug
> 2. If the slug exists (unique key error), add a random postfix to the slug and re-attempt the insert
>
> That would effectively attempt to retain the slug, but fall back to a behavior that preserves existing data.
No, random prefix is unexpected behavior.
There is a principal in design called least astonishment https://en.wikipedia.org/wiki/Principle_of_least_astonishment.
Users would expect to import as is and fail if conflict.
If we altered yaml without notice, that is an astonishment to the user.
Both removing the slug and appending random prefix to the slug are unexpected to the user.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-666943874
I think the re-written can be done in the future but as for now, I hope this fix could be merged.
There are cases that clear slug is in favor and there are cases that preserve slug is in favor.
The question is:
Before we re-written the logic and provide with a user option, how do we handle the conflicing scenarios.
If we clear the slug when importing, then we just deny all the other cases. Which is causing me pain because I have no way to update the dashboard later (I used slug as the key condition).
On the other hand, if we preserve the slug, the importing will fail due to slug conflict. Then user is able to detect that conflict and solve it by means at choice (like rename the slug or delete the dashboard with the same slug).
That is why I prefer to preserve the slug when importing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-666858936
The reason why I did this is that we'd get unique constraints on load, and that there's no way to know ahead of time how the conflict should be handled (should we merge/overwrite? should we clear and add a new entry?).
I punted and went in favor of the non destructive / non-raising approach. I think the import form/flow needs to be updated to provide options as to how to handle slug collisions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-669652798
> esired behavior is generally to overwrite. BUT! What if there's a collision on a generic nam
Yes I'm getting a unique constraint error but that's expected.
With this error I know there is a conflict and I can deal with it by deleteing the existing one in superset DB and re-import the dashboard.
There is no unexpected altering behind the scence.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu edited a comment on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu edited a comment on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-666943874
I think the re-written can be done in the future but as for now, I hope this fix could be merged.
There are cases that clear slug is in favor and there are cases that preserve slug is in favor.
The question is:
Before we re-written the logic and provide with a user option, how do we handle the conflicing scenarios.
If we clear the slug when importing, then we just deny all the other cases. Which is causing me pain because I have no way to update the dashboard later (I used slug as the key to find my dashboards).
On the other hand, if we preserve the slug, the importing will fail due to slug conflict. Then user is able to detect that conflict and solve it by means at choice (like rename the slug or delete the dashboard with the same slug).
That is why I prefer to preserve the slug when importing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-668329081
> In order to avoid introducing destructive changes, how about this as an alternative?
>
> 1. Attempt to insert with the given slug
> 2. If the slug exists (unique key error), add a random postfix to the slug and re-attempt the insert
>
> That would effectively attempt to retain the slug, but fall back to a behavior that preserves existing data.
No, random prefix is unexpected behavior.
There is a principal in design called least astonishment https://en.wikipedia.org/wiki/Principle_of_least_astonishment.
Users would hope to import as is and fail if conflict.
If we altered yaml without notice, that is an astonishment to the user.
No matter if we remove the slug or append random prefix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu edited a comment on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu edited a comment on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-666943874
I think the re-written can be done in the future but as for now, I hope this fix could be merged.
There are cases that clear slug is in favor and there are cases that preserve slug is in favor.
The question is:
Before we re-written the logic and provide with a user option, how do we handle the conflicing scenarios.
If we clear the slug when importing, then we just deny all the other cases. Which is causing me pain because then I have no way to update the dashboard later (I used slug as the key to find my dashboards).
On the other hand, if we preserve the slug, the importing will fail due to slug conflict. Then user is able to detect that conflict and solve it by means at choice (like rename the slug or delete the dashboard with the same slug). After that, user can re-import. There is no blocking issue in this approach.
That is why I prefer to preserve the slug when importing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-669638044
So the question remains: if the slug exist, what do you do? Overwrite or Insert?
The desired behavior is generally to overwrite. BUT! What if there's a collision on a generic name like `sales`, and someone overwrite someone else's dashboard?
I know in the code there's currently brittle logic around the "remote id" that gets stored in the target. Unlike uuids, they are not universally unique so it's not great. It also looked like the logic around it didn't work well since I was getting a unique constraint error.
With this *fix*, are you not getting a unique constraint error if you load the same dashobard twice?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu edited a comment on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu edited a comment on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-668329081
> In order to avoid introducing destructive changes, how about this as an alternative?
>
> 1. Attempt to insert with the given slug
> 2. If the slug exists (unique key error), add a random postfix to the slug and re-attempt the insert
>
> That would effectively attempt to retain the slug, but fall back to a behavior that preserves existing data.
No, random prefix is unexpected behavior.
There is a principal in design called least astonishment https://en.wikipedia.org/wiki/Principle_of_least_astonishment.
Users would hope to import as is and fail if conflict.
If we altered yaml without notice, that is an astonishment to the user.
Both removing the slug and appending random prefix to the slug are unexpected to the user.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch edited a comment on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
mistercrunch edited a comment on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-670335391
What does the error message look like on the import collision? Given the proper message like `A dashboard with the same slug titled "{dashboard_title}" already exists", please delete it prior to importing`, it could work well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-666859596
Personally I think the whole export/import logic needs to be largely re-written and merge/upsert be based on uuids that we should introduce in models. Somehow this relates to object versioning / non-destructive updates / deletes and history management.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-666271283
@mistercrunch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu edited a comment on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu edited a comment on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-666943874
I think the re-written can be done in the future but as for now, I hope this fix could be merged.
There are cases that clear slug is in favor and there are cases that preserve slug is in favor.
The question is:
Before we re-written the logic and provide with a user option, how do we handle the conflicing scenarios.
If we clear the slug when importing, then we just deny all the other cases. Which is causing me pain because I have no way to update the dashboard later (I used slug as the key to find my dashboards).
On the other hand, if we preserve the slug, the importing will fail due to slug conflict. Then user is able to detect that conflict and solve it by means at choice (like rename the slug or delete the dashboard with the same slug). Then re-import.
That is why I prefer to preserve the slug when importing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stale[bot] closed pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stale[bot] closed pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-670335391
What does the error message look like on the import collision? Given the proper message like `A dashboard with the same slug title "{dashboard_title}" already exists", please delete it prior to importing`, it could work well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-670367198
> What does the error message look like on the import collision? Given the proper message like `A dashboard with the same slug titled "{dashboard_title}" already exists", please delete it prior to importing`, it could work well.
I don't remember but the old ones good enough for me. Feel free to give any proper messages.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stuarthu edited a comment on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stuarthu edited a comment on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-666943874
I think the re-written can be done in the future but as for now, I hope this fix could be merged.
There are cases that clear slug is in favor and there are cases that preserve slug is in favor.
The question is:
Before we re-written the logic and provide with a user option, how do we handle the conflicing scenarios.
If we clear the slug when importing, then we just deny all the other cases. Which is causing me pain because I have no way to update the dashboard later (I used slug as the key to find my dashboards).
On the other hand, if we preserve the slug, the importing will fail due to slug conflict. Then user is able to detect that conflict and solve it by means at choice (like rename the slug or delete the dashboard with the same slug). After that, user can re-import.
That is why I prefer to preserve the slug when importing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stale[bot] commented on pull request #10480: fix(#10479): dashboard slug is cleared on import
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10480:
URL: https://github.com/apache/incubator-superset/pull/10480#issuecomment-706542837
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue `.pinned` to prevent stale bot from closing the issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org