You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "eschutho (via GitHub)" <gi...@apache.org> on 2026/07/26 16:42:16 UTC
[PR] fix(importers): catch YAMLError instead of narrower ParserError in dataset v0 importer [superset]
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(importers): catch YAMLError instead of narrower ParserError in dataset v0 importer [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #42442:
URL: https://github.com/apache/superset/pull/42442#issuecomment-5084477926
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/42442?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.41%. Comparing base ([`ba0900d`](https://app.codecov.io/gh/apache/superset/commit/ba0900d2b32fc4b294d5192312b136cd1851b349?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7dd7d14`](https://app.codecov.io/gh/apache/superset/commit/7dd7d14c1c2ee625c756b0f91f2bf5ab3d89a633?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/42442?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/dataset/importers/v0.py](https://app.codecov.io/gh/apache/superset/pull/42442?src=pr&el=tree&filepath=superset%2Fcommands%2Fdataset%2Fimporters%2Fv0.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGF0YXNldC9pbXBvcnRlcnMvdjAucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/42442?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #42442 +/- ##
==========================================
- Coverage 65.23% 64.41% -0.82%
==========================================
Files 2795 2795
Lines 157694 157694
Branches 36067 36067
==========================================
- Hits 102871 101585 -1286
- Misses 52847 54139 +1292
+ Partials 1976 1970 -6
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/42442/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/42442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.39% <0.00%> (ø)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/42442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/42442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/42442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.31% <0.00%> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/42442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.37% <0.00%> (-1.65%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/42442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.23% <0.00%> (ø)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/42442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/42442?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(importers): catch YAMLError instead of narrower ParserError in dataset v0 importer [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #42442:
URL: https://github.com/apache/superset/pull/42442
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(importers): catch YAMLError instead of narrower ParserError in dataset v0 importer [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #42442:
URL: https://github.com/apache/superset/pull/42442#issuecomment-5084412823
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org