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 2022/07/06 06:22:48 UTC

[GitHub] [superset] john-bodley opened a new pull request, #20617: fix: Add migration to add created_by_fk as owner

john-bodley opened a new pull request, #20617:
URL: https://github.com/apache/superset/pull/20617

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   
   https://github.com/apache/superset/pull/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](https://github.com/apache/superset/blob/master/superset/connectors/sqla/models.py#L694) relationship—whereas the backend uses the [check_ownership](https://github.com/apache/superset/blob/3483446c28113fb9eb1e8adb8978e305ff5e7c65/superset/views/base.py#L647-L691) method which checks (in addition to the `owner` and `owners` 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:
   
   1. The DAO logic correctly adds the creator as an explicit owner ([example](https://github.com/apache/superset/blob/master/superset/datasets/commands/create.py#L92-L93)) so this isn't an issue for any asset which was created via a DAO commend.
   2. Prior to the DAO logic only dashboards contained [pre_add](https://github.com/apache/superset/blob/8e29ec5a6685867ffc035d20999c54c2abe36fb1/superset/views/dashboard/views.py#L96-L97) logic which added the creator as an explicit owner meaning that this issue only impacts historical charts and datasets.
   3. The `check_ownership` method contained a check for an `owner` relationship however grepping the code per `git grep "\bowner = relationship\b"` yielded no matches, i.e., it served no purpose.
   
   Given these insights this PR performs the following:
   
   1. It adds a database migration to add all creators as owners (if not already listed) for both charts and datasets. Granted this may be a little presumptive, i.e., we may be re-adding someone as an "owner" who was previously removed, however there's no way to determine what the intent of the current database state is.
   2. It updates the `check_ownership` method to remove checking the `owner` (obsolete) and `created_by` (addressed in the migration) fields.
   
   Finally a few things worth noting:
   
   1. The frontend likely should rely on an API call to check ownership to adhere to the DRY principle and ensure that the frontend and backend logic for ownership is consistent.
   2. The backend `pre_add`, `pre_update`, etc. checks should likely be deprecated, i.e., for dashboards the [pre_add](https://github.com/apache/superset/blob/8e29ec5a6685867ffc035d20999c54c2abe36fb1/superset/views/dashboard/views.py#L90-L102) logic differs fairly significantly from the [DAO](https://github.com/apache/superset/blob/master/superset/dashboards/commands/create.py#L52-L80) logic both in terms of slugs and ownership propagation, i.e., for right or wrong the `pre_add` also makes the dashboard editor an owner of all the associated slices whereas this isn't the case for the DAO command. This really muddies the code logic and makes it hard to grok how things work (or should work).
   3. I'm not really sure why the DAO `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
   <!--- Skip this if not applicable -->
   
   ### TESTING INSTRUCTIONS
   
   Tested the migration and confirmed that, 
   
   ```sql
   SELECT 
     s.id, 
     s.created_by_fk
   FROM 
     slices s
   LEFT OUTER JOIN 
     slice_user su
   ON 
    s.id = su.slice_id AND 
    s.created_by_fk = su.user_id  
   WHERE 
     su.slice_id IS NULL
   ```
   
   and
   
   ```sql  
   SELECT 
     t.id, 
     t.created_by_fk
   FROM 
     tables t
   LEFT OUTER JOIN 
     sqlatable_user su
   ON 
    t.id = su.table_id AND 
    t.created_by_fk = su.user_id  
   WHERE 
     su.table_id IS NULL
   ```
   
   returned no rows.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [x] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [x] Migration is atomic, supports rollback & is backwards-compatible
     - [x] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] 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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #20617:
URL: https://github.com/apache/superset/pull/20617#issuecomment-1176859269

   > we may be re-adding someone as an "owner" who was previously removed
   
   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 when the owners list is empty?


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] john-bodley commented on pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #20617:
URL: https://github.com/apache/superset/pull/20617#issuecomment-1189757603

   ping @ktmud


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #20617:
URL: https://github.com/apache/superset/pull/20617#issuecomment-1201342279

   @AAfghahi can you share the error messages you got?


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] john-bodley merged pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
john-bodley merged PR #20617:
URL: https://github.com/apache/superset/pull/20617


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] john-bodley commented on a diff in pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #20617:
URL: https://github.com/apache/superset/pull/20617#discussion_r916314209


##########
superset/migrations/versions/2022-07-05_15-48_409c7b420ab0_add_created_by_fk_as_owner.py:
##########
@@ -0,0 +1,104 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add created_by_fk as owner
+
+Revision ID: 409c7b420ab0
+Revises: 7fb8bca906d2
+Create Date: 2022-07-05 15:48:06.029190
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "409c7b420ab0"
+down_revision = "7fb8bca906d2"
+
+from alembic import op
+from sqlalchemy import and_, Column, insert, Integer
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+    __tablename__ = "slices"
+
+    id = Column(Integer, primary_key=True)
+    created_by_fk = Column(Integer)
+
+
+class SliceUser(Base):
+    __tablename__ = "slice_user"
+
+    id = Column(Integer, primary_key=True)
+    user_id = Column(Integer)
+    slice_id = Column(Integer)
+
+
+class SqlaTableUser(Base):
+    __tablename__ = "sqlatable_user"
+
+    id = Column(Integer, primary_key=True)
+    user_id = Column(Integer)
+    table_id = Column(Integer)
+
+
+class Table(Base):
+    __tablename__ = "tables"
+
+    id = Column(Integer, primary_key=True)
+    created_by_fk = Column(Integer)
+
+
+def upgrade():
+    bind = op.get_bind()
+    session = db.Session(bind=bind)
+
+    op.execute(
+        insert(SliceUser).from_select(
+            ["user_id", "slice_id"],
+            session.query(Slice.created_by_fk, Slice.id)
+            .outerjoin(
+                SliceUser,
+                and_(
+                    SliceUser.slice_id == Slice.id,
+                    SliceUser.user_id == Slice.created_by_fk,
+                ),
+            )
+            .filter(SliceUser.slice_id == None),
+        )
+    )
+
+    op.execute(
+        insert(SqlaTableUser).from_select(
+            ["user_id", "table_id"],
+            session.query(Table.created_by_fk, Table.id)
+            .outerjoin(
+                SqlaTableUser,
+                and_(
+                    SqlaTableUser.table_id == Table.id,
+                    SqlaTableUser.user_id == Table.created_by_fk,
+                ),
+            )
+            .filter(SqlaTableUser.table_id == None),
+        )
+    )

Review Comment:
   Thanks @ktmud for the tip. I've updated the migration and re-tested.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] AAfghahi commented on pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
AAfghahi commented on PR #20617:
URL: https://github.com/apache/superset/pull/20617#issuecomment-1201311624

   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. 


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] codecov[bot] commented on pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20617:
URL: https://github.com/apache/superset/pull/20617#issuecomment-1175832445

   # [Codecov](https://codecov.io/gh/apache/superset/pull/20617?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#20617](https://codecov.io/gh/apache/superset/pull/20617?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5694fd3) into [master](https://codecov.io/gh/apache/superset/commit/818962cc89aad34afdb8ea673908416d99631a06?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (818962c) will **decrease** coverage by `11.98%`.
   > The diff coverage is `0.00%`.
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #20617       +/-   ##
   ===========================================
   - Coverage   66.82%   54.84%   -11.99%     
   ===========================================
     Files        1752     1752               
     Lines       65609    65603        -6     
     Branches     6938     6938               
   ===========================================
   - Hits        43842    35977     -7865     
   - Misses      20007    27866     +7859     
     Partials     1760     1760               
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `53.68% <0.00%> (+0.01%)` | :arrow_up: |
   | python | `58.05% <0.00%> (-24.84%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `50.69% <0.00%> (+<0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/20617?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset/views/base.py](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvYmFzZS5weQ==) | `55.85% <0.00%> (-20.87%)` | :arrow_down: |
   | [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset/key\_value/commands/upsert.py](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3Vwc2VydC5weQ==) | `0.00% <0.00%> (-89.59%)` | :arrow_down: |
   | [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-89.37%)` | :arrow_down: |
   | [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
   | [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
   | [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
   | [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.88% <0.00%> (-68.24%)` | :arrow_down: |
   | [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `30.18% <0.00%> (-67.93%)` | :arrow_down: |
   | ... and [278 more](https://codecov.io/gh/apache/superset/pull/20617/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20617?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20617?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [818962c...5694fd3](https://codecov.io/gh/apache/superset/pull/20617?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #20617:
URL: https://github.com/apache/superset/pull/20617#issuecomment-1189805224

   Do we need to a rebase? There is recently a new migration merged.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] ktmud commented on a diff in pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20617:
URL: https://github.com/apache/superset/pull/20617#discussion_r915328393


##########
superset/migrations/versions/2022-07-05_15-48_409c7b420ab0_add_created_by_fk_as_owner.py:
##########
@@ -0,0 +1,104 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add created_by_fk as owner
+
+Revision ID: 409c7b420ab0
+Revises: 7fb8bca906d2
+Create Date: 2022-07-05 15:48:06.029190
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "409c7b420ab0"
+down_revision = "7fb8bca906d2"
+
+from alembic import op
+from sqlalchemy import and_, Column, insert, Integer
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+    __tablename__ = "slices"
+
+    id = Column(Integer, primary_key=True)
+    created_by_fk = Column(Integer)
+
+
+class SliceUser(Base):
+    __tablename__ = "slice_user"
+
+    id = Column(Integer, primary_key=True)
+    user_id = Column(Integer)
+    slice_id = Column(Integer)
+
+
+class SqlaTableUser(Base):
+    __tablename__ = "sqlatable_user"
+
+    id = Column(Integer, primary_key=True)
+    user_id = Column(Integer)
+    table_id = Column(Integer)
+
+
+class Table(Base):
+    __tablename__ = "tables"
+
+    id = Column(Integer, primary_key=True)
+    created_by_fk = Column(Integer)
+
+
+def upgrade():
+    bind = op.get_bind()
+    session = db.Session(bind=bind)
+
+    op.execute(
+        insert(SliceUser).from_select(
+            ["user_id", "slice_id"],
+            session.query(Slice.created_by_fk, Slice.id)
+            .outerjoin(
+                SliceUser,
+                and_(
+                    SliceUser.slice_id == Slice.id,
+                    SliceUser.user_id == Slice.created_by_fk,
+                ),
+            )
+            .filter(SliceUser.slice_id == None),
+        )
+    )
+
+    op.execute(
+        insert(SqlaTableUser).from_select(
+            ["user_id", "table_id"],
+            session.query(Table.created_by_fk, Table.id)
+            .outerjoin(
+                SqlaTableUser,
+                and_(
+                    SqlaTableUser.table_id == Table.id,
+                    SqlaTableUser.user_id == Table.created_by_fk,
+                ),
+            )
+            .filter(SqlaTableUser.table_id == None),
+        )
+    )

Review Comment:
   Should we do the same for the new dataset models, too? As the shadow-writing is already in process.
   
   https://github.com/apache/superset/blob/231716cb50983b04178602b86c846b7673f9d8c3/superset/datasets/models.py#L65-L70



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] john-bodley commented on pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #20617:
URL: https://github.com/apache/superset/pull/20617#issuecomment-1178272141

   @ktmud per your comment,
   
   > 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?
   
   I hear what you're saying, though the old `check_ownership` logic was based on always including the creator as an owner regardless of who was listed and thus I think you could argue either way which approach is best—acknowledging that neither are perfect—, i.e., the current migration ensures code logic parity whereas your suggestion likely reduces the level of churn in terms of re-adding the creator as an owner who previously was removed.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] [superset] AAfghahi commented on pull request #20617: fix: Add migration to add created_by_fk as explicit owner for charts and datasets

Posted by GitBox <gi...@apache.org>.
AAfghahi commented on PR #20617:
URL: https://github.com/apache/superset/pull/20617#issuecomment-1201439395

   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 `sl_dataset_user` (which I think is the new fk) was null and that caused an error. The sql alchemy error said that it was most likely a database error. 
   ``


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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