refactor: Cleanup user get_id/get_user_id #20492

john-bodley · 2022-06-24T16:27:56Z

SUMMARY

This is the first PR in a slew of many to cleanup/standardize the user ID/username logic. Specifically it:

Replaces the Flask-AppBuilder User.get_id and Flask-Login UserMixin.get_id calls which returns Optional[str] with a get_user_id convenience method which returns Optional[int]. I'm not sure how problematic this was given I doubt authors know of this nuance and thus many Python == checks would have failed, i.e., 1 == "1" evaluates to False, however the database filters may have been more lenient, i.e., in MySQL SELECT 1 = '1' evaluates to true.
Fixes get_owner which returned the ID of the current user and not the referenced user.
Additional cleanup.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

john-bodley · 2022-06-24T16:28:50Z

superset/charts/api.py

@@ -805,7 +805,7 @@ def favorite_status(self, **kwargs: Any) -> Response:
        charts = ChartDAO.find_by_ids(requested_ids)
        if not charts:
            return self.response_404()
-        favorited_chart_ids = ChartDAO.favorited_ids(charts, g.user.get_id())
+        favorited_chart_ids = ChartDAO.favorited_ids(charts)


Avoiding passing around the global user. More on this in a later PR.

john-bodley · 2022-06-24T16:30:40Z

superset/dashboards/filters.py

@@ -57,9 +58,9 @@ def apply(self, query: Query, value: Any) -> Query:
        return query.filter(
            or_(
                Dashboard.created_by_fk  # pylint: disable=comparison-with-callable
-                == g.user.get_user_id(),


The get_user_id method is a classmethod and thus doesn't return the ID associated with the user object but rather the g.user.id. In this case they're the same but it's very misleading.

john-bodley · 2022-06-24T16:32:49Z

superset/key_value/utils.py

@@ -67,4 +67,4 @@ def get_uuid_namespace(seed: str) -> UUID:


 def get_owner(user: User) -> Optional[int]:
-    return user.get_user_id() if not user.is_anonymous else None


See above. This is wrong, user.get_user_id does not return user.id—for non-anonymous users—but rather g.user.id. The reason this isn't an issue is that user is always g.user. I'm working on another PR which will replace get_owner with get_user_id.

john-bodley · 2022-06-24T16:33:18Z

superset/migrations/versions/2016-06-27_08-43_27ae655e4247_make_creator_owners.py

    @declared_attr
    def created_by_fk(cls):
        return Column(
-            Integer, ForeignKey("ab_user.id"), default=cls.get_user_id, nullable=False
+            Integer, ForeignKey("ab_user.id"), default=get_user_id, nullable=False


Same logic but ensuring we use the same get_user_id method.

john-bodley · 2022-06-24T16:35:44Z

superset/views/base_api.py

@@ -145,7 +145,7 @@ def apply(self, query: Query, value: Any) -> Query:
            return query
        users_favorite_query = db.session.query(FavStar.obj_id).filter(
            and_(
-                FavStar.user_id == g.user.get_id(),
+                FavStar.user_id == get_user_id(),


Mixed types, though likely benign from the database's perspective—at least for MySQL.

john-bodley · 2022-06-24T16:40:05Z

superset/views/sql_lab.py

@@ -145,7 +146,7 @@ def post(self) -> FlaskResponse:  # pylint: disable=no-self-use
        )
        (
            db.session.query(TabState)
-            .filter_by(user_id=g.user.get_id())
+            .filter_by(user_id=get_user_id())


Mixed types, though likely benign from the database's perspective—at least for MySQL.

john-bodley · 2022-06-24T16:40:31Z

superset/views/sql_lab.py

@@ -134,7 +135,7 @@ class TabStateView(BaseSupersetView):
    def post(self) -> FlaskResponse:  # pylint: disable=no-self-use
        query_editor = json.loads(request.form["queryEditor"])
        tab_state = TabState(
-            user_id=g.user.get_id(),
+            user_id=get_user_id(),


I'm not sure what the ramifications of assigning user_id to a str was.

john-bodley · 2022-06-24T16:40:43Z

superset/views/sql_lab.py

            return Response(status=403)

        (
            db.session.query(TabState)
-            .filter_by(user_id=g.user.get_id())
+            .filter_by(user_id=get_user_id())


Mixed types, though likely benign from the database's perspective—at least for MySQL.

john-bodley · 2022-06-24T16:41:00Z

superset/views/sql_lab.py

@@ -242,7 +243,7 @@ def delete_query(  # pylint: disable=no-self-use
                .filter(
                    and_(
                        Query.client_id != client_id,
-                        Query.user_id == g.user.get_id(),
+                        Query.user_id == get_user_id(),


Mixed types, though likely benign from the database's perspective—at least for MySQL.

john-bodley · 2022-06-24T16:41:06Z

superset/views/sql_lab.py

@@ -255,7 +256,7 @@ def delete_query(  # pylint: disable=no-self-use

        db.session.query(Query).filter_by(
            client_id=client_id,
-            user_id=g.user.get_id(),
+            user_id=get_user_id(),


Mixed types, though likely benign from the database's perspective—at least for MySQL.

codecov · 2022-06-24T16:53:02Z

Codecov Report

Merging #20492 (fbd0f62) into master (b33c23e) will decrease coverage by 11.76%.
The diff coverage is 63.79%.

❗ Current head fbd0f62 differs from pull request most recent head 409b7d6. Consider uploading reports for the commit 409b7d6 to get more accurate results

@@             Coverage Diff             @@
##           master   #20492       +/-   ##
===========================================
- Coverage   66.75%   54.98%   -11.77%     
===========================================
  Files        1740     1745        +5     
  Lines       65156    65394      +238     
  Branches     6900     6900               
===========================================
- Hits        43492    35960     -7532     
- Misses      19915    27685     +7770     
  Partials     1749     1749

Flag	Coverage Δ
hive	`53.76% <63.79%> (+0.04%)`	⬆️
mysql	`?`
postgres	`?`
presto	`53.62% <63.79%> (+0.04%)`	⬆️
python	`58.39% <63.79%> (-24.46%)`	⬇️
sqlite	`?`
unit	`50.65% <51.72%> (+0.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/charts/api.py	`50.37% <0.00%> (-35.61%)`	⬇️
superset/dashboards/api.py	`51.45% <0.00%> (-41.28%)`	⬇️
superset/key_value/utils.py	`86.48% <0.00%> (-8.11%)`	⬇️
superset/views/base_schemas.py	`0.00% <0.00%> (ø)`
superset/views/sql_lab.py	`56.57% <14.28%> (-3.69%)`	⬇️
superset/jinja_context.py	`84.58% <25.00%> (-6.57%)`	⬇️
superset/security/manager.py	`62.33% <50.00%> (-33.02%)`	⬇️
superset/views/core.py	`34.79% <55.55%> (-42.57%)`	⬇️
superset/charts/dao.py	`48.88% <100.00%> (-39.75%)`	⬇️
superset/charts/data/api.py	`89.80% <100.00%> (ø)`
... and 299 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b33c23e...409b7d6. Read the comment docs.

1

ktmud · 2022-06-24T17:02:48Z

superset/charts/dao.py

@@ -70,15 +71,15 @@ def overwrite(slc: Slice, commit: bool = True) -> None:
            db.session.commit()

    @staticmethod
-    def favorited_ids(charts: List[Slice], current_user_id: int) -> List[FavStar]:
+    def favorited_ids(charts: List[Slice]) -> List[FavStar]:


Will we ever need to get a list of favorited charts for other users?

@ktmud no. Also this logic is going to refactored in another PR. In general Superset does not handle users other than the current logged in user. There have been recent mechanisms added to override the user globally if needed.

Co-authored-by: John Bodley <john.bodley@airbnb.com>

pull-request-size bot added the size/XXL label Jun 24, 2022

john-bodley commented Jun 24, 2022

View reviewed changes

john-bodley marked this pull request as ready for review June 24, 2022 16:41

john-bodley requested a review from a team as a code owner June 24, 2022 16:41

john-bodley requested review from betodealmeida, dpgaspar, ktmud and zhaoyongjie June 24, 2022 16:42

ktmud approved these changes Jun 24, 2022

View reviewed changes

john-bodley force-pushed the john-bodley--get-user-id branch from fbd0f62 to 409b7d6 Compare June 24, 2022 17:38

pull-request-size bot added size/L and removed size/XXL labels Jun 24, 2022

john-bodley force-pushed the john-bodley--get-user-id branch 5 times, most recently from 968ccc3 to 5fbada6 Compare June 24, 2022 23:33

refactor: Cleanup user get_id/get_user_id

02bf074

john-bodley force-pushed the john-bodley--get-user-id branch from 5fbada6 to 02bf074 Compare June 25, 2022 00:24

john-bodley merged commit 3483446 into apache:master Jun 25, 2022

john-bodley deleted the john-bodley--get-user-id branch June 25, 2022 00:57

akshatsri pushed a commit to charan1314/superset that referenced this pull request Jul 19, 2022

refactor: Cleanup user get_id/get_user_id (apache#20492)

7526ef9

Co-authored-by: John Bodley <john.bodley@airbnb.com>

john-bodley mentioned this pull request Jun 15, 2023

chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS #24400

Merged

9 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Cleanup user get_id/get_user_id #20492

refactor: Cleanup user get_id/get_user_id #20492

john-bodley commented Jun 24, 2022 •

edited

Loading

john-bodley Jun 24, 2022

john-bodley Jun 24, 2022 •

edited

Loading

john-bodley Jun 24, 2022

john-bodley Jun 24, 2022

john-bodley Jun 24, 2022

john-bodley Jun 24, 2022

john-bodley Jun 24, 2022

john-bodley Jun 24, 2022

john-bodley Jun 24, 2022

john-bodley Jun 24, 2022

codecov bot commented Jun 24, 2022 •

edited

Loading

ktmud Jun 24, 2022

john-bodley Jun 24, 2022

		@@ -67,4 +67,4 @@ def get_uuid_namespace(seed: str) -> UUID:


		def get_owner(user: User) -> Optional[int]:
		return user.get_user_id() if not user.is_anonymous else None

refactor: Cleanup user get_id/get_user_id #20492

refactor: Cleanup user get_id/get_user_id #20492

Conversation

john-bodley commented Jun 24, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

john-bodley Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Jun 24, 2022 •

edited

Loading

john-bodley Jun 24, 2022 •

edited

Loading

codecov bot commented Jun 24, 2022 •

edited

Loading