Skip to content
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: several disabled pylint rules in models/helpers.py #10909

Merged
merged 7 commits into from
Sep 18, 2020
36 changes: 20 additions & 16 deletions superset/models/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,9 @@
import logging
import re
from datetime import datetime, timedelta
from json.decoder import JSONDecodeError
from typing import Any, Dict, List, Optional, Set, Union

# isort and pylint disagree, isort should win
# pylint: disable=ungrouped-imports
import humanize
import pandas as pd
import pytz
Expand All @@ -45,9 +44,7 @@
def json_to_dict(json_str: str) -> Dict[Any, Any]:
if json_str:
val = re.sub(",[ \t\r\n]+}", "}", json_str)
val = re.sub(
",[ \t\r\n]+\]", "]", val # pylint: disable=anomalous-backslash-in-string
)
val = re.sub(",[ \t\r\n]+\\]", "]", val)
return json.loads(val)

return {}
Expand Down Expand Up @@ -89,6 +86,14 @@ def _unique_constrains(cls) -> List[Set[str]]:
)
return unique

@classmethod
def parent_foreign_key_mappings(cls) -> Dict[str, str]:
"""Get a mapping of foreign name to the local name of foreign keys"""
parent_rel = cls.__mapper__.relationships.get(cls.export_parent)
if parent_rel:
return {l.name: r.name for (l, r) in parent_rel.local_remote_pairs}
return {}

@classmethod
def export_schema(
cls, recursive: bool = True, include_parent_ref: bool = False
Expand Down Expand Up @@ -135,7 +140,7 @@ def import_from_dict(
"""Import obj from a dictionary"""
if sync is None:
sync = []
parent_refs = cls._parent_foreign_key_mappings()
parent_refs = cls.parent_foreign_key_mappings()
export_fields = set(cls.export_fields) | set(parent_refs.keys())
new_children = {c: dict_rep[c] for c in cls.export_children if c in dict_rep}
unique_constrains = cls._unique_constrains()
Expand Down Expand Up @@ -217,9 +222,7 @@ def import_from_dict(
# If children should get synced, delete the ones that did not
# get updated.
if child in sync and not is_new_obj:
back_refs = (
child_class._parent_foreign_key_mappings() # pylint: disable=protected-access
)
back_refs = child_class.parent_foreign_key_mappings()
delete_filters = [
getattr(child_class, k) == getattr(obj, back_refs.get(k))
for k in back_refs.keys()
Expand Down Expand Up @@ -306,11 +309,9 @@ def reset_ownership(self) -> None:
self.created_by = None
self.changed_by = None
# flask global context might not exist (in cli or tests for example)
try:
if g.user:
self.owners = [g.user]
except Exception: # pylint: disable=broad-except
self.owners = []
self.owners = []
if g and hasattr(g, "user"):
self.owners = [g.user]

@property
def params_dict(self) -> Dict[Any, Any]:
Expand All @@ -321,7 +322,7 @@ def template_params_dict(self) -> Dict[Any, Any]:
return json_to_dict(self.template_params) # type: ignore


def _user_link(user: User) -> Union[Markup, str]: # pylint: disable=no-self-use
def _user_link(user: User) -> Union[Markup, str]:
if not user:
return ""
url = "/superset/profile/{}/".format(user.username)
Expand Down Expand Up @@ -424,7 +425,10 @@ class ExtraJSONMixin:
def extra(self) -> Dict[str, Any]:
try:
return json.loads(self.extra_json)
except Exception: # pylint: disable=broad-except
except (TypeError, JSONDecodeError) as exc:
logger.error(
"Unable to load an extra json: %r. Leaving empty.", exc, exc_info=True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can exc or exc_info log data from self.extra_json ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I will add it to an exception logger.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I did not explain myself right, sorry. I want to make sure that self.extra_json never gets logged by TypeError or JSONDecodeError by exc or exc_info

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dpgaspar do you mind to take a look again? I have one doubt: do you think is it possible there could be raised exception while accessing self.extra_json for logger?

Copy link
Contributor Author

@kkucharc kkucharc Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Och, I see, then I just did the opposite thing! :) Reverting this change.
To be honest I think JSONDecodeError will show error message %s: line %d column %d (char %d).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure I check that and any of them won't show what self.extra_json contains.
JSONDecodeError should something like:
json.decoder.JSONDecodeError: Unterminated string starting at: line 1 column 11 (char 10)
with stacktrace.
And TypeError something like this for example:
TypeError: the JSON object must be str, bytes or bytearray, not dict

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice then, thks

)
return {}

def set_extra_json(self, extras: Dict[str, Any]) -> None:
Expand Down