msyavuz commented on code in PR #43334:
URL: https://github.com/apache/superset/pull/43334#discussion_r3845167438
##########
superset/commands/dataset/create.py:
##########
@@ -50,7 +54,25 @@ def run(self) -> Model:
self.validate()
dataset = DatasetDAO.create(attributes=self._properties)
- dataset.fetch_metadata()
+ try:
+ dataset.fetch_metadata()
+ except SupersetException as ex:
Review Comment:
`OAuth2RedirectError` is a `SupersetException` too, so this now swallows it
into a 422 and drops the `url`/`tab_id` extras the OAuth2 dance needs —
intentional? It was a 500 before, so not a regression.
##########
superset/commands/dataset/create.py:
##########
@@ -50,7 +54,25 @@ def run(self) -> Model:
self.validate()
dataset = DatasetDAO.create(attributes=self._properties)
- dataset.fetch_metadata()
+ try:
+ dataset.fetch_metadata()
+ except SupersetException as ex:
+ # Not a SQLAlchemyError, so ``on_error`` re-raises it untouched and
+ # it escapes to FAB's ``@safe`` as an opaque 500 "Fatal error".
+ # Deliberately covers the 403 ``SupersetSecurityException`` raised
+ # for mutation/multi-statement SQL too: ``validate()`` already
+ # reports that class of rejection as a 422 on ``sql`` via
+ # ``DatasetDataAccessIsNotAllowed``.
+ raise DatasetInvalidError(
Review Comment:
This makes the `except SupersetGenericDBErrorException` branch in
`mcp_service/dataset/tool/create_virtual_dataset.py:220` unreachable —
`fetch_metadata` here was its only source, so that tool now always lands in the
`DatasetInvalidError` branch.
##########
superset/commands/dataset/create.py:
##########
@@ -50,7 +54,25 @@ def run(self) -> Model:
self.validate()
dataset = DatasetDAO.create(attributes=self._properties)
- dataset.fetch_metadata()
+ try:
+ dataset.fetch_metadata()
+ except SupersetException as ex:
+ # Not a SQLAlchemyError, so ``on_error`` re-raises it untouched and
+ # it escapes to FAB's ``@safe`` as an opaque 500 "Fatal error".
+ # Deliberately covers the 403 ``SupersetSecurityException`` raised
+ # for mutation/multi-statement SQL too: ``validate()`` already
+ # reports that class of rejection as a 422 on ``sql`` via
+ # ``DatasetDataAccessIsNotAllowed``.
+ raise DatasetInvalidError(
+ exceptions=[
+ ValidationError(
+ # ``lazy_gettext`` messages aren't ``str``, so
+ # marshmallow won't wrap them into a list on its own.
+ [str(ex.message)],
Review Comment:
For `SupersetErrorsException` subclasses `ex.message` is
`str([SupersetError(...)])`, so `sql: "SELECT 1 {% if %}"` toasts `Template
processing error: [SupersetError(message=..., error_type=<...>)]` — unwrap
those before surfacing?
--
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]
