chore(importers): log field names instead of full config on validation failure#42399
chore(importers): log field names instead of full config on validation failure#42399rusackas wants to merge 1 commit into
Conversation
…n failure The debug log dumped the entire parsed config dict when schema validation failed. Those blobs can be enormous (inline example data, multi-line keys) and bury the actual validation error logged just above. Log the file name and its top-level field names instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dosubot
Bot
added
the
logging
Code Review Agent Run #49d1abActionable 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 AI Code Review powered by |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # Log field names only; full values can be huge (e.g. inline | ||
| # example data) and drown out the validation error above. | ||
| logger.debug( | ||
| "Config fields present in %s: %s", file_name, sorted(config) |
|
The code change in superset/commands/importers/v1/utils.py |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| logger.debug( | ||
| "Config fields present in %s: %s", file_name, sorted(config) | ||
| ) |
There was a problem hiding this comment.
Suggestion: The validation error handler can raise a new TypeError while evaluating sorted(config) when the invalid parsed configuration is None or contains incomparable top-level key types. This masks the original ValidationError and prevents it from being appended to exceptions; log the keys without sorting or guard this diagnostic operation so it cannot replace the validation failure. [error handling]
Severity Level: Major ⚠️
- ❌ Import validation can abort with `TypeError`.
- ❌ Original schema errors are not appended to `exceptions`.
- ⚠️ Invalid bundles may receive misleading failure diagnostics.Steps of Reproduction ✅
1. Run an importer path that calls `load_configs()` in
`superset/commands/importers/v1/utils.py`, which parses each bundle configuration and
validates it at lines 224-225 with `schema.load(config)`.
2. Provide a bundle configuration whose top-level YAML mapping contains mixed key types,
such as a string field name and an integer key; the schema validation at line 224 raises
the handled `ValidationError`.
3. The `except ValidationError` block at lines 226-231 begins handling the validation
failure, then evaluates `sorted(config)` at line 236.
4. Python cannot order unlike key types during sorting, so line 236 raises `TypeError`
before line 239 appends the original `ValidationError` to `exceptions`; `load_configs()`
therefore propagates the diagnostic logging error instead of returning the collected
validation failure. A `None` configuration would likewise be unsafe if it reaches this
block, although configurations failing earlier at `config["data"]` may raise before this
handler.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/importers/v1/utils.py
**Line:** 235:237
**Comment:**
*Error Handling: The validation error handler can raise a new `TypeError` while evaluating `sorted(config)` when the invalid parsed configuration is `None` or contains incomparable top-level key types. This masks the original `ValidationError` and prevents it from being appended to `exceptions`; log the keys without sorting or guard this diagnostic operation so it cannot replace the validation failure.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42399 +/- ##
==========================================
- Coverage 65.27% 65.20% -0.07%
==========================================
Files 2795 2795
Lines 157549 157549
Branches 36039 36039
==========================================
- Hits 102836 102734 -102
- Misses 52736 52833 +97
- Partials 1977 1982 +5
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:
|
2 participants
SUMMARY
When schema validation fails during an import, the debug log dumps the entire parsed config dict. Those blobs can be enormous (inline example data, multi-line keys) and bury the actual validation error that's logged right above. This trims the debug line to the file name plus its top-level field names, which is what you actually need to correlate the failure.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (log output only)
TESTING INSTRUCTIONS
With debug logging enabled, import a bundle with an invalid config and confirm the log shows the field-name list instead of the full config dump.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code