[eschutho]

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:

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)
  • 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

[bito-code-review]

Code Review Agent Run #df693a

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: e4d3b11..e4d3b11

  • superset/commands/importers/v1/utils.py
  • tests/unit_tests/commands/importers/v1/utils_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.24%. Comparing base (e1ffa53) to head (e4d3b11).

Files with missing lines	Patch %	Lines
superset/commands/importers/v1/utils.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #42426      +/-   ##
==========================================
- Coverage   65.24%   65.24%   -0.01%     
==========================================
  Files        2795     2795              
  Lines      157643   157643              
  Branches    36061    36061              
==========================================
- Hits       102853   102850       -3     
- Misses      52814    52816       +2     
- Partials     1976     1977       +1     

Flag	Coverage Δ
hive	38.39% <0.00%> (ø)
mysql	57.57% <0.00%> (ø)
postgres	57.61% <0.00%> (-0.01%)	⬇️
presto	40.31% <0.00%> (ø)
python	59.02% <0.00%> (-0.01%)	⬇️
sqlite	57.24% <0.00%> (ø)
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rebenitez1802]

LGTM