aminghadersohi commented on code in PR #43423:
URL: https://github.com/apache/superset/pull/43423#discussion_r3836580451
##########
superset/commands/tag/create.py:
##########
@@ -97,9 +99,14 @@ def _validate_object_access(
f"Access validation not supported for {object_type}"
)
)
- except SupersetSecurityException:
+ except (SupersetSecurityException, TemplateError):
Review Comment:
NIT: this `except` wraps all four `object_type` branches
(dashboard/chart/query/dataset), but only the `query` branch can currently
raise `TemplateError` (confirmed by reading `raise_for_access` — the
dashboard/chart/dataset branches never touch Jinja/`process_jinja_sql`). The
comment above implies the fix is scoped to the query case; consider narrowing
the except to just the `query` branch so a future unrelated `TemplateError`
raised from another branch isn't silently misreported as an access-validation
failure for that object type.
##########
superset/commands/tag/create.py:
##########
@@ -97,9 +99,14 @@ def _validate_object_access(
f"Access validation not supported for {object_type}"
)
)
- except SupersetSecurityException:
+ except (SupersetSecurityException, TemplateError):
Review Comment:
No logging when swallowing `TemplateError` here. `SupersetSecurityException`
being swallowed silently is intentional (routine, expected auth denial), but
`TemplateError` represents a genuinely unexpected condition — previously it
propagated as an unhandled 500 with a full traceback visible in server
logs/Sentry. Now it's converted to a generic `TagCreateFailedError` with zero
`logger.warning`/`logger.exception` call anywhere in this file. A
malformed-Jinja saved query (or any other bug that raises `TemplateError` here)
becomes invisible server-side. Suggest binding the exception (`except
(SupersetSecurityException, TemplateError) as ex:`) and logging `str(ex)`
before appending to `exceptions`.
##########
superset/commands/tag/create.py:
##########
@@ -97,9 +99,14 @@ def _validate_object_access(
f"Access validation not supported for {object_type}"
)
)
- except SupersetSecurityException:
+ except (SupersetSecurityException, TemplateError):
Review Comment:
The PR description says this follows the pattern already established in
`superset/commands/sql_lab/results.py`, but that file actually uses two
separate except arms with materially different handling:
```python
except SupersetSecurityException as ex:
raise SupersetErrorException(SupersetError(message=__("Cannot access the
query"), ...), status=403) from ex
except TemplateError as ex:
raise SupersetErrorException(SupersetError(message=str(ex), ...),
status=400) from ex
```
It preserves the real Jinja error text (`str(ex)`) and distinguishes
security vs. validation failures by status code. This PR instead merges both
exception types into one `except (...)` tuple with a single hardcoded generic
message, discarding `str(ex)` and the security/validation distinction the
referenced pattern was built to keep. Worth at least surfacing `str(ex)` so the
actual template error isn't lost.
--
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]
