feat: Augment schema parser error messages#277
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve diagnosability of FDB schema parsing failures by adding higher-level context (which parsing phase failed) to schema parser error handling, and introduces a small test suite + fixtures to exercise broken schema inputs.
Changes:
- Add parsing-stage context around schema parsing exceptions (types parsing vs rules parsing).
- Wrap schema-load parsing errors with additional file-path context.
- Add a new parsing test target with schema fixtures (valid schema and several intentionally broken schemas).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/fdb5/rules/SchemaParser.cc |
Adds try/catch blocks and logging to annotate errors during type/rule parsing phases. |
src/fdb5/rules/Schema.cc |
Wraps StreamParser::Error to add schema path context when load fails. |
tests/fdb/CMakeLists.txt |
Registers the new parsing test subdirectory. |
tests/fdb/parsing/CMakeLists.txt |
Adds a new unit test target and copies parsing fixtures into the build tree. |
tests/fdb/parsing/test_schema_parsing.cc |
Adds unit tests intended to exercise broken/valid schema inputs. |
tests/fdb/parsing/data/schema |
Adds a valid schema fixture. |
tests/fdb/parsing/data/schema_incomplete_rule |
Adds a broken schema fixture (missing closing bracket). |
tests/fdb/parsing/data/broken_schema_no_rule |
Adds a fixture with only type definitions and no rules. |
tests/fdb/parsing/data/broken_types_missing_semicolon |
Adds a broken types fixture (missing semicolon). |
tests/fdb/parsing/data/broken_types_no_name |
Adds a broken types fixture (missing name). |
tests/fdb/parsing/data/broken_types_no_type |
Adds a broken types fixture (missing type). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (result.size() == 0) { | ||
| eckit::Log::error() << "SchemaParser::parse: Empty rule list. Didn't find any rule in the provided schema file." | ||
| << std::endl; | ||
| } |
| try { | ||
| result.emplace_back(parseDatabase()); | ||
| } | ||
| catch (fdb5::SchemaParser::Error& spe) { |
There was a problem hiding this comment.
This is a suggestion: fdb5::SchemaParser::Error may be directly used in a catch clause by callers of the API and as such may better be defined under API and not as nested type, e.g. fdb5::ConfigError or fdb5::SchemaError.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #277 +/- ##
===========================================
+ Coverage 71.05% 71.18% +0.13%
===========================================
Files 371 372 +1
Lines 23464 23550 +86
Branches 2458 2459 +1
===========================================
+ Hits 16673 16765 +92
+ Misses 6791 6785 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
de693b2 to
2d2c36e
Compare
|
The fix from #276 should be merged first. |
47c5a6d to
daf484f
Compare
|
I agree that 0 rules should be an error. |
1d9d990 to
b2fd67d
Compare
Now showing in which step of the schema file parsing the error is occurring.
b2fd67d to
6399d0d
Compare
Description
Augment schema parser error messages. Now showing in which step of the schema file parsing the error is occurring.
Contributor Declaration
By opening this pull request, I affirm the following:
🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/fdb/pull-requests/PR-277