rebenitez1802 commented on PR #43433:
URL: https://github.com/apache/superset/pull/43433#issuecomment-5480502278
Approve — this supersedes my earlier request-changes. The follow-up
(`0d0704b`) resolves both prior blockers: `SupersetParseError` is now caught
alongside `TemplateError` (opaque 500 → clean 422, fail-closed, correctly
scoped to the query branch), and the collected error is now a proper
`ValidationError` (`TagAccessValidationError`) so it composites cleanly. Both
new tests are genuine red-pre-fix/green-post-fix guards, and CI + lint are
clean. Remaining items below are non-blocking nits.
🟢 **Low — Comment/test docstrings say the delete route calls
`normalized_messages()`, but it doesn't**
The new comment (`delete.py`, "the delete route calls
`normalized_messages()` on it") and both new test docstrings ("called by the
single-object DELETE route") are inaccurate: the only route running
`DeleteTaggedObjectCommand` is `delete_object` (`tags/api.py:472`), whose
handler returns a bare `self.response_422()` (`api.py:474-475`) and never calls
`normalized_messages()`. The routes that *do* call it
(`api.py:542-543,598,681-682`) run the different `DeleteTagsCommand`. The
`ValidationError` change is still the right call — just fix the rationale.
Suggestion: reword to e.g. "Append a `ValidationError` so it composites cleanly
if `normalized_messages()` is ever called (the bulk-delete route does so for
`DeleteTagsCommand`)."
🟢 **Low — The `normalized_messages()` footgun is only partly closed in this
command**
Only the query-access branch now appends a `ValidationError`. The other five
branches in `DeleteTaggedObjectCommand` still append non-`ValidationError`s —
`TaggedObjectDeleteFailedError` (missing-args/tag-not-found/bad-type at
`delete.py:65,70,77`, the `SupersetSecurityException` branch, and the
unsupported-type `else`) and `TaggedObjectNotFoundError` (`delete.py:90`). So a
mixed failure (e.g. the access-check raises *and* `find_tagged_object` returns
`None`) yields a `TagInvalidError` whose `normalized_messages()` would
`AttributeError`. It can't crash today because `delete_object` uses a bare
`response_422()` — hence purely latent — but the fix leaves the contract
half-applied. Optional hardening: make every item appended into
`TagInvalidError` in this command a `ValidationError` subclass, mirroring
`DeleteTagsCommand`.
🟢 **Low — `{object_type}` renders as the enum repr in the message**
`ObjectType` is a plain `enum.Enum`, so `f"Access validation failed for
{object_type} {object_id}: {ex}"` renders `"...for ObjectType.query 1: ..."`.
Cosmetic only (and log-only on this route), and it matches the pre-existing
sibling messages, so optional. Suggestion: use `{object_type.name}` for clean
text.
🟢 **Low (optional) — `except TemplateError` also swallows Jinja
`SecurityError`/`UndefinedError`**
Still downgrades a sandbox/internal templating failure from a 5xx to a
WARNING-logged 422 during access validation (observability, not
correctness/security — it stays fail-closed). Fine to leave; worth a note.
🟢 **Low (optional) — Tests stop at `.validate()`**
Both new tests exercise the command directly, so they don't assert the route
renders a real 422, that the tag survives (fail-closed), or that a
non-`TemplateError`/`SupersetParseError` still propagates. The
public-`normalized_messages()` assertions are a solid improvement; a single
route-level 422 test would close the last gap.
--
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]
