john-bodley commented on code in PR #20492:
URL: https://github.com/apache/superset/pull/20492#discussion_r906224319
##########
superset/migrations/versions/2016-06-27_08-43_27ae655e4247_make_creator_owners.py:
##########
@@ -63,17 +64,10 @@ class User(Base):
class AuditMixin:
- @classmethod
- def get_user_id(cls):
- try:
- return g.user.id
- except Exception:
- return None
-
@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
Review Comment:
Same logic but ensuring we use the same `get_user_id` method.
##########
superset/views/base_schemas.py:
##########
@@ -113,8 +115,9 @@ def pre_load(self, data: Dict[Any, Any]) -> None:
@staticmethod
def set_owners(instance: Model, owners: List[int]) -> None:
owner_objs = []
- if g.user.get_id() not in owners:
Review Comment:
This was wrong, i.e., `g.user.get_id()` returned a `str` which clearly
wasn't in `owners` (`List[int]`) and thus owners would end up like `[1, 2, 3,
"1"]`.
##########
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(),
Review Comment:
Mixed types, though likely benign from the database's perspective—at least
for MySQL.
##########
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(),
Review Comment:
The
[get_user_id](https://github.com/dpgaspar/Flask-AppBuilder/blob/090ddb4645992fb390da93906301c63b204db0df/flask_appbuilder/security/sqla/models.py#L141-L146)
method 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.
##########
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
Review Comment:
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`.
##########
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)
Review Comment:
Avoiding passing around the global user. More on this in a later PR.
##########
superset/views/core.py:
##########
@@ -2615,10 +2614,10 @@ def search_queries(self) -> FlaskResponse: # pylint:
disable=no-self-use
search_user_id = int(cast(int, request.args.get("user_id")))
except ValueError:
return Response(status=400, mimetype="application/json")
- if search_user_id != g.user.get_user_id():
+ if search_user_id != get_user_id():
Review Comment:
This was wrong, i.e., it would always evaluate to `True`.
##########
superset/views/core.py:
##########
@@ -2587,9 +2588,7 @@ def queries_exec(last_updated_ms: Union[float, int]) ->
FlaskResponse:
sql_queries = (
db.session.query(Query)
- .filter(
- Query.user_id == g.user.get_id(), Query.changed_on >=
last_updated_dt
- )
+ .filter(Query.user_id == get_user_id(), Query.changed_on >=
last_updated_dt)
Review Comment:
Mixed types, though likely benign from the database's perspective—at least
for MySQL.
##########
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(),
Review Comment:
I'm not sure what the ramifications of assigning `user_id` to a `str` was.
##########
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(),
Review Comment:
Mixed types, though likely benign from the database's perspective—at least
for MySQL.
##########
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())
Review Comment:
Mixed types, though likely benign from the database's perspective—at least
for MySQL.
##########
superset/views/sql_lab.py:
##########
@@ -188,12 +189,12 @@ def activate( # pylint: disable=no-self-use
owner_id = _get_owner_id(tab_state_id)
if owner_id is None:
return Response(status=404)
- if owner_id != int(g.user.get_id()):
+ if owner_id != get_user_id():
return Response(status=403)
(
db.session.query(TabState)
- .filter_by(user_id=g.user.get_id())
+ .filter_by(user_id=get_user_id())
Review Comment:
Mixed types, though likely benign from the database's perspective—at least
for MySQL.
##########
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(),
Review Comment:
Mixed types, though likely benign from the database's perspective—at least
for MySQL.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
