Skip to content

feat: Augment schema parser error messages#277

Open
tbkr wants to merge 1 commit into
developfrom
feature/schema-parser-error-msg
Open

feat: Augment schema parser error messages#277
tbkr wants to merge 1 commit into
developfrom
feature/schema-parser-error-msg

Conversation

@tbkr
Copy link
Copy Markdown
Contributor

@tbkr tbkr commented May 8, 2026

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:

  • All authors agree to the Contributor License Agreement.
  • The code follows the project's coding standards.
  • I have performed self-review and added comments where needed.
  • I have added or updated tests to verify that my changes are effective and functional.
  • I have run all existing tests and confirmed they pass.

🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/fdb/pull-requests/PR-277

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/fdb5/rules/SchemaParser.cc Outdated
Comment thread src/fdb5/rules/SchemaParser.cc
Comment thread src/fdb5/rules/SchemaParser.cc Outdated
Comment on lines +289 to 292
if (result.size() == 0) {
eckit::Log::error() << "SchemaParser::parse: Empty rule list. Didn't find any rule in the provided schema file."
<< std::endl;
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/fdb5/rules/Schema.cc Outdated
Comment thread tests/fdb/parsing/test_schema_parsing.cc
Comment thread tests/fdb/parsing/test_schema_parsing.cc
Comment thread tests/fdb/parsing/test_schema_parsing.cc Outdated
Comment thread tests/fdb/parsing/test_schema_parsing.cc Outdated
Comment thread tests/fdb/parsing/test_schema_parsing.cc
Comment thread src/fdb5/rules/SchemaParser.cc Outdated
try {
result.emplace_back(parseDatabase());
}
catch (fdb5::SchemaParser::Error& spe) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-commenter
Copy link
Copy Markdown

codecov-commenter commented May 8, 2026

Codecov Report

❌ Patch coverage is 98.91304% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 71.18%. Comparing base (dd9a587) to head (6399d0d).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
tests/fdb/parsing/test_schema_parsing.cc 98.18% 1 Missing ⚠️
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.
📢 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.

@tbkr tbkr force-pushed the feature/schema-parser-error-msg branch 4 times, most recently from de693b2 to 2d2c36e Compare May 8, 2026 12:54
@tbkr
Copy link
Copy Markdown
Contributor Author

tbkr commented May 8, 2026

The fix from #276 should be merged first.

@tbkr tbkr force-pushed the feature/schema-parser-error-msg branch 2 times, most recently from 47c5a6d to daf484f Compare May 11, 2026 13:42
@tbkr tbkr requested a review from danovaro May 11, 2026 13:43
Comment thread .github/workflows/pyfdb.yml Outdated
Comment thread src/fdb5/rules/SchemaParser.cc Outdated
@ChrisspyB
Copy link
Copy Markdown
Member

I agree that 0 rules should be an error.

@tbkr tbkr force-pushed the feature/schema-parser-error-msg branch 3 times, most recently from 1d9d990 to b2fd67d Compare May 12, 2026 12:46
Now showing in which step of the schema file parsing the
error is occurring.
@danovaro danovaro force-pushed the feature/schema-parser-error-msg branch from b2fd67d to 6399d0d Compare May 12, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants