eschutho commented on PR #43261: URL: https://github.com/apache/superset/pull/43261#issuecomment-5517419490
@rebenitez1802 — one thing worth scrutinising while you're in here, verified independently against source before this PR was routed to you: The new `except json.JSONDecodeError` clause doesn't cover the full failure mode. `simplejson.loads()` raises a plain `TypeError` (not a `JSONDecodeError`) when handed a non-string value — e.g. a YAML list/dict. `masked_encrypted_extra` comes straight from the uploaded import YAML and is only guarded by a truthy `config.get(...)` check upstream, so a non-empty non-string value reaches `json.loads()` and still raises the uncaught `TypeError`. Since `load_configs()` runs inside `ImportModelsCommand.validate()`, which sits outside `run()`'s try/except, that `TypeError` still propagates as an opaque 500 — the exact class of bug this PR is fixing, just not fully closed. Suggested widening: `except (json.JSONDecodeError, TypeError) as exc:`, same handling as the existing branch, plus a regression test with a non-string `masked_encrypted_extra`. There's in-repo precedent for the same tuple in `security/manager.py::query_context_modified` (#43795). Thanks! -- 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]
