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.
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
ccfef7f to
105c185
Compare
|
ecmwf/eckit#293 needs to be merged first. |
676fd9e to
9571e30
Compare
Also refactor error messages
9571e30 to
d144eea
Compare
| try { | ||
| load(in, replace); | ||
| } | ||
| catch (SchemaParser::Error& spe) { |
There was a problem hiding this comment.
I don't understand the point of this try/catch wrapper. It catches a SchemaParser - and throws a SchemaParser. There is no extra information to add here?
There was a problem hiding this comment.
Enriching the context of the error message by telling what the actual underlying issue is.
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