-
Notifications
You must be signed in to change notification settings - Fork 13.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
fix: Add migration to add created_by_fk as explicit owner for charts and datasets #20617
fix: Add migration to add created_by_fk as explicit owner for charts and datasets #20617
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20617 +/- ##
===========================================
- Coverage 66.79% 54.84% -11.96%
===========================================
Files 1753 1752 -1
Lines 65618 65613 -5
Branches 6952 6938 -14
===========================================
- Hits 43831 35986 -7845
- Misses 20023 27867 +7844
+ Partials 1764 1760 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. |
5694fd3
to
72ad48c
Compare
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.
The change makes sense to me.
) | ||
.filter(SqlaTableUser.table_id == 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.
Should we do the same for the new dataset models, too? As the shadow-writing is already in process.
superset/superset/datasets/models.py
Lines 65 to 70 in 231716c
dataset_user_association_table = sa.Table( | |
"sl_dataset_users", | |
Model.metadata, # pylint: disable=no-member | |
sa.Column("dataset_id", sa.ForeignKey("sl_datasets.id"), primary_key=True), | |
sa.Column("user_id", sa.ForeignKey("ab_user.id"), primary_key=True), | |
) |
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.
Thanks @ktmud for the tip. I've updated the migration and re-tested.
Since the problem seems to be a legacy dataset not created by DAO now has an empty owners list because the creator was not explicitly added, I wonder if it is safer to just set the creator as the sole owner for datasets/slices where the owners list is empty? |
72ad48c
to
e3d06dc
Compare
@ktmud per your comment,
I hear what you're saying, though the old |
ping @ktmud |
Do we need a rebase? There is recently a new migration merged. |
Hello, just to flag this. When I tried to do a migration with my database that has just examples, this required me to drop the database and then re-populate. |
@AAfghahi can you share the error messages you got? |
Hey, sorry I thought that I wrote in here but forgot to press comment apparently. I did not save the error message unfortunately, however. I remember that it would hit this migration and then error out because |
SUMMARY
#19854 introduced frontend logic which would check that (for Alpha users) only owners can edit datasets. The issue is the definition of ownership between the frontend and backend differs. Specifically the frontend merely checks whether the user is listed as a owner—per the owners relationship—whereas the backend uses the check_ownership method which checks (in addition to the
owner
andowners
relationships) whether the user is the creator. This dichotomy means that creators who are not explicitly listed as owners are unable to edit their datasets.Unwinding the logic surfaces additional observations regarding ownership for various assets, i.e., charts, dashboards, datasets, and reports:
check_ownership
method contained a check for anowner
relationship however grepping the code pergit grep "\bowner = relationship\b"
yielded no matches, i.e., it served no purpose.Given these insights this PR performs the following:
check_ownership
method to remove checking theowner
(obsolete) andcreated_by
(addressed in the migration) fields.Finally a few things worth noting:
pre_add
,pre_update
, etc. checks should likely be deprecated given the logic also resides in the DAO commands. This really muddies the code logic and makes it hard to grok how things work (or should work).validate
command also populates fields, i.e., adds owners. This seems somewhat contradictory and non-intuitive, i.e., validation shouldn't update the model. I gather it was likely for efficiency reasons, but I sense there is merit in rewriting the DAO logic to break up the validation and population phases.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Tested the migration and confirmed that,
,
and
returned no rows.
ADDITIONAL INFORMATION