eschutho opened a new pull request, #42426:
URL: https://github.com/apache/superset/pull/42426
### SUMMARY
`load_yaml()` in `superset/commands/importers/v1/utils.py` only caught
`yaml.parser.ParserError`, missing sibling exceptions under
`yaml.error.YAMLError` (`ScannerError`, `ComposerError`, `ReaderError`,
`ConstructorError`). A raw error from these classes propagated as an unhandled
500 instead of the intended `ValidationError` → `CommandInvalidError` (422)
with a clear "Not a valid YAML file" message. This mirrors the same bug class
fixed in #42366 and #42401, and this file already has a sibling call site
(`import_tag()`) that catches the broader `yaml.YAMLError` correctly — this PR
brings `load_yaml()` in line with that existing pattern.
### PROBLEM
`load_yaml()` is used by `load_metadata()` and `load_configs()` to parse
every YAML file in an import ZIP (databases, dashboards, charts, datasets, SSH
tunnels). It only catches `yaml.parser.ParserError`:
```python
try:
return yaml.safe_load(content)
except yaml.parser.ParserError as ex:
...
raise ValidationError({file_name: "Not a valid YAML file"}) from ex
```
But `yaml.parser.ParserError`, `yaml.scanner.ScannerError`,
`yaml.composer.ComposerError`, and others are siblings — all direct/indirect
subclasses of `yaml.error.YAMLError`, not of each other. For example,
`yaml.safe_load('key: "unterminated string')` raises
`yaml.scanner.ScannerError`, which is not caught here.
This is reachable end-to-end: `load_configs()` runs inside
`ImportModelsCommand.validate()`, called from `run()` *before* the try/except
that wraps `_import()`, so nothing in the command layer catches a leaked
exception. The API layer (e.g. `dashboards/api.py` `import_`, and the
chart/dataset/database equivalents) has no try/except around `command.run()`
either — it relies only on Flask-AppBuilder's `@safe` decorator, which turns
any uncaught exception into an opaque generic 500, discarding the actual
reason. So importing a dashboard/chart/dataset/database ZIP with a YAML file
that's malformed in a way other than a `ParserError` currently surfaces as an
opaque 500 instead of a proper validation error.
### FIX
Widen the `except` clause in `load_yaml()` to `except yaml.YAMLError as
ex:`, matching the already-correct sibling catch in `import_tag()` in this same
file. No other exception handling was touched; the deprecated v0 importer
(`superset/commands/dataset/importers/v0.py`) has the same narrow-catch pattern
but is out of scope for this PR.
### TESTING INSTRUCTIONS
- Added `TestLoadYaml` to
`tests/unit_tests/commands/importers/v1/utils_test.py` with two tests: one
confirming the existing `ParserError` path still raises `ValidationError`, and
one confirming a `ScannerError`-triggering input (`'key: "unterminated
string'`) now also raises `ValidationError` instead of leaking the raw
`yaml.scanner.ScannerError`.
- Verified via `git stash` that the new `ScannerError` test fails on pre-fix
code (`yaml.scanner.ScannerError` uncaught) and passes after the fix.
- Ran `pytest tests/unit_tests/commands/importers/v1/utils_test.py` — 8
passed, no regressions to the existing `TestConvertTemporalColumns` tests.
- Ran `ruff check`, `ruff format --check`, and `mypy` on both changed files
— no new issues.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] 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 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]
