gabotorresruiz commented on code in PR #45659:
URL: https://github.com/apache/superset/pull/45659#discussion_r4416212866
##########
superset/databases/api.py:
##########
@@ -994,7 +994,7 @@ def table_metadata_deprecated(
table_info = get_table_metadata(database, Table(table_name,
schema_name))
except SQLAlchemyError as ex:
self.incr_stats("error", self.table_metadata_deprecated.__name__)
- return self.response_422(error_msg_from_exception(ex))
+ return self.response_422("An error occurred while processing the
request.")
Review Comment:
`test_get_invalid_table_table_metadata` pins `{"message": "wrong_table"}`
here (the `NoSuchTableError` text for a missing table on postgres and mysql),
so it fails on both lanes, and this branch has no `logger.exception`, so the
cause is no longer recorded anywhere. Also, the current `GET
/api/v1/database/<pk>/table_metadata/` endpoint lets the same `SQLAlchemyError`
reach `show_unexpected_exception`, which still returns
`error_msg_from_exception(ex)`, so the same caller gets the raw text there.
Would it make sense to leave this deprecated endpoint out of this PR and look
at the catch all handler separately? I might be missing why this one was
singled out.
##########
superset/themes/api.py:
##########
@@ -323,10 +323,10 @@ def bulk_delete(self, **kwargs: Any) -> Response:
except ThemeForbiddenError:
return self.response_403()
except SystemThemeInUseError as ex:
- return self.response_422(message=str(ex))
+ return self.response_422(message="An error occurred while
processing the request.")
except ThemeDeleteFailedError as ex:
logger.exception("Theme delete failed for IDs: %s", item_ids)
- return self.response_422(message=str(ex))
+ return self.response_422(message="An error occurred while
processing the request.")
Review Comment:
Same here for the bulk delete.
```suggestion
except SystemThemeInUseError as ex:
return self.response_422(message=ex.message)
except ThemeDeleteFailedError as ex:
logger.exception("Theme delete failed for IDs: %s", item_ids)
return self.response_422(message=ex.message)
```
##########
superset/themes/api.py:
##########
@@ -258,10 +258,10 @@ def delete(self, pk: int) -> Response:
except ThemeForbiddenError:
return self.response_403()
except SystemThemeInUseError as ex:
- return self.response_422(message=str(ex))
+ return self.response_422(message="An error occurred while
processing the request.")
except ThemeDeleteFailedError as ex:
logger.exception("Theme delete failed for ID: %s", pk)
- return self.response_422(message=str(ex))
+ return self.response_422(message="An error occurred while
processing the request.")
Review Comment:
`SystemThemeInUseError` and `ThemeDeleteFailedError` carry fixed, translated
messages (`superset/commands/theme/exceptions.py`), so `str(ex)` here could not
leak anything, and the user loses the reason the delete was refused. The theme
list only pre-checks this from its loaded rows, so a stale list or any API
caller now gets the generic text. This is what
`test_system_theme_cannot_be_deleted` is failing on. What do you think of
keeping the curated message?
```suggestion
except SystemThemeInUseError as ex:
return self.response_422(message=ex.message)
except ThemeDeleteFailedError as ex:
logger.exception("Theme delete failed for ID: %s", pk)
return self.response_422(message=ex.message)
```
##########
superset/themes/api.py:
##########
@@ -661,7 +661,7 @@ def set_system_default(self, pk: int) -> Response:
except ThemeNotFoundError:
return self.response_404()
except Exception as ex:
- return self.response_422(message=str(ex))
+ return self.response_422(message="An error occurred while
processing the request.")
Review Comment:
The four system theme handlers (`set_system_default`, `set_system_dark`,
`unset_system_default`, `unset_system_dark`) have no `logger.exception`, and
`on_error(reraise=Exception)` in `set_system_theme.py` does not log a DB error
either, so with the generic message nothing records why the call failed. Could
we log it here, and the same in the other three?
```suggestion
except Exception:
logger.exception("Error setting system default theme %s", pk)
return self.response_422(
message="An error occurred while processing the request."
)
```
--
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]
