codeant-ai-for-open-source[bot] commented on code in PR #42463:
URL: https://github.com/apache/superset/pull/42463#discussion_r3663547187
##########
superset/commands/dataset/refresh.py:
##########
@@ -46,7 +49,23 @@ def __init__(self, model_id: int):
def run(self) -> Model:
self.validate()
assert self._model
- self._model.fetch_metadata()
+ try:
+ self._model.fetch_metadata()
+ except SupersetGenericDBErrorException as ex:
+ # Virtual datasets whose SQL contains Jinja templates (e.g.
+ # ``{% if from_dttm %}``) cannot be parsed at save time
+ # because the templates have no runtime context — sqlglot
+ # then rejects the empty ``{% if %}`` block. The dataset
+ # row itself has already been persisted by
+ # ``UpdateDatasetCommand`` before this refresh runs, so
+ # treat the metadata refresh as best-effort rather than
+ # surfacing an "Invalid SQL" toast for a save the user
+ # already committed. See #38012.
+ logger.warning(
+ "Dataset column refresh skipped for %s: %s",
+ self._model.table_name,
+ ex.message,
+ )
Review Comment:
**Suggestion:** `SupersetGenericDBErrorException` is not limited to Jinja
parse failures: `get_columns_description()` wraps database connection failures,
query execution errors, permission failures, and other driver exceptions in
this same class. Catching all instances here makes those refresh failures look
successful, allowing the update endpoint to return success while metadata was
not refreshed. Narrow the best-effort handling to the specific Jinja/template
parse case, or preserve the existing failure behavior for actual database
errors. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset updates can report success despite failed metadata queries.
- ⚠️ Column metadata remains stale after connection or driver failures.
- ⚠️ Existing refresh error reporting is bypassed for database errors.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Send a dataset update request with the `override_columns` query parameter
enabled;
`superset/datasets/api.py:593-595` parses this flag and
`superset/datasets/api.py:632-633`
invokes `RefreshDatasetCommand(pk).run()`.
2. `RefreshDatasetCommand.run()` calls `SqlaTable.fetch_metadata()` at
`superset/commands/dataset/refresh.py:52-53`, and virtual datasets route
through
`external_metadata()` at `superset/connectors/sqla/models.py:1575-1582`.
3. During metadata discovery, `get_columns_description()` executes the
metadata query and
catches every exception at `superset/connectors/sqla/utils.py:146-181`,
wrapping
connection failures, driver errors, query execution failures, and
result-processing errors
in `SupersetGenericDBErrorException`.
4. The catch at `superset/commands/dataset/refresh.py:54-68` treats all of
those failures
as a skipped refresh, allowing the update endpoint to continue to its
success response
instead of raising the existing `DatasetRefreshFailedError` path handled at
`superset/datasets/api.py:656-660`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=864cdfe5a8124e4c9268466aa0997dc4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=864cdfe5a8124e4c9268466aa0997dc4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/refresh.py
**Line:** 52:68
**Comment:**
*Api Mismatch: `SupersetGenericDBErrorException` is not limited to
Jinja parse failures: `get_columns_description()` wraps database connection
failures, query execution errors, permission failures, and other driver
exceptions in this same class. Catching all instances here makes those refresh
failures look successful, allowing the update endpoint to return success while
metadata was not refreshed. Narrow the best-effort handling to the specific
Jinja/template parse case, or preserve the existing failure behavior for actual
database errors.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42463&comment_hash=0fb2e4ed86ccd423c7f7c969e7eeb64bdddf488f086352a6c489d9f76bcb4551&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42463&comment_hash=0fb2e4ed86ccd423c7f7c969e7eeb64bdddf488f086352a6c489d9f76bcb4551&reaction=dislike'>👎</a>
--
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]
