eschutho opened a new pull request, #42442:
URL: https://github.com/apache/superset/pull/42442
### SUMMARY
`ImportDatasetsCommand.validate()` in
`superset/commands/dataset/importers/v0.py` (the legacy/unversioned dataset
importer) 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 `IncorrectVersionError` → (after the dispatcher
exhausts all import versions) `CommandInvalidError` (422) with a clear message.
This is the same bug class fixed in #42366, #42401, and #42426 — #42426 fixed
the identical narrow-catch pattern in the sibling v1 importer
(`superset/commands/importers/v1/utils.py`'s `load_yaml()`) and explicitly
flagged this v0 site as the same pattern but out of scope for that PR. This PR
closes that gap.
### PROBLEM
`ImportDatasetsCommand.validate()` parses every file in a dataset import as
YAML:
```python
try:
config = yaml.safe_load(content)
except yaml.parser.ParserError as ex:
logger.exception("Invalid YAML file")
raise IncorrectVersionError(
f"{file_name} is not a valid YAML file"
) from ex
```
`yaml.parser.ParserError`, `yaml.scanner.ScannerError`,
`yaml.composer.ComposerError`, etc. are siblings — all subclasses of
`yaml.error.YAMLError`, not of each other (confirmed via
`yaml.parser.ParserError.__mro__`). For example, `yaml.safe_load('key:
"unterminated string')` raises `yaml.scanner.ScannerError`, which is not caught
here.
This is reachable end-to-end: this v0 `ImportDatasetsCommand` is the last
entry in `superset/commands/dataset/importers/dispatcher.py`'s fallback list,
reached whenever the v1 importer raises `IncorrectVersionError` (i.e. exactly
the unversioned/legacy export format this v0 path exists to handle). If
`validate()` leaks a raw `ScannerError`/etc., it propagates through `run()`,
through the dispatcher's own generic `except Exception: logger.exception(...);
raise` (which logs and re-raises, doesn't swallow it), and out through
`superset/datasets/api.py`'s `import_` endpoint (`POST
/api/v1/dataset/import/`), which has no try/except around `command.run()` —
only Flask-AppBuilder's `@safe` decorator, turning it into an opaque generic
500. The same path is also reachable via the `legacy_import_datasources` CLI
command. So importing a legacy-format dataset YAML file malformed in a way
other than a `ParserError` currently 500s instead of surfacing a proper
validation error.
### FIX
Widen the `except` clause in `validate()` to `except yaml.YAMLError as ex:`.
No other exception handling in this file was touched (this is the only `yaml.`
try/except in `v0.py`). No behavior change for the already-working
`ParserError` case; new coverage for
`ScannerError`/`ComposerError`/`ReaderError`/`ConstructorError`.
### TESTING INSTRUCTIONS
- Added two tests to
`tests/unit_tests/datasets/commands/importers/v0/import_test.py`:
- `test_validate_parser_error_raises_incorrect_version_error` — confirms
the existing `ParserError` path (`"[1, 2"`) still raises
`IncorrectVersionError`, unchanged.
- `test_validate_scanner_error_raises_incorrect_version_error` — confirms
a `ScannerError`-triggering input (`'key: "unterminated string'`) is now also
caught and raises `IncorrectVersionError`, instead of leaking the raw
`yaml.scanner.ScannerError`.
- Verified the new `ScannerError` test fails on pre-fix code (raw
`yaml.scanner.ScannerError` propagates uncaught) and passes after the fix.
- Ran the full test file — all 5 tests pass (3 pre-existing + 2 new), no
regressions.
- 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]
