feat: initial work to make v1 API compatible with SIP-40 and SIP-41 #13960

betodealmeida · 2021-04-06T03:29:15Z

SUMMARY

This PR is a first step in making the v1 API compatible with SIP-40 and SIP-41.

Currently, the API returns exceptions using helper methods from Flask-AppBuilder, eg:

        try:
            TestConnectionDatabaseCommand(g.user, item).run()
            return self.response(200, message="OK")
        except DatabaseTestConnectionFailedError as ex:
            return self.response_422(message=str(ex))
        except SupersetErrorException as ex:
            return self.response(ex.status, message=ex.error.message)

These responses are simple JSON payloads, not compatible with the standard error format introduced in SIP-40. For example, when adding a database with a non-existent driver (foo://) this is returned:

{"message": "Could not load database driver: BaseEngineSpec"}

And this is the message displayed in the frontend (in a toast):

ERROR: Could not load database driver: BaseEngineSpec

To conform with SIP-41 I added ~~a new decorator called handle_exception~~ an error handler that captures exceptions and formats them according to SIP-40. This allows us to simplify the API logic, removing the need to capture exceptions when running commands (see the new test_connection method in this PR).

With the decorator, the backend now returns:

{
	"errors": [
		{
			"message": "Could not load database driver: BaseEngineSpec",
			"error_type": "GENERIC_COMMAND_ERROR",
			"level": "error",
			"extra": {
				"issue_codes": [
					{
						"code": 1010,
						"message": "Issue 1010 - Superset found an error while running a command."
					}
				]
			}
		}
	]
}

And the message is displayed correctly in the frontend as before, in a toast, without requiring any modifications:

ERROR: Could not load database driver: BaseEngineSpec

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

See above.

TEST PLAN

Will add unit tests, I wanted to discuss this first.

ADDITIONAL INFORMATION

Has associated issue: [chart] New chart data API should format errors according to SIP-40 #11882
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

etr2460

So in this case, you've got the DatabaseTestConnectionFailedError that test_connection can raise. Should this be something that extends a SupersetErrorException or should it be caught in the handle_exception wrapper and then return a response with a constructed SupersetError of type TEST_CONNECTION_PORT_CLOSED_ERROR or TEST_CONNECTION_HOST_DOWN_ERROR?

codecov · 2021-04-06T03:50:45Z

Codecov Report

Merging #13960 (0ee5033) into master (f19f2c3) will increase coverage by 0.32%.
The diff coverage is 85.18%.

❗ Current head 0ee5033 differs from pull request most recent head a2d5833. Consider uploading reports for the commit a2d5833 to get more accurate results

@@            Coverage Diff             @@
##           master   #13960      +/-   ##
==========================================
+ Coverage   78.37%   78.70%   +0.32%     
==========================================
  Files         935      936       +1     
  Lines       47375    47407      +32     
  Branches     5973     5956      -17     
==========================================
+ Hits        37131    37310     +179     
+ Misses      10097     9954     -143     
+ Partials      147      143       -4

Flag	Coverage Δ
cypress	`56.06% <100.00%> (+2.27%)`	⬆️
hive	`80.27% <84.00%> (+<0.01%)`	⬆️
javascript	`67.30% <100.00%> (+0.18%)`	⬆️
mysql	`80.56% <84.00%> (+<0.01%)`	⬆️
postgres	`80.60% <84.00%> (+<0.01%)`	⬆️
presto	`80.29% <84.00%> (+<0.01%)`	⬆️
python	`81.15% <84.00%> (+<0.01%)`	⬆️
sqlite	`80.16% <84.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...rset-frontend/src/components/ErrorMessage/types.ts	`100.00% <ø> (ø)`
...src/dashboard/components/PropertiesModal/index.jsx	`85.61% <ø> (ø)`
superset/exceptions.py	`96.66% <60.00%> (+0.05%)`	⬆️
superset/views/base.py	`76.11% <78.57%> (+0.28%)`	⬆️
...rBar/CascadeFilters/CascadeFilterControl/index.tsx	`85.71% <100.00%> (ø)`
.../FilterBar/CascadeFilters/CascadePopover/index.tsx	`83.09% <100.00%> (ø)`
.../controls/MetricControl/AdhocMetricEditPopover.jsx	`79.54% <100.00%> (ø)`
superset/connectors/sqla/models.py	`90.76% <100.00%> (-0.02%)`	⬇️
superset/databases/api.py	`91.80% <100.00%> (-0.23%)`	⬇️
superset/databases/commands/exceptions.py	`93.33% <100.00%> (+0.15%)`	⬆️
... and 28 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 f19f2c3...a2d5833. Read the comment docs.

betodealmeida · 2021-04-06T04:58:56Z

So in this case, you've got the DatabaseTestConnectionFailedError that test_connection can raise. Should this be something that extends a SupersetErrorException or should it be caught in the handle_exception wrapper and then return a response with a constructed SupersetError of type TEST_CONNECTION_PORT_CLOSED_ERROR or TEST_CONNECTION_HOST_DOWN_ERROR?

Right, I actually have another PR where I started doing that:

class DatabaseTestConnectionFailedError(SupersetErrorsException):
    message = _("Connection failed, please check your connection settings")

This way it will expose the proper errors (and additional errors that I'm adding, eg, TEST_CONNECTION_INVALID_USERNAME_ERROR).

dpgaspar

Sometime ago @john-bodley proposed to use this Flask capability: https://flask.palletsprojects.com/en/1.1.x/errorhandling/#registering. I like the idea, and it seems doable, that way we would not need to implement our own decorator for error handling

dpgaspar · 2021-04-06T08:26:38Z

superset/views/base_api.py

+            logger.warning(ex)
+            return json_errors_response(
+                errors=[
+                    SupersetError(


Are we always going to throw HTTP 500 on these? would it make sense that the exception itself could identify the HTTP status code?

I don't think that command exceptions should have HTTP status codes, they should care only about business logic and be decoupled from the API.

Ideally the API layer should do the mapping between command exceptions to SupersetError(s)Exception. The reason why I added CommandInvalidError and CommandException is so we can gradually implement that mapping, while in the meantime any non-covered command exceptions return a 500.

superset/databases/api.py

betodealmeida · 2021-04-06T16:18:54Z

Sometime ago @john-bodley proposed to use this Flask capability: https://flask.palletsprojects.com/en/1.1.x/errorhandling/#registering. I like the idea, and it seems doable, that way we would not need to implement our own decorator for error handling

I agree, I love that pattern. Let me give it a try, it might be easier since we register the error handler and then we can gradually change the APIs to raise SupersetErrorException and SupersetErrorsException (or ComandException initially, but I'd rather have all APIs only returning one of the first two).

docs/src/pages/docs/Miscellaneous/issue_codes.mdx

superset/errors.py

etr2460

this all makes sense to me, stamping from the frontend perspective

etr2460 · 2021-04-07T01:00:53Z

tests/databases/api_tests.py

-            "message": "Could not load database driver: BaseEngineSpec",
+            "errors": [
+                {
+                    "message": "Could not load database driver: BaseEngineSpec",


i guess ideally in the future these messages will become actual errors with details on how to resolve them in the documentation

Right, we're going to work on all the messaging in the test connection flow and add database flow.

…pache#13960) * WIP * Use errorhandler * Add response schema * Fix status on HTTPException * s/found/encountered/g * Fix test * Fix lint * Fix lint and test

WIP

f7caae7

pull-request-size bot added the size/L label Apr 6, 2021

betodealmeida requested review from etr2460 and dpgaspar April 6, 2021 03:29

etr2460 reviewed Apr 6, 2021

View reviewed changes

dpgaspar reviewed Apr 6, 2021

View reviewed changes

Use errorhandler

c11d9d1

betodealmeida force-pushed the v1api_sip40_integration branch from d77fe8b to c11d9d1 Compare April 6, 2021 19:54

Add response schema

cded967

betodealmeida force-pushed the v1api_sip40_integration branch from d49b790 to cded967 Compare April 6, 2021 20:19

etr2460 reviewed Apr 6, 2021

View reviewed changes

docs/src/pages/docs/Miscellaneous/issue_codes.mdx Outdated Show resolved Hide resolved

superset/errors.py Outdated Show resolved Hide resolved

betodealmeida added 4 commits April 6, 2021 15:40

Fix status on HTTPException

37ee273

s/found/encountered/g

6cfd49c

Fix test

0946cfe

Fix lint

273667f

etr2460 approved these changes Apr 7, 2021

View reviewed changes

Fix lint and test

a2d5833

betodealmeida merged commit a82d72f into apache:master Apr 7, 2021

etr2460 mentioned this pull request Apr 9, 2021

feat: handle chart/data API errors #14040

Merged

8 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial work to make v1 API compatible with SIP-40 and SIP-41 #13960

feat: initial work to make v1 API compatible with SIP-40 and SIP-41 #13960

betodealmeida commented Apr 6, 2021 •

edited

Loading

etr2460 left a comment

codecov bot commented Apr 6, 2021 •

edited

Loading

betodealmeida commented Apr 6, 2021

dpgaspar left a comment

dpgaspar Apr 6, 2021

betodealmeida Apr 6, 2021

betodealmeida commented Apr 6, 2021

etr2460 left a comment

etr2460 Apr 7, 2021

betodealmeida Apr 7, 2021

feat: initial work to make v1 API compatible with SIP-40 and SIP-41 #13960

feat: initial work to make v1 API compatible with SIP-40 and SIP-41 #13960

Conversation

betodealmeida commented Apr 6, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

etr2460 left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 6, 2021 • edited Loading

Codecov Report

betodealmeida commented Apr 6, 2021

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Apr 6, 2021

Choose a reason for hiding this comment

betodealmeida Apr 6, 2021

Choose a reason for hiding this comment

betodealmeida commented Apr 6, 2021

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Apr 7, 2021

Choose a reason for hiding this comment

betodealmeida Apr 7, 2021

Choose a reason for hiding this comment

betodealmeida commented Apr 6, 2021 •

edited

Loading

codecov bot commented Apr 6, 2021 •

edited

Loading