From aa608e9e6ab4e565b106038032222978882b3d8e Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Tue, 15 Sep 2020 14:40:20 +0200 Subject: [PATCH 1/7] Removed conflicting lint and isort check in model helpers seems it's not appearing anymore --- superset/models/helpers.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 105a727761ee4..68386586a2549 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -21,8 +21,6 @@ from datetime import datetime, timedelta 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 From 858a27541e17f9a466aec37678a5ea1312c36ded Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Wed, 16 Sep 2020 13:30:26 +0200 Subject: [PATCH 2/7] Removed disabled linting for accessing private method. `parent_foreign_key_mappings` becomes public because it is accessed by other instance than `self`. --- superset/models/helpers.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 68386586a2549..d328becea532f 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -87,6 +87,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 @@ -133,7 +141,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() @@ -215,9 +223,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() From aea33030d0a16055cf488530bf67cf1c6ced8946 Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Wed, 16 Sep 2020 13:32:40 +0200 Subject: [PATCH 3/7] Updated model's helper - removed unecessary exception and replaced with check while accessing global context to reset ownerships. --- superset/models/helpers.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index d328becea532f..2cd3b750253d9 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -310,11 +310,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]: From 4632bb32c92950250ed9c93b570211c61ce9bec4 Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Wed, 16 Sep 2020 13:40:03 +0200 Subject: [PATCH 4/7] Updated model's helper - renamed unused attribute to private in user link method. --- superset/models/helpers.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 2cd3b750253d9..2682517299df5 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -323,11 +323,11 @@ 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 - if not user: +def _user_link(_user: User) -> Union[Markup, str]: + if not _user: return "" - url = "/superset/profile/{}/".format(user.username) - return Markup('{}'.format(url, escape(user) or "")) + url = "/superset/profile/{}/".format(_user.username) + return Markup('{}'.format(url, escape(_user) or "")) class AuditMixinNullable(AuditMixin): From 422c026686b9a6f8cd30ad733a20be3f7aedcd9a Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Wed, 16 Sep 2020 15:11:21 +0200 Subject: [PATCH 5/7] Updated model's helper - added specific exception for adding extra json column. Removed disabled pylint rule. --- superset/models/helpers.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 2682517299df5..49b86eaf654e5 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -19,6 +19,7 @@ import logging import re from datetime import datetime, timedelta +from json.decoder import JSONDecodeError from typing import Any, Dict, List, Optional, Set, Union import humanize @@ -43,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 {} @@ -426,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 + ) return {} def set_extra_json(self, extras: Dict[str, Any]) -> None: From e22195b4212b420de823e2b3da9e80c091a5df61 Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Fri, 18 Sep 2020 16:14:49 +0200 Subject: [PATCH 6/7] Applied changes after review to `models/helpers.py`: - removed unecesary function's param rename - added extra JSON content in exception --- superset/models/helpers.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 49b86eaf654e5..2825e3ac9f81c 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -322,11 +322,11 @@ 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]: - if not _user: +def _user_link(user: User) -> Union[Markup, str]: + if not user: return "" - url = "/superset/profile/{}/".format(_user.username) - return Markup('{}'.format(url, escape(_user) or "")) + url = "/superset/profile/{}/".format(user.username) + return Markup('{}'.format(url, escape(user) or "")) class AuditMixinNullable(AuditMixin): @@ -427,7 +427,11 @@ def extra(self) -> Dict[str, Any]: return json.loads(self.extra_json) except (TypeError, JSONDecodeError) as exc: logger.error( - "Unable to load an extra json: %r. Leaving empty.", exc, exc_info=True + "Unable to load an extra JSON: %r. " + "Leaving empty. Extra JSON content: %d", + exc, + self.extra_json, + exc_info=True, ) return {} From 17e3a544ca1efd1d0ae23cd32d46a3dea179ef2a Mon Sep 17 00:00:00 2001 From: Kasia Kucharczyk Date: Fri, 18 Sep 2020 16:55:51 +0200 Subject: [PATCH 7/7] Removed self.extra_json content from exception message. --- superset/models/helpers.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 2825e3ac9f81c..fd55bba5ce27c 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -427,11 +427,7 @@ def extra(self) -> Dict[str, Any]: return json.loads(self.extra_json) except (TypeError, JSONDecodeError) as exc: logger.error( - "Unable to load an extra JSON: %r. " - "Leaving empty. Extra JSON content: %d", - exc, - self.extra_json, - exc_info=True, + "Unable to load an extra json: %r. Leaving empty.", exc, exc_info=True ) return {}