Repository navigation
fix(importers): catch YAMLError instead of narrower ParserError in dataset v0 importer - #42442
Conversation
…taset v0 importer ImportDatasetsCommand.validate() only caught yaml.parser.ParserError when loading a legacy/unversioned dataset export, but yaml.safe_load can raise several sibling exceptions under yaml.YAMLError depending on how the file is malformed (ScannerError, ComposerError, ReaderError, ConstructorError). Those siblings were not caught here, so they propagated past this command, past the dispatcher's generic re-raise, and out through the POST /api/v1/dataset/import/ endpoint as a raw, opaque 500 instead of the intended 422 IncorrectVersionError response. Widen the except clause to yaml.YAMLError, matching the pattern already fixed in the v1 importer's load_yaml() (apache#42426).
Code Review Agent Run #b42831Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42442 +/- ##
==========================================
- Coverage 65.23% 65.23% -0.01%
==========================================
Files 2795 2795
Lines 157694 157694
Branches 36067 36067
==========================================
- Hits 102871 102868 -3
- Misses 52847 52849 +2
- Partials 1976 1977 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SUMMARY
ImportDatasetsCommand.validate()insuperset/commands/dataset/importers/v0.py(the legacy/unversioned dataset importer) only caughtyaml.parser.ParserError, missing sibling exceptions underyaml.error.YAMLError(ScannerError,ComposerError,ReaderError,ConstructorError). A raw error from these classes propagated as an unhandled 500 instead of the intendedIncorrectVersionError→ (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'sload_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:yaml.parser.ParserError,yaml.scanner.ScannerError,yaml.composer.ComposerError, etc. are siblings — all subclasses ofyaml.error.YAMLError, not of each other (confirmed viayaml.parser.ParserError.__mro__). For example,yaml.safe_load('key: "unterminated string')raisesyaml.scanner.ScannerError, which is not caught here.This is reachable end-to-end: this v0
ImportDatasetsCommandis the last entry insuperset/commands/dataset/importers/dispatcher.py's fallback list, reached whenever the v1 importer raisesIncorrectVersionError(i.e. exactly the unversioned/legacy export format this v0 path exists to handle). Ifvalidate()leaks a rawScannerError/etc., it propagates throughrun(), through the dispatcher's own genericexcept Exception: logger.exception(...); raise(which logs and re-raises, doesn't swallow it), and out throughsuperset/datasets/api.py'simport_endpoint (POST /api/v1/dataset/import/), which has no try/except aroundcommand.run()— only Flask-AppBuilder's@safedecorator, turning it into an opaque generic 500. The same path is also reachable via thelegacy_import_datasourcesCLI command. So importing a legacy-format dataset YAML file malformed in a way other than aParserErrorcurrently 500s instead of surfacing a proper validation error.FIX
Widen the
exceptclause invalidate()toexcept yaml.YAMLError as ex:. No other exception handling in this file was touched (this is the onlyyaml.try/except inv0.py). No behavior change for the already-workingParserErrorcase; new coverage forScannerError/ComposerError/ReaderError/ConstructorError.TESTING INSTRUCTIONS
tests/unit_tests/datasets/commands/importers/v0/import_test.py:test_validate_parser_error_raises_incorrect_version_error— confirms the existingParserErrorpath ("[1, 2") still raisesIncorrectVersionError, unchanged.test_validate_scanner_error_raises_incorrect_version_error— confirms aScannerError-triggering input ('key: "unterminated string') is now also caught and raisesIncorrectVersionError, instead of leaking the rawyaml.scanner.ScannerError.ScannerErrortest fails on pre-fix code (rawyaml.scanner.ScannerErrorpropagates uncaught) and passes after the fix.ruff check,ruff format --check, andmypyon both changed files — no new issues.ADDITIONAL INFORMATION