feat: convert backend chart errors to the new error type #9753

etr2460 · 2020-05-06T16:52:03Z

SUMMARY

Continuing to implement SIP-40, this time adding some of the backend logic. My plan is as follows:

Start replacing the error field in api responses with the errors list of SupersetError Objects
Support backwards compatibility on the frontend by pulling the message out of the errors list into the error field again
Provide the necessary info to the front end to render better error messages once designs are ready

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

No ui changes

TEST PLAN

Test dashboards loading with and without errors in charts, see errors still render properly
Test SQL Lab with and without query errors

ADDITIONAL INFORMATION

REVIEWERS

to: @john-bodley @villebro @dpgaspar

etr2460 · 2020-05-06T16:52:58Z

superset/views/datasource.py

@@ -39,7 +39,7 @@ class Datasource(BaseSupersetView):
    def save(self) -> Response:
        data = request.form.get("data")
        if not isinstance(data, str):
-            return json_error_response("Request missing data field.", status="500")
+            return json_error_response("Request missing data field.", status=500)


cleaning up some typing

etr2460 · 2020-05-06T16:55:08Z

superset/errors.py

+from typing import Any, Dict
+
+
+class SupersetErrorType(str, Enum):


inheriting from str provides automatic json serialization

john-bodley · 2020-05-06T16:56:39Z

superset-frontend/src/components/ErrorMessage/types.ts

  FRONTEND_CSRF_ERROR: 'FRONTEND_CSRF_ERROR',
  FRONTEND_NETWORK_ERROR: 'FRONTEND_NETWORK_ERROR',
  FRONTEND_TIMEOUT_ERROR: 'FRONTEND_TIMEOUT_ERROR',
+


Nit. Nix empty lines?

I put the empty lines here to try and organize these errors into categories, since i expect there to be many of them going forward

john-bodley · 2020-05-06T16:58:13Z

setup.cfg

@@ -45,7 +45,7 @@ combine_as_imports = true
 include_trailing_comma = true
 line_length = 88
 known_first_party = superset
-known_third_party =alembic,apispec,backoff,bleach,celery,click,colorama,contextlib2,croniter,cryptography,dataclasses,dateutil,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,geohash,geopy,humanize,isodate,jinja2,markdown,markupsafe,marshmallow,msgpack,numpy,pandas,parsedatetime,pathlib2,polyline,prison,pyarrow,pyhive,pytz,retry,selenium,setuptools,simplejson,sphinx_rtd_theme,sqlalchemy,sqlalchemy_utils,sqlparse,werkzeug,wtforms,wtforms_json,yaml
+known_third_party =alembic,apispec,backoff,bleach,celery,click,colorama,contextlib2,croniter,cryptography,dateutil,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,geohash,geopy,humanize,isodate,jinja2,markdown,markupsafe,marshmallow,msgpack,numpy,pandas,parsedatetime,pathlib2,polyline,prison,pyarrow,pyhive,pytz,retry,selenium,setuptools,simplejson,sphinx_rtd_theme,sqlalchemy,sqlalchemy_utils,sqlparse,werkzeug,wtforms,wtforms_json,yaml


dataclasses should be included here as it's not part of the stdlib in Python 3.6.

john-bodley · 2020-05-06T16:58:46Z

superset/errors.py

+    FRONTEND_CSRF_ERROR = "FRONTEND_CSRF_ERROR"
+    FRONTEND_NETWORK_ERROR = "FRONTEND_NETWORK_ERROR"
+    FRONTEND_TIMEOUT_ERROR = "FRONTEND_TIMEOUT_ERROR"
+


Nit. Nix empty lines?

john-bodley · 2020-05-06T17:02:28Z

superset/errors.py

+    message: str
+    error_type: SupersetErrorType
+    level: ErrorLevel
+    extra: Dict[str, Any]


Do you want to make this optional?

I went back and forth on that... I guess optional is probably safer in the long run, and means people don't need to remember to initialize with an empty dict, although it pushes a bit of work to the front end to check that the object exists

john-bodley · 2020-05-06T17:03:02Z

superset/viz.py

+
+                error = dataclasses.asdict(
+                    SupersetError(
+                        message="{}".format(ex),


Why not message=str(ex)?

this was copy pasted from before, i assume str(ex) works the same way, so i'll change it

john-bodley · 2020-05-06T17:03:19Z

superset/viz.py

+                        message="{}".format(ex),
+                        level=ErrorLevel.ERROR,
+                        type=SupersetErrorType.VIZ_GET_DF_ERROR,
+                        extra={},


See note regarding making this optional.

john-bodley · 2020-05-06T17:09:46Z

superset/viz.py

+                        extra={},
+                    )
+                )
+                if not self.errors:


See previous comment which would simplify this to self.errors.append(error).

not sure how your previous comment would affect this

@etr2460 apologies if I'm misreading the logic but isn't self.errors going to be a list by default and thus rather than:

if not self.errors: self.errors = [error] else: self.errors.append(error)

this could be:

self.errors.append(error)

etr2460

@john-bodley replied to/addressed your comments

etr2460 · 2020-05-06T17:34:12Z

superset-frontend/src/components/ErrorMessage/types.ts

  FRONTEND_CSRF_ERROR: 'FRONTEND_CSRF_ERROR',
  FRONTEND_NETWORK_ERROR: 'FRONTEND_NETWORK_ERROR',
  FRONTEND_TIMEOUT_ERROR: 'FRONTEND_TIMEOUT_ERROR',
+


I put the empty lines here to try and organize these errors into categories, since i expect there to be many of them going forward

etr2460 · 2020-05-06T17:35:10Z

superset/errors.py

+    message: str
+    error_type: SupersetErrorType
+    level: ErrorLevel
+    extra: Dict[str, Any]


I went back and forth on that... I guess optional is probably safer in the long run, and means people don't need to remember to initialize with an empty dict, although it pushes a bit of work to the front end to check that the object exists

etr2460 · 2020-05-06T17:36:10Z

superset/viz.py

+
+                error = dataclasses.asdict(
+                    SupersetError(
+                        message="{}".format(ex),


this was copy pasted from before, i assume str(ex) works the same way, so i'll change it

etr2460 · 2020-05-06T17:37:03Z

superset/viz.py

+                        extra={},
+                    )
+                )
+                if not self.errors:


not sure how your previous comment would affect this

codecov-io · 2020-05-06T17:50:28Z

Codecov Report

Merging #9753 into master will increase coverage by 0.19%.
The diff coverage is 80.43%.

@@            Coverage Diff             @@
##           master    #9753      +/-   ##
==========================================
+ Coverage   70.79%   70.98%   +0.19%     
==========================================
  Files         587      184     -403     
  Lines       30435    17854   -12581     
  Branches     3152        0    -3152     
==========================================
- Hits        21545    12673    -8872     
+ Misses       8776     5181    -3595     
+ Partials      114        0     -114

Flag	Coverage Δ
#cypress	`?`
#javascript	`?`
#python	`70.98% <80.43%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
superset/viz_sip38.py	`0.00% <0.00%> (ø)`
superset/views/datasource.py	`93.33% <50.00%> (ø)`
superset/viz.py	`72.00% <66.66%> (+0.03%)`	⬆️
superset/connectors/sqla/models.py	`87.80% <100.00%> (-0.77%)`	⬇️
superset/db_engine_specs/base.py	`87.67% <100.00%> (-0.12%)`	⬇️
superset/errors.py	`100.00% <100.00%> (ø)`
superset/models/helpers.py	`87.95% <100.00%> (+0.06%)`	⬆️
superset/views/base.py	`72.35% <100.00%> (ø)`
superset/views/core.py	`75.11% <100.00%> (+0.22%)`	⬆️
superset/db_engine_specs/sqlite.py	`63.33% <0.00%> (-10.01%)`	⬇️
... and 409 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 65d185f...7b87ad2. Read the comment docs.

john-bodley · 2020-05-07T02:13:20Z

superset/models/helpers.py

    ):
        self.df: pd.DataFrame = df
        self.query: str = query
        self.duration: int = duration
        self.status: str = status
        self.error_message: Optional[str] = error_message
+        self.errors: Optional[List[Dict[str, Any]]] = errors


@etr2460 sorry this could be:

self.errors: List[Dict[str, Any]] = errors or []

which simplifies the related content in viz.py.

I actually think it's better to keep this optional? Because at the end of the day we send errors down to the client, and i think it being null is better than []

@dpgaspar @villebro any thoughts on consistency here, i.e., should we use an empty list or None to represent no errors?

I moved to empty list to make the backend logic a bit neater

john-bodley

@etr2460 I think you'll also have to update viz_sip38.py. cc @villebro.

graceguo-supercat · 2020-06-03T22:57:16Z

superset-frontend/src/utils/getClientErrorObject.ts

@@ -48,6 +50,12 @@ export default function getClientErrorObject(
          .json()
          .then(errorJson => {
            let error = { ...responseObject, ...errorJson };
+
+            // Backwards compatibility for old error renderers with the new error object
+            if (error.errors && error.errors.length > 0) {


I suspect this new line cause some error_details logged as empty.
this is how front-end use error field:
https://github.com/apache/incubator-superset/blob/38a6bd79da2be8c13a89a5f67f77faeb55c4e08b/superset-frontend/src/chart/chartAction.js#L403

Can you check what is in err.errors[0].message?

pull-request-size bot added the size/L label May 6, 2020

etr2460 force-pushed the erik-ritter--error-messages-2 branch from 8c32f79 to 6232b47 Compare May 6, 2020 16:54

etr2460 commented May 6, 2020

View reviewed changes

etr2460 force-pushed the erik-ritter--error-messages-2 branch from 6232b47 to ed1e2fe Compare May 6, 2020 17:06

john-bodley reviewed May 6, 2020

View reviewed changes

etr2460 force-pushed the erik-ritter--error-messages-2 branch 3 times, most recently from 213096b to 81a1153 Compare May 6, 2020 17:39

etr2460 commented May 6, 2020

View reviewed changes

etr2460 force-pushed the erik-ritter--error-messages-2 branch from 81a1153 to d00f6dd Compare May 6, 2020 17:43

mistercrunch added the airbnb Airbnb related label May 6, 2020

john-bodley reviewed May 7, 2020

View reviewed changes

etr2460 force-pushed the erik-ritter--error-messages-2 branch 5 times, most recently from 389f774 to dc776e9 Compare May 12, 2020 22:13

john-bodley approved these changes May 12, 2020

View reviewed changes

etr2460 force-pushed the erik-ritter--error-messages-2 branch 3 times, most recently from bd9517b to 7b87ad2 Compare May 13, 2020 00:32

feat: convert backend chart errors to the new error type

fa15615

etr2460 force-pushed the erik-ritter--error-messages-2 branch from 7b87ad2 to fa15615 Compare May 13, 2020 00:57

etr2460 merged commit 83ec736 into apache:master May 13, 2020

etr2460 added the SIP-40 label Jun 2, 2020

graceguo-supercat reviewed Jun 3, 2020

View reviewed changes

mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Feb 28, 2024

mistercrunch added the 🚢 0.37.0 label Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: convert backend chart errors to the new error type #9753

feat: convert backend chart errors to the new error type #9753

etr2460 commented May 6, 2020

etr2460 May 6, 2020

etr2460 May 6, 2020

john-bodley May 6, 2020

etr2460 May 6, 2020

john-bodley May 6, 2020

john-bodley May 6, 2020

john-bodley May 6, 2020

etr2460 May 6, 2020

john-bodley May 6, 2020

etr2460 May 6, 2020

john-bodley May 6, 2020

john-bodley May 6, 2020

etr2460 May 6, 2020

john-bodley May 7, 2020

etr2460 left a comment

etr2460 May 6, 2020

etr2460 May 6, 2020

etr2460 May 6, 2020

etr2460 May 6, 2020

codecov-io commented May 6, 2020 •

edited

john-bodley May 7, 2020 •

edited

etr2460 May 8, 2020

john-bodley May 11, 2020

etr2460 May 12, 2020

john-bodley left a comment

graceguo-supercat Jun 3, 2020 •

edited

		from typing import Any, Dict


		class SupersetErrorType(str, Enum):

feat: convert backend chart errors to the new error type #9753

feat: convert backend chart errors to the new error type #9753

Conversation

etr2460 commented May 6, 2020

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

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

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

etr2460 left a 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-io commented May 6, 2020 • edited

Codecov Report

john-bodley May 7, 2020 • edited

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

john-bodley left a comment

Choose a reason for hiding this comment

graceguo-supercat Jun 3, 2020 • edited

Choose a reason for hiding this comment

codecov-io commented May 6, 2020 •

edited

john-bodley May 7, 2020 •

edited

graceguo-supercat Jun 3, 2020 •

edited