eschutho opened a new pull request, #43261:
URL: https://github.com/apache/superset/pull/43261
### SUMMARY
`load_configs()` in `superset/commands/importers/v1/utils.py` merges
caller-supplied `encrypted_extra_secrets` into each config's
`masked_encrypted_extra` field. To do so it calls `json.loads()` on that
field
**before** schema validation runs.
**Problem:** `masked_encrypted_extra` comes straight from the user-uploaded
import YAML (inside the import ZIP), and `encrypted_extra_secrets` is read
directly from `request.form["encrypted_extra_secrets"]` on
`POST /api/v1/database/import/` and the generic asset-import endpoint. If the
YAML value is not valid JSON (e.g. a hand-edited or corrupted export),
`json.loads()` raises a raw `simplejson.JSONDecodeError` — which is a
`ValueError`, **not** a `marshmallow.ValidationError`. The enclosing `except`
in the per-file loop only catches `ValidationError`, so the decode error
escapes uncaught out of `ImportModelsCommand.validate()` (which is not
wrapped
in a try/except) and surfaces as an opaque **HTTP 500**, instead of the
structured **422** (`CommandInvalidError` aggregating a list of
`ValidationError`s) that every other validation failure in this function
produces.
**Fix:** Add a sibling `except json.JSONDecodeError` clause next to the
existing `except ValidationError`, converting the decode error into a
`ValidationError` with the same `{file_name: {field: [msg]}}` shape. It then
flows into the existing `exceptions` list and the downstream
`CommandInvalidError` aggregation exactly like every other per-file
validation
failure. `json.JSONDecodeError` is already re-exported by
`superset.utils.json`
(the module the file imports as `json`), so no new import is needed — this is
the same idiom already used in ~15 other files across the codebase. This is
an
**additive catch only**: existing success paths and the existing
`ValidationError` handling are unchanged.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend-only error-handling change).
### TESTING INSTRUCTIONS
Unit tests (no DB/app fixtures required) added in
`tests/unit_tests/commands/importers/v1/utils_test.py::TestLoadConfigs`:
- `test_invalid_json_in_masked_encrypted_extra_is_collected` — calls
`load_configs()` with a config whose `masked_encrypted_extra` is a non-JSON
string and `encrypted_extra_secrets` populated for that file; asserts it
returns without raising and appends exactly one `ValidationError` with the
`{file_name: {"masked_encrypted_extra": [...]}}` shape.
- `test_valid_json_in_masked_encrypted_extra_still_merges` — control: valid
JSON still has secrets merged in with no exceptions.
Run:
```
pytest tests/unit_tests/commands/importers/v1/utils_test.py -k
TestLoadConfigs
```
Verified fail-before/pass-after: with the fix reverted, the invalid-JSON test
fails with a raw `simplejson.errors.JSONDecodeError: Expecting value: line 1
column 1 (char 0)`; with the fix applied, all tests pass.
Existing integration coverage for the success path:
`tests/integration_tests/databases/api_tests.py::test_import_database_with_encrypted_extra_secrets`.
### ADDITIONAL INFORMATION
- [x] Has associated issue: internal Shortcut sc-117563
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
This follows the same "convert a raw library exception into a structured
marshmallow `ValidationError` so it aggregates into `CommandInvalidError`"
pattern established in #42366, #42401, #43145, and #43226. Additive catch
with
no behavior change to existing success paths, so no Tradeoffs section is
warranted.
--
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]
