sha174n commented on PR #44080: URL: https://github.com/apache/superset/pull/44080#issuecomment-5886768417
@rusackas all three points you raised are in and unchanged: the omitted-key read in d16ddd8, the non-dataset guard in 5ce70d6, and the `sql` gap in ccb5395. One commit on top in 6c803a4, from a review pass on that last one. The save body is unvalidated JSON, so `sql` could arrive as any type and reach the parser as a dict or an int, which failed on the type rather than as a parse error and came back a 500. The same applies to `table_name`/`schema`/`catalog`, which this diff had widened from cross-database saves only to every same-database table change. `_requested_target` now type-checks all four up front and answers 422, with a parametrized test over each field. A malformed SQL *string* needed nothing: `SupersetParseError` is a `SupersetErrorException` with `status = 422`, and `@handle_api_exception` already returns it with the parse position intact. The same commit lifts the target parse and the access check into helpers, which keeps `save` under the complexity limit the extra branch would otherwise have broken. It also corrects the `_repoints_sql` docstring, which described `is_virtual` even though that function never reads it, and notes where the view is deliberately stricter than the command it ports: `_validate_sql_access` gates on changed SQL text alone, so it misses a connection or catalog/schema move under unchanged SQL. 45 tests green, pre-commit clean. -- 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]
